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

["Laszlo Ersek" <lersek@redhat.com>]

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.PcdRestartMmDispatcherOnceMmEntryRegistered
> +
>  #
>  # 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.PcdRestartMmDispatcherOnceMmEntryRegistered|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?)

- 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



-=-=-=-=-=-=-=-=-=-=-=-
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/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-