* [edk2-devel] [PATCH 0/1] StandaloneMmPkg/Core: Restart dispatcher once MmEntryPoint is registered
@ 2023-11-20 8:30 Xu, Wei6
2023-11-20 8:30 ` [edk2-devel] [PATCH 1/1] " Xu, Wei6
0 siblings, 1 reply; 9+ messages in thread
From: Xu, Wei6 @ 2023-11-20 8:30 UTC (permalink / raw)
To: devel; +Cc: Wei6 Xu, Ard Biesheuvel, Sami Mujawar, Ray Ni
This patch is to add support to StandaloneMmCore for restarting MmDispatcher once MmEntryPoint is registered.
Please check in detail in Bugzilla and PullRequest:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4599
PR: https://github.com/tianocore/edk2/pull/5056
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Ray Ni <ray.ni@intel.com>
Wei6 Xu (1):
StandaloneMmPkg/Core: Restart dispatcher once MmEntryPoint is
registered
StandaloneMmPkg/Core/Dispatcher.c | 76 +++++++++++++++++++++++
StandaloneMmPkg/Core/StandaloneMmCore.c | 1 +
StandaloneMmPkg/Core/StandaloneMmCore.inf | 3 +
StandaloneMmPkg/StandaloneMmPkg.dec | 6 ++
4 files changed, 86 insertions(+)
--
2.29.2.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111463): https://edk2.groups.io/g/devel/message/111463
Mute This Topic: https://groups.io/mt/102703850/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* [edk2-devel] [PATCH 1/1] StandaloneMmPkg/Core: Restart dispatcher once MmEntryPoint is registered
2023-11-20 8:30 [edk2-devel] [PATCH 0/1] StandaloneMmPkg/Core: Restart dispatcher once MmEntryPoint is registered Xu, Wei6
@ 2023-11-20 8:30 ` Xu, Wei6
2023-11-22 11:45 ` Laszlo Ersek
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Xu, Wei6 @ 2023-11-20 8:30 UTC (permalink / raw)
To: devel; +Cc: Wei6 Xu, Ard Biesheuvel, Sami Mujawar, Ray Ni
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
--
2.29.2.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111464): https://edk2.groups.io/g/devel/message/111464
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] StandaloneMmPkg/Core: Restart dispatcher once MmEntryPoint is registered
2023-11-20 8:30 ` [edk2-devel] [PATCH 1/1] " Xu, Wei6
@ 2023-11-22 11:45 ` Laszlo Ersek
2023-11-22 15:11 ` Ard Biesheuvel
2023-11-23 16:20 ` Xu, Wei6
2023-11-23 1:48 ` Ni, Ray
2023-11-27 12:15 ` Wu, Jiaxin
2 siblings, 2 replies; 9+ messages in thread
From: Laszlo Ersek @ 2023-11-22 11:45 UTC (permalink / raw)
To: devel, wei6.xu; +Cc: Ard Biesheuvel, Sami Mujawar, Ray Ni
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] StandaloneMmPkg/Core: Restart dispatcher once MmEntryPoint is registered
2023-11-22 11:45 ` Laszlo Ersek
@ 2023-11-22 15:11 ` Ard Biesheuvel
2023-11-24 11:06 ` Laszlo Ersek
2023-11-23 16:20 ` Xu, Wei6
1 sibling, 1 reply; 9+ messages in thread
From: Ard Biesheuvel @ 2023-11-22 15:11 UTC (permalink / raw)
To: devel, lersek; +Cc: wei6.xu, Ard Biesheuvel, Sami Mujawar, Ray Ni
On Wed, 22 Nov 2023 at 12:45, Laszlo Ersek <lersek@redhat.com> wrote:
>
...
> (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.
>
The above describes traditional MM but not standalone MM. I would have
to refer to the PI spec for details, but the primary difference is
that SMM drivers have no access to the DXE protocol database or boot
services at all (hence the name 'standalone'). The 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. Notably, the SMM model where the initial
context is unified and only separated later in the boot (at EndOfDxe?)
does not apply here. This also means that dispatching FVs into
standalone MM and retaining other SMM legacy is meaningless, which is
why I removed all that code.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111606): https://edk2.groups.io/g/devel/message/111606
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] StandaloneMmPkg/Core: Restart dispatcher once MmEntryPoint is registered
2023-11-20 8:30 ` [edk2-devel] [PATCH 1/1] " Xu, Wei6
2023-11-22 11:45 ` Laszlo Ersek
@ 2023-11-23 1:48 ` Ni, Ray
2023-11-27 12:15 ` Wu, Jiaxin
2 siblings, 0 replies; 9+ messages in thread
From: Ni, Ray @ 2023-11-23 1:48 UTC (permalink / raw)
To: Xu, Wei6, devel@edk2.groups.io; +Cc: Ard Biesheuvel, Sami Mujawar
> + gRequestDispatch = TRUE;
> + gDispatcherRunning = FALSE;
1. It's good to set RequestDispatch to TRUE as it exits unconditionally and very likely more drivers can be dispatched later.
2. Since StandaloneMmCore registers a SMI handler, it's possible that some SMM driver entry calls MmiManage() to explicitly
request to dispatch. DispatcherRunning is a flag to avoid re-entrance. It's good.
> /**
> 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 },
3. I assume the StandaloneMmIpl won't respond to the DxeDispatch event signaled from DxeCore. Right?
> +[PcdsFeatureFlag]
4. Can we use [PcdsFeatureFlag.IA32, PcdsFeatureFlags.X64] to make sure X86 CPU always have the PCD as "TRUE"?
This could avoid possible platform DSC mis-configuration that causes all MM drivers dispatched in non-SMM mode.
> + ## 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.PcdRestartMmDispatcherOnceMmEntry
> Registered|FALSE|BOOLEAN|0x00000001
> --
> 2.29.2.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111640): https://edk2.groups.io/g/devel/message/111640
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] StandaloneMmPkg/Core: Restart dispatcher once MmEntryPoint is registered
2023-11-22 11:45 ` Laszlo Ersek
2023-11-22 15:11 ` Ard Biesheuvel
@ 2023-11-23 16:20 ` Xu, Wei6
2023-11-24 11:13 ` Laszlo Ersek
1 sibling, 1 reply; 9+ messages in thread
From: Xu, Wei6 @ 2023-11-23 16:20 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io; +Cc: Ard Biesheuvel, Sami Mujawar, Ni, Ray
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] StandaloneMmPkg/Core: Restart dispatcher once MmEntryPoint is registered
2023-11-22 15:11 ` Ard Biesheuvel
@ 2023-11-24 11:06 ` Laszlo Ersek
0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2023-11-24 11:06 UTC (permalink / raw)
To: Ard Biesheuvel, devel; +Cc: wei6.xu, Ard Biesheuvel, Sami Mujawar, Ray Ni
On 11/22/23 16:11, Ard Biesheuvel wrote:
> On Wed, 22 Nov 2023 at 12:45, Laszlo Ersek <lersek@redhat.com> wrote:
>>
> ...
>> (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.
>>
>
> The above describes traditional MM but not standalone MM. I would have
> to refer to the PI spec for details, but the primary difference is
> that SMM drivers have no access to the DXE protocol database or boot
> services at all (hence the name 'standalone'). The 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. Notably, the SMM model where the initial
> context is unified and only separated later in the boot (at EndOfDxe?)
> does not apply here. This also means that dispatching FVs into
> standalone MM and retaining other SMM legacy is meaningless, which is
> why I removed all that code.
Right, this is what I suspected. It didn't seem like your code removal
was an incidental simplification; my understanding (or more like:
guesswork) of "Standalone" in "StandaloneMmPkg" was that there was *no*
MM "IPL" to talk back to.
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111696): https://edk2.groups.io/g/devel/message/111696
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] StandaloneMmPkg/Core: Restart dispatcher once MmEntryPoint is registered
2023-11-23 16:20 ` Xu, Wei6
@ 2023-11-24 11:13 ` Laszlo Ersek
0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2023-11-24 11:13 UTC (permalink / raw)
To: Xu, Wei6, devel@edk2.groups.io; +Cc: Ard Biesheuvel, Sami Mujawar, Ni, Ray
On 11/23/23 17:20, Xu, Wei6 wrote:
> 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.
- 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?
>
>>
>> - 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.
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.
>
>>
>> - 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 for that!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
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/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] StandaloneMmPkg/Core: Restart dispatcher once MmEntryPoint is registered
2023-11-20 8:30 ` [edk2-devel] [PATCH 1/1] " Xu, Wei6
2023-11-22 11:45 ` Laszlo Ersek
2023-11-23 1:48 ` Ni, Ray
@ 2023-11-27 12:15 ` Wu, Jiaxin
2 siblings, 0 replies; 9+ messages in thread
From: Wu, Jiaxin @ 2023-11-27 12:15 UTC (permalink / raw)
To: devel@edk2.groups.io, Xu, Wei6; +Cc: Ard Biesheuvel, Sami Mujawar, Ni, Ray
> //
> MM_CORE_MMI_HANDLERS mMmCoreMmiHandlers[] = {
> + { MmDriverDispatchHandler, &gEfiEventDxeDispatchGuid, NULL,
> TRUE },
Should we need define a new HandlerType name instead of using the gEfiEventDxeDispatchGuid?
We just need a simple MM Dispatch Event, which is trigged by IPL to dispatch again after StandaloneMmCore returns. Not depends on dxe dispatch event.
And besides, should we update & correct the below code comments?
///
/// Define values for the communications buffer used when gEfiEventDxeDispatchGuid is
/// event signaled. This event is signaled by the DXE Core each time the DXE Core
/// dispatcher has completed its work. When this event is signaled, the MM Core
/// if notified, so the MM Core can dispatch MM drivers. If COMM_BUFFER_MM_DISPATCH_ERROR
/// is returned in the communication buffer, then an error occurred dispatching MM
/// Drivers. If COMM_BUFFER_MM_DISPATCH_SUCCESS is returned, then the MM Core
/// dispatched all the drivers it could. If COMM_BUFFER_MM_DISPATCH_RESTART is
/// returned, then the MM Core just dispatched the MM Driver that registered
/// the MM Entry Point enabling the use of MM Mode. In this case, the MM Core
/// should be notified again to dispatch more MM Drivers using MM Mode.
///
#define COMM_BUFFER_MM_DISPATCH_ERROR 0x00
#define COMM_BUFFER_MM_DISPATCH_SUCCESS 0x01
#define COMM_BUFFER_MM_DISPATCH_RESTART 0x02
Thanks,
Jiaxin
> { 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.PcdRestartMmDispatcherOnceMmEntry
> Registered
> +
> #
> # 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.PcdRestartMmDispatcherOnceMmEntry
> Registered|FALSE|BOOLEAN|0x00000001
> --
> 2.29.2.windows.2
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111735): https://edk2.groups.io/g/devel/message/111735
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-11-27 12:15 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-20 8:30 [edk2-devel] [PATCH 0/1] StandaloneMmPkg/Core: Restart dispatcher once MmEntryPoint is registered Xu, Wei6
2023-11-20 8:30 ` [edk2-devel] [PATCH 1/1] " Xu, Wei6
2023-11-22 11:45 ` Laszlo Ersek
2023-11-22 15:11 ` Ard Biesheuvel
2023-11-24 11:06 ` Laszlo Ersek
2023-11-23 16:20 ` Xu, Wei6
2023-11-24 11:13 ` Laszlo Ersek
2023-11-23 1:48 ` Ni, Ray
2023-11-27 12:15 ` Wu, Jiaxin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox