Re: [edk2-devel] [PATCH 1/1] StandaloneMmPkg/Core: Restart dispatcher once MmEntryPoint is registered

["Xu, Wei6" <wei6.xu@intel.com>]

Thanks a lot for the review. Replied inline.

BR,
Wei

>-----Original Message-----
>From: Laszlo Ersek <lersek@redhat.com>
>Sent: Wednesday, November 22, 2023 7:46 PM
>To: devel@edk2.groups.io; Xu, Wei6 <wei6.xu@intel.com>
>Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar
><sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>
>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=4599
>>
>> 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 <ardb+tianocore@kernel.org>
>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Signed-off-by: Wei6 Xu <wei6.xu@intel.com>
>> ---
>>  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 = TRUE;
>>        RemoveEntryList (&DriverEntry->ScheduledLink);
>>
>> +      //
>> +      // Cache state of MmEntryPointRegistered before calling entry point
>> +      //
>> +      PreviousMmEntryPointRegistered =
>> + 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 by the
>MM
>> +          // Driver that was just dispatched. 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.
>> +          // Once the MM Entry Point has been registered, then MM Mode will
>be
>> +          // used.
>> +          //
>> +          gRequestDispatch   = TRUE;
>> +          gDispatcherRunning = 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 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 = MmDispatcher ();
>> +
>> +  //
>> +  // Check to see if CommBuffer and CommBufferSize are valid  //  if
>> + ((CommBuffer != NULL) && (CommBufferSize != NULL)) {
>> +    if (*CommBufferSize > 0) {
>> +      if (!EFI_ERROR (Status)) {
>> +        //
>> +        // Set the flag to show that the MM Dispatcher executed without
>errors
>> +        //
>> +        *(UINT8 *)CommBuffer = COMM_BUFFER_MM_DISPATCH_SUCCESS;
>> +      } else {
>> +        //
>> +        // Set the flag to show that the MM Dispatcher encountered an error
>> +        //
>> +        *(UINT8 *)CommBuffer = 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/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 = {  // Table
>of
>> MMI Handlers that are registered by the MM Core when it is initialized
>> //  MM_CORE_MMI_HANDLERS  mMmCoreMmiHandlers[] = {
>> +  { 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      = { 0xf00497e3, 0xbfa2, 0x41a1,
>{ 0x9d, 0x29, 0x54, 0xc2, 0xe9, 0x37, 0x21, 0xc5 }}
>>    gEfiArmTfCpuDriverEpDescriptorGuid       = { 0x6ecbd5a1, 0xc0f8, 0x4702,
>{ 0x83, 0x01, 0x4f, 0xc2, 0xc5, 0x47, 0x0a, 0x51 }}
>>
>> +[PcdsFeatureFlag]
>> +  ## Indicates if restart MM Dispatcher once MM Entry Point is
>registered.<BR><BR>
>> +  #   TRUE  - Restart MM Dispatcher once MM Entry Point is registered.<BR>
>> +  #   FALSE - Do not restart MM Dispatcher once MM Entry Point is
>registered.<BR>
>> +  # @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 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?)
>

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 only 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. StandaloneMmCore continues the remaining job and returns to the MM IPL. Then the IPL will immediately trigger the MmDriverDispatchHandler() to dispatch the remaining drivers, which is *Round 2*.
As the CPU driver is dispatched in the *Round 1*, not dispatched by MmDriverDispatchHandler(), COMM_BUFFER_MM_DISPATCH_RESTART will not happen in MmDriverDispatchHandler(). Therefore the  COMM_BUFFER_MM_DISPATCH_RESTART is removed.

Sorry for that I didn't explain the dispatch flow clearly in the commit message. I will add it in Patch V2.

>
>- not having an upstream / open source MM IPL makes this patch untestable -
>- perhaps even nonsensical!
>

We have the MM IPL in our close source and we are also going to upstream it. The plan is early Q1,2024.

>
>- the commit message does not reference commit 84249babd703
>

Will mention the commit 84249babd703 and explain the difference in patch v2.

>
>- cleaning up "gMmFvDispatchGuid" (independently) would be nice.
>

Yes, we also have plan to clean up the unused content in StandaloneMmCore. Will do it within the 'clean up' task in future.

>
>Thanks
>Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111685): https://edk2.groups.io/g/devel/message/111685
Mute This Topic: https://groups.io/mt/102703852/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-