[PATCH 0/6] Abstract allocation of PEI accessible memory

[PATCH 0/6] Abstract allocation of PEI accessible memory

[Ard Biesheuvel]

At the moment, FirmwarePerformanceTableDataDxe or DxeCorePerformanceLib
are unusable on systems such as AMD Seattle, because they unconditionally
attempt to allocate memory below 4 GB, and ASSERT() if this fails. On AMD
Seattle, no 32-bit addressable DRAM exists, and so the driver will always
assert, and crash a running DEBUG build.

The reason for this is that some platforms (i.e., X64 builds consisting of
a 32-bit PEI stage and 64-bit remaining stages) require these allocations
to be below 4 GB, and for some reason, open coding this practice throughout
the code without regard for other architectures has been regarded as an
acceptable approach.

Instead, I would like to propose the approach implemented in this series:
- create an abstracted EfiAllocatePeiAccessiblePages() routine in UefiLib
  that simply allocates pages from any region on all architectures except
  X64, and allocate below 4 GB for X64
- update the various call sites with calls to this new function.

The above is implemented in patches #3 - #6. Patches #1 and #2 are fixes
for issues that I observed in ArmVirtPkg and OvmfPkg while working on
these patches.

Code can be found here:
https://github.com/ardbiesheuvel/edk2/tree/allocate-pei

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com> 

Ard Biesheuvel (6):
  OvmfPkg/PlatformBootManagerLib: add missing report status code call
  ArmVirtPkg/PlatformBootManagerLib: add missing report status code call
  MdePkg/UefiLib: introduce EfiAllocatePeiAccessiblePages routine
  IntelFrameworkPkg/FrameworkUefiLib: add EfiAllocatePeiAccessiblePages
    routine
  MdeModulePkg/DxeCorePerformanceLib: use EfiAllocatePeiAccessiblePages
  MdeModulePkg/FirmwarePerformanceDataTableDxe: use
    EfiAllocatePeiAccessiblePages

 .../PlatformBootManagerLib.inf                |  1 +
 .../PlatformBootManagerLib/QemuKernel.c       |  4 ++
 .../Library/FrameworkUefiLib/UefiLib.c        | 48 ++++++++++++++++++
 .../DxeCorePerformanceLib.c                   | 45 ++---------------
 .../FirmwarePerformanceDxe.c                  | 50 +++----------------
 MdePkg/Include/Library/UefiLib.h              | 23 +++++++++
 MdePkg/Library/UefiLib/UefiLib.c              | 48 ++++++++++++++++++
 .../PlatformBootManagerLib.inf                |  1 +
 .../PlatformBootManagerLib/QemuKernel.c       |  4 ++
 9 files changed, 141 insertions(+), 83 deletions(-)

-- 
2.17.0

[PATCH 1/6] OvmfPkg/PlatformBootManagerLib: add missing report status code call

[Ard Biesheuvel]

Consumers of status code reports may rely on a status code to be
reported when the ReadyToBoot event is signalled. For instance,
FirmwarePerformanceDxe will fail to install the FPDT ACPI table
in this case. So add the missing call.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 +
 OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c               | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index 27789b7377bc..f10b68424b91 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
@@ -55,6 +55,7 @@ [LibraryClasses]
   QemuFwCfgS3Lib
   LoadLinuxLib
   QemuBootOrderLib
+  ReportStatusCodeLib
   UefiLib
 
 [Pcd]
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c b/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
index ef728dfdeb60..f20df9533fda 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
@@ -18,6 +18,7 @@
 #include <Library/LoadLinuxLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/QemuFwCfgLib.h>
+#include <Library/ReportStatusCodeLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiLib.h>
 
@@ -149,6 +150,9 @@ TryRunningQemuKernel (
   //
   EfiSignalEventReadyToBoot();
 
+  REPORT_STATUS_CODE (EFI_PROGRESS_CODE,
+    (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_PC_READY_TO_BOOT_EVENT));
+
   Status = LoadLinux (KernelBuf, SetupBuf);
 
 FreeAndReturn:
-- 
2.17.0

[PATCH 2/6] ArmVirtPkg/PlatformBootManagerLib: add missing report status code call

[Ard Biesheuvel]

Consumers of status code reports may rely on a status code to be
reported when the ReadyToBoot event is signalled. For instance,
FirmwarePerformanceDxe will fail to install the FPDT ACPI table
in this case. So add the missing call.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 +
 ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c               | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index d6c1ef95dc44..0cbc82f5d27d 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
@@ -53,6 +53,7 @@ [LibraryClasses]
   PrintLib
   QemuBootOrderLib
   QemuFwCfgLib
+  ReportStatusCodeLib
   UefiBootManagerLib
   UefiBootServicesTableLib
   UefiLib
diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c b/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c
index ac47d21e71c8..7b59f57eb19f 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c
@@ -20,6 +20,7 @@
 #include <Guid/FileSystemVolumeLabelInfo.h>
 #include <Library/PrintLib.h>
 #include <Library/QemuFwCfgLib.h>
+#include <Library/ReportStatusCodeLib.h>
 #include <Protocol/DevicePath.h>
 #include <Protocol/LoadedImage.h>
 #include <Protocol/SimpleFileSystem.h>
@@ -1072,6 +1073,9 @@ TryRunningQemuKernel (
   //
   EfiSignalEventReadyToBoot();
 
+  REPORT_STATUS_CODE (EFI_PROGRESS_CODE,
+    (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_PC_READY_TO_BOOT_EVENT));
+
   //
   // Start the image.
   //
-- 
2.17.0

[PATCH 3/6] MdePkg/UefiLib: introduce EfiAllocatePeiAccessiblePages routine

[Ard Biesheuvel]

Add a routine to UefiLib that abstracts the allocation of memory that
should be accessible by PEI after a warm reboot. We will use it to
replace open coded implementations that limit the address to < 4 GB,
which may not be possible on non-Intel systems that have no 32-bit
addressable memory at all.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdePkg/Include/Library/UefiLib.h | 23 ++++++++++
 MdePkg/Library/UefiLib/UefiLib.c | 48 ++++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
index 256498e3fd8d..8fa077af41e0 100644
--- a/MdePkg/Include/Library/UefiLib.h
+++ b/MdePkg/Include/Library/UefiLib.h
@@ -1520,4 +1520,27 @@ EfiLocateProtocolBuffer (
   OUT UINTN     *NoProtocols,
   OUT VOID      ***Buffer
   );
+
+/**
+  Allocates one or more 4KB pages of a given type from a memory region that is
+  accessible to PEI.
+
+  Allocates the number of 4KB pages of type 'MemoryType' and returns a
+  pointer to the allocated buffer.  The buffer returned is aligned on a 4KB
+  boundary.  If Pages is 0, then NULL is returned.  If there is not enough
+  memory remaining to satisfy the request, then NULL is returned.
+
+  @param[in]  MemoryType            The memory type to allocate
+  @param[in]  Pages                 The number of 4 KB pages to allocate.
+
+  @return A pointer to the allocated buffer or NULL if allocation fails.
+
+**/
+VOID *
+EFIAPI
+EfiAllocatePeiAccessiblePages (
+  IN EFI_MEMORY_TYPE  MemoryType,
+  IN UINTN            Pages
+  );
+
 #endif
diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
index ba449a1c34ce..3a9d75149dd7 100644
--- a/MdePkg/Library/UefiLib/UefiLib.c
+++ b/MdePkg/Library/UefiLib/UefiLib.c
@@ -1715,3 +1715,51 @@ EfiLocateProtocolBuffer (
 
   return EFI_SUCCESS;
 }
+
+/**
+  Allocates one or more 4KB pages of a given type from a memory region that is
+  accessible to PEI.
+
+  Allocates the number of 4KB pages of type 'MemoryType' and returns a
+  pointer to the allocated buffer.  The buffer returned is aligned on a 4KB
+  boundary.  If Pages is 0, then NULL is returned.  If there is not enough
+  memory remaining to satisfy the request, then NULL is returned.
+
+  @param[in]  MemoryType            The memory type to allocate
+  @param[in]  Pages                 The number of 4 KB pages to allocate.
+
+  @return A pointer to the allocated buffer or NULL if allocation fails.
+
+**/
+VOID *
+EFIAPI
+EfiAllocatePeiAccessiblePages (
+  IN EFI_MEMORY_TYPE  MemoryType,
+  IN UINTN            Pages
+  )
+{
+  EFI_STATUS            Status;
+  EFI_PHYSICAL_ADDRESS  Memory;
+  EFI_ALLOCATE_TYPE     AllocType;
+
+  if (Pages == 0) {
+    return NULL;
+  }
+
+#ifdef MDE_CPU_X64
+  //
+  // On X64 systems, a X64 build of DXE may be combined with a 32-bit build of
+  // PEI, and so we need to allocate below 4 GB to ensure that the allocation
+  // is accessible by PEI.
+  //
+  AllocType = AllocateMaxAddress;
+  Memory = MAX_UINT32;
+#else
+  AllocType = AllocateAnyPages;
+#endif
+  Status = gBS->AllocatePages (AllocType, MemoryType, Pages, &Memory);
+  if (EFI_ERROR (Status)) {
+    return NULL;
+  }
+  return (VOID *)(UINTN)Memory;
+}
-- 
2.17.0

[PATCH 4/6] IntelFrameworkPkg/FrameworkUefiLib: add EfiAllocatePeiAccessiblePages routine

[Ard Biesheuvel]

Add the newly introduced EfiAllocatePeiAccessiblePages() routine which
allocates memory in a way that guarantees that PEI can access it after
a warm reboot.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLib.c | 48 ++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLib.c b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLib.c
index 443a73917215..a488fe780b04 100644
--- a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLib.c
+++ b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLib.c
@@ -1687,3 +1687,51 @@ EfiLocateProtocolBuffer (
 
   return EFI_SUCCESS;
 }
+
+/**
+  Allocates one or more 4KB pages of a given type from a memory region that is
+  accessible to PEI.
+
+  Allocates the number of 4KB pages of type 'MemoryType' and returns a
+  pointer to the allocated buffer.  The buffer returned is aligned on a 4KB
+  boundary.  If Pages is 0, then NULL is returned.  If there is not enough
+  memory remaining to satisfy the request, then NULL is returned.
+
+  @param[in]  MemoryType            The memory type to allocate
+  @param[in]  Pages                 The number of 4 KB pages to allocate.
+
+  @return A pointer to the allocated buffer or NULL if allocation fails.
+
+**/
+VOID *
+EFIAPI
+EfiAllocatePeiAccessiblePages (
+  IN EFI_MEMORY_TYPE  MemoryType,
+  IN UINTN            Pages
+  )
+{
+  EFI_STATUS            Status;
+  EFI_PHYSICAL_ADDRESS  Memory;
+  EFI_ALLOCATE_TYPE     AllocType;
+
+  if (Pages == 0) {
+    return NULL;
+  }
+
+#ifdef MDE_CPU_X64
+  //
+  // On X64 systems, a X64 build of DXE may be combined with a 32-bit build of
+  // PEI, and so we need to allocate below 4 GB to ensure that the allocation
+  // is accessible by PEI.
+  //
+  AllocType = AllocateMaxAddress;
+  Memory = MAX_UINT32;
+#else
+  AllocType = AllocateAnyPages;
+#endif
+  Status = gBS->AllocatePages (AllocType, MemoryType, Pages, &Memory);
+  if (EFI_ERROR (Status)) {
+    return NULL;
+  }
+  return (VOID *)(UINTN)Memory;
+}
-- 
2.17.0

[PATCH 5/6] MdeModulePkg/DxeCorePerformanceLib: use EfiAllocatePeiAccessiblePages

[Ard Biesheuvel]

Replace the call to and implementation of the function
FpdtAllocateReservedMemoryBelow4G() with a call to
EfiAllocatePeiAccessiblePages, which boils down to the same on X64,
but does not crash non-X64 systems that lack memory below 4 GB.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c | 45 ++------------------
 1 file changed, 4 insertions(+), 41 deletions(-)

diff --git a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c
index 71d624fc9ce9..301ea4c32a78 100644
--- a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c
+++ b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c
@@ -165,46 +165,6 @@ IsKnownID (
   }
 }
 
-/**
-  Allocate EfiReservedMemoryType below 4G memory address.
-
-  This function allocates EfiReservedMemoryType below 4G memory address.
-
-  @param[in]  Size   Size of memory to allocate.
-
-  @return Allocated address for output.
-
-**/
-VOID *
-FpdtAllocateReservedMemoryBelow4G (
-  IN UINTN       Size
-  )
-{
-  UINTN                 Pages;
-  EFI_PHYSICAL_ADDRESS  Address;
-  EFI_STATUS            Status;
-  VOID                  *Buffer;
-
-  Buffer  = NULL;
-  Pages   = EFI_SIZE_TO_PAGES (Size);
-  Address = 0xffffffff;
-
-  Status = gBS->AllocatePages (
-                  AllocateMaxAddress,
-                  EfiReservedMemoryType,
-                  Pages,
-                  &Address
-                  );
-  ASSERT_EFI_ERROR (Status);
-
-  if (!EFI_ERROR (Status)) {
-    Buffer = (VOID *) (UINTN) Address;
-    ZeroMem (Buffer, Size);
-  }
-
-  return Buffer;
-}
-
 /**
   Allocate buffer for Boot Performance table.
 
@@ -348,7 +308,10 @@ AllocateBootPerformanceTable (
     //
     // Fail to allocate at specified address, continue to allocate at any address.
     //
-    mAcpiBootPerformanceTable = (BOOT_PERFORMANCE_TABLE *) FpdtAllocateReservedMemoryBelow4G (BootPerformanceDataSize);
+    mAcpiBootPerformanceTable = (BOOT_PERFORMANCE_TABLE *) EfiAllocatePeiAccessiblePages (
+                                                             EfiReservedMemoryType,
+                                                             EFI_SIZE_TO_PAGES (BootPerformanceDataSize)
+                                                             );
   }
   DEBUG ((DEBUG_INFO, "DxeCorePerformanceLib: ACPI Boot Performance Table address = 0x%x\n", mAcpiBootPerformanceTable));
 
-- 
2.17.0

[PATCH 6/6] MdeModulePkg/FirmwarePerformanceDataTableDxe: use EfiAllocatePeiAccessiblePages

[Ard Biesheuvel]

Replace the call to and implementation of the function
FpdtAllocateReservedMemoryBelow4G() with a call to
EfiAllocatePeiAccessiblePages, which boils down to the same on X64,
but does not crash non-X64 systems that lack memory below 4 GB.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c | 50 ++++----------------
 1 file changed, 8 insertions(+), 42 deletions(-)

diff --git a/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c b/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
index e719e9e482cb..6663cbeac501 100644
--- a/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
+++ b/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
@@ -179,46 +179,6 @@ FpdtAcpiTableChecksum (
   Buffer[ChecksumOffset] = CalculateCheckSum8 (Buffer, Size);
 }
 
-/**
-  Allocate EfiReservedMemoryType below 4G memory address.
-
-  This function allocates EfiReservedMemoryType below 4G memory address.
-
-  @param[in]  Size   Size of memory to allocate.
-
-  @return Allocated address for output.
-
-**/
-VOID *
-FpdtAllocateReservedMemoryBelow4G (
-  IN UINTN       Size
-  )
-{
-  UINTN                 Pages;
-  EFI_PHYSICAL_ADDRESS  Address;
-  EFI_STATUS            Status;
-  VOID                  *Buffer;
-
-  Buffer  = NULL;
-  Pages   = EFI_SIZE_TO_PAGES (Size);
-  Address = 0xffffffff;
-
-  Status = gBS->AllocatePages (
-                  AllocateMaxAddress,
-                  EfiReservedMemoryType,
-                  Pages,
-                  &Address
-                  );
-  ASSERT_EFI_ERROR (Status);
-
-  if (!EFI_ERROR (Status)) {
-    Buffer = (VOID *) (UINTN) Address;
-    ZeroMem (Buffer, Size);
-  }
-
-  return Buffer;
-}
-
 /**
   Callback function upon VariableArchProtocol and LockBoxProtocol
   to allocate S3 performance table memory and save the pointer to LockBox.
@@ -287,7 +247,10 @@ FpdtAllocateS3PerformanceTableMemory (
         //
         // Fail to allocate at specified address, continue to allocate at any address.
         //
-        mAcpiS3PerformanceTable = (S3_PERFORMANCE_TABLE *) FpdtAllocateReservedMemoryBelow4G (sizeof (S3_PERFORMANCE_TABLE));
+        mAcpiS3PerformanceTable = (S3_PERFORMANCE_TABLE *) EfiAllocatePeiAccessiblePages (
+                                                               EfiReservedMemoryType,
+                                                               EFI_SIZE_TO_PAGES (sizeof (S3_PERFORMANCE_TABLE))
+                                                               );
       }
       DEBUG ((EFI_D_INFO, "FPDT: ACPI S3 Performance Table address = 0x%x\n", mAcpiS3PerformanceTable));
       if (mAcpiS3PerformanceTable != NULL) {
@@ -368,7 +331,10 @@ InstallFirmwarePerformanceDataTable (
       //
       // Fail to allocate at specified address, continue to allocate at any address.
       //
-      mAcpiBootPerformanceTable = (BOOT_PERFORMANCE_TABLE *) FpdtAllocateReservedMemoryBelow4G (BootPerformanceDataSize);
+      mAcpiBootPerformanceTable = (BOOT_PERFORMANCE_TABLE *) EfiAllocatePeiAccessiblePages (
+                                                               EfiReservedMemoryType,
+                                                               EFI_SIZE_TO_PAGES (BootPerformanceDataSize)
+                                                               );
     }
     DEBUG ((DEBUG_INFO, "FPDT: ACPI Boot Performance Table address = 0x%x\n", mAcpiBootPerformanceTable));
     if (mAcpiBootPerformanceTable == NULL) {
-- 
2.17.0

Re: [PATCH 3/6] MdePkg/UefiLib: introduce EfiAllocatePeiAccessiblePages routine

[Kinney, Michael D]

Ard,

The corner case that does not work with this
approach is when X64 DXE is combined with an
X64 PEI.  OVMF uses this and other platforms
could choose to use X64 PEI phase.

The other mismatch here is adding some PI Spec
Concepts (e.g. PEI phase) to a UEFI library.
Maybe DxeServicesLib would be a better place
to put this type of API.

One clue we have about the memory usage in the
PEI phase is from the EFI_HOB_HANDOFF_INFO_TABLE
HOB.

  ///
  /// The highest address location of memory that is allocated for use by the HOB producer
  /// phase. This address must be 4-KB aligned to meet page restrictions of UEFI.
  ///
  EFI_PHYSICAL_ADDRESS    EfiMemoryTop;

  ///
  /// The highest address location of free memory that is currently available
  /// for use by the HOB producer phase.
  ///
  EFI_PHYSICAL_ADDRESS    EfiFreeMemoryTop;

So maybe we could have an X64 specific implementation
of this new API that checks one of these HOB fields.
If they are below 4GB, then allocate memory below
4GB.  If one is above 4GB, then no restrictions.
All other archs allocate with no restrictions.

Now this approach will still allocate below 4GB for
X64 PEI if the this HOB contains addressed below 4GB,
but that would match the memory usage for that
X64 PEI implementation.

Best regards,

Mike

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Tuesday, May 22, 2018 7:09 AM
> To: edk2-devel@lists.01.org
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo
> Ersek <lersek@redhat.com>; Leif Lindholm
> <leif.lindholm@linaro.org>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Bi, Dandan <dandan.bi@intel.com>
> Subject: [PATCH 3/6] MdePkg/UefiLib: introduce
> EfiAllocatePeiAccessiblePages routine
> 
> Add a routine to UefiLib that abstracts the allocation
> of memory that
> should be accessible by PEI after a warm reboot. We will
> use it to
> replace open coded implementations that limit the
> address to < 4 GB,
> which may not be possible on non-Intel systems that have
> no 32-bit
> addressable memory at all.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> ---
>  MdePkg/Include/Library/UefiLib.h | 23 ++++++++++
>  MdePkg/Library/UefiLib/UefiLib.c | 48
> ++++++++++++++++++++
>  2 files changed, 71 insertions(+)
> 
> diff --git a/MdePkg/Include/Library/UefiLib.h
> b/MdePkg/Include/Library/UefiLib.h
> index 256498e3fd8d..8fa077af41e0 100644
> --- a/MdePkg/Include/Library/UefiLib.h
> +++ b/MdePkg/Include/Library/UefiLib.h
> @@ -1520,4 +1520,27 @@ EfiLocateProtocolBuffer (
>    OUT UINTN     *NoProtocols,
>    OUT VOID      ***Buffer
>    );
> +
> +/**
> +  Allocates one or more 4KB pages of a given type from
> a memory region that is
> +  accessible to PEI.
> +
> +  Allocates the number of 4KB pages of type
> 'MemoryType' and returns a
> +  pointer to the allocated buffer.  The buffer returned
> is aligned on a 4KB
> +  boundary.  If Pages is 0, then NULL is returned.  If
> there is not enough
> +  memory remaining to satisfy the request, then NULL is
> returned.
> +
> +  @param[in]  MemoryType            The memory type to
> allocate
> +  @param[in]  Pages                 The number of 4 KB
> pages to allocate.
> +
> +  @return A pointer to the allocated buffer or NULL if
> allocation fails.
> +
> +**/
> +VOID *
> +EFIAPI
> +EfiAllocatePeiAccessiblePages (
> +  IN EFI_MEMORY_TYPE  MemoryType,
> +  IN UINTN            Pages
> +  );
> +
>  #endif
> diff --git a/MdePkg/Library/UefiLib/UefiLib.c
> b/MdePkg/Library/UefiLib/UefiLib.c
> index ba449a1c34ce..3a9d75149dd7 100644
> --- a/MdePkg/Library/UefiLib/UefiLib.c
> +++ b/MdePkg/Library/UefiLib/UefiLib.c
> @@ -1715,3 +1715,51 @@ EfiLocateProtocolBuffer (
> 
>    return EFI_SUCCESS;
>  }
> +
> +/**
> +  Allocates one or more 4KB pages of a given type from
> a memory region that is
> +  accessible to PEI.
> +
> +  Allocates the number of 4KB pages of type
> 'MemoryType' and returns a
> +  pointer to the allocated buffer.  The buffer returned
> is aligned on a 4KB
> +  boundary.  If Pages is 0, then NULL is returned.  If
> there is not enough
> +  memory remaining to satisfy the request, then NULL is
> returned.
> +
> +  @param[in]  MemoryType            The memory type to
> allocate
> +  @param[in]  Pages                 The number of 4 KB
> pages to allocate.
> +
> +  @return A pointer to the allocated buffer or NULL if
> allocation fails.
> +
> +**/
> +VOID *
> +EFIAPI
> +EfiAllocatePeiAccessiblePages (
> +  IN EFI_MEMORY_TYPE  MemoryType,
> +  IN UINTN            Pages
> +  )
> +{
> +  EFI_STATUS            Status;
> +  EFI_PHYSICAL_ADDRESS  Memory;
> +  EFI_ALLOCATE_TYPE     AllocType;
> +
> +  if (Pages == 0) {
> +    return NULL;
> +  }
> +
> +#ifdef MDE_CPU_X64
> +  //
> +  // On X64 systems, a X64 build of DXE may be combined
> with a 32-bit build of
> +  // PEI, and so we need to allocate below 4 GB to
> ensure that the allocation
> +  // is accessible by PEI.
> +  //
> +  AllocType = AllocateMaxAddress;
> +  Memory = MAX_UINT32;
> +#else
> +  AllocType = AllocateAnyPages;
> +#endif
> +  Status = gBS->AllocatePages (AllocType, MemoryType,
> Pages, &Memory);
> +  if (EFI_ERROR (Status)) {
> +    return NULL;
> +  }
> +  return (VOID *)(UINTN)Memory;
> +}
> --
> 2.17.0

Re: [PATCH 3/6] MdePkg/UefiLib: introduce EfiAllocatePeiAccessiblePages routine

[Ard Biesheuvel]

On 22 May 2018 at 19:40, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
> Ard,
>
> The corner case that does not work with this
> approach is when X64 DXE is combined with an
> X64 PEI.  OVMF uses this and other platforms
> could choose to use X64 PEI phase.
>

Actually, this approach simply encodes the current status quo. X64 DXE
builds unconditionally limit the allocations to < 4 GB if PEI needs to
access them, regardless of whether PEI is 32-bit or 64-bit.

Also, OVMF's X64 PEI still only maps the lower 4 GB of DRAM, so it
actually relies on the current behavior, and allocating above 4 GB
under the assumption that a 64-bit PEI will be able to access it will
actually break things.

> The other mismatch here is adding some PI Spec
> Concepts (e.g. PEI phase) to a UEFI library.
> Maybe DxeServicesLib would be a better place
> to put this type of API.
>

OK, fair enough.

> One clue we have about the memory usage in the
> PEI phase is from the EFI_HOB_HANDOFF_INFO_TABLE
> HOB.
>
>   ///
>   /// The highest address location of memory that is allocated for use by the HOB producer
>   /// phase. This address must be 4-KB aligned to meet page restrictions of UEFI.
>   ///
>   EFI_PHYSICAL_ADDRESS    EfiMemoryTop;
>
>   ///
>   /// The highest address location of free memory that is currently available
>   /// for use by the HOB producer phase.
>   ///
>   EFI_PHYSICAL_ADDRESS    EfiFreeMemoryTop;
>
> So maybe we could have an X64 specific implementation
> of this new API that checks one of these HOB fields.
> If they are below 4GB, then allocate memory below
> 4GB.  If one is above 4GB, then no restrictions.
> All other archs allocate with no restrictions.
>

That works for me.

> Now this approach will still allocate below 4GB for
> X64 PEI if the this HOB contains addressed below 4GB,
> but that would match the memory usage for that
> X64 PEI implementation.
>

OK, to summarize:
- move the implementation of EfiAllocatePeiAccessiblePages() to
DxeServicesLib (and perhaps rename it to something more appropriate
for its new home)
- only restrict the X64 version to below 4 GB if EfiMemoryTop and
EfiFreeMemoryTop are both below 4 GB.

I will give others some time to respond to this.

Thanks,
Ard.


>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Tuesday, May 22, 2018 7:09 AM
>> To: edk2-devel@lists.01.org
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo
>> Ersek <lersek@redhat.com>; Leif Lindholm
>> <leif.lindholm@linaro.org>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; Zeng, Star
>> <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>;
>> Bi, Dandan <dandan.bi@intel.com>
>> Subject: [PATCH 3/6] MdePkg/UefiLib: introduce
>> EfiAllocatePeiAccessiblePages routine
>>
>> Add a routine to UefiLib that abstracts the allocation
>> of memory that
>> should be accessible by PEI after a warm reboot. We will
>> use it to
>> replace open coded implementations that limit the
>> address to < 4 GB,
>> which may not be possible on non-Intel systems that have
>> no 32-bit
>> addressable memory at all.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>
>> ---
>>  MdePkg/Include/Library/UefiLib.h | 23 ++++++++++
>>  MdePkg/Library/UefiLib/UefiLib.c | 48
>> ++++++++++++++++++++
>>  2 files changed, 71 insertions(+)
>>
>> diff --git a/MdePkg/Include/Library/UefiLib.h
>> b/MdePkg/Include/Library/UefiLib.h
>> index 256498e3fd8d..8fa077af41e0 100644
>> --- a/MdePkg/Include/Library/UefiLib.h
>> +++ b/MdePkg/Include/Library/UefiLib.h
>> @@ -1520,4 +1520,27 @@ EfiLocateProtocolBuffer (
>>    OUT UINTN     *NoProtocols,
>>    OUT VOID      ***Buffer
>>    );
>> +
>> +/**
>> +  Allocates one or more 4KB pages of a given type from
>> a memory region that is
>> +  accessible to PEI.
>> +
>> +  Allocates the number of 4KB pages of type
>> 'MemoryType' and returns a
>> +  pointer to the allocated buffer.  The buffer returned
>> is aligned on a 4KB
>> +  boundary.  If Pages is 0, then NULL is returned.  If
>> there is not enough
>> +  memory remaining to satisfy the request, then NULL is
>> returned.
>> +
>> +  @param[in]  MemoryType            The memory type to
>> allocate
>> +  @param[in]  Pages                 The number of 4 KB
>> pages to allocate.
>> +
>> +  @return A pointer to the allocated buffer or NULL if
>> allocation fails.
>> +
>> +**/
>> +VOID *
>> +EFIAPI
>> +EfiAllocatePeiAccessiblePages (
>> +  IN EFI_MEMORY_TYPE  MemoryType,
>> +  IN UINTN            Pages
>> +  );
>> +
>>  #endif
>> diff --git a/MdePkg/Library/UefiLib/UefiLib.c
>> b/MdePkg/Library/UefiLib/UefiLib.c
>> index ba449a1c34ce..3a9d75149dd7 100644
>> --- a/MdePkg/Library/UefiLib/UefiLib.c
>> +++ b/MdePkg/Library/UefiLib/UefiLib.c
>> @@ -1715,3 +1715,51 @@ EfiLocateProtocolBuffer (
>>
>>    return EFI_SUCCESS;
>>  }
>> +
>> +/**
>> +  Allocates one or more 4KB pages of a given type from
>> a memory region that is
>> +  accessible to PEI.
>> +
>> +  Allocates the number of 4KB pages of type
>> 'MemoryType' and returns a
>> +  pointer to the allocated buffer.  The buffer returned
>> is aligned on a 4KB
>> +  boundary.  If Pages is 0, then NULL is returned.  If
>> there is not enough
>> +  memory remaining to satisfy the request, then NULL is
>> returned.
>> +
>> +  @param[in]  MemoryType            The memory type to
>> allocate
>> +  @param[in]  Pages                 The number of 4 KB
>> pages to allocate.
>> +
>> +  @return A pointer to the allocated buffer or NULL if
>> allocation fails.
>> +
>> +**/
>> +VOID *
>> +EFIAPI
>> +EfiAllocatePeiAccessiblePages (
>> +  IN EFI_MEMORY_TYPE  MemoryType,
>> +  IN UINTN            Pages
>> +  )
>> +{
>> +  EFI_STATUS            Status;
>> +  EFI_PHYSICAL_ADDRESS  Memory;
>> +  EFI_ALLOCATE_TYPE     AllocType;
>> +
>> +  if (Pages == 0) {
>> +    return NULL;
>> +  }
>> +
>> +#ifdef MDE_CPU_X64
>> +  //
>> +  // On X64 systems, a X64 build of DXE may be combined
>> with a 32-bit build of
>> +  // PEI, and so we need to allocate below 4 GB to
>> ensure that the allocation
>> +  // is accessible by PEI.
>> +  //
>> +  AllocType = AllocateMaxAddress;
>> +  Memory = MAX_UINT32;
>> +#else
>> +  AllocType = AllocateAnyPages;
>> +#endif
>> +  Status = gBS->AllocatePages (AllocType, MemoryType,
>> Pages, &Memory);
>> +  if (EFI_ERROR (Status)) {
>> +    return NULL;
>> +  }
>> +  return (VOID *)(UINTN)Memory;
>> +}
>> --
>> 2.17.0
>

Re: [PATCH 1/6] OvmfPkg/PlatformBootManagerLib: add missing report status code call

[Laszlo Ersek]

On 05/22/18 16:08, Ard Biesheuvel wrote:
> Consumers of status code reports may rely on a status code to be
> reported when the ReadyToBoot event is signalled. For instance,
> FirmwarePerformanceDxe will fail to install the FPDT ACPI table
> in this case. So add the missing call.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 +
>  OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c               | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> index 27789b7377bc..f10b68424b91 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> @@ -55,6 +55,7 @@ [LibraryClasses]
>    QemuFwCfgS3Lib
>    LoadLinuxLib
>    QemuBootOrderLib
> +  ReportStatusCodeLib
>    UefiLib
>  
>  [Pcd]
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c b/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
> index ef728dfdeb60..f20df9533fda 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
> @@ -18,6 +18,7 @@
>  #include <Library/LoadLinuxLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/QemuFwCfgLib.h>
> +#include <Library/ReportStatusCodeLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/UefiLib.h>
>  
> @@ -149,6 +150,9 @@ TryRunningQemuKernel (
>    //
>    EfiSignalEventReadyToBoot();
>  
> +  REPORT_STATUS_CODE (EFI_PROGRESS_CODE,
> +    (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_PC_READY_TO_BOOT_EVENT));
> +
>    Status = LoadLinux (KernelBuf, SetupBuf);
>  
>  FreeAndReturn:
> 

Good catch!

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Laszlo

Re: [PATCH 2/6] ArmVirtPkg/PlatformBootManagerLib: add missing report status code call

[Laszlo Ersek]

On 05/22/18 16:08, Ard Biesheuvel wrote:
> Consumers of status code reports may rely on a status code to be
> reported when the ReadyToBoot event is signalled. For instance,
> FirmwarePerformanceDxe will fail to install the FPDT ACPI table
> in this case. So add the missing call.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 +
>  ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c               | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> index d6c1ef95dc44..0cbc82f5d27d 100644
> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> @@ -53,6 +53,7 @@ [LibraryClasses]
>    PrintLib
>    QemuBootOrderLib
>    QemuFwCfgLib
> +  ReportStatusCodeLib
>    UefiBootManagerLib
>    UefiBootServicesTableLib
>    UefiLib
> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c b/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c
> index ac47d21e71c8..7b59f57eb19f 100644
> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c
> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c
> @@ -20,6 +20,7 @@
>  #include <Guid/FileSystemVolumeLabelInfo.h>
>  #include <Library/PrintLib.h>
>  #include <Library/QemuFwCfgLib.h>
> +#include <Library/ReportStatusCodeLib.h>
>  #include <Protocol/DevicePath.h>
>  #include <Protocol/LoadedImage.h>
>  #include <Protocol/SimpleFileSystem.h>
> @@ -1072,6 +1073,9 @@ TryRunningQemuKernel (
>    //
>    EfiSignalEventReadyToBoot();
>  
> +  REPORT_STATUS_CODE (EFI_PROGRESS_CODE,
> +    (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_PC_READY_TO_BOOT_EVENT));
> +
>    //
>    // Start the image.
>    //
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 3/6] MdePkg/UefiLib: introduce EfiAllocatePeiAccessiblePages routine

[Laszlo Ersek]

On 05/22/18 19:47, Ard Biesheuvel wrote:

> OK, to summarize:
> - move the implementation of EfiAllocatePeiAccessiblePages() to
> DxeServicesLib (and perhaps rename it to something more appropriate
> for its new home)
> - only restrict the X64 version to below 4 GB if EfiMemoryTop and
> EfiFreeMemoryTop are both below 4 GB.

If this works with pure X64 OVMF (including S3 without SMM), I'd be very
happy with it.

Also we should regression test whether IA32X64 OVMF continues working
(including S3 without SMM, and S3 with SMM).

I'm glad to help with the testing once patches are posted.

OVMF installs the permanent PEI RAM in PublishPeiMemory()
[OvmfPkg/PlatformPei/MemDetect.c]. It keeps things under 4GB, so I think
this approach should work.

Thanks Ard & Mike!
Laszlo

Re: [PATCH 3/6] MdePkg/UefiLib: introduce EfiAllocatePeiAccessiblePages routine

[Zeng, Star]

On 2018/5/22 22:08, Ard Biesheuvel wrote:
> Add a routine to UefiLib that abstracts the allocation of memory that
> should be accessible by PEI after a warm reboot. We will use it to

Glad to see this patch series.

It is the case "PEI for S3", but not "PEI after a warm reboot".


Thanks,
Star

> replace open coded implementations that limit the address to < 4 GB,
> which may not be possible on non-Intel systems that have no 32-bit
> addressable memory at all.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>   MdePkg/Include/Library/UefiLib.h | 23 ++++++++++
>   MdePkg/Library/UefiLib/UefiLib.c | 48 ++++++++++++++++++++
>   2 files changed, 71 insertions(+)
> 
> diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
> index 256498e3fd8d..8fa077af41e0 100644
> --- a/MdePkg/Include/Library/UefiLib.h
> +++ b/MdePkg/Include/Library/UefiLib.h
> @@ -1520,4 +1520,27 @@ EfiLocateProtocolBuffer (
>     OUT UINTN     *NoProtocols,
>     OUT VOID      ***Buffer
>     );
> +
> +/**
> +  Allocates one or more 4KB pages of a given type from a memory region that is
> +  accessible to PEI.
> +
> +  Allocates the number of 4KB pages of type 'MemoryType' and returns a
> +  pointer to the allocated buffer.  The buffer returned is aligned on a 4KB
> +  boundary.  If Pages is 0, then NULL is returned.  If there is not enough
> +  memory remaining to satisfy the request, then NULL is returned.
> +
> +  @param[in]  MemoryType            The memory type to allocate
> +  @param[in]  Pages                 The number of 4 KB pages to allocate.
> +
> +  @return A pointer to the allocated buffer or NULL if allocation fails.
> +
> +**/
> +VOID *
> +EFIAPI
> +EfiAllocatePeiAccessiblePages (
> +  IN EFI_MEMORY_TYPE  MemoryType,
> +  IN UINTN            Pages
> +  );
> +
>   #endif
> diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
> index ba449a1c34ce..3a9d75149dd7 100644
> --- a/MdePkg/Library/UefiLib/UefiLib.c
> +++ b/MdePkg/Library/UefiLib/UefiLib.c
> @@ -1715,3 +1715,51 @@ EfiLocateProtocolBuffer (
>   
>     return EFI_SUCCESS;
>   }
> +
> +/**
> +  Allocates one or more 4KB pages of a given type from a memory region that is
> +  accessible to PEI.
> +
> +  Allocates the number of 4KB pages of type 'MemoryType' and returns a
> +  pointer to the allocated buffer.  The buffer returned is aligned on a 4KB
> +  boundary.  If Pages is 0, then NULL is returned.  If there is not enough
> +  memory remaining to satisfy the request, then NULL is returned.
> +
> +  @param[in]  MemoryType            The memory type to allocate
> +  @param[in]  Pages                 The number of 4 KB pages to allocate.
> +
> +  @return A pointer to the allocated buffer or NULL if allocation fails.
> +
> +**/
> +VOID *
> +EFIAPI
> +EfiAllocatePeiAccessiblePages (
> +  IN EFI_MEMORY_TYPE  MemoryType,
> +  IN UINTN            Pages
> +  )
> +{
> +  EFI_STATUS            Status;
> +  EFI_PHYSICAL_ADDRESS  Memory;
> +  EFI_ALLOCATE_TYPE     AllocType;
> +
> +  if (Pages == 0) {
> +    return NULL;
> +  }
> +
> +#ifdef MDE_CPU_X64
> +  //
> +  // On X64 systems, a X64 build of DXE may be combined with a 32-bit build of
> +  // PEI, and so we need to allocate below 4 GB to ensure that the allocation
> +  // is accessible by PEI.
> +  //
> +  AllocType = AllocateMaxAddress;
> +  Memory = MAX_UINT32;
> +#else
> +  AllocType = AllocateAnyPages;
> +#endif
> +  Status = gBS->AllocatePages (AllocType, MemoryType, Pages, &Memory);
> +  if (EFI_ERROR (Status)) {
> +    return NULL;
> +  }
> +  return (VOID *)(UINTN)Memory;
> +}
> 

Re: [PATCH 3/6] MdePkg/UefiLib: introduce EfiAllocatePeiAccessiblePages routine

[Zeng, Star]

On 2018/5/23 3:11, Laszlo Ersek wrote:
> On 05/22/18 19:47, Ard Biesheuvel wrote:
> 
>> OK, to summarize:
>> - move the implementation of EfiAllocatePeiAccessiblePages() to
>> DxeServicesLib (and perhaps rename it to something more appropriate
>> for its new home)
>> - only restrict the X64 version to below 4 GB if EfiMemoryTop and
>> EfiFreeMemoryTop are both below 4 GB.

Just checking EfiFreeMemoryTop should be enough as EfiFreeMemoryTop 
means the capable TOP to be allocated.

And seemingly, there is an assumption here that next boot PEI S3 has 
same situation with current boot PEI.


Thanks,
Star
> 
> If this works with pure X64 OVMF (including S3 without SMM), I'd be very
> happy with it.
> 
> Also we should regression test whether IA32X64 OVMF continues working
> (including S3 without SMM, and S3 with SMM).
> 
> I'm glad to help with the testing once patches are posted.
> 
> OVMF installs the permanent PEI RAM in PublishPeiMemory()
> [OvmfPkg/PlatformPei/MemDetect.c]. It keeps things under 4GB, so I think
> this approach should work.
> 
> Thanks Ard & Mike!
> Laszlo
>