From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 16F7B740035 for ; Wed, 22 Nov 2023 11:45:40 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=821uxi6DTDenVsDJewd1HAnEEN1G99viuCyg5ejAk90=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1700653539; v=1; b=vbm7fbdMxIy8CTn15+y6FZjM01xbcEHRiNfFbuQ4PnNXr0CffD9JIBH+Am7UHvhzrswtqf64 xkS/fJoYFeiCyIld3zOMrRh1odKW1pAyBtwEcN61TXo0T2IE0ao2PwL7n2ioEKb9QvsJgOmB+KI 7ibBddTTQoemJM9/4NsOOF+A= X-Received: by 127.0.0.2 with SMTP id qVpdYY7687511xYrv2VsP7Dd; Wed, 22 Nov 2023 03:45:39 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.17451.1700653539086679031 for ; Wed, 22 Nov 2023 03:45:39 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-454-aqnhto6TMhKCz6ogHUrVzw-1; Wed, 22 Nov 2023 06:45:34 -0500 X-MC-Unique: aqnhto6TMhKCz6ogHUrVzw-1 X-Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 33E2B185A780; Wed, 22 Nov 2023 11:45:34 +0000 (UTC) X-Received: from [10.39.193.187] (unknown [10.39.193.187]) by smtp.corp.redhat.com (Postfix) with ESMTPS id F2122492BE0; Wed, 22 Nov 2023 11:45:32 +0000 (UTC) Message-ID: <7614875b-fb11-8b2c-1411-da0b5c1224b3@redhat.com> Date: Wed, 22 Nov 2023 12:45:31 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/1] StandaloneMmPkg/Core: Restart dispatcher once MmEntryPoint is registered To: devel@edk2.groups.io, wei6.xu@intel.com Cc: Ard Biesheuvel , Sami Mujawar , Ray Ni References: From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: Ld2V8zbfw37RmsnfU7lbnSl7x7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=vbm7fbdM; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 11/20/23 09:30, Xu, Wei6 wrote: > Defer the dispatch of the remaining MM drivers once the CPU driver has > been dispatched. > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4599 > > In MmDispatcher, return immediately if the MM Entry Point was registered. > Then the MM IPL will reinvoke the MM Core Dispatcher. This is required > so MM Mode may be enabled as soon as all the dependent MM Drivers for MM > Mode have been dispatched. > > Introduce a FeatureFlag PCD to control if MmDispatcher returns or not > when MmEntryPointPoint is registered. Default value is FALSE. > > Cc: Ard Biesheuvel > Cc: Sami Mujawar > Cc: Ray Ni > Signed-off-by: Wei6 Xu > --- > StandaloneMmPkg/Core/Dispatcher.c | 76 +++++++++++++++++++++++ > StandaloneMmPkg/Core/StandaloneMmCore.c | 1 + > StandaloneMmPkg/Core/StandaloneMmCore.inf | 3 + > StandaloneMmPkg/StandaloneMmPkg.dec | 6 ++ > 4 files changed, 86 insertions(+) > > diff --git a/StandaloneMmPkg/Core/Dispatcher.c b/StandaloneMmPkg/Core/Dis= patcher.c > index b1ccba15b060..a3983785070b 100644 > --- a/StandaloneMmPkg/Core/Dispatcher.c > +++ b/StandaloneMmPkg/Core/Dispatcher.c > @@ -586,6 +586,7 @@ MmDispatcher ( > LIST_ENTRY *Link; > EFI_MM_DRIVER_ENTRY *DriverEntry; > BOOLEAN ReadyToRun; > + BOOLEAN PreviousMmEntryPointRegistered; > > DEBUG ((DEBUG_INFO, "MmDispatcher\n")); > > @@ -649,6 +650,11 @@ MmDispatcher ( > DriverEntry->Initialized =3D TRUE; > RemoveEntryList (&DriverEntry->ScheduledLink); > > + // > + // Cache state of MmEntryPointRegistered before calling entry poin= t > + // > + PreviousMmEntryPointRegistered =3D gMmCorePrivate->MmEntryPointReg= istered; > + > // > // For each MM driver, pass NULL as ImageHandle > // > @@ -667,6 +673,22 @@ MmDispatcher ( > DEBUG ((DEBUG_INFO, "StartImage Status - %r\n", Status)); > MmFreePages (DriverEntry->ImageBuffer, DriverEntry->NumberOfPage= ); > } > + > + if (!PreviousMmEntryPointRegistered && gMmCorePrivate->MmEntryPoin= tRegistered) { > + if (FeaturePcdGet (PcdRestartMmDispatcherOnceMmEntryRegistered))= { > + // > + // Return immediately if the MM Entry Point was registered by = the MM > + // Driver that was just dispatched. The MM IPL will reinvoke t= he MM > + // Core Dispatcher. This is required so MM Mode may be enabled= as soon > + // as all the dependent MM Drivers for MM Mode have been dispa= tched. > + // Once the MM Entry Point has been registered, then MM Mode w= ill be > + // used. > + // > + gRequestDispatch =3D TRUE; > + gDispatcherRunning =3D FALSE; > + return EFI_NOT_READY; > + } > + } > } > > // > @@ -897,6 +919,60 @@ MmAddToDriverList ( > return EFI_SUCCESS; > } > > +/** > + Event notification that is fired MM IPL to dispatch the previously dis= covered MM drivers. > + > + @param[in] DispatchHandle The unique handle assigned to this ha= ndler by MmiHandlerRegister(). > + @param[in] Context Points to an optional handler context= which was specified when the > + handler was registered. > + @param[in, out] CommBuffer A pointer to a collection of data in = memory that will > + be conveyed from a non-MM environment= into an MM environment. > + @param[in, out] CommBufferSize The size of the CommBuffer. > + > + @return EFI_SUCCESS Dispatcher is executed. > + > +**/ > +EFI_STATUS > +EFIAPI > +MmDriverDispatchHandler ( > + IN EFI_HANDLE DispatchHandle, > + IN CONST VOID *Context OPTIONAL, > + IN OUT VOID *CommBuffer OPTIONAL, > + IN OUT UINTN *CommBufferSize OPTIONAL > + ) > +{ > + EFI_STATUS Status; > + > + DEBUG ((DEBUG_INFO, "MmDriverDispatchHandler\n")); > + > + // > + // Execute the MM Dispatcher on MM drivers that have been discovered > + // previously but not dispatched. > + // > + Status =3D MmDispatcher (); > + > + // > + // Check to see if CommBuffer and CommBufferSize are valid > + // > + if ((CommBuffer !=3D NULL) && (CommBufferSize !=3D NULL)) { > + if (*CommBufferSize > 0) { > + if (!EFI_ERROR (Status)) { > + // > + // Set the flag to show that the MM Dispatcher executed without = errors > + // > + *(UINT8 *)CommBuffer =3D COMM_BUFFER_MM_DISPATCH_SUCCESS; > + } else { > + // > + // Set the flag to show that the MM Dispatcher encountered an er= ror > + // > + *(UINT8 *)CommBuffer =3D COMM_BUFFER_MM_DISPATCH_ERROR; > + } > + } > + } > + > + return EFI_SUCCESS; > +} > + > /** > Traverse the discovered list for any drivers that were discovered but = not loaded > because the dependency expressions evaluated to false. > diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.c b/StandaloneMmPkg/Co= re/StandaloneMmCore.c > index d221f1d1115d..e65edee6d8c2 100644 > --- a/StandaloneMmPkg/Core/StandaloneMmCore.c > +++ b/StandaloneMmPkg/Core/StandaloneMmCore.c > @@ -84,6 +84,7 @@ EFI_MM_SYSTEM_TABLE gMmCoreMmst =3D { > // Table of MMI Handlers that are registered by the MM Core when it is i= nitialized > // > MM_CORE_MMI_HANDLERS mMmCoreMmiHandlers[] =3D { > + { MmDriverDispatchHandler, &gEfiEventDxeDispatchGuid, NULL, T= RUE }, > { MmReadyToLockHandler, &gEfiDxeMmReadyToLockProtocolGuid, NULL, T= RUE }, > { MmEndOfDxeHandler, &gEfiEndOfDxeEventGroupGuid, NULL, F= ALSE }, > { MmExitBootServiceHandler, &gEfiEventExitBootServicesGuid, NULL, F= ALSE }, > diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf b/StandaloneMmPkg/= Core/StandaloneMmCore.inf > index c44b9ff33303..845fed831c47 100644 > --- a/StandaloneMmPkg/Core/StandaloneMmCore.inf > +++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf > @@ -76,6 +76,9 @@ [Guids] > gEfiEventExitBootServicesGuid > gEfiEventReadyToBootGuid > > +[Pcd] > + gStandaloneMmPkgTokenSpaceGuid.PcdRestartMmDispatcherOnceMmEntryRegist= ered > + > # > # This configuration fails for CLANGPDB, which does not support PIE in t= he GCC > # sense. Such however is required for ARM family StandaloneMmCore > diff --git a/StandaloneMmPkg/StandaloneMmPkg.dec b/StandaloneMmPkg/Standa= loneMmPkg.dec > index 46784d94e421..bb4d1520f7d9 100644 > --- a/StandaloneMmPkg/StandaloneMmPkg.dec > +++ b/StandaloneMmPkg/StandaloneMmPkg.dec > @@ -48,3 +48,9 @@ [Guids] > gEfiStandaloneMmNonSecureBufferGuid =3D { 0xf00497e3, 0xbfa2, 0x4= 1a1, { 0x9d, 0x29, 0x54, 0xc2, 0xe9, 0x37, 0x21, 0xc5 }} > gEfiArmTfCpuDriverEpDescriptorGuid =3D { 0x6ecbd5a1, 0xc0f8, 0x4= 702, { 0x83, 0x01, 0x4f, 0xc2, 0xc5, 0x47, 0x0a, 0x51 }} > > +[PcdsFeatureFlag] > + ## Indicates if restart MM Dispatcher once MM Entry Point is registere= d.

> + # TRUE - Restart MM Dispatcher once MM Entry Point is registered. > + # FALSE - Do not restart MM Dispatcher once MM Entry Point is regist= ered.
> + # @Prompt Restart MM Dispatcher once MM Entry Point is registered. > + gStandaloneMmPkgTokenSpaceGuid.PcdRestartMmDispatcherOnceMmEntryRegist= ered|FALSE|BOOLEAN|0x00000001 (1) This patch more or less undoes (reverts) Ard's earlier commit 84249babd703 ("StandaloneMmPkg/Core: dispatch all drivers at init time", 2019-03-11), which was patch#7 in his series [edk2] [PATCH 00/10] StandaloneMmPkg, ArmPkg: cleanups and improvements http://mid.mail-archive.com/20190305133248.4828-1-ard.biesheuvel@linaro.o= rg The revert is, in my opinion, technically correct, with the addition of the new Feature PCD (and compensating for contextual changes). *However*, the commit message makes no reference to commit 84249babd703. (Side comment: I found out about this being effectively a revert the following way. I noticed that this patch didn't add a *declaration* for the function MmDriverDispatchHandler(). Git-blame then showed me that the declaration survived from original commit 6b46d77243e0 ("StandaloneMmPkg/Core: Implementation of Standalone MM Core Module.", 2018-07-20). However, that commit also had a *definition* for the function -- so why don't we have it today? And then git-log led me to Ard's commit 84249babd703. The commit didn't remove the declaration, which was likely a small omission/oversight; that's why we have the declaration today.) There *is* a technical difference compared to a faithful revert of commit 84249babd703 (i.e., the patch does not restore the pre-84249babd703 state entirely faithfully), but I'll come to that soon. (2) Looking at other patches in Ard's original series, I've found commit 094c0bc7d7a5 ("StandaloneMmPkg/Core: drop support for dispatching FVs into MM", 2019-03-11). It's a different topic, but for cleaning up StandaloneMmPkg, I think we should remove an artifact that commit 094c0bc7d7a5 *unreferenced*, and is therefore no longer used: namely "gMmFvDispatchGuid". It would be nice to include a patch for removing that. (3) Most importantly, speaking to a larger context, I don't understand how this patch can work *at all*. Namely, I can find no MM IPL inside edk2! The DXE and MM dispatcher are supposed to work together in the following way: (3.1) Whenever the DXE Core signals the PI-defined event group EFI_EVENT_GROUP_DXE_DISPATCH_GUID, the MM IPL takes notice. (The MM IPL learns that the DXE Dispatcher has completed one round of dispatching, and new DXE/UEFI protocols may have become available.) (3.2) The MM IPL notifies the MM Core to run one round of MM dispatch. This gives another chance to those MM drivers that failed to launch previously due to missing DXE/UEFI protocols (which they might want to consume in their entry points). The notification happens via an MMI / a particular communication buffer carrying EFI_EVENT_GROUP_DXE_DISPATCH_GUID in the header. (3.3) The MM Core runs said one round of dispatch, and then *informs* the MM IPL about the result. The result can be one of three cases: success, error, and "restart". (3.4) As long as the result is "restart" (for *whatever* reason), the MM IPL doesn't complete the notification function for EFI_EVENT_GROUP_DXE_DISPATCH_GUID, but jumps back to step (3.2). In practice, this is used for handling the situation described in the commit message -- namely, if the MM Core notices that the MM Entry Point was installed in the last round of MM dispatch, then it exits early back to the MM IPL with status "restart". The subsequent MM Dispatch run gives a chance to those MM drivers that needed access to Management Mode (or perhaps MM RAM). So in effect this is an "inner" re-iteration that aims at noticing the MM Entry Point, instead of new DXE/UEFI protocols. But here's why this pattern breaks down (two reasons): - While this pattern is implemented well in "MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c", function SmmIplDxeDispatchEventNotify(), there is *no* MM IPL in edk2. We simply don't seem to have an upstream implementation for the module that implements steps (3.1), (3.2) and (3.4). In particular, nothing *consumes* the result of the MM Dispatch round. - This is where I'm coming to the problem with the current patch. The current patch does not restore the logic from before commit 84249babd703 where MmDriverDispatchHandler() (i) noticed that MmDispatcher() returned EFI_NOT_READY, and (ii) propagated that fact to the MM_IPL with COMM_BUFFER_MM_DISPATCH_RESTART. In other words, with the present patch, *even if* the MM dispatcher notices that the MM Entry Point has just been installed, and even if we have some out-of-tree MM IPL module, the MM IPL will never know that it has to request another iteration of MM dispatch with an MMI, because it will not receive COMM_BUFFER_MM_DISPATCH_RESTART. It will only receive "success" or "failure". ** Summary: - not returning COMM_BUFFER_MM_DISPATCH_RESTART is a bug (how was the patch tested?) - not having an upstream / open source MM IPL makes this patch untestable -- perhaps even nonsensical! - the commit message does not reference commit 84249babd703 - cleaning up "gMmFvDispatchGuid" (independently) would be nice. Thanks Laszlo -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111601): https://edk2.groups.io/g/devel/message/111601 Mute This Topic: https://groups.io/mt/102703852/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-