[PATCH] OvmfPkg: reserve igd memory by E820

["Corvin Köhne" <c.koehne@beckhoff.com>]

Hi all,

I'd like to discuss the following patch. At the moment, it's bhyve 
specific but I'd like to merge it into stock OVMF too. I'm working 
on GPU passthrough support for Intels integrated graphics devices 
(Intel calls it GVT-d). In order to get GVT-d working properly, 
the guest bios should allocate two memory areas (Graphics Stolen 
Memory and OpRegion). I'm trying to create a platform independent 
patch which allows to allocate those regions.

At the moment, I'm using an E820 table that bhyve provides to the 
guest. Another approach would be to use the QemuLoader which is 
used for loader Qemu's ACPI tables. While this has the advantage 
that the guest bios allocates the memory by it's own, the 
hypervisor has less control over the address. E.g. the ACRN 
hypervisor uses an indentical mapping for the Graphics Stolen 
Memory on Tiger Lake platforms and above (see 
https://github.com/projectacrn/acrn-hypervisor/blob/master/devicemodel/hw/pci/passthrough.c#L519-L523).

What do you think about this patch? Do you prefer using the E820 table 
or the QemuLoader? Do you have any other ideas?

Btw: I'm using the appended patch with an own bhyve implementation for 
several month now without any issues.

Here's the patch:

Detecting Graphics Stolen Memory and OpRegion address and length is
platform specific. OVMF shouldn't be platform specific. Move the
platform specific part to bhyve. Bhyve detects Graphics Stolen
Memory and OpRegion address and length and passes these
information as E820 table to the firmware. This will additionally
allow us to pass other platform specific memory ranges to the guest in
the future.

Signed-off-by: Corvin Köhne <CorvinK@beckhoff.com>
Reviewed-by: Patrick Bruenn <p.bruenn@beckhoff.com>
---
 OvmfPkg/Bhyve/BhyveBhfX64.dsc             |   1 +
 OvmfPkg/Bhyve/PlatformPei/MemDetect.c     |  78 ++++++++++++++++++
 OvmfPkg/Bhyve/PlatformPei/Platform.c      | 127 ++++++++++++++++++++++++++++++
 OvmfPkg/Bhyve/PlatformPei/PlatformPei.inf |   1 +
 4 files changed, 207 insertions(+)

diff --git a/OvmfPkg/Bhyve/BhyveBhfX64.dsc b/OvmfPkg/Bhyve/BhyveBhfX64.dsc
index 0a81b1de75..03d1fb50d9 100644
--- a/OvmfPkg/Bhyve/BhyveBhfX64.dsc
+++ b/OvmfPkg/Bhyve/BhyveBhfX64.dsc
@@ -284,6 +284,7 @@
 !endif
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
   PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
+  QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf
 
   MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
 
diff --git a/OvmfPkg/Bhyve/PlatformPei/MemDetect.c b/OvmfPkg/Bhyve/PlatformPei/MemDetect.c
index 1949e586a0..0681586927 100644
--- a/OvmfPkg/Bhyve/PlatformPei/MemDetect.c
+++ b/OvmfPkg/Bhyve/PlatformPei/MemDetect.c
@@ -30,6 +30,7 @@ Module Name:
 #include <Library/PcdLib.h>
 #include <Library/PciLib.h>
 #include <Library/PeimEntryPoint.h>
+#include <Library/QemuFwCfgLib.h>
 #include <Library/ResourcePublicationLib.h>
 #include <Library/MtrrLib.h>
 
@@ -357,6 +358,9 @@ PublishPeiMemory (
   VOID
   )
 {
+  EFI_E820_ENTRY64      E820Entry;
+  FIRMWARE_CONFIG_ITEM  Item;
+  UINTN                 ItemSize;
   EFI_STATUS            Status;
   EFI_PHYSICAL_ADDRESS  MemoryBase;
   UINT64                MemorySize;
@@ -418,6 +422,80 @@ PublishPeiMemory (
   }
 
   //
+  // Bhyve uses an E820 table to reserve certain kinds of memory like Graphics
+  // Stolen Memory. These reserved memory regions could overlap with the PEI
+  // core memory which leads to undefined behaviour. Check the E820 table for
+  // a memory region starting at or below MemoryBase which is equal or larger
+  // than MemorySize.
+  //
+  if (!EFI_ERROR (QemuFwCfgFindFile ("etc/e820", &Item, &ItemSize))) {
+    //
+    // Set a new base address based on E820 table.
+    //
+
+    EFI_PHYSICAL_ADDRESS  MaxAddress;
+    UINT64                EntryEnd;
+    UINT64                EndAddr;
+
+    if (ItemSize % sizeof (E820Entry) != 0) {
+      DEBUG ((DEBUG_INFO, "%a: invalid E820 size\n", __FUNCTION__));
+      return EFI_PROTOCOL_ERROR;
+    }
+
+    QemuFwCfgSelectItem (Item);
+
+    MaxAddress = MemoryBase + MemorySize;
+    MemoryBase = 0;
+
+    for (UINTN i = 0; i < ItemSize; i += sizeof (E820Entry)) {
+      QemuFwCfgReadBytes (sizeof (E820Entry), &E820Entry);
+
+      if (E820Entry.BaseAddr > MaxAddress) {
+        //
+        // E820 is always sorted in ascending order. For that reason, BaseAddr
+        // will always be larger than MaxAddress for the following entries.
+        //
+        break;
+      } else if (E820Entry.Type != EfiAcpiAddressRangeMemory) {
+        continue;
+      }
+
+      //
+      // Since MemoryBase should be at top of the memory, calculate the end
+      // address of the entry. Additionally, MemoryBase and MemorySize are page
+      // aligned. For that reason, page align EntryEnd too.
+      //
+      EntryEnd = (E820Entry.BaseAddr + E820Entry.Length) & ~EFI_PAGE_MASK;
+      if (E820Entry.BaseAddr > EntryEnd) {
+        //
+        // Either BaseAddr + Length overflows or the entry is smaller than a
+        // page. In both cases, we can't use it.
+        //
+        continue;
+      }
+
+      EndAddr = MIN (EntryEnd, MaxAddress);
+      if (EndAddr - E820Entry.BaseAddr < MemorySize) {
+        //
+        // E820 entry is too small for the Pei core memory.
+        //
+        continue;
+      }
+
+      MemoryBase = EndAddr - MemorySize;
+    }
+
+    if (MemoryBase == 0) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: Unable to find suitable PeiCore address\n",
+        __FUNCTION__
+        ));
+      return EFI_OUT_OF_RESOURCES;
+    }
+  }
+
+  //
   // Publish this memory to the PEI Core
   //
   Status = PublishSystemMemory (MemoryBase, MemorySize);
diff --git a/OvmfPkg/Bhyve/PlatformPei/Platform.c b/OvmfPkg/Bhyve/PlatformPei/Platform.c
index eba7c60fce..81062dbf44 100644
--- a/OvmfPkg/Bhyve/PlatformPei/Platform.c
+++ b/OvmfPkg/Bhyve/PlatformPei/Platform.c
@@ -27,6 +27,7 @@
 #include <Library/PciLib.h>
 #include <Library/PeimEntryPoint.h>
 #include <Library/PeiServicesLib.h>
+#include <Library/QemuFwCfgLib.h>
 #include <Library/ResourcePublicationLib.h>
 #include <Guid/MemoryTypeInformation.h>
 #include <Ppi/MasterBootMode.h>
@@ -491,6 +492,118 @@ DebugDumpCmos (
   }
 }
 
+STATIC
+EFI_STATUS
+E820ReserveMemory (
+  VOID
+  )
+{
+  EFI_E820_ENTRY64      E820Entry;
+  FIRMWARE_CONFIG_ITEM  Item;
+  UINTN                 Size;
+  EFI_STATUS            Status;
+
+  Status = QemuFwCfgFindFile ("etc/e820", &Item, &Size);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_INFO, "%a: E820 not found: %r\n", __FUNCTION__, Status));
+    return Status;
+  }
+
+  if (Size % sizeof (E820Entry) != 0) {
+    DEBUG ((DEBUG_INFO, "%a: invalid E820 size\n", __FUNCTION__));
+    return EFI_PROTOCOL_ERROR;
+  }
+
+  QemuFwCfgSelectItem (Item);
+  for (UINTN i = 0; i < Size; i += sizeof (E820Entry)) {
+    UINT64  Base;
+    UINT64  End;
+
+    QemuFwCfgReadBytes (sizeof (E820Entry), &E820Entry);
+
+    DEBUG_CODE (
+      CHAR8 *E820TypeName;
+      switch (E820Entry.Type) {
+      case EfiAcpiAddressRangeMemory:
+        E820TypeName = "RAM     ";
+        break;
+      case EfiAcpiAddressRangeReserved:
+        E820TypeName = "Reserved";
+        break;
+      case EfiAcpiAddressRangeACPI:
+        E820TypeName = "ACPI    ";
+        break;
+      case EfiAcpiAddressRangeNVS:
+        E820TypeName = "NVS     ";
+        break;
+      default:
+        E820TypeName = "Unknown ";
+        break;
+    }
+
+      DEBUG ((
+        DEBUG_INFO,
+        "E820 entry [ %16lx, %16lx] (%a)\n",
+        E820Entry.BaseAddr,
+        E820Entry.BaseAddr + E820Entry.Length,
+        E820TypeName
+        ));
+      );
+
+    //
+    // Round down base and round up length to page boundary
+    //
+    Base = E820Entry.BaseAddr & ~(UINT64)EFI_PAGE_MASK;
+    End  = ALIGN_VALUE (
+             E820Entry.BaseAddr + E820Entry.Length,
+             (UINT64)EFI_PAGE_SIZE
+             );
+
+    switch (E820Entry.Type) {
+      case EfiAcpiAddressRangeReserved:
+        BuildMemoryAllocationHob (
+          Base,
+          End - Base,
+          EfiReservedMemoryType
+          );
+        break;
+
+      case EfiAcpiAddressRangeACPI:
+        BuildMemoryAllocationHob (
+          Base,
+          End - Base,
+          EfiACPIReclaimMemory
+          );
+        break;
+
+      case EfiAcpiAddressRangeNVS:
+        BuildMemoryAllocationHob (
+          Base,
+          End - Base,
+          EfiACPIMemoryNVS
+          );
+        break;
+
+      case EfiAcpiAddressRangeMemory:
+        //
+        // EfiAcpiAddressRangeMemory is usable. Do not build an Allocation HOB.
+        //
+        break;
+
+      default:
+        DEBUG ((
+          DEBUG_ERROR,
+          "%a: Unknown E820 type %d\n",
+          __FUNCTION__,
+          E820Entry.Type
+          ));
+        return EFI_PROTOCOL_ERROR;
+    }
+  }
+
+  return EFI_SUCCESS;
+}
+
 VOID
 S3Verification (
   VOID
@@ -579,6 +692,10 @@ InitializePlatform (
   IN CONST EFI_PEI_SERVICES     **PeiServices
   )
 {
+  EFI_STATUS            Status;
+  FIRMWARE_CONFIG_ITEM  Item;
+  UINTN                 Size;
+
   DEBUG ((DEBUG_INFO, "Platform PEIM Loaded\n"));
 
   //
@@ -591,6 +708,16 @@ InitializePlatform (
 
   DebugDumpCmos ();
 
+  //
+  // If an E820 table exists, Memory should be reserved by E820.
+  //
+  if (!EFI_ERROR (QemuFwCfgFindFile ("etc/e820", &Item, &Size))) {
+    Status = E820ReserveMemory ();
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+  }
+
   BootModeInitialization ();
   AddressWidthInitialization ();
   MaxCpuCountInitialization ();
diff --git a/OvmfPkg/Bhyve/PlatformPei/PlatformPei.inf b/OvmfPkg/Bhyve/PlatformPei/PlatformPei.inf
index 739d63098b..557a8f890e 100644
--- a/OvmfPkg/Bhyve/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/Bhyve/PlatformPei/PlatformPei.inf
@@ -59,6 +59,7 @@
   PeiServicesLib
   PeiServicesTablePointerLib
   PcdLib
+  QemuFwCfgLib
   ResourcePublicationLib
 
 [Pcd]
-- 
2.11.0

Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075