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 8EC307803D2 for ; Fri, 24 Nov 2023 11:13:43 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=+ihm5qGebAzz+KBmRAgWJxxxHk3zcitBYRJP6FIbawM=; 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=1700824422; v=1; b=eyyCSaABn7uHzp2aFEFPpeZiXy5d7+bxu2TcMz8j/I3smnpzynlEUs180CEmtXAquR6JPzvy O8/2nhqN2BoMkTg/nr6gszNsbxcYpMfbefj1Hrau4jSz1PaMC8PwLm0YFp8wH/auOulrhH/xqBp f89UfNdFVIhPVU1PXycJLp/o= X-Received: by 127.0.0.2 with SMTP id 8UXJYY7687511xKYUjAnunnE; Fri, 24 Nov 2023 03:13:42 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.133172.1700824421422100415 for ; Fri, 24 Nov 2023 03:13:41 -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-63--8KKf6djOCOM7nGEFe3hIQ-1; Fri, 24 Nov 2023 06:13:36 -0500 X-MC-Unique: -8KKf6djOCOM7nGEFe3hIQ-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 9FCD3828CEE; Fri, 24 Nov 2023 11:13:35 +0000 (UTC) X-Received: from [10.39.192.172] (unknown [10.39.192.172]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4763C492BE7; Fri, 24 Nov 2023 11:13:33 +0000 (UTC) Message-ID: <6cda52f8-8ce7-fd77-0aca-960807fd6782@redhat.com> Date: Fri, 24 Nov 2023 12:13:32 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/1] StandaloneMmPkg/Core: Restart dispatcher once MmEntryPoint is registered To: "Xu, Wei6" , "devel@edk2.groups.io" Cc: Ard Biesheuvel , Sami Mujawar , "Ni, Ray" References: <7614875b-fb11-8b2c-1411-da0b5c1224b3@redhat.com> 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: V897DZDw6y1sJF1kQmQYONWqx7686176AA= 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=eyyCSaAB; 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/23/23 17:20, Xu, Wei6 wrote: > Thanks a lot for the review. Replied inline. >=20 > BR, > Wei >=20 >> -----Original Message----- >> From: Laszlo Ersek >> Sent: Wednesday, November 22, 2023 7:46 PM >> To: devel@edk2.groups.io; Xu, Wei6 >> Cc: Ard Biesheuvel ; Sami Mujawar >> ; Ni, Ray >> Subject: Re: [edk2-devel] [PATCH 1/1] StandaloneMmPkg/Core: Restart >> dispatcher once MmEntryPoint is registered >> >> 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 registere= d. >>> 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/Dispatcher.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 po= int >>> + // >>> + PreviousMmEntryPointRegistered =3D >>> + gMmCorePrivate->MmEntryPointRegistered; >>> + >>> // >>> // 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- >>> MmEntryPointRegistered) { >>> + if (FeaturePcdGet (PcdRestartMmDispatcherOnceMmEntryRegistered= )) >> { >>> + // >>> + // Return immediately if the MM Entry Point was registered b= y the >> MM >>> + // Driver that was just dispatched. The MM IPL will reinvoke= the MM >>> + // Core Dispatcher. This is required so MM Mode may be enabl= ed as >> soon >>> + // as all the dependent MM Drivers for MM Mode have been >> dispatched. >>> + // Once the MM Entry Point has been registered, then MM Mode= will >> 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 >> discovered MM drivers. >>> + >>> + @param[in] DispatchHandle The unique handle assigned to this >> handler by MmiHandlerRegister(). >>> + @param[in] Context Points to an optional handler conte= xt which >> was specified when the >>> + handler was registered. >>> + @param[in, out] CommBuffer A pointer to a collection of data i= n >> memory that will >>> + be conveyed from a non-MM environme= nt 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 withou= t >> errors >>> + // >>> + *(UINT8 *)CommBuffer =3D COMM_BUFFER_MM_DISPATCH_SUCCESS; >>> + } else { >>> + // >>> + // Set the flag to show that the MM Dispatcher encountered an = error >>> + // >>> + *(UINT8 *)CommBuffer =3D COMM_BUFFER_MM_DISPATCH_ERROR; >>> + } >>> + } >>> + } >>> + >>> + return EFI_SUCCESS; >>> +} >>> + >>> /** >>> Traverse the discovered list for any drivers that were discovered bu= t not >> loaded >>> because the dependency expressions evaluated to false. >>> diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.c >>> b/StandaloneMmPkg/Core/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 initialized >>> // MM_CORE_MMI_HANDLERS mMmCoreMmiHandlers[] =3D { >>> + { MmDriverDispatchHandler, &gEfiEventDxeDispatchGuid, NULL, >> TRUE }, >>> { MmReadyToLockHandler, &gEfiDxeMmReadyToLockProtocolGuid, >> NULL, TRUE }, >>> { MmEndOfDxeHandler, &gEfiEndOfDxeEventGroupGuid, NULL, >> FALSE }, >>> { MmExitBootServiceHandler, &gEfiEventExitBootServicesGuid, NULL, >> FALSE }, >>> 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.PcdRestartMmDispatcherOnceMmEntr >> yRegis >>> +tered >>> + >>> # >>> # This configuration fails for CLANGPDB, which does not support PIE >>> in the GCC # sense. Such however is required for ARM family >>> StandaloneMmCore diff --git a/StandaloneMmPkg/StandaloneMmPkg.dec >>> b/StandaloneMmPkg/StandaloneMmPkg.dec >>> index 46784d94e421..bb4d1520f7d9 100644 >>> --- a/StandaloneMmPkg/StandaloneMmPkg.dec >>> +++ b/StandaloneMmPkg/StandaloneMmPkg.dec >>> @@ -48,3 +48,9 @@ [Guids] >>> gEfiStandaloneMmNonSecureBufferGuid =3D { 0xf00497e3, 0xbfa2, 0= x41a1, >> { 0x9d, 0x29, 0x54, 0xc2, 0xe9, 0x37, 0x21, 0xc5 }} >>> gEfiArmTfCpuDriverEpDescriptorGuid =3D { 0x6ecbd5a1, 0xc0f8, 0= x4702, >> { 0x83, 0x01, 0x4f, 0xc2, 0xc5, 0x47, 0x0a, 0x51 }} >>> >>> +[PcdsFeatureFlag] >>> + ## Indicates if restart MM Dispatcher once MM Entry Point is >> registered.

>>> + # TRUE - Restart MM Dispatcher once MM Entry Point is registered.=
>>> + # FALSE - Do not restart MM Dispatcher once MM Entry Point is >> registered.
>>> + # @Prompt Restart MM Dispatcher once MM Entry Point is registered. >>> + >>> >> +gStandaloneMmPkgTokenSpaceGuid.PcdRestartMmDispatcherOnceMmEntr >> yRegis >>> +tered|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.org >> >> 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 fol= lowing >> way. I noticed that this patch didn't add a *declaration* for the functi= on >> 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 Ar= d's >> commit 84249babd703. The commit didn't remove the declaration, which was >> likely a small omission/oversight; that's why we have the declaration to= day.) >> >> There *is* a technical difference compared to a faithful revert of commi= t >> 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 t= o >> include a patch for removing that. >> >> >> (3) Most importantly, speaking to a larger context, I don't understand h= ow 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 prev= iously >> 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* th= e >> 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 co= mmit >> 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 cu= rrent >> 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 noti= ces >> that the MM Entry Point has just been installed, and even if we have som= e >> 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 no= t >> 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?) >> >=20 > It's different from the PiSmmCore's dispatch. StandaloneMmCore's dispatch= will not hook to the EFI_EVENT_GROUP_DXE_DISPATCH_GUID and there will be o= nly two rounds of dispatch. > *Round 1* is the StandaloneMmCore calls MmDispatcher() in its entry point= . Once the CPU driver is dispatched, MmDispatcher() stops the dispatch. Sta= ndaloneMmCore continues the remaining job and returns to the MM IPL. Then t= he IPL will immediately trigger the MmDriverDispatchHandler() to dispatch t= he remaining drivers, which is *Round 2*. > As the CPU driver is dispatched in the *Round 1*, not dispatched by MmDri= verDispatchHandler(), COMM_BUFFER_MM_DISPATCH_RESTART will not happen in Mm= DriverDispatchHandler(). Therefore the COMM_BUFFER_MM_DISPATCH_RESTART is = removed. >=20 > Sorry for that I didn't explain the dispatch flow clearly in the commit m= essage. I will add it in Patch V2. - Yes, please, include the design in more details. - If you have two static rounds, then does it even make sense to distinguish COMM_BUFFER_MM_DISPATCH_SUCCESS from COMM_BUFFER_MM_DISPATCH_ERROR, as the outcome of round #1? >=20 >> >> - not having an upstream / open source MM IPL makes this patch untestabl= e - >> - perhaps even nonsensical! >> >=20 > We have the MM IPL in our close source and we are also going to upstream = it. The plan is early Q1,2024. On one hand, this is good (i.e., your upstreaming intent). On the other hand, I have two counter-arguments: (a) if you standardize an MM IPL (simply by upstreaming it, and by *defining* an interface between the MM IPL and the MM Core), then that *act* takes the "standalone" out of "StandaloneMmPkg". As Ard describes elsewhere in this thread, "[t]he standalone MM is a separate execution context that comes into existence magically (i.e., in a way not defined by PI/UEFI) and can be invoked only via special traps or instructions". And it seems like that particular criterion will no longer apply. I'm not against this work, but I need to point out that the initial design is *shifting*, and that always runs the risk of creating a big mess down the road. In this instance, the *beginnings* of that are shown by the introduction of a Feature PCD that *truly* stand-alone MM cores do not need. I feel we should somehow adjust the naming, so that it reflect this new MM IPL concept better. Again, IMO it's not stand-alone any longer. (b) I'd prefer if this code were upstreamed together with the MM IPL, in Q1 2024. That would keep both sides of the interface close to each other. >=20 >> >> - the commit message does not reference commit 84249babd703 >> >=20 > Will mention the commit 84249babd703 and explain the difference in patch = v2. >=20 >> >> - cleaning up "gMmFvDispatchGuid" (independently) would be nice. >> >=20 > Yes, we also have plan to clean up the unused content in StandaloneMmCore= . Will do it within the 'clean up' task in future. Thanks for that! 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 (#111699): https://edk2.groups.io/g/devel/message/111699 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-