Re: [edk2-devel] [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory at EBS if needed
On Thu, 29 Sept 2022 at 20:14, Dionna Amalie Glaze wrote: > > > > > Can you explain a bit more why this PCD is needed? > > > > I'll expand the comment further, but essentially we need a bit in the > firmware to persist from a call into a protocol to the eventual call > to ExitBootServices. If we needed offline persistence, then we'd need > to standardize a new bit in the OsIndications variable. We don't so we > make due with a Pcd. > As I mentioned elsewhere, I'd prefer to avoid a dynamic PCD here. For Ray's benefit, I'll copy/paste my proposal here that I made in one of the other thread: """ Given that dynamic PCDs are optional but protocols are not, can we just drop the PCD and (in view of some other comments below) do the following: - Define a protocol in MdeModulePkg that ExitBootServices() will invoke after disarming the timer but before finalizing the memory map. It doesn't have to be specific to memory acceptance at all, the only thing that matters is that it is invoked at the right time (and perhaps only on the first call to EBS()). E.g., gEdkiiExitBootServicesCallbackProtocol with a single method called TerminateMemoryMapPreHook(). This protocol is fundamentally internal to EDK2 and does not require inclusion in PI or UEFI - Define a protocol in OvmfPkg that will be called by the OS loader to indicate that it is capable of taking charge of the unaccepted memory, and so it does not need the firmware to do so on its behalf, by accepting all of it during ExitBootServices(). - Implement a single DXE driver in OvmfPkg (or add the functionality to an existing one) that provides an implementation of the former protocol, and implements the latter protocol by uninstalling the former again. This means the 'accept all memory' routine can be moved out of DXE core as well, and into your driver. """ > > > > > I am not following the logic here 100%. As far as I can tell, if > > accepting all memory succeeded without errors, ExitBootServices() > > returns with EFI_SUCCESS, even though it has modified the memory map. > > This means the actual memory map is out of sync with the last > > GetMemoryMap() call performed by the OS loader before it called > > ExitBootServices(), and so it will still contain unaccepted memory, > > right? > > Ah yes you're right, I missed that part. I'll change it to return > EFI_WARN_STALE_DATA if any memory region gets accepted, and force a > failure in DxeMain if the status is an error or that warning. > The only documented error code for ExitBootServices() is EFI_INVALID_PARAMETER, which indicates that the map key has changes, so this is the one you should return in this case. EFI_WARN_STALE_DATA is not an error code (it has the MSB cleared) so macros like EFI_ERROR() will not identify it as an error. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#94565): https://edk2.groups.io/g/devel/message/94565 Mute This Topic: https://groups.io/mt/93975251/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory at EBS if needed
> > Can you explain a bit more why this PCD is needed? > I'll expand the comment further, but essentially we need a bit in the firmware to persist from a call into a protocol to the eventual call to ExitBootServices. If we needed offline persistence, then we'd need to standardize a new bit in the OsIndications variable. We don't so we make due with a Pcd. > > I am not following the logic here 100%. As far as I can tell, if > accepting all memory succeeded without errors, ExitBootServices() > returns with EFI_SUCCESS, even though it has modified the memory map. > This means the actual memory map is out of sync with the last > GetMemoryMap() call performed by the OS loader before it called > ExitBootServices(), and so it will still contain unaccepted memory, > right? Ah yes you're right, I missed that part. I'll change it to return EFI_WARN_STALE_DATA if any memory region gets accepted, and force a failure in DxeMain if the status is an error or that warning. -- -Dionna Glaze, PhD (she/her) -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#94527): https://edk2.groups.io/g/devel/message/94527 Mute This Topic: https://groups.io/mt/93975251/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory at EBS if needed
Can you explain a bit more why this PCD is needed?
> -Original Message-
> From: devel@edk2.groups.io On Behalf Of Dionna Glaze
> via groups.io
> Sent: Wednesday, September 28, 2022 11:33 PM
> To: devel@edk2.groups.io
> Cc: Dionna Glaze ; Gerd Hoffmann ;
> James Bottomley
> ; Yao, Jiewen ; Tom Lendacky
> ; Ard
> Biesheuvel
> Subject: [edk2-devel] [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory
> at EBS if needed
>
> With the addition of the EfiUnacceptedMemory memory type, it is possible
> the EFI-enlightened guests do not themselves support the new memory
> type. This commit uses the new PcdEnableUnacceptedMemory to enable
> unaccepted memory support before ExitBootServices is called by not
> accepting all unaccepted memory at EBS.
>
> The expected usage is to set the new Pcd with a protocol that is usable
> by bootloaders and directly-booted OSes when they can determine that the
> OS does indeed support unaccepted memory.
>
> Cc: Gerd Hoffmann
> Cc: James Bottomley
> Cc: Jiewen Yao
> Cc: Tom Lendacky
> Cc: Ard Biesheuvel
>
> Signed-off-by: Dionna Glaze
> ---
> MdeModulePkg/Core/Dxe/DxeMain.h | 10 +++
> MdeModulePkg/Core/Dxe/DxeMain.inf | 2 +
> MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 14 +++-
> MdeModulePkg/Core/Dxe/Mem/Page.c| 87
> 4 files changed, 112 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
> index 815a6b4bd8..ac943c87a3 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> @@ -2698,6 +2698,16 @@ CoreInitializeMemoryProtection (
>VOID
>);
>
> +/**
> + Accept and convert unaccepted memory to conventional memory if unaccepted
> + memory is not enabled and there is an implementation of
> MemoryAcceptProtocol
> + installed.
> + **/
> +EFI_STATUS
> +CoreResolveUnacceptedMemory (
> + VOID
> + );
> +
> /**
>Install MemoryAttributesTable on memory allocation.
>
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index e4bca89577..deb8bb2ba8 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> @@ -153,6 +153,7 @@
>gEfiHiiPackageListProtocolGuid## SOMETIMES_PRODUCES
>gEfiSmmBase2ProtocolGuid ## SOMETIMES_CONSUMES
>gEdkiiPeCoffImageEmulatorProtocolGuid ## SOMETIMES_CONSUMES
> + gEfiMemoryAcceptProtocolGuid ## SOMETIMES_CONSUMES
>
># Arch Protocols
>gEfiBdsArchProtocolGuid ## CONSUMES
> @@ -186,6 +187,7 @@
>gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
> ## CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
> ## CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth
> ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory
> ## CONSUMES
>
> # [Hob]
> # RESOURCE_DESCRIPTOR ## CONSUMES
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> index 5733f0c8ec..8d1de32fe7 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> @@ -768,13 +768,25 @@ CoreExitBootServices (
>//
>gTimer->SetTimerPeriod (gTimer, 0);
>
> + //
> + // Accept all memory if unaccepted memory isn't enabled.
> + //
> + Status = CoreResolveUnacceptedMemory();
> + if (EFI_ERROR (Status)) {
> +//
> +// Notify other drivers that ExitBootServices failed
> +//
> +CoreNotifySignalList (&gEventExitBootServicesFailedGuid);
> +return Status;
> + }
> +
>//
>// Terminate memory services if the MapKey matches
>//
>Status = CoreTerminateMemoryMap (MapKey);
>if (EFI_ERROR (Status)) {
> //
> -// Notify other drivers that ExitBootServices fail
> +// Notify other drivers that ExitBootServices failed
> //
> CoreNotifySignalList (&gEventExitBootServicesFailedGuid);
> return Status;
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index ffe79dcca9..cbebe62a28 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -9,6 +9,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include "DxeMain.h"
> #include "Imem.h"
> #include "HeapGuard.h"
> +#include
> +#include
>
> //
> // Entry for tracking the memory regions for each memory ty
Re: [edk2-devel] [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory at EBS if needed
On Wed, 28 Sept 2022 at 17:33, Dionna Glaze wrote:
>
> With the addition of the EfiUnacceptedMemory memory type, it is possible
> the EFI-enlightened guests do not themselves support the new memory
> type. This commit uses the new PcdEnableUnacceptedMemory to enable
> unaccepted memory support before ExitBootServices is called by not
> accepting all unaccepted memory at EBS.
>
> The expected usage is to set the new Pcd with a protocol that is usable
> by bootloaders and directly-booted OSes when they can determine that the
> OS does indeed support unaccepted memory.
>
> Cc: Gerd Hoffmann
> Cc: James Bottomley
> Cc: Jiewen Yao
> Cc: Tom Lendacky
> Cc: Ard Biesheuvel
>
> Signed-off-by: Dionna Glaze
> ---
> MdeModulePkg/Core/Dxe/DxeMain.h | 10 +++
> MdeModulePkg/Core/Dxe/DxeMain.inf | 2 +
> MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 14 +++-
> MdeModulePkg/Core/Dxe/Mem/Page.c| 87
> 4 files changed, 112 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
> index 815a6b4bd8..ac943c87a3 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> @@ -2698,6 +2698,16 @@ CoreInitializeMemoryProtection (
>VOID
>);
>
> +/**
> + Accept and convert unaccepted memory to conventional memory if unaccepted
> + memory is not enabled and there is an implementation of
> MemoryAcceptProtocol
> + installed.
> + **/
> +EFI_STATUS
> +CoreResolveUnacceptedMemory (
> + VOID
> + );
> +
> /**
>Install MemoryAttributesTable on memory allocation.
>
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index e4bca89577..deb8bb2ba8 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> @@ -153,6 +153,7 @@
>gEfiHiiPackageListProtocolGuid## SOMETIMES_PRODUCES
>gEfiSmmBase2ProtocolGuid ## SOMETIMES_CONSUMES
>gEdkiiPeCoffImageEmulatorProtocolGuid ## SOMETIMES_CONSUMES
> + gEfiMemoryAcceptProtocolGuid ## SOMETIMES_CONSUMES
>
># Arch Protocols
>gEfiBdsArchProtocolGuid ## CONSUMES
> @@ -186,6 +187,7 @@
>gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
> ## CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
> ## CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth
> ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory
> ## CONSUMES
>
> # [Hob]
> # RESOURCE_DESCRIPTOR ## CONSUMES
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> index 5733f0c8ec..8d1de32fe7 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> @@ -768,13 +768,25 @@ CoreExitBootServices (
>//
>gTimer->SetTimerPeriod (gTimer, 0);
>
> + //
> + // Accept all memory if unaccepted memory isn't enabled.
> + //
> + Status = CoreResolveUnacceptedMemory();
> + if (EFI_ERROR (Status)) {
> +//
> +// Notify other drivers that ExitBootServices failed
> +//
> +CoreNotifySignalList (&gEventExitBootServicesFailedGuid);
> +return Status;
> + }
> +
>//
>// Terminate memory services if the MapKey matches
>//
>Status = CoreTerminateMemoryMap (MapKey);
>if (EFI_ERROR (Status)) {
> //
> -// Notify other drivers that ExitBootServices fail
> +// Notify other drivers that ExitBootServices failed
> //
> CoreNotifySignalList (&gEventExitBootServicesFailedGuid);
> return Status;
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index ffe79dcca9..cbebe62a28 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -9,6 +9,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include "DxeMain.h"
> #include "Imem.h"
> #include "HeapGuard.h"
> +#include
> +#include
>
> //
> // Entry for tracking the memory regions for each memory type to coalesce
> similar memory types
> @@ -2118,6 +2120,91 @@ CoreFreePoolPages (
>CoreConvertPages (Memory, NumberOfPages, EfiConventionalMemory);
> }
>
> +EFI_EVENT gExitBootServiceEvent = NULL;
> +
> +STATIC
> +EFI_STATUS
> +AcceptAllUnacceptedMemory (
> + IN EFI_MEMORY_ACCEPT_PROTOCOL *AcceptMemory
> + )
> +{
> + EFI_GCD_MEMORY_SPACE_DESCRIPTOR *AllDescMap;
> + UINTNNumEntries;
> + UINTNIndex;
> + EFI_STATUS Status;
> +
> + /*
> + * Get a copy of the memory space map to iterate over while
> + * changing the map.
> + */
> + Status = CoreGetMemorySpaceMap (&NumEntries, &AllDescMap);
> + if (EFI_ERROR (Status)) {
> +return Status;
> + }
> + for (Index = 0; Index < NumEntries; Index++) {
> +CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Desc
[edk2-devel] [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory at EBS if needed
With the addition of the EfiUnacceptedMemory memory type, it is possible
the EFI-enlightened guests do not themselves support the new memory
type. This commit uses the new PcdEnableUnacceptedMemory to enable
unaccepted memory support before ExitBootServices is called by not
accepting all unaccepted memory at EBS.
The expected usage is to set the new Pcd with a protocol that is usable
by bootloaders and directly-booted OSes when they can determine that the
OS does indeed support unaccepted memory.
Cc: Gerd Hoffmann
Cc: James Bottomley
Cc: Jiewen Yao
Cc: Tom Lendacky
Cc: Ard Biesheuvel
Signed-off-by: Dionna Glaze
---
MdeModulePkg/Core/Dxe/DxeMain.h | 10 +++
MdeModulePkg/Core/Dxe/DxeMain.inf | 2 +
MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 14 +++-
MdeModulePkg/Core/Dxe/Mem/Page.c| 87
4 files changed, 112 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index 815a6b4bd8..ac943c87a3 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -2698,6 +2698,16 @@ CoreInitializeMemoryProtection (
VOID
);
+/**
+ Accept and convert unaccepted memory to conventional memory if unaccepted
+ memory is not enabled and there is an implementation of MemoryAcceptProtocol
+ installed.
+ **/
+EFI_STATUS
+CoreResolveUnacceptedMemory (
+ VOID
+ );
+
/**
Install MemoryAttributesTable on memory allocation.
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
b/MdeModulePkg/Core/Dxe/DxeMain.inf
index e4bca89577..deb8bb2ba8 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -153,6 +153,7 @@
gEfiHiiPackageListProtocolGuid## SOMETIMES_PRODUCES
gEfiSmmBase2ProtocolGuid ## SOMETIMES_CONSUMES
gEdkiiPeCoffImageEmulatorProtocolGuid ## SOMETIMES_CONSUMES
+ gEfiMemoryAcceptProtocolGuid ## SOMETIMES_CONSUMES
# Arch Protocols
gEfiBdsArchProtocolGuid ## CONSUMES
@@ -186,6 +187,7 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask ##
CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ##
CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth ##
CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory ##
CONSUMES
# [Hob]
# RESOURCE_DESCRIPTOR ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
index 5733f0c8ec..8d1de32fe7 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
+++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
@@ -768,13 +768,25 @@ CoreExitBootServices (
//
gTimer->SetTimerPeriod (gTimer, 0);
+ //
+ // Accept all memory if unaccepted memory isn't enabled.
+ //
+ Status = CoreResolveUnacceptedMemory();
+ if (EFI_ERROR (Status)) {
+//
+// Notify other drivers that ExitBootServices failed
+//
+CoreNotifySignalList (&gEventExitBootServicesFailedGuid);
+return Status;
+ }
+
//
// Terminate memory services if the MapKey matches
//
Status = CoreTerminateMemoryMap (MapKey);
if (EFI_ERROR (Status)) {
//
-// Notify other drivers that ExitBootServices fail
+// Notify other drivers that ExitBootServices failed
//
CoreNotifySignalList (&gEventExitBootServicesFailedGuid);
return Status;
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index ffe79dcca9..cbebe62a28 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -9,6 +9,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include "DxeMain.h"
#include "Imem.h"
#include "HeapGuard.h"
+#include
+#include
//
// Entry for tracking the memory regions for each memory type to coalesce
similar memory types
@@ -2118,6 +2120,91 @@ CoreFreePoolPages (
CoreConvertPages (Memory, NumberOfPages, EfiConventionalMemory);
}
+EFI_EVENT gExitBootServiceEvent = NULL;
+
+STATIC
+EFI_STATUS
+AcceptAllUnacceptedMemory (
+ IN EFI_MEMORY_ACCEPT_PROTOCOL *AcceptMemory
+ )
+{
+ EFI_GCD_MEMORY_SPACE_DESCRIPTOR *AllDescMap;
+ UINTNNumEntries;
+ UINTNIndex;
+ EFI_STATUS Status;
+
+ /*
+ * Get a copy of the memory space map to iterate over while
+ * changing the map.
+ */
+ Status = CoreGetMemorySpaceMap (&NumEntries, &AllDescMap);
+ if (EFI_ERROR (Status)) {
+return Status;
+ }
+ for (Index = 0; Index < NumEntries; Index++) {
+CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Desc;
+
+Desc = &AllDescMap[Index];
+if (Desc->GcdMemoryType != EfiGcdMemoryTypeUnaccepted) {
+ continue;
+}
+
+Status = AcceptMemory->AcceptMemory (
+ AcceptMemory,
+ Desc->BaseAddress,
+ Desc->Length
+ );
+if (EFI_ERROR(Status)) {
+ goto done;
+}
+
+Status = CoreRemoveMem
