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 <kraxel@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Ard Biesheuvel <ardb@kernel.org>

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 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 <Library/PcdLib.h>
+#include <Protocol/MemoryAccept.h>
 
 //
 // 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;
+  UINTN                            NumEntries;
+  UINTN                            Index;
+  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 = CoreRemoveMemorySpace(Desc->BaseAddress, Desc->Length);
+    if (EFI_ERROR(Status)) {
+      goto done;
+    }
+
+    Status = CoreAddMemorySpace (
+      EfiGcdMemoryTypeSystemMemory,
+      Desc->BaseAddress,
+      Desc->Length,
+      EFI_MEMORY_CPU_CRYPTO | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_RP
+      );
+    if (EFI_ERROR(Status)) {
+      goto done;
+    }
+  }
+
+done:
+  FreePool (AllDescMap);
+  return Status;
+}
+
+EFI_STATUS
+CoreResolveUnacceptedMemory (
+  VOID
+  )
+{
+  EFI_MEMORY_ACCEPT_PROTOCOL *AcceptMemory;
+  EFI_STATUS                 Status;
+
+  // No need to accept anything. Unaccepted memory is enabled.
+  if (PcdGetBool(PcdEnableUnacceptedMemory)) {
+    return EFI_SUCCESS;
+  }
+
+  Status = gBS->LocateProtocol (&gEfiMemoryAcceptProtocolGuid, NULL,
+    (VOID **)&AcceptMemory);
+  if (Status == EFI_NOT_FOUND) {
+    return EFI_SUCCESS;
+  }
+  if (Status != EFI_SUCCESS) {
+    DEBUG ((DEBUG_ERROR, "Error locating MemoryAcceptProtocol: %d\n", Status));
+    return Status;
+  }
+
+  return AcceptAllUnacceptedMemory(AcceptMemory);
+}
+
 /**
   Make sure the memory map is following all the construction rules,
   it is the last time to check memory map error before exit boot services.
-- 
2.37.3.998.g577e59143f-goog



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#94472): https://edk2.groups.io/g/devel/message/94472
Mute This Topic: https://groups.io/mt/93975251/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Can you explain a bit more why this PCD is needed?

> -----Original Message-----
> From: devel@edk2.groups.io <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 <dionnaglaze@google.com>; Gerd Hoffmann <kraxel@redhat.com>; James Bottomley
> <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>; Ard
> Biesheuvel <ardb@kernel.org>
> 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 <kraxel@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> 
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>  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 <Library/PcdLib.h>
> +#include <Protocol/MemoryAccept.h>
> 
>  //
>  // 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;
> +  UINTN                            NumEntries;
> +  UINTN                            Index;
> +  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 = CoreRemoveMemorySpace(Desc->BaseAddress, Desc->Length);
> +    if (EFI_ERROR(Status)) {
> +      goto done;
> +    }
> +
> +    Status = CoreAddMemorySpace (
> +      EfiGcdMemoryTypeSystemMemory,
> +      Desc->BaseAddress,
> +      Desc->Length,
> +      EFI_MEMORY_CPU_CRYPTO | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_RP
> +      );
> +    if (EFI_ERROR(Status)) {
> +      goto done;
> +    }
> +  }
> +
> +done:
> +  FreePool (AllDescMap);
> +  return Status;
> +}
> +
> +EFI_STATUS
> +CoreResolveUnacceptedMemory (
> +  VOID
> +  )
> +{
> +  EFI_MEMORY_ACCEPT_PROTOCOL *AcceptMemory;
> +  EFI_STATUS                 Status;
> +
> +  // No need to accept anything. Unaccepted memory is enabled.
> +  if (PcdGetBool(PcdEnableUnacceptedMemory)) {
> +    return EFI_SUCCESS;
> +  }
> +
> +  Status = gBS->LocateProtocol (&gEfiMemoryAcceptProtocolGuid, NULL,
> +    (VOID **)&AcceptMemory);
> +  if (Status == EFI_NOT_FOUND) {
> +    return EFI_SUCCESS;
> +  }
> +  if (Status != EFI_SUCCESS) {
> +    DEBUG ((DEBUG_ERROR, "Error locating MemoryAcceptProtocol: %d\n", Status));
> +    return Status;
> +  }
> +
> +  return AcceptAllUnacceptedMemory(AcceptMemory);
> +}
> +
>  /**
>    Make sure the memory map is following all the construction rules,
>    it is the last time to check memory map error before exit boot services.
> --
> 2.37.3.998.g577e59143f-goog
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#94487): https://edk2.groups.io/g/devel/message/94487
Mute This Topic: https://groups.io/mt/93975251/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

>
> 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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, 29 Sept 2022 at 20:14, Dionna Amalie Glaze
<dionnaglaze@google.com> 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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, 28 Sept 2022 at 17:33, Dionna Glaze <dionnaglaze@google.com> 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 <kraxel@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>  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 <Library/PcdLib.h>
> +#include <Protocol/MemoryAccept.h>
>
>  //
>  // 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;
> +  UINTN                            NumEntries;
> +  UINTN                            Index;
> +  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 = CoreRemoveMemorySpace(Desc->BaseAddress, Desc->Length);
> +    if (EFI_ERROR(Status)) {
> +      goto done;
> +    }
> +
> +    Status = CoreAddMemorySpace (
> +      EfiGcdMemoryTypeSystemMemory,
> +      Desc->BaseAddress,
> +      Desc->Length,
> +      EFI_MEMORY_CPU_CRYPTO | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_RP
> +      );
> +    if (EFI_ERROR(Status)) {
> +      goto done;
> +    }
> +  }
> +
> +done:
> +  FreePool (AllDescMap);
> +  return Status;
> +}
> +

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?

The approach I suggested before was to accept all memory and then
forcible fail the ExitBootServices() call [which is documented in the
spec as an expected occurrence, as events dispatched off the timer
interrupt may race and allocate or free pages between GetMemoryMap and
ExitBootServices). Doing so would force the caller to call
GetMemoryMap() again, which now no longer contains any unaccepted
memory, and call ExitBootServices() a second time.

This means that, afaict, the call to CoreResolveUnacceptedMemory () is
in the right spot, i.e., after the point where the timer interrupt is
disabled (so we don't risk failing in ExitBootServices() twice). I
also wonder whether we need to deal specifically with the fact that,
if CoreResolveUnacceptedMemory() accepts any memory, it will be called
again the second time around as well, but perhaps we can just rely on
the fact that no unaccepted regions should remain in the GCD memory
map. But a comment to that effect would be helpful.

> +EFI_STATUS
> +CoreResolveUnacceptedMemory (
> +  VOID
> +  )
> +{
> +  EFI_MEMORY_ACCEPT_PROTOCOL *AcceptMemory;
> +  EFI_STATUS                 Status;
> +
> +  // No need to accept anything. Unaccepted memory is enabled.
> +  if (PcdGetBool(PcdEnableUnacceptedMemory)) {
> +    return EFI_SUCCESS;
> +  }
> +
> +  Status = gBS->LocateProtocol (&gEfiMemoryAcceptProtocolGuid, NULL,
> +    (VOID **)&AcceptMemory);
> +  if (Status == EFI_NOT_FOUND) {
> +    return EFI_SUCCESS;
> +  }
> +  if (Status != EFI_SUCCESS) {
> +    DEBUG ((DEBUG_ERROR, "Error locating MemoryAcceptProtocol: %d\n", Status));
> +    return Status;
> +  }
> +
> +  return AcceptAllUnacceptedMemory(AcceptMemory);
> +}
> +
>  /**
>    Make sure the memory map is following all the construction rules,
>    it is the last time to check memory map error before exit boot services.
> --
> 2.37.3.998.g577e59143f-goog
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#94479): https://edk2.groups.io/g/devel/message/94479
Mute This Topic: https://groups.io/mt/93975251/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-