[PATCH v2 0/4] OvmfPkg: check 64bit mmio window for resource conflicts

[PATCH v2 0/4] OvmfPkg: check 64bit mmio window for resource conflicts

[Gerd Hoffmann]

v2:
 - split up PlatformScanOrAdd64BitE820Ram() into scan function with
   callbacks, store results in PlatformInfoHob struct.

Gerd Hoffmann (4):
  OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB
  OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB
  OvmfPkg/PlatformInitLib: Add PlatformAddHobCB
  OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB

 OvmfPkg/Include/Library/PlatformInitLib.h     |   3 +-
 OvmfPkg/Library/PeilessStartupLib/Hob.c       |   3 +-
 .../PeilessStartupLib/PeilessStartup.c        |   7 +-
 OvmfPkg/Library/PlatformInitLib/MemDetect.c   | 328 ++++++++++--------
 OvmfPkg/Library/PlatformInitLib/Platform.c    |   7 +-
 OvmfPkg/PlatformPei/MemDetect.c               |   3 +-
 6 files changed, 192 insertions(+), 159 deletions(-)

-- 
2.39.0

[PATCH v2 1/4] OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB

[Gerd Hoffmann]

First step replacing the PlatformScanOrAdd64BitE820Ram() function.

Add a PlatformScanE820() function which loops over the e280 entries
from FwCfg and calls a callback for each of them.

Add a GetFirstNonAddressCB() function which will store the first free
address (right after the last RAM block) in
PlatformInfoHob->FirstNonAddress.  This replaces calls to
PlatformScanOrAdd64BitE820Ram() with non-NULL MaxAddress.

Also drop local FirstNonAddress variables and use
PlatformInfoHob->FirstNonAddress instead everywhere.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/Library/PlatformInitLib/MemDetect.c | 114 ++++++++++++++++----
 1 file changed, 91 insertions(+), 23 deletions(-)

diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index 0c4956852689..a2a4dc043f2e 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -251,6 +251,83 @@ PlatformScanOrAdd64BitE820Ram (
   return EFI_SUCCESS;
 }
 
+typedef VOID (*E820_SCAN_CALLBACK) (
+  EFI_E820_ENTRY64       *E820Entry,
+  EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
+  );
+
+/**
+  Store first address not used by e820 RAM entries in
+  PlatformInfoHob->FirstNonAddress
+**/
+VOID
+PlatformGetFirstNonAddressCB (
+  IN     EFI_E820_ENTRY64       *E820Entry,
+  IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
+  )
+{
+  UINT64  Candidate;
+
+  if (E820Entry->Type != EfiAcpiAddressRangeMemory) {
+    return;
+  }
+
+  Candidate = E820Entry->BaseAddr + E820Entry->Length;
+  if (PlatformInfoHob->FirstNonAddress < Candidate) {
+    DEBUG ((DEBUG_INFO, "%a: FirstNonAddress=0x%Lx\n", __FUNCTION__, Candidate));
+    PlatformInfoHob->FirstNonAddress = Candidate;
+  }
+}
+
+/**
+  Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map, call the
+  passed callback for each entry.
+
+  @param[in] Callback              The callback function to be called.
+
+  @param[in out]  PlatformInfoHob  PlatformInfo struct which is passed
+                                   through to the callback.
+
+  @retval EFI_SUCCESS              The fw_cfg E820 RAM map was found and processed.
+
+  @retval EFI_PROTOCOL_ERROR       The RAM map was found, but its size wasn't a
+                                   whole multiple of sizeof(EFI_E820_ENTRY64). No
+                                   RAM entry was processed.
+
+  @return                          Error codes from QemuFwCfgFindFile(). No RAM
+                                   entry was processed.
+**/
+STATIC
+EFI_STATUS
+PlatformScanE820 (
+  IN      E820_SCAN_CALLBACK     Callback,
+  IN OUT  EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
+  )
+{
+  EFI_STATUS            Status;
+  FIRMWARE_CONFIG_ITEM  FwCfgItem;
+  UINTN                 FwCfgSize;
+  EFI_E820_ENTRY64      E820Entry;
+  UINTN                 Processed;
+
+  Status = QemuFwCfgFindFile ("etc/e820", &FwCfgItem, &FwCfgSize);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  if (FwCfgSize % sizeof E820Entry != 0) {
+    return EFI_PROTOCOL_ERROR;
+  }
+
+  QemuFwCfgSelectItem (FwCfgItem);
+  for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) {
+    QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry);
+    Callback (&E820Entry, PlatformInfoHob);
+  }
+
+  return EFI_SUCCESS;
+}
+
 /**
   Returns PVH memmap
 
@@ -384,23 +461,17 @@ PlatformGetSystemMemorySizeAbove4gb (
   Return the highest address that DXE could possibly use, plus one.
 **/
 STATIC
-UINT64
+VOID
 PlatformGetFirstNonAddress (
   IN OUT  EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
   )
 {
-  UINT64                FirstNonAddress;
   UINT32                FwCfgPciMmio64Mb;
   EFI_STATUS            Status;
   FIRMWARE_CONFIG_ITEM  FwCfgItem;
   UINTN                 FwCfgSize;
   UINT64                HotPlugMemoryEnd;
 
-  //
-  // set FirstNonAddress to suppress incorrect compiler/analyzer warnings
-  //
-  FirstNonAddress = 0;
-
   //
   // If QEMU presents an E820 map, then get the highest exclusive >=4GB RAM
   // address from it. This can express an address >= 4GB+1TB.
@@ -408,9 +479,9 @@ PlatformGetFirstNonAddress (
   // Otherwise, get the flat size of the memory above 4GB from the CMOS (which
   // can only express a size smaller than 1TB), and add it to 4GB.
   //
-  Status = PlatformScanOrAdd64BitE820Ram (FALSE, NULL, &FirstNonAddress);
+  Status = PlatformScanE820 (PlatformGetFirstNonAddressCB, PlatformInfoHob);
   if (EFI_ERROR (Status)) {
-    FirstNonAddress = BASE_4GB + PlatformGetSystemMemorySizeAbove4gb ();
+    PlatformInfoHob->FirstNonAddress = BASE_4GB + PlatformGetSystemMemorySizeAbove4gb ();
   }
 
   //
@@ -420,7 +491,7 @@ PlatformGetFirstNonAddress (
   //
  #ifdef MDE_CPU_IA32
   if (!FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
-    return FirstNonAddress;
+    return;
   }
 
  #endif
@@ -473,7 +544,7 @@ PlatformGetFirstNonAddress (
     // determines the highest address plus one. The memory hotplug area (see
     // below) plays no role for the firmware in this case.
     //
-    return FirstNonAddress;
+    return;
   }
 
   //
@@ -497,15 +568,15 @@ PlatformGetFirstNonAddress (
       HotPlugMemoryEnd
       ));
 
-    ASSERT (HotPlugMemoryEnd >= FirstNonAddress);
-    FirstNonAddress = HotPlugMemoryEnd;
+    ASSERT (HotPlugMemoryEnd >= PlatformInfoHob->FirstNonAddress);
+    PlatformInfoHob->FirstNonAddress = HotPlugMemoryEnd;
   }
 
   //
   // SeaBIOS aligns both boundaries of the 64-bit PCI host aperture to 1GB, so
   // that the host can map it with 1GB hugepages. Follow suit.
   //
-  PlatformInfoHob->PcdPciMmio64Base = ALIGN_VALUE (FirstNonAddress, (UINT64)SIZE_1GB);
+  PlatformInfoHob->PcdPciMmio64Base = ALIGN_VALUE (PlatformInfoHob->FirstNonAddress, (UINT64)SIZE_1GB);
   PlatformInfoHob->PcdPciMmio64Size = ALIGN_VALUE (PlatformInfoHob->PcdPciMmio64Size, (UINT64)SIZE_1GB);
 
   //
@@ -519,8 +590,8 @@ PlatformGetFirstNonAddress (
   //
   // The useful address space ends with the 64-bit PCI host aperture.
   //
-  FirstNonAddress = PlatformInfoHob->PcdPciMmio64Base + PlatformInfoHob->PcdPciMmio64Size;
-  return FirstNonAddress;
+  PlatformInfoHob->FirstNonAddress = PlatformInfoHob->PcdPciMmio64Base + PlatformInfoHob->PcdPciMmio64Size;
+  return;
 }
 
 /*
@@ -781,7 +852,6 @@ PlatformAddressWidthInitialization (
   IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
   )
 {
-  UINT64      FirstNonAddress;
   UINT8       PhysMemAddressWidth;
   EFI_STATUS  Status;
 
@@ -794,7 +864,7 @@ PlatformAddressWidthInitialization (
   // First scan host-provided hardware information to assess if the address
   // space is already known. If so, guest must use those values.
   //
-  Status = PlatformScanHostProvided64BitPciMmioEnd (&FirstNonAddress);
+  Status = PlatformScanHostProvided64BitPciMmioEnd (&PlatformInfoHob->FirstNonAddress);
 
   if (EFI_ERROR (Status)) {
     //
@@ -806,13 +876,12 @@ PlatformAddressWidthInitialization (
     // The DXL IPL keys off of the physical address bits advertized in the CPU
     // HOB. To conserve memory, we calculate the minimum address width here.
     //
-    FirstNonAddress = PlatformGetFirstNonAddress (PlatformInfoHob);
+    PlatformGetFirstNonAddress (PlatformInfoHob);
   }
 
   PlatformAddressWidthFromCpuid (PlatformInfoHob, TRUE);
   if (PlatformInfoHob->PhysMemAddressWidth != 0) {
     // physical address width is known
-    PlatformInfoHob->FirstNonAddress = FirstNonAddress;
     PlatformDynamicMmioWindow (PlatformInfoHob);
     return;
   }
@@ -823,13 +892,13 @@ PlatformAddressWidthInitialization (
   //   -> try be conservstibe to stay below the guaranteed minimum of
   //      36 phys bits (aka 64 GB).
   //
-  PhysMemAddressWidth = (UINT8)HighBitSet64 (FirstNonAddress);
+  PhysMemAddressWidth = (UINT8)HighBitSet64 (PlatformInfoHob->FirstNonAddress);
 
   //
   // If FirstNonAddress is not an integral power of two, then we need an
   // additional bit.
   //
-  if ((FirstNonAddress & (FirstNonAddress - 1)) != 0) {
+  if ((PlatformInfoHob->FirstNonAddress & (PlatformInfoHob->FirstNonAddress - 1)) != 0) {
     ++PhysMemAddressWidth;
   }
 
@@ -857,7 +926,6 @@ PlatformAddressWidthInitialization (
   ASSERT (PhysMemAddressWidth <= 48);
  #endif
 
-  PlatformInfoHob->FirstNonAddress     = FirstNonAddress;
   PlatformInfoHob->PhysMemAddressWidth = PhysMemAddressWidth;
 }
 
-- 
2.39.0

Re: [PATCH v2 1/4] OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB

[Laszlo Ersek]

On 1/10/23 09:21, Gerd Hoffmann wrote:
> First step replacing the PlatformScanOrAdd64BitE820Ram() function.
> 
> Add a PlatformScanE820() function which loops over the e280 entries
> from FwCfg and calls a callback for each of them.
> 
> Add a GetFirstNonAddressCB() function which will store the first free
> address (right after the last RAM block) in
> PlatformInfoHob->FirstNonAddress.  This replaces calls to
> PlatformScanOrAdd64BitE820Ram() with non-NULL MaxAddress.
> 
> Also drop local FirstNonAddress variables and use
> PlatformInfoHob->FirstNonAddress instead everywhere.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/Library/PlatformInitLib/MemDetect.c | 114 ++++++++++++++++----
>  1 file changed, 91 insertions(+), 23 deletions(-)
> 
> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index 0c4956852689..a2a4dc043f2e 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> @@ -251,6 +251,83 @@ PlatformScanOrAdd64BitE820Ram (
>    return EFI_SUCCESS;
>  }
>  
> +typedef VOID (*E820_SCAN_CALLBACK) (
> +  EFI_E820_ENTRY64       *E820Entry,
> +  EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
> +  );
> +
> +/**
> +  Store first address not used by e820 RAM entries in
> +  PlatformInfoHob->FirstNonAddress
> +**/
> +VOID
> +PlatformGetFirstNonAddressCB (
> +  IN     EFI_E820_ENTRY64       *E820Entry,
> +  IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
> +  )
> +{
> +  UINT64  Candidate;
> +
> +  if (E820Entry->Type != EfiAcpiAddressRangeMemory) {
> +    return;
> +  }
> +
> +  Candidate = E820Entry->BaseAddr + E820Entry->Length;
> +  if (PlatformInfoHob->FirstNonAddress < Candidate) {
> +    DEBUG ((DEBUG_INFO, "%a: FirstNonAddress=0x%Lx\n", __FUNCTION__, Candidate));
> +    PlatformInfoHob->FirstNonAddress = Candidate;
> +  }
> +}
> +
> +/**
> +  Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map, call the
> +  passed callback for each entry.
> +
> +  @param[in] Callback              The callback function to be called.
> +
> +  @param[in out]  PlatformInfoHob  PlatformInfo struct which is passed
> +                                   through to the callback.
> +
> +  @retval EFI_SUCCESS              The fw_cfg E820 RAM map was found and processed.
> +
> +  @retval EFI_PROTOCOL_ERROR       The RAM map was found, but its size wasn't a
> +                                   whole multiple of sizeof(EFI_E820_ENTRY64). No
> +                                   RAM entry was processed.
> +
> +  @return                          Error codes from QemuFwCfgFindFile(). No RAM
> +                                   entry was processed.
> +**/

(1) I suggest removing the "RAM" references from the above, now that the
type checking is being moved to the callbacks. I think we can just say
"entries" and "entry", without "RAM". Not a huge deal though.

> +STATIC
> +EFI_STATUS
> +PlatformScanE820 (
> +  IN      E820_SCAN_CALLBACK     Callback,
> +  IN OUT  EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
> +  )
> +{
> +  EFI_STATUS            Status;
> +  FIRMWARE_CONFIG_ITEM  FwCfgItem;
> +  UINTN                 FwCfgSize;
> +  EFI_E820_ENTRY64      E820Entry;
> +  UINTN                 Processed;
> +
> +  Status = QemuFwCfgFindFile ("etc/e820", &FwCfgItem, &FwCfgSize);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (FwCfgSize % sizeof E820Entry != 0) {
> +    return EFI_PROTOCOL_ERROR;
> +  }
> +
> +  QemuFwCfgSelectItem (FwCfgItem);
> +  for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) {
> +    QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry);
> +    Callback (&E820Entry, PlatformInfoHob);
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
>  /**
>    Returns PVH memmap
>  
> @@ -384,23 +461,17 @@ PlatformGetSystemMemorySizeAbove4gb (
>    Return the highest address that DXE could possibly use, plus one.
>  **/
>  STATIC
> -UINT64
> +VOID
>  PlatformGetFirstNonAddress (
>    IN OUT  EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
>    )
>  {
> -  UINT64                FirstNonAddress;
>    UINT32                FwCfgPciMmio64Mb;
>    EFI_STATUS            Status;
>    FIRMWARE_CONFIG_ITEM  FwCfgItem;
>    UINTN                 FwCfgSize;
>    UINT64                HotPlugMemoryEnd;
>  
> -  //
> -  // set FirstNonAddress to suppress incorrect compiler/analyzer warnings
> -  //
> -  FirstNonAddress = 0;
> -
>    //
>    // If QEMU presents an E820 map, then get the highest exclusive >=4GB RAM
>    // address from it. This can express an address >= 4GB+1TB.
> @@ -408,9 +479,9 @@ PlatformGetFirstNonAddress (
>    // Otherwise, get the flat size of the memory above 4GB from the CMOS (which
>    // can only express a size smaller than 1TB), and add it to 4GB.
>    //
> -  Status = PlatformScanOrAdd64BitE820Ram (FALSE, NULL, &FirstNonAddress);
> +  Status = PlatformScanE820 (PlatformGetFirstNonAddressCB, PlatformInfoHob);

(2) There is one omission in this patch. Namely, we're performing a
conditional maximum search, and because it is conditional, it is
possible that we find zero candidates. In that case, we still need to
have "PlatformInfoHob->FirstNonAddress" set to a sane value.

The original code handles this by initializing FirstNonAddress to 4GB:

  if (MaxAddress != NULL) {
    *MaxAddress = BASE_4GB;
  }

Which is consistent with the comments that *remain* in the code now.

So my request is that, right before this call to PlatformScanE820(),
please set

  PlatformInfoHob->FirstNonAddress = BASE_4GB;

With (2) updated, and (1) updated or not:

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

Thanks,
Laszlo


>    if (EFI_ERROR (Status)) {
> -    FirstNonAddress = BASE_4GB + PlatformGetSystemMemorySizeAbove4gb ();
> +    PlatformInfoHob->FirstNonAddress = BASE_4GB + PlatformGetSystemMemorySizeAbove4gb ();
>    }
>  
>    //
> @@ -420,7 +491,7 @@ PlatformGetFirstNonAddress (
>    //
>   #ifdef MDE_CPU_IA32
>    if (!FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
> -    return FirstNonAddress;
> +    return;
>    }
>  
>   #endif
> @@ -473,7 +544,7 @@ PlatformGetFirstNonAddress (
>      // determines the highest address plus one. The memory hotplug area (see
>      // below) plays no role for the firmware in this case.
>      //
> -    return FirstNonAddress;
> +    return;
>    }
>  
>    //
> @@ -497,15 +568,15 @@ PlatformGetFirstNonAddress (
>        HotPlugMemoryEnd
>        ));
>  
> -    ASSERT (HotPlugMemoryEnd >= FirstNonAddress);
> -    FirstNonAddress = HotPlugMemoryEnd;
> +    ASSERT (HotPlugMemoryEnd >= PlatformInfoHob->FirstNonAddress);
> +    PlatformInfoHob->FirstNonAddress = HotPlugMemoryEnd;
>    }
>  
>    //
>    // SeaBIOS aligns both boundaries of the 64-bit PCI host aperture to 1GB, so
>    // that the host can map it with 1GB hugepages. Follow suit.
>    //
> -  PlatformInfoHob->PcdPciMmio64Base = ALIGN_VALUE (FirstNonAddress, (UINT64)SIZE_1GB);
> +  PlatformInfoHob->PcdPciMmio64Base = ALIGN_VALUE (PlatformInfoHob->FirstNonAddress, (UINT64)SIZE_1GB);
>    PlatformInfoHob->PcdPciMmio64Size = ALIGN_VALUE (PlatformInfoHob->PcdPciMmio64Size, (UINT64)SIZE_1GB);
>  
>    //
> @@ -519,8 +590,8 @@ PlatformGetFirstNonAddress (
>    //
>    // The useful address space ends with the 64-bit PCI host aperture.
>    //
> -  FirstNonAddress = PlatformInfoHob->PcdPciMmio64Base + PlatformInfoHob->PcdPciMmio64Size;
> -  return FirstNonAddress;
> +  PlatformInfoHob->FirstNonAddress = PlatformInfoHob->PcdPciMmio64Base + PlatformInfoHob->PcdPciMmio64Size;
> +  return;
>  }
>  
>  /*
> @@ -781,7 +852,6 @@ PlatformAddressWidthInitialization (
>    IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
>    )
>  {
> -  UINT64      FirstNonAddress;
>    UINT8       PhysMemAddressWidth;
>    EFI_STATUS  Status;
>  
> @@ -794,7 +864,7 @@ PlatformAddressWidthInitialization (
>    // First scan host-provided hardware information to assess if the address
>    // space is already known. If so, guest must use those values.
>    //
> -  Status = PlatformScanHostProvided64BitPciMmioEnd (&FirstNonAddress);
> +  Status = PlatformScanHostProvided64BitPciMmioEnd (&PlatformInfoHob->FirstNonAddress);
>  
>    if (EFI_ERROR (Status)) {
>      //
> @@ -806,13 +876,12 @@ PlatformAddressWidthInitialization (
>      // The DXL IPL keys off of the physical address bits advertized in the CPU
>      // HOB. To conserve memory, we calculate the minimum address width here.
>      //
> -    FirstNonAddress = PlatformGetFirstNonAddress (PlatformInfoHob);
> +    PlatformGetFirstNonAddress (PlatformInfoHob);
>    }
>  
>    PlatformAddressWidthFromCpuid (PlatformInfoHob, TRUE);
>    if (PlatformInfoHob->PhysMemAddressWidth != 0) {
>      // physical address width is known
> -    PlatformInfoHob->FirstNonAddress = FirstNonAddress;
>      PlatformDynamicMmioWindow (PlatformInfoHob);
>      return;
>    }
> @@ -823,13 +892,13 @@ PlatformAddressWidthInitialization (
>    //   -> try be conservstibe to stay below the guaranteed minimum of
>    //      36 phys bits (aka 64 GB).
>    //
> -  PhysMemAddressWidth = (UINT8)HighBitSet64 (FirstNonAddress);
> +  PhysMemAddressWidth = (UINT8)HighBitSet64 (PlatformInfoHob->FirstNonAddress);
>  
>    //
>    // If FirstNonAddress is not an integral power of two, then we need an
>    // additional bit.
>    //
> -  if ((FirstNonAddress & (FirstNonAddress - 1)) != 0) {
> +  if ((PlatformInfoHob->FirstNonAddress & (PlatformInfoHob->FirstNonAddress - 1)) != 0) {
>      ++PhysMemAddressWidth;
>    }
>  
> @@ -857,7 +926,6 @@ PlatformAddressWidthInitialization (
>    ASSERT (PhysMemAddressWidth <= 48);
>   #endif
>  
> -  PlatformInfoHob->FirstNonAddress     = FirstNonAddress;
>    PlatformInfoHob->PhysMemAddressWidth = PhysMemAddressWidth;
>  }
>  

[PATCH v2 2/4] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB

[Gerd Hoffmann]

Add PlatformGetLowMemoryCB() callback function for use with
PlatformScanE820().  It stores the low memory size in
PlatformInfoHob->LowMemory.  This replaces calls to
PlatformScanOrAdd64BitE820Ram() with non-NULL LowMemory.

Also change PlatformGetSystemMemorySizeBelow4gb() to likewise set
PlatformInfoHob->LowMemory instead of returning the value.  Update
all Callers to the new convention.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/Include/Library/PlatformInitLib.h     |  3 +-
 OvmfPkg/Library/PeilessStartupLib/Hob.c       |  3 +-
 .../PeilessStartupLib/PeilessStartup.c        |  7 +-
 OvmfPkg/Library/PlatformInitLib/MemDetect.c   | 69 +++++++++++++------
 OvmfPkg/Library/PlatformInitLib/Platform.c    |  7 +-
 OvmfPkg/PlatformPei/MemDetect.c               |  3 +-
 6 files changed, 59 insertions(+), 33 deletions(-)

diff --git a/OvmfPkg/Include/Library/PlatformInitLib.h b/OvmfPkg/Include/Library/PlatformInitLib.h
index bf6f90a5761c..051b31191194 100644
--- a/OvmfPkg/Include/Library/PlatformInitLib.h
+++ b/OvmfPkg/Include/Library/PlatformInitLib.h
@@ -26,6 +26,7 @@ typedef struct {
   BOOLEAN              Q35SmramAtDefaultSmbase;
   UINT16               Q35TsegMbytes;
 
+  UINT32               LowMemory;
   UINT64               FirstNonAddress;
   UINT8                PhysMemAddressWidth;
   UINT32               Uc32Base;
@@ -144,7 +145,7 @@ PlatformQemuUc32BaseInitialization (
   IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
   );
 
-UINT32
+VOID
 EFIAPI
 PlatformGetSystemMemorySizeBelow4gb (
   IN EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
diff --git a/OvmfPkg/Library/PeilessStartupLib/Hob.c b/OvmfPkg/Library/PeilessStartupLib/Hob.c
index 630ce445ebec..784a8ba194de 100644
--- a/OvmfPkg/Library/PeilessStartupLib/Hob.c
+++ b/OvmfPkg/Library/PeilessStartupLib/Hob.c
@@ -41,8 +41,9 @@ ConstructSecHobList (
   EFI_HOB_PLATFORM_INFO       PlatformInfoHob;
 
   ZeroMem (&PlatformInfoHob, sizeof (PlatformInfoHob));
+  PlatformGetSystemMemorySizeBelow4gb (&PlatformInfoHob);
   PlatformInfoHob.HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
-  LowMemorySize                   = PlatformGetSystemMemorySizeBelow4gb (&PlatformInfoHob);
+  LowMemorySize                   = PlatformInfoHob.LowMemory;
   ASSERT (LowMemorySize != 0);
   LowMemoryStart = FixedPcdGet32 (PcdOvmfDxeMemFvBase) + FixedPcdGet32 (PcdOvmfDxeMemFvSize);
   LowMemorySize -= LowMemoryStart;
diff --git a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c b/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
index 380e71597206..928120d183ba 100644
--- a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
+++ b/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
@@ -41,8 +41,7 @@ InitializePlatform (
   EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
   )
 {
-  UINT32  LowerMemorySize;
-  VOID    *VariableStore;
+  VOID  *VariableStore;
 
   DEBUG ((DEBUG_INFO, "InitializePlatform in Pei-less boot\n"));
   PlatformDebugDumpCmos ();
@@ -70,14 +69,14 @@ InitializePlatform (
     PlatformInfoHob->PcdCpuBootLogicalProcessorNumber
     ));
 
-  LowerMemorySize = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
+  PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
   PlatformQemuUc32BaseInitialization (PlatformInfoHob);
   DEBUG ((
     DEBUG_INFO,
     "Uc32Base = 0x%x, Uc32Size = 0x%x, LowerMemorySize = 0x%x\n",
     PlatformInfoHob->Uc32Base,
     PlatformInfoHob->Uc32Size,
-    LowerMemorySize
+    PlatformInfoHob->LowMemory
     ));
 
   VariableStore                                  = PlatformReserveEmuVariableNvStore ();
diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index a2a4dc043f2e..63329c4e796a 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -51,18 +51,16 @@ PlatformQemuUc32BaseInitialization (
   IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
   )
 {
-  UINT32  LowerMemorySize;
-
   if (PlatformInfoHob->HostBridgeDevId == 0xffff /* microvm */) {
     return;
   }
 
   if (PlatformInfoHob->HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
-    LowerMemorySize = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
+    PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
     ASSERT (PcdGet64 (PcdPciExpressBaseAddress) <= MAX_UINT32);
-    ASSERT (PcdGet64 (PcdPciExpressBaseAddress) >= LowerMemorySize);
+    ASSERT (PcdGet64 (PcdPciExpressBaseAddress) >= PlatformInfoHob->LowMemory);
 
-    if (LowerMemorySize <= BASE_2GB) {
+    if (PlatformInfoHob->LowMemory <= BASE_2GB) {
       // Newer qemu with gigabyte aligned memory,
       // 32-bit pci mmio window is 2G -> 4G then.
       PlatformInfoHob->Uc32Base = BASE_2GB;
@@ -92,8 +90,8 @@ PlatformQemuUc32BaseInitialization (
   // variable MTRR suffices by truncating the size to a whole power of two,
   // while keeping the end affixed to 4GB. This will round the base up.
   //
-  LowerMemorySize           = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
-  PlatformInfoHob->Uc32Size = GetPowerOfTwo32 ((UINT32)(SIZE_4GB - LowerMemorySize));
+  PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
+  PlatformInfoHob->Uc32Size = GetPowerOfTwo32 ((UINT32)(SIZE_4GB - PlatformInfoHob->LowMemory));
   PlatformInfoHob->Uc32Base = (UINT32)(SIZE_4GB - PlatformInfoHob->Uc32Size);
   //
   // Assuming that LowerMemorySize is at least 1 byte, Uc32Size is at most 2GB.
@@ -101,13 +99,13 @@ PlatformQemuUc32BaseInitialization (
   //
   ASSERT (PlatformInfoHob->Uc32Base >= BASE_2GB);
 
-  if (PlatformInfoHob->Uc32Base != LowerMemorySize) {
+  if (PlatformInfoHob->Uc32Base != PlatformInfoHob->LowMemory) {
     DEBUG ((
       DEBUG_VERBOSE,
       "%a: rounded UC32 base from 0x%x up to 0x%x, for "
       "an UC32 size of 0x%x\n",
       __FUNCTION__,
-      LowerMemorySize,
+      PlatformInfoHob->LowMemory,
       PlatformInfoHob->Uc32Base,
       PlatformInfoHob->Uc32Size
       ));
@@ -279,6 +277,33 @@ PlatformGetFirstNonAddressCB (
   }
 }
 
+/**
+  Store the low (below 4G) memory size in
+  PlatformInfoHob->LowMemory
+**/
+VOID
+PlatformGetLowMemoryCB (
+  IN     EFI_E820_ENTRY64       *E820Entry,
+  IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
+  )
+{
+  UINT64  Candidate;
+
+  if (E820Entry->Type != EfiAcpiAddressRangeMemory) {
+    return;
+  }
+
+  Candidate = E820Entry->BaseAddr + E820Entry->Length;
+  if (Candidate >= BASE_4GB) {
+    return;
+  }
+
+  if (PlatformInfoHob->LowMemory < Candidate) {
+    DEBUG ((DEBUG_INFO, "%a: LowMemory=0x%Lx\n", __FUNCTION__, Candidate));
+    PlatformInfoHob->LowMemory = Candidate;
+  }
+}
+
 /**
   Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map, call the
   passed callback for each entry.
@@ -395,14 +420,13 @@ GetHighestSystemMemoryAddressFromPvhMemmap (
   return HighestAddress;
 }
 
-UINT32
+VOID
 EFIAPI
 PlatformGetSystemMemorySizeBelow4gb (
   IN EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
   )
 {
   EFI_STATUS  Status;
-  UINT64      LowerMemorySize = 0;
   UINT8       Cmos0x34;
   UINT8       Cmos0x35;
 
@@ -410,12 +434,13 @@ PlatformGetSystemMemorySizeBelow4gb (
       (CcProbe () != CcGuestTypeIntelTdx))
   {
     // Get the information from PVH memmap
-    return (UINT32)GetHighestSystemMemoryAddressFromPvhMemmap (TRUE);
+    PlatformInfoHob->LowMemory = GetHighestSystemMemoryAddressFromPvhMemmap (TRUE);
+    return;
   }
 
-  Status = PlatformScanOrAdd64BitE820Ram (FALSE, &LowerMemorySize, NULL);
-  if ((Status == EFI_SUCCESS) && (LowerMemorySize > 0)) {
-    return (UINT32)LowerMemorySize;
+  Status = PlatformScanE820 (PlatformGetLowMemoryCB, PlatformInfoHob);
+  if ((Status == EFI_SUCCESS) && (PlatformInfoHob->LowMemory > 0)) {
+    return;
   }
 
   //
@@ -430,7 +455,7 @@ PlatformGetSystemMemorySizeBelow4gb (
   Cmos0x34 = (UINT8)PlatformCmosRead8 (0x34);
   Cmos0x35 = (UINT8)PlatformCmosRead8 (0x35);
 
-  return (UINT32)(((UINTN)((Cmos0x35 << 8) + Cmos0x34) << 16) + SIZE_16MB);
+  PlatformInfoHob->LowMemory = ((((UINTN)Cmos0x35 << 8) + Cmos0x34) << 16) + SIZE_16MB;
 }
 
 STATIC
@@ -965,7 +990,6 @@ PlatformQemuInitializeRam (
   IN EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
   )
 {
-  UINT64         LowerMemorySize;
   UINT64         UpperMemorySize;
   MTRR_SETTINGS  MtrrSettings;
   EFI_STATUS     Status;
@@ -975,7 +999,7 @@ PlatformQemuInitializeRam (
   //
   // Determine total memory size available
   //
-  LowerMemorySize = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
+  PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
 
   if (PlatformInfoHob->BootMode == BOOT_ON_S3_RESUME) {
     //
@@ -1009,14 +1033,14 @@ PlatformQemuInitializeRam (
       UINT32  TsegSize;
 
       TsegSize = PlatformInfoHob->Q35TsegMbytes * SIZE_1MB;
-      PlatformAddMemoryRangeHob (BASE_1MB, LowerMemorySize - TsegSize);
+      PlatformAddMemoryRangeHob (BASE_1MB, PlatformInfoHob->LowMemory - TsegSize);
       PlatformAddReservedMemoryBaseSizeHob (
-        LowerMemorySize - TsegSize,
+        PlatformInfoHob->LowMemory - TsegSize,
         TsegSize,
         TRUE
         );
     } else {
-      PlatformAddMemoryRangeHob (BASE_1MB, LowerMemorySize);
+      PlatformAddMemoryRangeHob (BASE_1MB, PlatformInfoHob->LowMemory);
     }
 
     //
@@ -1194,9 +1218,10 @@ PlatformQemuInitializeRamForS3 (
       // Make sure the TSEG area that we reported as a reserved memory resource
       // cannot be used for reserved memory allocations.
       //
+      PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
       TsegSize = PlatformInfoHob->Q35TsegMbytes * SIZE_1MB;
       BuildMemoryAllocationHob (
-        PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob) - TsegSize,
+        PlatformInfoHob->LowMemory - TsegSize,
         TsegSize,
         EfiReservedMemoryType
         );
diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c b/OvmfPkg/Library/PlatformInitLib/Platform.c
index 3e13c5d4b34f..9ab0342fd8c0 100644
--- a/OvmfPkg/Library/PlatformInitLib/Platform.c
+++ b/OvmfPkg/Library/PlatformInitLib/Platform.c
@@ -128,7 +128,6 @@ PlatformMemMapInitialization (
 {
   UINT64  PciIoBase;
   UINT64  PciIoSize;
-  UINT32  TopOfLowRam;
   UINT64  PciExBarBase;
   UINT32  PciBase;
   UINT32  PciSize;
@@ -150,7 +149,7 @@ PlatformMemMapInitialization (
     return;
   }
 
-  TopOfLowRam  = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
+  PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
   PciExBarBase = 0;
   if (PlatformInfoHob->HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
     //
@@ -158,11 +157,11 @@ PlatformMemMapInitialization (
     // the base of the 32-bit PCI host aperture.
     //
     PciExBarBase = PcdGet64 (PcdPciExpressBaseAddress);
-    ASSERT (TopOfLowRam <= PciExBarBase);
+    ASSERT (PlatformInfoHob->LowMemory <= PciExBarBase);
     ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
     PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
   } else {
-    ASSERT (TopOfLowRam <= PlatformInfoHob->Uc32Base);
+    ASSERT (PlatformInfoHob->LowMemory <= PlatformInfoHob->Uc32Base);
     PciBase = PlatformInfoHob->Uc32Base;
   }
 
diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 3d8375320dcb..41d186986ba8 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -271,7 +271,8 @@ PublishPeiMemory (
   UINT32                S3AcpiReservedMemoryBase;
   UINT32                S3AcpiReservedMemorySize;
 
-  LowerMemorySize = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
+  PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
+  LowerMemorySize = PlatformInfoHob->LowMemory;
   if (PlatformInfoHob->SmmSmramRequire) {
     //
     // TSEG is chipped from the end of low RAM
-- 
2.39.0

Re: [PATCH v2 2/4] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB

[Laszlo Ersek]

Quoting the hunks out of order:

On 1/10/23 09:21, Gerd Hoffmann wrote:
> Add PlatformGetLowMemoryCB() callback function for use with
> PlatformScanE820().  It stores the low memory size in
> PlatformInfoHob->LowMemory.  This replaces calls to
> PlatformScanOrAdd64BitE820Ram() with non-NULL LowMemory.
>
> Also change PlatformGetSystemMemorySizeBelow4gb() to likewise set
> PlatformInfoHob->LowMemory instead of returning the value.  Update
> all Callers to the new convention.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/Include/Library/PlatformInitLib.h     |  3 +-
>  OvmfPkg/Library/PeilessStartupLib/Hob.c       |  3 +-
>  .../PeilessStartupLib/PeilessStartup.c        |  7 +-
>  OvmfPkg/Library/PlatformInitLib/MemDetect.c   | 69 +++++++++++++------
>  OvmfPkg/Library/PlatformInitLib/Platform.c    |  7 +-
>  OvmfPkg/PlatformPei/MemDetect.c               |  3 +-
>  6 files changed, 59 insertions(+), 33 deletions(-)
>
> diff --git a/OvmfPkg/Include/Library/PlatformInitLib.h b/OvmfPkg/Include/Library/PlatformInitLib.h
> index bf6f90a5761c..051b31191194 100644
> --- a/OvmfPkg/Include/Library/PlatformInitLib.h
> +++ b/OvmfPkg/Include/Library/PlatformInitLib.h
> @@ -26,6 +26,7 @@ typedef struct {
>    BOOLEAN              Q35SmramAtDefaultSmbase;
>    UINT16               Q35TsegMbytes;
>
> +  UINT32               LowMemory;
>    UINT64               FirstNonAddress;
>    UINT8                PhysMemAddressWidth;
>    UINT32               Uc32Base;
> @@ -144,7 +145,7 @@ PlatformQemuUc32BaseInitialization (
>    IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
>    );
>
> -UINT32
> +VOID
>  EFIAPI
>  PlatformGetSystemMemorySizeBelow4gb (
>    IN EFI_HOB_PLATFORM_INFO  *PlatformInfoHob

OK.

> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index a2a4dc043f2e..63329c4e796a 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c

> @@ -279,6 +277,33 @@ PlatformGetFirstNonAddressCB (
>    }
>  }
>
> +/**
> +  Store the low (below 4G) memory size in
> +  PlatformInfoHob->LowMemory
> +**/
> +VOID
> +PlatformGetLowMemoryCB (
> +  IN     EFI_E820_ENTRY64       *E820Entry,
> +  IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
> +  )
> +{
> +  UINT64  Candidate;
> +
> +  if (E820Entry->Type != EfiAcpiAddressRangeMemory) {
> +    return;
> +  }
> +
> +  Candidate = E820Entry->BaseAddr + E820Entry->Length;
> +  if (Candidate >= BASE_4GB) {
> +    return;
> +  }
> +
> +  if (PlatformInfoHob->LowMemory < Candidate) {
> +    DEBUG ((DEBUG_INFO, "%a: LowMemory=0x%Lx\n", __FUNCTION__, Candidate));
> +    PlatformInfoHob->LowMemory = Candidate;
> +  }
> +}
> +
>  /**
>    Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map, call the
>    passed callback for each entry.

(1) This looks like a faithful conversion / extraction, but I'd vaguely
noticed something earlier, in the original code. Namely, when the
exclusive end of the range is exactly 4GB, that should still qualify as
low memory, shouldn't it?

Not that it matters in practice, because just below 4GB, we'll never
ever have RAM on QEMU (pc or q35 -- I think even microvm, too). But, for
clarity, changing the limit condition as a separate patch (afterwards)
might make sense.

For now, this conversion is faithful.

> @@ -395,14 +420,13 @@ GetHighestSystemMemoryAddressFromPvhMemmap (
>    return HighestAddress;
>  }
>
> -UINT32
> +VOID
>  EFIAPI
>  PlatformGetSystemMemorySizeBelow4gb (
>    IN EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
>    )
>  {
>    EFI_STATUS  Status;
> -  UINT64      LowerMemorySize = 0;
>    UINT8       Cmos0x34;
>    UINT8       Cmos0x35;
>
> @@ -410,12 +434,13 @@ PlatformGetSystemMemorySizeBelow4gb (
>        (CcProbe () != CcGuestTypeIntelTdx))
>    {
>      // Get the information from PVH memmap
> -    return (UINT32)GetHighestSystemMemoryAddressFromPvhMemmap (TRUE);
> +    PlatformInfoHob->LowMemory = GetHighestSystemMemoryAddressFromPvhMemmap (TRUE);
> +    return;
>    }

OK, so the way this function looks now is new to me. :)

(2) Here you are converting a UINT64 from
GetHighestSystemMemoryAddressFromPvhMemmap() to UINT32; I think that
might trip up some MSVC compilers. I suggest preserving the cast.

>
> -  Status = PlatformScanOrAdd64BitE820Ram (FALSE, &LowerMemorySize, NULL);
> -  if ((Status == EFI_SUCCESS) && (LowerMemorySize > 0)) {
> -    return (UINT32)LowerMemorySize;
> +  Status = PlatformScanE820 (PlatformGetLowMemoryCB, PlatformInfoHob);

(3) Similar comment as under the previous patch: please set

  PlatformInfoHob->LowMemory = 0;

before calling PlatformScanE820(), to preserve

  if (LowMemory != NULL) {
    *LowMemory = 0;
  }

from PlatformScanOrAdd64BitE820Ram().

(I realize the platform info HOB could be zero-filled upon allocation --
but then at least for consistency with the 4GB+ search initialization, a
comment could be justified.)

> +  if ((Status == EFI_SUCCESS) && (PlatformInfoHob->LowMemory > 0)) {
> +    return;
>    }

(4) Side comment (for the future):

  Status == EFI_SUCCESS

is more idiomatically written in edk2 as

  !EFI_ERROR (Status)

>
>    //
> @@ -430,7 +455,7 @@ PlatformGetSystemMemorySizeBelow4gb (
>    Cmos0x34 = (UINT8)PlatformCmosRead8 (0x34);
>    Cmos0x35 = (UINT8)PlatformCmosRead8 (0x35);
>
> -  return (UINT32)(((UINTN)((Cmos0x35 << 8) + Cmos0x34) << 16) + SIZE_16MB);
> +  PlatformInfoHob->LowMemory = ((((UINTN)Cmos0x35 << 8) + Cmos0x34) << 16) + SIZE_16MB;
>  }
>

(5) The UINT32 cast has been dropped; if UINTN is UINT64, this might
again trigger a truncation warning from MSVC.

(6) There is no need to change the scope that the original UINTN cast
applies to. Pre-patch, the UINTN cast is bound to the following
sub-expression:

  ((Cmos0x35 << 8) + Cmos0x34)

here the variables are UINTN8, so they are promoted to INT32. The
maximum mathematical value is 0xFFFF here, having with in INT32 is safe.
We then cast it to UINTN (= at least UINT32) and then shift it further
left by 16 bits (max: 0xFFFF_0000). The max value we may get after
adding 16M is then 0x1_00FF_0000, which is above 4GB; it's up to QEMU
not to provide such CMOS content then. Either way, there's no risk of
INT32 overflow pre-patch.

The post-patch code is certainly *easier to read*, so I don't have
anything against re-binding the UINTN cast; but it should not be hidden
in this patch. It makes it harder to verify the code refactoring.


So anyway, what I need to remember from this point onward is that *each*
call to PlatformGetSystemMemorySizeBelow4gb() doesn't just fetch and
return a value, but *overwrites* "PlatformInfoHob->LowMemory".

OK, let's continue with the callers of
PlatformGetSystemMemorySizeBelow4gb() -- again, quoting hunks out of
order:

> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 3d8375320dcb..41d186986ba8 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -271,7 +271,8 @@ PublishPeiMemory (
>    UINT32                S3AcpiReservedMemoryBase;
>    UINT32                S3AcpiReservedMemorySize;
>
> -  LowerMemorySize = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
> +  PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
> +  LowerMemorySize = PlatformInfoHob->LowMemory;
>    if (PlatformInfoHob->SmmSmramRequire) {
>      //
>      // TSEG is chipped from the end of low RAM

So I really like how small this hunk is, and I wondered why it differed
so much from the rest, where you removed the local variables.

I understand now: because PublishPeiMemory() actually modifies
"LowerMemorySize" in multiple steps. OK then, I see both points; here we
need to keep "LowerMemorySize", because we can't modify the platform
info HOB; but in the rest of the hunks, it's better if we just remove
the useless locals. OK.


> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index a2a4dc043f2e..63329c4e796a 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c

> @@ -51,18 +51,16 @@ PlatformQemuUc32BaseInitialization (
>    IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
>    )
>  {
> -  UINT32  LowerMemorySize;
> -
>    if (PlatformInfoHob->HostBridgeDevId == 0xffff /* microvm */) {
>      return;
>    }
>
>    if (PlatformInfoHob->HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
> -    LowerMemorySize = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
> +    PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
>      ASSERT (PcdGet64 (PcdPciExpressBaseAddress) <= MAX_UINT32);
> -    ASSERT (PcdGet64 (PcdPciExpressBaseAddress) >= LowerMemorySize);
> +    ASSERT (PcdGet64 (PcdPciExpressBaseAddress) >= PlatformInfoHob->LowMemory);
>
> -    if (LowerMemorySize <= BASE_2GB) {
> +    if (PlatformInfoHob->LowMemory <= BASE_2GB) {
>        // Newer qemu with gigabyte aligned memory,
>        // 32-bit pci mmio window is 2G -> 4G then.
>        PlatformInfoHob->Uc32Base = BASE_2GB;

OK.

> @@ -92,8 +90,8 @@ PlatformQemuUc32BaseInitialization (
>    // variable MTRR suffices by truncating the size to a whole power of two,
>    // while keeping the end affixed to 4GB. This will round the base up.
>    //
> -  LowerMemorySize           = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
> -  PlatformInfoHob->Uc32Size = GetPowerOfTwo32 ((UINT32)(SIZE_4GB - LowerMemorySize));

(7) Request for a *separate* follow-up patch:

Commit 2a0bd3bffc80 ("OvmfPkg/PlatformInitLib: q35 mtrr setup fix",
2022-09-28) changed the structure of
PlatformQemuUc32BaseInitialization() to the following one:

- microvm: do nothing, just return
- q35: depend on LowerMemorySize (change from 2a0bd3bffc80)
- cloudhv: constant assignments, then return
- i440fx: depend on LowerMemorySize

Therefore we now have to branches (an explicit q35 branch and a
"default" or "remaining" i440fx branch) that fetch LowerMemorySize. That
code duplication is causing some churn for the present patch. I suggest
reordering the branches as follows:

- microvm: do nothing, just return
- cloudhv: constant assignments, then return
- grab LowerMemorySize -- commonly needed for the rest!
- handle q35
- handle i440fx as default / fallback.

This will centralize the LowerMemorySize fetching.


> +  PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
> +  PlatformInfoHob->Uc32Size = GetPowerOfTwo32 ((UINT32)(SIZE_4GB - PlatformInfoHob->LowMemory));
>    PlatformInfoHob->Uc32Base = (UINT32)(SIZE_4GB - PlatformInfoHob->Uc32Size);
>    //
>    // Assuming that LowerMemorySize is at least 1 byte, Uc32Size is at most 2GB.
> @@ -101,13 +99,13 @@ PlatformQemuUc32BaseInitialization (
>    //
>    ASSERT (PlatformInfoHob->Uc32Base >= BASE_2GB);
>
> -  if (PlatformInfoHob->Uc32Base != LowerMemorySize) {
> +  if (PlatformInfoHob->Uc32Base != PlatformInfoHob->LowMemory) {
>      DEBUG ((
>        DEBUG_VERBOSE,
>        "%a: rounded UC32 base from 0x%x up to 0x%x, for "
>        "an UC32 size of 0x%x\n",
>        __FUNCTION__,
> -      LowerMemorySize,
> +      PlatformInfoHob->LowMemory,
>        PlatformInfoHob->Uc32Base,
>        PlatformInfoHob->Uc32Size
>        ));

OK.

>  STATIC
> @@ -965,7 +990,6 @@ PlatformQemuInitializeRam (
>    IN EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
>    )
>  {
> -  UINT64         LowerMemorySize;
>    UINT64         UpperMemorySize;
>    MTRR_SETTINGS  MtrrSettings;
>    EFI_STATUS     Status;
> @@ -975,7 +999,7 @@ PlatformQemuInitializeRam (
>    //
>    // Determine total memory size available
>    //
> -  LowerMemorySize = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
> +  PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
>
>    if (PlatformInfoHob->BootMode == BOOT_ON_S3_RESUME) {
>      //
> @@ -1009,14 +1033,14 @@ PlatformQemuInitializeRam (
>        UINT32  TsegSize;
>
>        TsegSize = PlatformInfoHob->Q35TsegMbytes * SIZE_1MB;
> -      PlatformAddMemoryRangeHob (BASE_1MB, LowerMemorySize - TsegSize);
> +      PlatformAddMemoryRangeHob (BASE_1MB, PlatformInfoHob->LowMemory - TsegSize);
>        PlatformAddReservedMemoryBaseSizeHob (
> -        LowerMemorySize - TsegSize,
> +        PlatformInfoHob->LowMemory - TsegSize,
>          TsegSize,
>          TRUE
>          );
>      } else {
> -      PlatformAddMemoryRangeHob (BASE_1MB, LowerMemorySize);
> +      PlatformAddMemoryRangeHob (BASE_1MB, PlatformInfoHob->LowMemory);
>      }
>
>      //

OK. I think I used UINT64 here because these HOB-adding functions take
64-bit params. But UINT32 should work fine too.

(8) Note that a bit lower, we have a comment:

  //
  // We'd like to keep the following ranges uncached:
  // - [640 KB, 1 MB)
  // - [LowerMemorySize, 4 GB)
  //

The "LowerMemorySize" reference in this commit is actually my fault;
it's an oversight / omission from commit 49edde15230a
("OvmfPkg/PlatformPei: set 32-bit UC area at PciBase / PciExBarBase
(pc/q35)", 2019-06-03). The comment should now say
"PlatformInfoHob->Uc32Base".

If you can submit a separate patch for that, that would be great; it's
not too important though.

(9) BTW, still regarding commit 2a0bd3bffc80 ("OvmfPkg/PlatformInitLib:
q35 mtrr setup fix", 2022-09-28) -- does the following code comment
still apply?

    //
    // Set the memory range from the start of the 32-bit MMIO area (32-bit PCI
    // MMIO aperture on i440fx, PCIEXBAR on q35) to 4GB as uncacheable.
    //

Because, in case the new branch introduced by commit 2a0bd3bffc80 takes
effect (namely, "Uc32Base = BASE_2GB"), I'm not sure where the PCIEXBAR
starts, and then the above comment may no longer hold.

> @@ -1194,9 +1218,10 @@ PlatformQemuInitializeRamForS3 (
>        // Make sure the TSEG area that we reported as a reserved memory resource
>        // cannot be used for reserved memory allocations.
>        //
> +      PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
>        TsegSize = PlatformInfoHob->Q35TsegMbytes * SIZE_1MB;
>        BuildMemoryAllocationHob (
> -        PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob) - TsegSize,
> +        PlatformInfoHob->LowMemory - TsegSize,
>          TsegSize,
>          EfiReservedMemoryType
>          );

OK.

> diff --git a/OvmfPkg/Library/PeilessStartupLib/Hob.c b/OvmfPkg/Library/PeilessStartupLib/Hob.c
> index 630ce445ebec..784a8ba194de 100644
> --- a/OvmfPkg/Library/PeilessStartupLib/Hob.c
> +++ b/OvmfPkg/Library/PeilessStartupLib/Hob.c
> @@ -41,8 +41,9 @@ ConstructSecHobList (
>    EFI_HOB_PLATFORM_INFO       PlatformInfoHob;
>
>    ZeroMem (&PlatformInfoHob, sizeof (PlatformInfoHob));
> +  PlatformGetSystemMemorySizeBelow4gb (&PlatformInfoHob);
>    PlatformInfoHob.HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
> -  LowMemorySize                   = PlatformGetSystemMemorySizeBelow4gb (&PlatformInfoHob);
> +  LowMemorySize                   = PlatformInfoHob.LowMemory;
>    ASSERT (LowMemorySize != 0);
>    LowMemoryStart = FixedPcdGet32 (PcdOvmfDxeMemFvBase) + FixedPcdGet32 (PcdOvmfDxeMemFvSize);
>    LowMemorySize -= LowMemoryStart;

(10) I think this PlatformGetSystemMemorySizeBelow4gb() call is not
placed correctly.

PlatformGetSystemMemorySizeBelow4gb() depends on "HostBridgeDevId", but
this hunk reorders the setting of "HostBridgeDevId" against the call to
PlatformGetSystemMemorySizeBelow4gb().


> diff --git a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c b/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
> index 380e71597206..928120d183ba 100644
> --- a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
> +++ b/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
> @@ -41,8 +41,7 @@ InitializePlatform (
>    EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
>    )
>  {
> -  UINT32  LowerMemorySize;
> -  VOID    *VariableStore;
> +  VOID  *VariableStore;
>
>    DEBUG ((DEBUG_INFO, "InitializePlatform in Pei-less boot\n"));
>    PlatformDebugDumpCmos ();
> @@ -70,14 +69,14 @@ InitializePlatform (
>      PlatformInfoHob->PcdCpuBootLogicalProcessorNumber
>      ));
>
> -  LowerMemorySize = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
> +  PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
>    PlatformQemuUc32BaseInitialization (PlatformInfoHob);
>    DEBUG ((
>      DEBUG_INFO,
>      "Uc32Base = 0x%x, Uc32Size = 0x%x, LowerMemorySize = 0x%x\n",
>      PlatformInfoHob->Uc32Base,
>      PlatformInfoHob->Uc32Size,
> -    LowerMemorySize
> +    PlatformInfoHob->LowMemory
>      ));
>
>    VariableStore                                  = PlatformReserveEmuVariableNvStore ();

Seems OK.

> diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c b/OvmfPkg/Library/PlatformInitLib/Platform.c
> index 3e13c5d4b34f..9ab0342fd8c0 100644
> --- a/OvmfPkg/Library/PlatformInitLib/Platform.c
> +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c
> @@ -128,7 +128,6 @@ PlatformMemMapInitialization (
>  {
>    UINT64  PciIoBase;
>    UINT64  PciIoSize;
> -  UINT32  TopOfLowRam;
>    UINT64  PciExBarBase;
>    UINT32  PciBase;
>    UINT32  PciSize;
> @@ -150,7 +149,7 @@ PlatformMemMapInitialization (
>      return;
>    }
>
> -  TopOfLowRam  = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
> +  PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
>    PciExBarBase = 0;
>    if (PlatformInfoHob->HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>      //
> @@ -158,11 +157,11 @@ PlatformMemMapInitialization (
>      // the base of the 32-bit PCI host aperture.
>      //
>      PciExBarBase = PcdGet64 (PcdPciExpressBaseAddress);
> -    ASSERT (TopOfLowRam <= PciExBarBase);
> +    ASSERT (PlatformInfoHob->LowMemory <= PciExBarBase);
>      ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
>      PciBase = (UINT32)(PciExBarBase + SIZE_256MB);

This change looks OK, but, similarly to my question (9):

(11) Is the following comment still up to date:

    //
    // The MMCONFIG area is expected to fall between the top of low RAM and
    // the base of the 32-bit PCI host aperture.
    //

with regard to the new branch introduced by commit 2a0bd3bffc80
("OvmfPkg/PlatformInitLib: q35 mtrr setup fix", 2022-09-28)?

>    } else {
> -    ASSERT (TopOfLowRam <= PlatformInfoHob->Uc32Base);
> +    ASSERT (PlatformInfoHob->LowMemory <= PlatformInfoHob->Uc32Base);
>      PciBase = PlatformInfoHob->Uc32Base;
>    }
>

OK.

Thanks,
Laszlo

Re: [PATCH v2 2/4] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB

[Gerd Hoffmann]

> > +  Candidate = E820Entry->BaseAddr + E820Entry->Length;
> > +  if (Candidate >= BASE_4GB) {
> > +    return;
> > +  }

> (1) This looks like a faithful conversion / extraction, but I'd vaguely
> noticed something earlier, in the original code. Namely, when the
> exclusive end of the range is exactly 4GB, that should still qualify as
> low memory, shouldn't it?
> 
> Not that it matters in practice, because just below 4GB, we'll never
> ever have RAM on QEMU (pc or q35 -- I think even microvm, too).

Yes.

> But, for clarity, changing the limit condition as a separate patch
> (afterwards) might make sense.

Well, BASE_4GB-1 fits into LowMemory (which is UINT32) whereas BASE_4GB
does not ...

> > -    return (UINT32)GetHighestSystemMemoryAddressFromPvhMemmap (TRUE);
> > +    PlatformInfoHob->LowMemory = GetHighestSystemMemoryAddressFromPvhMemmap (TRUE);

> (2) Here you are converting a UINT64 from
> GetHighestSystemMemoryAddressFromPvhMemmap() to UINT32; I think that
> might trip up some MSVC compilers. I suggest preserving the cast.

OK.

>   PlatformInfoHob->LowMemory = 0;

> (I realize the platform info HOB could be zero-filled upon allocation --
> but then at least for consistency with the 4GB+ search initialization, a
> comment could be justified.)

It's explicitly cleared with ZeroMem (in BuildPlatformInfoHob), so there
is no zero-initialize LowMemory again.

> > diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> > index 3d8375320dcb..41d186986ba8 100644
> > --- a/OvmfPkg/PlatformPei/MemDetect.c
> > +++ b/OvmfPkg/PlatformPei/MemDetect.c
> > @@ -271,7 +271,8 @@ PublishPeiMemory (
> >    UINT32                S3AcpiReservedMemoryBase;
> >    UINT32                S3AcpiReservedMemorySize;
> >
> > -  LowerMemorySize = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
> > +  PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
> > +  LowerMemorySize = PlatformInfoHob->LowMemory;
> >    if (PlatformInfoHob->SmmSmramRequire) {
> >      //
> >      // TSEG is chipped from the end of low RAM
> 
> So I really like how small this hunk is, and I wondered why it differed
> so much from the rest, where you removed the local variables.
> 
> I understand now: because PublishPeiMemory() actually modifies
> "LowerMemorySize" in multiple steps. OK then, I see both points; here we
> need to keep "LowerMemorySize", because we can't modify the platform
> info HOB; but in the rest of the hunks, it's better if we just remove
> the useless locals. OK.

Yes, that is the pattern.  LowMemory holds the memory installed.
PublishPeiMemory calculates how edk2 uses that memory, which is
something different.

> code duplication is causing some churn for the present patch. I suggest
> reordering the branches as follows:
> 
> - microvm: do nothing, just return
> - cloudhv: constant assignments, then return
> - grab LowerMemorySize -- commonly needed for the rest!
> - handle q35
> - handle i440fx as default / fallback.

Makes sense indeed.

> (9) BTW, still regarding commit 2a0bd3bffc80 ("OvmfPkg/PlatformInitLib:
> q35 mtrr setup fix", 2022-09-28) -- does the following code comment
> still apply?
> 
>     //
>     // Set the memory range from the start of the 32-bit MMIO area (32-bit PCI
>     // MMIO aperture on i440fx, PCIEXBAR on q35) to 4GB as uncacheable.
>     //
> 
> Because, in case the new branch introduced by commit 2a0bd3bffc80 takes
> effect (namely, "Uc32Base = BASE_2GB"), I'm not sure where the PCIEXBAR
> starts, and then the above comment may no longer hold.

PCIEXBAR location doesn't change, it's still at 0xb0000000.

> >    ZeroMem (&PlatformInfoHob, sizeof (PlatformInfoHob));
> > +  PlatformGetSystemMemorySizeBelow4gb (&PlatformInfoHob);
> >    PlatformInfoHob.HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
> > -  LowMemorySize                   = PlatformGetSystemMemorySizeBelow4gb (&PlatformInfoHob);
> > +  LowMemorySize                   = PlatformInfoHob.LowMemory;
> >    ASSERT (LowMemorySize != 0);
> >    LowMemoryStart = FixedPcdGet32 (PcdOvmfDxeMemFvBase) + FixedPcdGet32 (PcdOvmfDxeMemFvSize);
> >    LowMemorySize -= LowMemoryStart;
> 
> (10) I think this PlatformGetSystemMemorySizeBelow4gb() call is not
> placed correctly.
> 
> PlatformGetSystemMemorySizeBelow4gb() depends on "HostBridgeDevId", but
> this hunk reorders the setting of "HostBridgeDevId" against the call to
> PlatformGetSystemMemorySizeBelow4gb().

Correct, nice catch.  Placed it there for optical reasons (keep the nice '='
alignment) but didn't realize that.

> > diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c b/OvmfPkg/Library/PlatformInitLib/Platform.c
> > index 3e13c5d4b34f..9ab0342fd8c0 100644
> > --- a/OvmfPkg/Library/PlatformInitLib/Platform.c
> > +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c
> > @@ -128,7 +128,6 @@ PlatformMemMapInitialization (
> >  {
> >    UINT64  PciIoBase;
> >    UINT64  PciIoSize;
> > -  UINT32  TopOfLowRam;
> >    UINT64  PciExBarBase;
> >    UINT32  PciBase;
> >    UINT32  PciSize;
> > @@ -150,7 +149,7 @@ PlatformMemMapInitialization (
> >      return;
> >    }
> >
> > -  TopOfLowRam  = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
> > +  PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
> >    PciExBarBase = 0;
> >    if (PlatformInfoHob->HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
> >      //
> > @@ -158,11 +157,11 @@ PlatformMemMapInitialization (
> >      // the base of the 32-bit PCI host aperture.
> >      //
> >      PciExBarBase = PcdGet64 (PcdPciExpressBaseAddress);
> > -    ASSERT (TopOfLowRam <= PciExBarBase);
> > +    ASSERT (PlatformInfoHob->LowMemory <= PciExBarBase);
> >      ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
> >      PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
> 
> This change looks OK, but, similarly to my question (9):
> 
> (11) Is the following comment still up to date:
> 
>     //
>     // The MMCONFIG area is expected to fall between the top of low RAM and
>     // the base of the 32-bit PCI host aperture.
>     //
> 
> with regard to the new branch introduced by commit 2a0bd3bffc80
> ("OvmfPkg/PlatformInitLib: q35 mtrr setup fix", 2022-09-28)?

root@fedora ~# cat /proc/iomem 
[ ... ]
7ebfe000-7effffff : System RAM
7f000000-7fffffff : Reserved
80000000-afffffff : PCI Bus 0000:00
b0000000-bfffffff : PCI MMCONFIG 0000 [bus 00-ff]
  b0000000-bfffffff : Reserved
    b0000000-bfffffff : pnp 00:04
c0000000-febfffff : PCI Bus 0000:00
[ ... ]
root@fedora ~# cat /proc/mtrr 
reg00: base=0x080000000 ( 2048MB), size= 2048MB, count=1: uncachable
reg01: base=0x800000000 (32768MB), size=32768MB, count=1: uncachable

With gigabyte-alignment being the common case these days it might make
sense to place the MMCONFIG area at 0xe0000000 instead ...

take care,
  Gerd

Re: [PATCH v2 2/4] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB

[Laszlo Ersek]

On 1/11/23 08:29, Gerd Hoffmann wrote:

>> (9) BTW, still regarding commit 2a0bd3bffc80 ("OvmfPkg/PlatformInitLib:
>> q35 mtrr setup fix", 2022-09-28) -- does the following code comment
>> still apply?
>>
>>     //
>>     // Set the memory range from the start of the 32-bit MMIO area (32-bit PCI
>>     // MMIO aperture on i440fx, PCIEXBAR on q35) to 4GB as uncacheable.
>>     //
>>
>> Because, in case the new branch introduced by commit 2a0bd3bffc80 takes
>> effect (namely, "Uc32Base = BASE_2GB"), I'm not sure where the PCIEXBAR
>> starts, and then the above comment may no longer hold.
>
> PCIEXBAR location doesn't change, it's still at 0xb0000000.

>>> @@ -158,11 +157,11 @@ PlatformMemMapInitialization (
>>>      // the base of the 32-bit PCI host aperture.
>>>      //
>>>      PciExBarBase = PcdGet64 (PcdPciExpressBaseAddress);
>>> -    ASSERT (TopOfLowRam <= PciExBarBase);
>>> +    ASSERT (PlatformInfoHob->LowMemory <= PciExBarBase);
>>>      ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
>>>      PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
>>
>> This change looks OK, but, similarly to my question (9):
>>
>> (11) Is the following comment still up to date:
>>
>>     //
>>     // The MMCONFIG area is expected to fall between the top of low RAM and
>>     // the base of the 32-bit PCI host aperture.
>>     //
>>
>> with regard to the new branch introduced by commit 2a0bd3bffc80
>> ("OvmfPkg/PlatformInitLib: q35 mtrr setup fix", 2022-09-28)?
>
> root@fedora ~# cat /proc/iomem
> [ ... ]
> 7ebfe000-7effffff : System RAM
> 7f000000-7fffffff : Reserved
> 80000000-afffffff : PCI Bus 0000:00
> b0000000-bfffffff : PCI MMCONFIG 0000 [bus 00-ff]
>   b0000000-bfffffff : Reserved
>     b0000000-bfffffff : pnp 00:04
> c0000000-febfffff : PCI Bus 0000:00
> [ ... ]
> root@fedora ~# cat /proc/mtrr
> reg00: base=0x080000000 ( 2048MB), size= 2048MB, count=1: uncachable
> reg01: base=0x800000000 (32768MB), size=32768MB, count=1: uncachable

Ugh, what? :)

I was about to point out a contradiction between (a) the following from
commit 2a0bd3bffc80:

+      // Newer qemu with gigabyte aligned memory,
+      // 32-bit pci mmio window is 2G -> 4G then.

and (b) your confirmation that the PCIEXBAR location does not change.
Namely, I was about to point out that PCIEXBAR -- *config space*
expressed as MMIO -- would then overlap the 32-bit MMIO aperture, the
one that's assignable to BARs as MMIO space.

But then your /proc/iomem quote actually confirms this is what happens
in practice -- and apparently works??? In Linux anyways?

FWIW I don't see how this is safe with regard to the firmware. Even if
QEMU is capable of generating a set of discontiguous resource
descriptors in the DSDT / _CRS, and Linux is capable of dealing with
that, I don't understand how the firmware does it.

Has it only been working by chance, perhaps? PciHostBridgeDxe uses this
intersection-based MMIO addition that we discussed in the BZ, so I
certainly don't expect a conflict there; after all, the MMIO space used
for MMCONFIG and the MMIO space used for BAR assignments should mostly
be the same; IOW there should be no conflict or compatibility problem at
the GCD level that PciHostBridgeDxe chokes on.

But when the actual BARs are assigned (allocated), what prevents them
from being allocated from the overlapping MMCONFIG "sub" interval? I
think there's a problem there, we just have not hit it yet.

OK, OK, let me review what the code actually does. Here's the relevant
part of the call tree:

    InitializePlatform()                   [OvmfPkg/PlatformPei/Platform.c]
[1]   PlatformQemuUc32BaseInitialization() [OvmfPkg/Library/PlatformInitLib/MemDetect.c]
      InitializeRamRegions()               [OvmfPkg/PlatformPei/MemDetect.c]
[2]     PlatformQemuInitializeRam()        [OvmfPkg/Library/PlatformInitLib/MemDetect.c]
      MemMapInitialization()               [OvmfPkg/PlatformPei/Platform.c]
[3]     PlatformMemMapInitialization()     [OvmfPkg/Library/PlatformInitLib/Platform.c]
      MiscInitialization()                 [OvmfPkg/PlatformPei/Platform.c]
        PlatformMiscInitialization()       [OvmfPkg/Library/PlatformInitLib/Platform.c]
[4]       PciExBarInitialization()         [OvmfPkg/Library/PlatformInitLib/Platform.c]

[5] AmdSevDxeEntryPoint()                  [OvmfPkg/AmdSevDxe/AmdSevDxe.c]

- At [1], the most recent state is from commit 2a0bd3bffc80
  ("OvmfPkg/PlatformInitLib: q35 mtrr setup fix", 2022-09-28). What this
  commit does is, it lowers (conditionally) the base of the area that
  we'll mark as UC ("Uc32Base"), from 0xB0000000 (2816 MB, the start of
  the EXBAR) to 2048 MB.

  It does not change the location of MMCONFIG *or* the 32-bit MMIO
  aperture. It only adds a *comment* like this:

  +      // Newer qemu with gigabyte aligned memory,
  +      // 32-bit pci mmio window is 2G -> 4G then.

  which is a *natural language statement* about the 32-bit MMIO
  aperture, but no code change.

- At [2], we mark the area [Uc32Base, 4GB) as uncacheable in the MTRRs.
  There is a comment saying:

    //
    // Set the memory range from the start of the 32-bit MMIO area (32-bit PCI
    // MMIO aperture on i440fx, PCIEXBAR on q35) to 4GB as uncacheable.
    //

  And this comment *conflicts* with the one from [1]. Because on Q35,
  the new Uc32Base (2GB) may not actually match the PCIEXBAR start (2816
  MB). Therefore commit 2a0bd3bffc80 made the comment at [2] stale. This
  is issue {1}.

  Still, this is not a functional error, as we're only marking a
  *larger* (and simpler-to-express) area as UC than before. When this
  new logic fires, we have *nothing* in the space between the 32-bit RAM
  and the EXBAR base -- thus far, anyway!.

- At [3], we have a comment saying

    //
    // The MMCONFIG area is expected to fall between the top of low RAM and
    // the base of the 32-bit PCI host aperture.
    //

  plus code that actually *implements* this. In spite of the *comment*
  added in commit 2a0bd3bffc80, at [1], the logic at [3] *still* --
  correctly -- places the 32-bit MMIO aperture *above* the PCIEXBAR.

  This is what PciHostBridgeDxe will expose as aperture, and what
  PciBusDxe will assign BARs from. No conflict is possible with the
  MMCONFIG area.

  This is in fact confirmed by the following log snippet, when booting a
  pc-q35-7.2 guest with 1792 MB of RAM:

> PciHostBridgeUtilityInitRootBridge: populated root bus 0, with room for 255 subordinate bus(es)
> RootBridge: PciRoot(0x0)
>   Support/Attr: 70069 / 70069
>     DmaAbove4G: No
> NoExtConfSpace: No
>      AllocAttr: 3 (CombineMemPMem Mem64Decode)
>            Bus: 0 - FF Translation=0
>             Io: 6000 - FFFF Translation=0
>            Mem: C0000000 - FBFFFFFF Translation=0
>     MemAbove4G: 800000000 - FFFFFFFFF Translation=0
>           PMem: FFFFFFFFFFFFFFFF - 0 Translation=0
>    PMemAbove4G: FFFFFFFFFFFFFFFF - 0 Translation=0

  Note "Mem: C0000000 - FBFFFFFF Translation=0".

- Still at [3], we cover the MMCONFIG area with a reserved memory HOB
  (not MMIO HOB). This is alright; and again the reason we have no
  conflict in PciHostBridgeDxe is that the 32-bit MMIO aperture *still*
  starts above the EXBAR; it does not "surround" it.

- At [4], we actually program the EXBAR.

- At [5], early in the DXE phase, knowing that we marked the EXBAR as
  reserved memory and not as MMIO, we explicitly decrypt (when on SEV)
  the EXBAR.

OK, so what do we learn from the above?

- issue {1}: the statement in [2] PlatformQemuInitializeRam(), in the
  following comment:

    //
    // Set the memory range from the start of the 32-bit MMIO area (32-bit PCI
    // MMIO aperture on i440fx, PCIEXBAR on q35) to 4GB as uncacheable.
    //

   was broken by commit 2a0bd3bffc80.

- issue {2}: the statement from commit 2a0bd3bffc80

  +      // 32-bit pci mmio window is 2G -> 4G then.

  is not factual, as far as the firmware is concerned. I don't know
  *how* Linux ends up extending the MMIO aperture so far down that it
  streddles / embeds MMCONFIG, but AFAICT it has nothing to do with the
  firmware.

  Therefore, I also don't understand where the requirement comes (from
  Linux? where?) that the firmware mark the "gap" between 2048 MB and
  2816 MB as uncached. The firmware does not use it for anything, so why
  does the Linux kernel do? And if the Linux kernel does, then why does
  it not reprogram the MTRRs as well?

  The commit message from commit 2a0bd3bffc80 states, "Which effectively
  makes the 32-bit pci mmio window start at 0x80000000".

  I'm precisely after that "effectively" adverb here: placing the 32-bit
  MMIO aperture at 2048 MB is not *at all* what the firmware does.

... OK, I think I've found it! It's the following QEMU commit:
4a4418369d6d ("q35: fix mmconfig and PCI0._CRS", 2019-06-16).

I've even found the original discussion:

- v1: http://mid.mail-archive.com/20190528204331.5280-1-kraxel@redhat.com
- v2: http://mid.mail-archive.com/20190607073429.3436-1-kraxel@redhat.com

So, this seems to happen by QEMU moving the hole as low as possible in
the \SB.PCI0 _CRS, in the DSDT, punching gaps as neeed. And Linux
adheres to that.

I've tested it with said pc-q35-7.2 guest with 1792 MB of RAM, running
RHEL-7.9 (that's what I've had handy). The MTRR update from edk2 commit
2a0bd3bffc80 takes effect:

> [    0.000000] MTRR variable ranges enabled:
> [    0.000000]   0 base 0080000000 mask FF80000000 uncachable

*but* the start of the 32-bit MMIO range (which is discontiguous) appears
even lower, per QEMU commit 4a4418369d6d:

> [    0.000000] e820: [mem 0x70000000-0xafffffff] available for PCI devices
> ...
> [    0.281120] pci_bus 0000:00: root bus resource [mem 0x70000000-0xafffffff window]
> ...
> [    0.529741] pci 0000:00:02.0: BAR 6: assigned [mem 0x70000000-0x7000ffff pref]
> ...
> [    0.565903] pci_bus 0000:00: resource 7 [mem 0x70000000-0xafffffff window]

and

> 70000000-afffffff : PCI Bus 0000:00
>   70000000-7000ffff : 0000:00:02.0
> b0000000-bfffffff : PCI MMCONFIG 0000 [bus 00-ff]
>   b0000000-bfffffff : reserved
>     b0000000-bfffffff : pnp 00:04
> c0000000-febfffff : PCI Bus 0000:00
>   c0000000-c0ffffff : 0000:00:02.0
>     c0000000-c0ffffff : bochs-drm

There is *no sign* of 0x70000000 in the firmware log. So Linux
effectively ressigns some PCI resources, and utilizes the
(discontiguous) area that QEMU exposes in the DSDT, with the firmware
knowing nothing about it.

Note, again, that our UC settings in the MTRR start *higher*, at 2GB.

So not only am I unconvinced (or at least confused!) that edk2 commit
2a0bd3bffc80 was necessary, it even *seems* insufficient, for
UC-coverage of the 32-bit MMIO aperture.

In practice though, it doesn't seem to cause issues! (Which again
questions if the original change in 2a0bd3bffc80 was really necessary.)

I've filed a new TianoCore BZ about clarifying the comments please:

  https://bugzilla.tianocore.org/show_bug.cgi?id=4289

> With gigabyte-alignment being the common case these days it might make
> sense to place the MMCONFIG area at 0xe0000000 instead ...

I feel really unsafe about complicating this code even further.

Laszlo

Re: [PATCH v2 2/4] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB

[Gerd Hoffmann]

> > root@fedora ~# cat /proc/iomem
> > [ ... ]
> > 7ebfe000-7effffff : System RAM
> > 7f000000-7fffffff : Reserved
> > 80000000-afffffff : PCI Bus 0000:00
> > b0000000-bfffffff : PCI MMCONFIG 0000 [bus 00-ff]
> >   b0000000-bfffffff : Reserved
> >     b0000000-bfffffff : pnp 00:04
> > c0000000-febfffff : PCI Bus 0000:00
> > [ ... ]
> > root@fedora ~# cat /proc/mtrr
> > reg00: base=0x080000000 ( 2048MB), size= 2048MB, count=1: uncachable
> > reg01: base=0x800000000 (32768MB), size=32768MB, count=1: uncachable
> 
> Ugh, what? :)
> 
> I was about to point out a contradiction between (a) the following from
> commit 2a0bd3bffc80:
> 
> +      // Newer qemu with gigabyte aligned memory,
> +      // 32-bit pci mmio window is 2G -> 4G then.
> 
> and (b) your confirmation that the PCIEXBAR location does not change.
> Namely, I was about to point out that PCIEXBAR -- *config space*
> expressed as MMIO -- would then overlap the 32-bit MMIO aperture, the
> one that's assignable to BARs as MMIO space.
> 
> But then your /proc/iomem quote actually confirms this is what happens
> in practice -- and apparently works??? In Linux anyways?
> 
> FWIW I don't see how this is safe with regard to the firmware. Even if
> QEMU is capable of generating a set of discontiguous resource
> descriptors in the DSDT / _CRS, and Linux is capable of dealing with
> that, I don't understand how the firmware does it.

It doesn't.  It still operates with the 0xc0000000+ range as 32bit mmio
window.  Which is why the 80000000-afffffff range is unused.  Linux
could map hotplug device resources there, but that's it.

> >            Bus: 0 - FF Translation=0
> >             Io: 6000 - FFFF Translation=0
> >            Mem: C0000000 - FBFFFFFF Translation=0
> >     MemAbove4G: 800000000 - FFFFFFFFF Translation=0
> >           PMem: FFFFFFFFFFFFFFFF - 0 Translation=0
> >    PMemAbove4G: FFFFFFFFFFFFFFFF - 0 Translation=0
> 
>   Note "Mem: C0000000 - FBFFFFFF Translation=0".

Yes.

>   Therefore, I also don't understand where the requirement comes (from
>   Linux? where?) that the firmware mark the "gap" between 2048 MB and
>   2816 MB as uncached. The firmware does not use it for anything, so why
>   does the Linux kernel do? And if the Linux kernel does, then why does
>   it not reprogram the MTRRs as well?

Some test case complained because the 80000000-afffffff range is io
address space (according to /proc/iomem) but not tagged as uncachable
in mtrr registers.

>   The commit message from commit 2a0bd3bffc80 states, "Which effectively
>   makes the 32-bit pci mmio window start at 0x80000000".

.. according to the guest os view because qemu generates _CRS resources
with 80000000-afffffff included.

>   I'm precisely after that "effectively" adverb here: placing the 32-bit
>   MMIO aperture at 2048 MB is not *at all* what the firmware does.

Yes.

> I've filed a new TianoCore BZ about clarifying the comments please:
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=4289

OK.

> > With gigabyte-alignment being the common case these days it might make
> > sense to place the MMCONFIG area at 0xe0000000 instead ...
> 
> I feel really unsafe about complicating this code even further.

I think it should actually simplify things.  All the inconsistencies we
have (as you outlined above) due to the hole punching and edk2
supporting only a single range for 32bit mmio should go away, and we
will have less address space layout differences between q35 and pc.

We'll set LowMemory  -> 4G to UC via mtrr (both pc and q35)

We'll use LowMemory  -> 0xFBFFFFFF (pc) or
          LowMemory  -> 0xdfffffff (q35) as 32bit mmio window.

We'll use 0xe0000000 -> 0xeffffffff for mmconfig (q35 only).

Qemu will add 0xf0000000 -> 0xFBFFFFFF to the PCI bus _CRS so
linux could use it but the firmware wouldn't do anything with
it (q35 only).

take care,
  Gerd

Re: [PATCH v2 2/4] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB

[Laszlo Ersek]

On 1/11/23 16:23, Gerd Hoffmann wrote:

> Some test case complained because the 80000000-afffffff range is io
> address space (according to /proc/iomem) but not tagged as uncachable
> in mtrr registers.

Ah, very interesting! I didn't know there was a test case for this.

(And now I'm curious, per the new BZ, whether this same test case
complained if it saw us go deeper with the low mem amount -- e.g. the
same situation arises between 0x7000_0000 and 0x8000_0000.)

>>> With gigabyte-alignment being the common case these days it might make
>>> sense to place the MMCONFIG area at 0xe0000000 instead ...
>>
>> I feel really unsafe about complicating this code even further.
> 
> I think it should actually simplify things.  All the inconsistencies we
> have (as you outlined above) due to the hole punching and edk2
> supporting only a single range for 32bit mmio should go away, and we
> will have less address space layout differences between q35 and pc.

We've tried 0xE000_0000 in the past, in commit 75136b29541b.

But had to revert it in commit eb4d62b0779c, due to 0xE000_0000 tickling
a bug in QEMU.

The bug tickling was actually reported by you :) See
<https://bugzilla.tianocore.org/show_bug.cgi?id=1859>.

The current 0xB000_0000 value comes from commit b07de0974b65a, which was
a replacement for 75136b29541b (after the revert -- for re-fixing the
original issue <https://bugzilla.tianocore.org/show_bug.cgi?id=1814>,
which 75136b29541b intended to fix in the first place).

> 
> We'll set LowMemory  -> 4G to UC via mtrr (both pc and q35)

This is problematic, as LowMemory can have any kind of "resolution"
(i.e. an effectively unrestricted count of 1-bits in its binary
representation). That's a problem because MtrrLib's algorithm (probably
justifiedly) cannot find enough variable MTRRs to cover the boundary
precisely -- and will fail.

This is why our current logic exists for i440fx, in
PlatformQemuUc32BaseInitialization(), rounding up Uc32Base from LowMemory.

(Well, if you mean to keep the same logic for both i440fx adn q35,
that's OK then.)

> 
> We'll use LowMemory  -> 0xFBFFFFFF (pc) or
>           LowMemory  -> 0xdfffffff (q35) as 32bit mmio window.

This is precisely the situation (32-bit MMIO aperture below EXBAR) that
triggered the QEMU bug described in TianoCore#1859 and commit eb4d62b0779c.

I don't know if that QEMU bug is now fixed (and how far in the QEMU
release history the breakage goes back). At the time of the edk2 revert
commit eb4d62b0779c (2019-JUN-03), QEMU's latest release was v4.0.0
(2019-APR-23). Release v4.1.0 would follow at 2019-AUG-15.

> We'll use 0xe0000000 -> 0xeffffffff for mmconfig (q35 only).
> 
> Qemu will add 0xf0000000 -> 0xFBFFFFFF to the PCI bus _CRS so
> linux could use it but the firmware wouldn't do anything with
> it (q35 only).

If QEMU only *added* this range to the _CRS, that would be fine, but the
QEMU issue in TianoCore#1859 was that QEMU *moved* the aperture to this
range in the _CRS, effectively invalidating all the firmware-assigned
BARs (which would now all fall outside of the _CRS).

If you can ascertain that this problem is no longer relevant in any QEMU
releases that are still in use, then I won't try to block this
direction. In that case, it might be sufficient to just "re-play" commit
75136b29541b. (Note however that the MTRR setup was tied to the approach
taken, please refer to commits 39b9a5ffe661 and 49edde15230a.)

Either way, this has been a brittle area of OVMF platform code, and I'd
feel real uncomfortable, providing an explicit ACK. ... Luckily, you
wouldn't need an ACK from me :)

Laszlo

Re: [PATCH v2 2/4] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB

[Gerd Hoffmann]

  Hi,

> > I think it should actually simplify things.  All the inconsistencies we
> > have (as you outlined above) due to the hole punching and edk2
> > supporting only a single range for 32bit mmio should go away, and we
> > will have less address space layout differences between q35 and pc.
> 
> We've tried 0xE000_0000 in the past, in commit 75136b29541b.
> 
> But had to revert it in commit eb4d62b0779c, due to 0xE000_0000 tickling
> a bug in QEMU.
> 
> The bug tickling was actually reported by you :) See
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1859>.

Oh.  I totally forgot about that.  The patch from (I think) 2019 which
added _CRS for the range below the MMCONFIG should have fixed that, and
with recent qemu everything works fine.

I suspect we can't easily detect whenever qemu is broken or not.  Hmm.

Is more than three years being passed enough to just do it
unconditionally and effectively raise the bar for the minimum
supported qemu version?

> (Well, if you mean to keep the same logic for both i440fx adn q35,
> that's OK then.)

Yes, it would be Uc32Base.

LowMemory and Uc32Base are identical anyway most of the time due to qemu
preferring gigabyte pages when possible, you need odd memory sizes like
1.5 or 2.5 GB to see they actually can be different.

take care,
  Gerd

Re: [PATCH v2 2/4] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB

[Laszlo Ersek]

On 1/12/23 15:03, Gerd Hoffmann wrote:
>   Hi,
> 
>>> I think it should actually simplify things.  All the inconsistencies we
>>> have (as you outlined above) due to the hole punching and edk2
>>> supporting only a single range for 32bit mmio should go away, and we
>>> will have less address space layout differences between q35 and pc.
>>
>> We've tried 0xE000_0000 in the past, in commit 75136b29541b.
>>
>> But had to revert it in commit eb4d62b0779c, due to 0xE000_0000 tickling
>> a bug in QEMU.
>>
>> The bug tickling was actually reported by you :) See
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=1859>.
> 
> Oh.  I totally forgot about that.  The patch from (I think) 2019 which
> added _CRS for the range below the MMCONFIG should have fixed that, and
> with recent qemu everything works fine.
> 
> I suspect we can't easily detect whenever qemu is broken or not.  Hmm.
> 
> Is more than three years being passed enough to just do it
> unconditionally and effectively raise the bar for the minimum
> supported qemu version?

Well, in the other thread ("[PATCH v2] OvmfPkg/PlatformInitLib: catch
QEMU's CPU hotplug reg block regression"), I'm proposing a hard hang for
a QEMU bug that's not been fixed in *any* public release yet, and will
only be fixed in v8 :)

Can you determine the exact QEMU bugfix (commit hash) and the first QEMU
release that included it?

Maybe even build & test that version?

If we determine the minimum functional QEMU version precisely, and it's
like v5 or v6, making it the "official minimum" should be fine.

> 
>> (Well, if you mean to keep the same logic for both i440fx adn q35,
>> that's OK then.)
> 
> Yes, it would be Uc32Base.
> 
> LowMemory and Uc32Base are identical anyway most of the time due to qemu
> preferring gigabyte pages when possible, you need odd memory sizes like
> 1.5 or 2.5 GB to see they actually can be different.
> 

Thanks
Laszlo

[PATCH v2 3/4] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB

[Gerd Hoffmann]

Add PlatformAddHobCB() callback function for use with
PlatformScanE820().  It adds HOBs for high memory and reservations (low
memory is handled elsewhere because there are some special cases to
consider).  This replaces calls to PlatformScanOrAdd64BitE820Ram() with
AddHighHobs = TRUE.

Also remove PlatformScanOrAdd64BitE820Ram() which is not used any more.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/Library/PlatformInitLib/MemDetect.c | 174 ++++----------------
 1 file changed, 36 insertions(+), 138 deletions(-)

diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index 63329c4e796a..83a219581a1b 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -112,143 +112,6 @@ PlatformQemuUc32BaseInitialization (
   }
 }
 
-/**
-  Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map that start outside
-  of the 32-bit address range.
-
-  Find the highest exclusive >=4GB RAM address, or produce memory resource
-  descriptor HOBs for RAM entries that start at or above 4GB.
-
-  @param[out] MaxAddress  If MaxAddress is NULL, then PlatformScanOrAdd64BitE820Ram()
-                          produces memory resource descriptor HOBs for RAM
-                          entries that start at or above 4GB.
-
-                          Otherwise, MaxAddress holds the highest exclusive
-                          >=4GB RAM address on output. If QEMU's fw_cfg E820
-                          RAM map contains no RAM entry that starts outside of
-                          the 32-bit address range, then MaxAddress is exactly
-                          4GB on output.
-
-  @retval EFI_SUCCESS         The fw_cfg E820 RAM map was found and processed.
-
-  @retval EFI_PROTOCOL_ERROR  The RAM map was found, but its size wasn't a
-                              whole multiple of sizeof(EFI_E820_ENTRY64). No
-                              RAM entry was processed.
-
-  @return                     Error codes from QemuFwCfgFindFile(). No RAM
-                              entry was processed.
-**/
-STATIC
-EFI_STATUS
-PlatformScanOrAdd64BitE820Ram (
-  IN BOOLEAN  AddHighHob,
-  OUT UINT64  *LowMemory OPTIONAL,
-  OUT UINT64  *MaxAddress OPTIONAL
-  )
-{
-  EFI_STATUS            Status;
-  FIRMWARE_CONFIG_ITEM  FwCfgItem;
-  UINTN                 FwCfgSize;
-  EFI_E820_ENTRY64      E820Entry;
-  UINTN                 Processed;
-
-  Status = QemuFwCfgFindFile ("etc/e820", &FwCfgItem, &FwCfgSize);
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
-  if (FwCfgSize % sizeof E820Entry != 0) {
-    return EFI_PROTOCOL_ERROR;
-  }
-
-  if (LowMemory != NULL) {
-    *LowMemory = 0;
-  }
-
-  if (MaxAddress != NULL) {
-    *MaxAddress = BASE_4GB;
-  }
-
-  QemuFwCfgSelectItem (FwCfgItem);
-  for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) {
-    QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry);
-    DEBUG ((
-      DEBUG_VERBOSE,
-      "%a: Base=0x%Lx Length=0x%Lx Type=%u\n",
-      __FUNCTION__,
-      E820Entry.BaseAddr,
-      E820Entry.Length,
-      E820Entry.Type
-      ));
-    if (E820Entry.Type == EfiAcpiAddressRangeMemory) {
-      if (AddHighHob && (E820Entry.BaseAddr >= BASE_4GB)) {
-        UINT64  Base;
-        UINT64  End;
-
-        //
-        // Round up the start address, and round down the end address.
-        //
-        Base = ALIGN_VALUE (E820Entry.BaseAddr, (UINT64)EFI_PAGE_SIZE);
-        End  = (E820Entry.BaseAddr + E820Entry.Length) &
-               ~(UINT64)EFI_PAGE_MASK;
-        if (Base < End) {
-          PlatformAddMemoryRangeHob (Base, End);
-          DEBUG ((
-            DEBUG_VERBOSE,
-            "%a: PlatformAddMemoryRangeHob [0x%Lx, 0x%Lx)\n",
-            __FUNCTION__,
-            Base,
-            End
-            ));
-        }
-      }
-
-      if (MaxAddress || LowMemory) {
-        UINT64  Candidate;
-
-        Candidate = E820Entry.BaseAddr + E820Entry.Length;
-        if (MaxAddress && (Candidate > *MaxAddress)) {
-          *MaxAddress = Candidate;
-          DEBUG ((
-            DEBUG_VERBOSE,
-            "%a: MaxAddress=0x%Lx\n",
-            __FUNCTION__,
-            *MaxAddress
-            ));
-        }
-
-        if (LowMemory && (Candidate > *LowMemory) && (Candidate < BASE_4GB)) {
-          *LowMemory = Candidate;
-          DEBUG ((
-            DEBUG_VERBOSE,
-            "%a: LowMemory=0x%Lx\n",
-            __FUNCTION__,
-            *LowMemory
-            ));
-        }
-      }
-    } else if (E820Entry.Type == EfiAcpiAddressRangeReserved) {
-      if (AddHighHob) {
-        DEBUG ((
-          DEBUG_INFO,
-          "%a: Reserved: Base=0x%Lx Length=0x%Lx\n",
-          __FUNCTION__,
-          E820Entry.BaseAddr,
-          E820Entry.Length
-          ));
-        BuildResourceDescriptorHob (
-          EFI_RESOURCE_MEMORY_RESERVED,
-          0,
-          E820Entry.BaseAddr,
-          E820Entry.Length
-          );
-      }
-    }
-  }
-
-  return EFI_SUCCESS;
-}
-
 typedef VOID (*E820_SCAN_CALLBACK) (
   EFI_E820_ENTRY64       *E820Entry,
   EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
@@ -304,6 +167,41 @@ PlatformGetLowMemoryCB (
   }
 }
 
+/**
+  Create HOBs for reservations and RAM (except low memory).
+**/
+VOID
+PlatformAddHobCB (
+  IN     EFI_E820_ENTRY64       *E820Entry,
+  IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
+  )
+{
+  UINT64  Base = E820Entry->BaseAddr;
+  UINT64  End  = E820Entry->BaseAddr + E820Entry->Length;
+
+  switch (E820Entry->Type) {
+    case EfiAcpiAddressRangeMemory:
+      //
+      // Round up the start address, and round down the end address.
+      //
+      Base = ALIGN_VALUE (Base, (UINT64)EFI_PAGE_SIZE);
+      End  = End & ~(UINT64)EFI_PAGE_MASK;
+      if ((Base >= BASE_4GB) && (Base < End)) {
+        DEBUG ((DEBUG_INFO, "%a: HighMemory [0x%Lx, 0x%Lx]\n", __FUNCTION__, Base, End));
+        PlatformAddMemoryRangeHob (Base, End);
+      }
+
+      break;
+    case EfiAcpiAddressRangeReserved:
+      BuildResourceDescriptorHob (EFI_RESOURCE_MEMORY_RESERVED, 0, Base, End);
+      DEBUG ((DEBUG_INFO, "%a: Reserved [0x%Lx, 0x%Lx]\n", __FUNCTION__, Base, End));
+      break;
+    default:
+      DEBUG ((DEBUG_WARN, "%a: Type %d [0x%Lx, 0x%Lx] (NOT HANDLED)\n", __FUNCTION__, E820Entry->Type, Base, End));
+      break;
+  }
+}
+
 /**
   Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map, call the
   passed callback for each entry.
@@ -1048,7 +946,7 @@ PlatformQemuInitializeRam (
     // entries. Otherwise, create a single memory HOB with the flat >=4GB
     // memory size read from the CMOS.
     //
-    Status = PlatformScanOrAdd64BitE820Ram (TRUE, NULL, NULL);
+    Status = PlatformScanE820 (PlatformAddHobCB, PlatformInfoHob);
     if (EFI_ERROR (Status)) {
       UpperMemorySize = PlatformGetSystemMemorySizeAbove4gb ();
       if (UpperMemorySize != 0) {
-- 
2.39.0

Re: [PATCH v2 3/4] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB

[Laszlo Ersek]

On 1/10/23 09:21, Gerd Hoffmann wrote:
> Add PlatformAddHobCB() callback function for use with
> PlatformScanE820().  It adds HOBs for high memory and reservations (low
> memory is handled elsewhere because there are some special cases to
> consider).  This replaces calls to PlatformScanOrAdd64BitE820Ram() with
> AddHighHobs = TRUE.
>
> Also remove PlatformScanOrAdd64BitE820Ram() which is not used any more.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/Library/PlatformInitLib/MemDetect.c | 174 ++++----------------
>  1 file changed, 36 insertions(+), 138 deletions(-)
>
> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index 63329c4e796a..83a219581a1b 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> @@ -112,143 +112,6 @@ PlatformQemuUc32BaseInitialization (
>    }
>  }
>
> -/**
> -  Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map that start outside
> -  of the 32-bit address range.
> -
> -  Find the highest exclusive >=4GB RAM address, or produce memory resource
> -  descriptor HOBs for RAM entries that start at or above 4GB.
> -
> -  @param[out] MaxAddress  If MaxAddress is NULL, then PlatformScanOrAdd64BitE820Ram()
> -                          produces memory resource descriptor HOBs for RAM
> -                          entries that start at or above 4GB.
> -
> -                          Otherwise, MaxAddress holds the highest exclusive
> -                          >=4GB RAM address on output. If QEMU's fw_cfg E820
> -                          RAM map contains no RAM entry that starts outside of
> -                          the 32-bit address range, then MaxAddress is exactly
> -                          4GB on output.
> -
> -  @retval EFI_SUCCESS         The fw_cfg E820 RAM map was found and processed.
> -
> -  @retval EFI_PROTOCOL_ERROR  The RAM map was found, but its size wasn't a
> -                              whole multiple of sizeof(EFI_E820_ENTRY64). No
> -                              RAM entry was processed.
> -
> -  @return                     Error codes from QemuFwCfgFindFile(). No RAM
> -                              entry was processed.
> -**/
> -STATIC
> -EFI_STATUS
> -PlatformScanOrAdd64BitE820Ram (
> -  IN BOOLEAN  AddHighHob,
> -  OUT UINT64  *LowMemory OPTIONAL,
> -  OUT UINT64  *MaxAddress OPTIONAL
> -  )
> -{
> -  EFI_STATUS            Status;
> -  FIRMWARE_CONFIG_ITEM  FwCfgItem;
> -  UINTN                 FwCfgSize;
> -  EFI_E820_ENTRY64      E820Entry;
> -  UINTN                 Processed;
> -
> -  Status = QemuFwCfgFindFile ("etc/e820", &FwCfgItem, &FwCfgSize);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -
> -  if (FwCfgSize % sizeof E820Entry != 0) {
> -    return EFI_PROTOCOL_ERROR;
> -  }
> -
> -  if (LowMemory != NULL) {
> -    *LowMemory = 0;
> -  }
> -
> -  if (MaxAddress != NULL) {
> -    *MaxAddress = BASE_4GB;
> -  }
> -
> -  QemuFwCfgSelectItem (FwCfgItem);
> -  for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) {
> -    QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry);
> -    DEBUG ((
> -      DEBUG_VERBOSE,
> -      "%a: Base=0x%Lx Length=0x%Lx Type=%u\n",
> -      __FUNCTION__,
> -      E820Entry.BaseAddr,
> -      E820Entry.Length,
> -      E820Entry.Type
> -      ));
> -    if (E820Entry.Type == EfiAcpiAddressRangeMemory) {
> -      if (AddHighHob && (E820Entry.BaseAddr >= BASE_4GB)) {
> -        UINT64  Base;
> -        UINT64  End;
> -
> -        //
> -        // Round up the start address, and round down the end address.
> -        //
> -        Base = ALIGN_VALUE (E820Entry.BaseAddr, (UINT64)EFI_PAGE_SIZE);
> -        End  = (E820Entry.BaseAddr + E820Entry.Length) &
> -               ~(UINT64)EFI_PAGE_MASK;
> -        if (Base < End) {
> -          PlatformAddMemoryRangeHob (Base, End);
> -          DEBUG ((
> -            DEBUG_VERBOSE,
> -            "%a: PlatformAddMemoryRangeHob [0x%Lx, 0x%Lx)\n",
> -            __FUNCTION__,
> -            Base,
> -            End
> -            ));
> -        }
> -      }
> -
> -      if (MaxAddress || LowMemory) {
> -        UINT64  Candidate;
> -
> -        Candidate = E820Entry.BaseAddr + E820Entry.Length;
> -        if (MaxAddress && (Candidate > *MaxAddress)) {
> -          *MaxAddress = Candidate;
> -          DEBUG ((
> -            DEBUG_VERBOSE,
> -            "%a: MaxAddress=0x%Lx\n",
> -            __FUNCTION__,
> -            *MaxAddress
> -            ));
> -        }
> -
> -        if (LowMemory && (Candidate > *LowMemory) && (Candidate < BASE_4GB)) {
> -          *LowMemory = Candidate;
> -          DEBUG ((
> -            DEBUG_VERBOSE,
> -            "%a: LowMemory=0x%Lx\n",
> -            __FUNCTION__,
> -            *LowMemory
> -            ));
> -        }
> -      }
> -    } else if (E820Entry.Type == EfiAcpiAddressRangeReserved) {
> -      if (AddHighHob) {
> -        DEBUG ((
> -          DEBUG_INFO,
> -          "%a: Reserved: Base=0x%Lx Length=0x%Lx\n",
> -          __FUNCTION__,
> -          E820Entry.BaseAddr,
> -          E820Entry.Length
> -          ));
> -        BuildResourceDescriptorHob (
> -          EFI_RESOURCE_MEMORY_RESERVED,
> -          0,
> -          E820Entry.BaseAddr,
> -          E820Entry.Length
> -          );
> -      }
> -    }
> -  }
> -
> -  return EFI_SUCCESS;
> -}
> -
>  typedef VOID (*E820_SCAN_CALLBACK) (
>    EFI_E820_ENTRY64       *E820Entry,
>    EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
> @@ -304,6 +167,41 @@ PlatformGetLowMemoryCB (
>    }
>  }
>
> +/**
> +  Create HOBs for reservations and RAM (except low memory).
> +**/
> +VOID
> +PlatformAddHobCB (
> +  IN     EFI_E820_ENTRY64       *E820Entry,
> +  IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
> +  )
> +{
> +  UINT64  Base = E820Entry->BaseAddr;
> +  UINT64  End  = E820Entry->BaseAddr + E820Entry->Length;

(1) To my understanding, the edk2 coding style forbids initialization of
variables with automatic storage duration ("non-static locals").

> +
> +  switch (E820Entry->Type) {
> +    case EfiAcpiAddressRangeMemory:

(Side comment: I'm sure you've run this through Uncrustify -- so that's
a coding style change then. What I remember is that "switch" and "case"
were supposed to start in the same column.)

> +      //
> +      // Round up the start address, and round down the end address.
> +      //
> +      Base = ALIGN_VALUE (Base, (UINT64)EFI_PAGE_SIZE);
> +      End  = End & ~(UINT64)EFI_PAGE_MASK;
> +      if ((Base >= BASE_4GB) && (Base < End)) {

(2) This is not faithful conversion. The original code first compares
the base against 4GB, then rounds it up; this variant reverses the order
of those steps. I don't think if it will ever matter, but this is a
refactoring, so let's stick with the original logic (not hard to do).


> +        DEBUG ((DEBUG_INFO, "%a: HighMemory [0x%Lx, 0x%Lx]\n", __FUNCTION__, Base, End));

(3) I've not noticed before, but I'm noticing now: before this series,
the DEBUG levels (or more precisely, "debug masks") for the messages
emitted by PlatformScanOrAdd64BitE820Ram() were inconsistent. Most of
them were DEBUG_VERBOSE (per original intent), but then commit
bf65d7ee8842 ("OvmfPkg/PlatformInitLib: pass through reservations from
qemu", 2022-12-23) introduced a new branch with DEBUG_INFO. From the
perspective of this series, that's a preexistent inconsistency.

Is that worth fixing up at first, separately? (Changing it to
DEBUG_VERBOSE.)

(4) Also relating to logging. The current patch set seems to move all
DEBUG masks here to DEBUG_INFO. The uniformity is welcome, but I'm
unsure if DEBUG_INFO is justified. Writes to the QEMU debug console are
slow; are we sure we want these logs emitted in a defult build of OVMF?

(5) Still about logging. Before this series, the
PlatformScanOrAdd64BitE820Ram() function would log each E820 entry
before investigating them. (And, based on the investigation, further
messages may be logged.) With the whole series applied however, as far
as I can tell, we'll never get a complete dump of the E820 map, because
PlatformScanE820() doesn't log entries at all, and the callbacks only
log entries that prove "interesting" for them.

Is this intentional? I think it may make debugging harder. I didn't
notice it under patch#1, but I think we should add generic
(unconditional) logging there.

(6) *Yet more* logging-related observations. The original log message
uses a bracket "[" on the left hand side of the logged intervals, and a
parenthesis ")" on the RHS, to express that the RHS limit is exclusive:

            "%a: PlatformAddMemoryRangeHob [0x%Lx, 0x%Lx)\n",

This detail is lost in this patch (in all three DEBUG messages); both
sides use brackets.

(7) Sorry, I'm getting tired and my observations are starting to fall
apart. Anyway -- I think all the actual callback functions should be
STATIC.

(8) Furthermore, *perhaps* we should put E820 in their names somewhere
(I don't insist at all), instead of the "Platform" prefix -- these
functions are not public PlatformInitLib APIs.



> +        PlatformAddMemoryRangeHob (Base, End);
> +      }
> +
> +      break;

(9) Empty line intentional?

> +    case EfiAcpiAddressRangeReserved:
> +      BuildResourceDescriptorHob (EFI_RESOURCE_MEMORY_RESERVED, 0, Base, End);
> +      DEBUG ((DEBUG_INFO, "%a: Reserved [0x%Lx, 0x%Lx]\n", __FUNCTION__, Base, End));
> +      break;
> +    default:
> +      DEBUG ((DEBUG_WARN, "%a: Type %d [0x%Lx, 0x%Lx] (NOT HANDLED)\n", __FUNCTION__, E820Entry->Type, Base, End));

(10) I meant to ask earlier -- what is now the maximum line length?

I notice, in ".pytool/Plugin/UncrustifyCheck/uncrustify.cfg":

> # Code width / line splitting
> #code_width                      =120     # TODO: This causes non-deterministic behaviour in some cases when code wraps
> ls_code_width                   =false

Does that mean that 120 is considered a soft limit? (I used to ask for
80 chars under OvmfPkg, but there's no need to stick with that anymore.)

(11) "E820Entry->Type" is an enumeration type; per UEFI 2.10, 2.3.1 Data
Types, the UEFI build environment is supposed to make the compiler pick
INT32 or UINT32. (This is from Mantis ticket 913.)

Now, as long as QEMU sticks with the small set of enumeration constants
we define in "OvmfPkg/Include/IndustryStandard/E820.h", it's all fine,
and we can indeed print "E820Entry->Type" with both %d and %u,
interchangeably. I used "%u" in the original logging because I find the
printing of unexpected values as positive integers rather than negative
ones more tolerable. I don't mind %d as long as we're conscious of it.

Back to the patch:

On 1/10/23 09:21, Gerd Hoffmann wrote:
> +      break;
> +  }
> +}
> +
>  /**
>    Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map, call the
>    passed callback for each entry.
> @@ -1048,7 +946,7 @@ PlatformQemuInitializeRam (
>      // entries. Otherwise, create a single memory HOB with the flat >=4GB
>      // memory size read from the CMOS.
>      //
> -    Status = PlatformScanOrAdd64BitE820Ram (TRUE, NULL, NULL);
> +    Status = PlatformScanE820 (PlatformAddHobCB, PlatformInfoHob);
>      if (EFI_ERROR (Status)) {
>        UpperMemorySize = PlatformGetSystemMemorySizeAbove4gb ();
>        if (UpperMemorySize != 0) {

Looks OK.

Thanks,
Laszlo

Re: [PATCH v2 3/4] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB

[Gerd Hoffmann]

> > +        DEBUG ((DEBUG_INFO, "%a: HighMemory [0x%Lx, 0x%Lx]\n", __FUNCTION__, Base, End));
> 
> (3) I've not noticed before, but I'm noticing now: before this series,
> the DEBUG levels (or more precisely, "debug masks") for the messages
> emitted by PlatformScanOrAdd64BitE820Ram() were inconsistent. Most of
> them were DEBUG_VERBOSE (per original intent), but then commit
> bf65d7ee8842 ("OvmfPkg/PlatformInitLib: pass through reservations from
> qemu", 2022-12-23) introduced a new branch with DEBUG_INFO. From the
> perspective of this series, that's a preexistent inconsistency.
> 
> Is that worth fixing up at first, separately? (Changing it to
> DEBUG_VERBOSE.)
> 
> (4) Also relating to logging. The current patch set seems to move all
> DEBUG masks here to DEBUG_INFO. The uniformity is welcome, but I'm
> unsure if DEBUG_INFO is justified. Writes to the QEMU debug console are
> slow; are we sure we want these logs emitted in a defult build of OVMF?
> 
> (5) Still about logging. Before this series, the
> PlatformScanOrAdd64BitE820Ram() function would log each E820 entry
> before investigating them. (And, based on the investigation, further
> messages may be logged.) With the whole series applied however, as far
> as I can tell, we'll never get a complete dump of the E820 map, because
> PlatformScanE820() doesn't log entries at all, and the callbacks only
> log entries that prove "interesting" for them.
> 
> Is this intentional? I think it may make debugging harder. I didn't
> notice it under patch#1, but I think we should add generic
> (unconditional) logging there.

Yes, all intentional.

I've dropped all logging from PlatformScanE820, we run that multiple
times and logging the e820 map there would make it show up multiple
times in the logs.  Instead I'm logging at the places where the code
actually does something with the values (set LowMemory, add HOB, ...),
which should give us good insights into the code flow in the logs.

I've turned them on by default because the logging should be less
verbose than it used to be and also because I've found myself flipping
these from VERBOSE to INFO frequently when debugging something.

> (6) *Yet more* logging-related observations. The original log message
> uses a bracket "[" on the left hand side of the logged intervals, and a
> parenthesis ")" on the RHS, to express that the RHS limit is exclusive:
> 
>             "%a: PlatformAddMemoryRangeHob [0x%Lx, 0x%Lx)\n",
> 
> This detail is lost in this patch (in all three DEBUG messages); both
> sides use brackets.

Oh, I wasn't aware of that.  It looked like a typo to me so I "fixed" it
along the way.  I think I prefer to stick with the (inclusive) brackets
and print "End - 1" then so it actually is inclusive.

> (7) Sorry, I'm getting tired and my observations are starting to fall
> apart. Anyway -- I think all the actual callback functions should be
> STATIC.

Yes.  PlatformScanE820() is static too.  Isn't there a gcc warning which
complains about non-static functions without prototype in some header?
Seems this is not turned on for edk2, I'm somehow used to gcc throwing
warnings on that.

> (8) Furthermore, *perhaps* we should put E820 in their names somewhere
> (I don't insist at all), instead of the "Platform" prefix -- these
> functions are not public PlatformInitLib APIs.

There is a simple reason for the naming:
I love 'grep ^Platform' on edk2 log files ;)

> > +      DEBUG ((DEBUG_WARN, "%a: Type %d [0x%Lx, 0x%Lx] (NOT HANDLED)\n", __FUNCTION__, E820Entry->Type, Base, End));
> 
> (10) I meant to ask earlier -- what is now the maximum line length?
> 
> I notice, in ".pytool/Plugin/UncrustifyCheck/uncrustify.cfg":
> 
> > # Code width / line splitting
> > #code_width                      =120     # TODO: This causes non-deterministic behaviour in some cases when code wraps
> > ls_code_width                   =false
> 
> Does that mean that 120 is considered a soft limit? (I used to ask for
> 80 chars under OvmfPkg, but there's no need to stick with that anymore.)

Oh, 120.  I already wondered why uncrustify didn't wrap that one, I
didn't expect the limit being higher than 100 which seems to be the new
standard in many projects.

take care,
  Gerd

Re: [PATCH v2 3/4] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB

[Laszlo Ersek]

On 1/11/23 09:06, Gerd Hoffmann wrote:
>>> +        DEBUG ((DEBUG_INFO, "%a: HighMemory [0x%Lx, 0x%Lx]\n", __FUNCTION__, Base, End));
>>
>> (3) I've not noticed before, but I'm noticing now: before this series,
>> the DEBUG levels (or more precisely, "debug masks") for the messages
>> emitted by PlatformScanOrAdd64BitE820Ram() were inconsistent. Most of
>> them were DEBUG_VERBOSE (per original intent), but then commit
>> bf65d7ee8842 ("OvmfPkg/PlatformInitLib: pass through reservations from
>> qemu", 2022-12-23) introduced a new branch with DEBUG_INFO. From the
>> perspective of this series, that's a preexistent inconsistency.
>>
>> Is that worth fixing up at first, separately? (Changing it to
>> DEBUG_VERBOSE.)
>>
>> (4) Also relating to logging. The current patch set seems to move all
>> DEBUG masks here to DEBUG_INFO. The uniformity is welcome, but I'm
>> unsure if DEBUG_INFO is justified. Writes to the QEMU debug console are
>> slow; are we sure we want these logs emitted in a defult build of OVMF?
>>
>> (5) Still about logging. Before this series, the
>> PlatformScanOrAdd64BitE820Ram() function would log each E820 entry
>> before investigating them. (And, based on the investigation, further
>> messages may be logged.) With the whole series applied however, as far
>> as I can tell, we'll never get a complete dump of the E820 map, because
>> PlatformScanE820() doesn't log entries at all, and the callbacks only
>> log entries that prove "interesting" for them.
>>
>> Is this intentional? I think it may make debugging harder. I didn't
>> notice it under patch#1, but I think we should add generic
>> (unconditional) logging there.
> 
> Yes, all intentional.
> 
> I've dropped all logging from PlatformScanE820, we run that multiple
> times and logging the e820 map there would make it show up multiple
> times in the logs.  Instead I'm logging at the places where the code
> actually does something with the values (set LowMemory, add HOB, ...),
> which should give us good insights into the code flow in the logs.
> 
> I've turned them on by default because the logging should be less
> verbose than it used to be and also because I've found myself flipping
> these from VERBOSE to INFO frequently when debugging something.

I think the last part should be replaced by you just building with
DEBUG_VERBOSE :), carrying perhaps a few patches for the DSC files for
re-disabling DEBUG_VERBOSE for a number of unjustifiably chatty modules
(I can give you a list); *regardless*, I think the above is all good,
just please mention it somewhere in the commit messages.

(Best would be to change the debug levels after the conversion,
separately, but I don't want to get on your nerves.)

> 
>> (6) *Yet more* logging-related observations. The original log message
>> uses a bracket "[" on the left hand side of the logged intervals, and a
>> parenthesis ")" on the RHS, to express that the RHS limit is exclusive:
>>
>>             "%a: PlatformAddMemoryRangeHob [0x%Lx, 0x%Lx)\n",
>>
>> This detail is lost in this patch (in all three DEBUG messages); both
>> sides use brackets.
> 
> Oh, I wasn't aware of that.  It looked like a typo to me so I "fixed" it
> along the way.  I think I prefer to stick with the (inclusive) brackets
> and print "End - 1" then so it actually is inclusive.

Works for me -- as long as you won't be searching the firmware logs for
the *exclusive end* hex strings :) :) :)

(Now of course you can satisfy both goals, as long as you don't print
(End-1) with "0x%Lx", but print (End) with "0x%Lx - 1"! In fact, you may
have meant the latter already, above.)

> 
>> (7) Sorry, I'm getting tired and my observations are starting to fall
>> apart. Anyway -- I think all the actual callback functions should be
>> STATIC.
> 
> Yes.  PlatformScanE820() is static too.  Isn't there a gcc warning which
> complains about non-static functions without prototype in some header?
> Seems this is not turned on for edk2, I'm somehow used to gcc throwing
> warnings on that.

Haha, good catch -- it's intentionally not turned on in edk2. Namely,
some version of MSVC (maybe even bleeding edge ones, I don't know) screw
up source-level debugging for such functions that are marked STATIC, as
far as I remember. This is why, in core packages, you'll see an
inexplicably low number of STATIC functions. So the MSVC shortcoming (if
I can call it that) actively conflicts with the nice gcc warning you
mention. The gcc warning is a no-brainer for when you can actually
enable it.

> 
>> (8) Furthermore, *perhaps* we should put E820 in their names somewhere
>> (I don't insist at all), instead of the "Platform" prefix -- these
>> functions are not public PlatformInitLib APIs.
> 
> There is a simple reason for the naming:
> I love 'grep ^Platform' on edk2 log files ;)

That sounds convincing enough, thanks. Log analysis is important, we
should support it!

> 
>>> +      DEBUG ((DEBUG_WARN, "%a: Type %d [0x%Lx, 0x%Lx] (NOT HANDLED)\n", __FUNCTION__, E820Entry->Type, Base, End));
>>
>> (10) I meant to ask earlier -- what is now the maximum line length?
>>
>> I notice, in ".pytool/Plugin/UncrustifyCheck/uncrustify.cfg":
>>
>>> # Code width / line splitting
>>> #code_width                      =120     # TODO: This causes non-deterministic behaviour in some cases when code wraps
>>> ls_code_width                   =false
>>
>> Does that mean that 120 is considered a soft limit? (I used to ask for
>> 80 chars under OvmfPkg, but there's no need to stick with that anymore.)
> 
> Oh, 120.  I already wondered why uncrustify didn't wrap that one, I
> didn't expect the limit being higher than 100 which seems to be the new
> standard in many projects.
> 
> take care,
>   Gerd
> 

Thanks!
Laszlo

[PATCH v2 4/4] OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB

[Gerd Hoffmann]

Add PlatformReservationConflictCB() callback function for use with
PlatformScanE820().  It checks whenever the 64bit PCI MMIO window
overlaps with a reservation from qemu.  If so move down the MMIO window
to resolve the conflict.

This happens on (virtal) AMD machines with 1TB address space,
because the AMD IOMMU uses an address window just below 1GB.

Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4251
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/Library/PlatformInitLib/MemDetect.c | 41 +++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index 83a219581a1b..f12d48cad755 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -202,6 +202,46 @@ PlatformAddHobCB (
   }
 }
 
+/**
+  Check whenever the 64bit PCI MMIO window overlaps with a reservation
+  from qemu.  If so move down the MMIO window to resolve the conflict.
+
+  This happens on (virtal) AMD machines with 1TB address space,
+  because the AMD IOMMU uses an address window just below 1GB.
+**/
+VOID
+PlatformReservationConflictCB (
+  IN     EFI_E820_ENTRY64       *E820Entry,
+  IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
+  )
+{
+  UINT64  IntersectionBase = MAX (
+                               E820Entry->BaseAddr,
+                               PlatformInfoHob->PcdPciMmio64Base
+                               );
+  UINT64  IntersectionEnd = MIN (
+                              E820Entry->BaseAddr + E820Entry->Length,
+                              PlatformInfoHob->PcdPciMmio64Base +
+                              PlatformInfoHob->PcdPciMmio64Size
+                              );
+  UINT64  NewBase;
+
+  if (IntersectionBase >= IntersectionEnd) {
+    return;  // no overlap
+  }
+
+  NewBase = (PlatformInfoHob->PcdPciMmio64Base -
+             PlatformInfoHob->PcdPciMmio64Size);
+  DEBUG ((
+    DEBUG_INFO,
+    "%a: move mmio: 0x%Lx => %Lx\n",
+    __FUNCTION__,
+    PlatformInfoHob->PcdPciMmio64Base,
+    NewBase
+    ));
+  PlatformInfoHob->PcdPciMmio64Base = NewBase;
+}
+
 /**
   Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map, call the
   passed callback for each entry.
@@ -638,6 +678,7 @@ PlatformDynamicMmioWindow (
     DEBUG ((DEBUG_INFO, "%a:   MMIO Space 0x%Lx (%Ld GB)\n", __func__, MmioSpace, RShiftU64 (MmioSpace, 30)));
     PlatformInfoHob->PcdPciMmio64Size = MmioSpace;
     PlatformInfoHob->PcdPciMmio64Base = AddrSpace - MmioSpace;
+    PlatformScanE820 (PlatformReservationConflictCB, PlatformInfoHob);
   } else {
     DEBUG ((DEBUG_INFO, "%a: using classic mmio window\n", __func__));
   }
-- 
2.39.0

Re: [PATCH v2 4/4] OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB

[Laszlo Ersek]

On 1/10/23 09:21, Gerd Hoffmann wrote:
> Add PlatformReservationConflictCB() callback function for use with
> PlatformScanE820().  It checks whenever the 64bit PCI MMIO window
> overlaps with a reservation from qemu.  If so move down the MMIO window
> to resolve the conflict.
> 
> This happens on (virtal) AMD machines with 1TB address space,

(1) s/virtal/virtual/

> because the AMD IOMMU uses an address window just below 1GB.

(2) s/1GB/1TB/?

> 
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4251
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/Library/PlatformInitLib/MemDetect.c | 41 +++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index 83a219581a1b..f12d48cad755 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> @@ -202,6 +202,46 @@ PlatformAddHobCB (
>    }
>  }
>  
> +/**
> +  Check whenever the 64bit PCI MMIO window overlaps with a reservation
> +  from qemu.  If so move down the MMIO window to resolve the conflict.
> +
> +  This happens on (virtal) AMD machines with 1TB address space,
> +  because the AMD IOMMU uses an address window just below 1GB.

(3) Same two typos, I think, as in the commit message.

> +**/
> +VOID
> +PlatformReservationConflictCB (

(4) should be STATIC etc, maybe use E820 in the name rathern than
Platform, etc... up to you.

> +  IN     EFI_E820_ENTRY64       *E820Entry,
> +  IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
> +  )
> +{
> +  UINT64  IntersectionBase = MAX (
> +                               E820Entry->BaseAddr,
> +                               PlatformInfoHob->PcdPciMmio64Base
> +                               );
> +  UINT64  IntersectionEnd = MIN (
> +                              E820Entry->BaseAddr + E820Entry->Length,
> +                              PlatformInfoHob->PcdPciMmio64Base +
> +                              PlatformInfoHob->PcdPciMmio64Size
> +                              );

(5) Locals should not be initialized (per my last memories of the edk2
coding style).

> +  UINT64  NewBase;
> +
> +  if (IntersectionBase >= IntersectionEnd) {
> +    return;  // no overlap
> +  }
> +
> +  NewBase = (PlatformInfoHob->PcdPciMmio64Base -
> +             PlatformInfoHob->PcdPciMmio64Size);

(6) This appears a typo; we'll want

  NewBase + PcdPciMmio64Size == E820Entry->BaseAddr

where the LHS stands for the exclusive end of the moved MMIO aperture,
with unchanged size, and the RHS stands for the inclusive base of the
(unmovable) reservation. The above formula ensures that the intersection
be empty, without changing sizes, or moving the reservation. Then,
reordering the formula for an assignment, we get

  NewBase = E820Entry->BaseAddr - PcdPciMmio64Size;

as an assignment.

So, yes, a typo.

> +  DEBUG ((
> +    DEBUG_INFO,
> +    "%a: move mmio: 0x%Lx => %Lx\n",

(7) pls consider DEBUG_VERBOSE, like before

(8) usage of the 0x prefix in the debug message is inconsistent, we
should prefix the NewBase output with it, too

> +    __FUNCTION__,
> +    PlatformInfoHob->PcdPciMmio64Base,
> +    NewBase
> +    ));
> +  PlatformInfoHob->PcdPciMmio64Base = NewBase;
> +}

(9) Do we need any other checks or maybe assertions that we're only
conflicting with a reserved area, and/or that the subtraction for
NewBase does not underflow?

I don't think we can "armor" this very well, I'm just pondering if there
are any egregious misunderstandings between QEMU and the firmware that
we might want to catch here. If not, that's OK of course.

> +
>  /**
>    Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map, call the
>    passed callback for each entry.
> @@ -638,6 +678,7 @@ PlatformDynamicMmioWindow (
>      DEBUG ((DEBUG_INFO, "%a:   MMIO Space 0x%Lx (%Ld GB)\n", __func__, MmioSpace, RShiftU64 (MmioSpace, 30)));
>      PlatformInfoHob->PcdPciMmio64Size = MmioSpace;
>      PlatformInfoHob->PcdPciMmio64Base = AddrSpace - MmioSpace;
> +    PlatformScanE820 (PlatformReservationConflictCB, PlatformInfoHob);
>    } else {
>      DEBUG ((DEBUG_INFO, "%a: using classic mmio window\n", __func__));
>    }

Looks fine.

Thanks,
Laszlo

Re: [PATCH v2 4/4] OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB

[Gerd Hoffmann]

  Hi,

> > +/**
> > +  Check whenever the 64bit PCI MMIO window overlaps with a reservation
> > +  from qemu.  If so move down the MMIO window to resolve the conflict.
> > +
> > +  This happens on (virtal) AMD machines with 1TB address space,
> > +  because the AMD IOMMU uses an address window just below 1GB.
> 
> (3) Same two typos, I think, as in the commit message.

Duplicated by the power of cut + paste.

> > +  NewBase = (PlatformInfoHob->PcdPciMmio64Base -
> > +             PlatformInfoHob->PcdPciMmio64Size);
> 
> (6) This appears a typo; we'll want
> 
>   NewBase + PcdPciMmio64Size == E820Entry->BaseAddr

But then NewBase is not aligned.  The assignment above moves it down
while maintaining the existing alignment.

> (9) Do we need any other checks or maybe assertions that we're only
> conflicting with a reserved area, and/or that the subtraction for
> NewBase does not underflow?
> 
> I don't think we can "armor" this very well, I'm just pondering if there
> are any egregious misunderstandings between QEMU and the firmware that
> we might want to catch here. If not, that's OK of course.

Yes, it's hard to design something which can handle all reservations
qemu might do in the future correctly.  And, yes, the code above works
because we know the qemu reservation is smaller than the mmio window, so
moving down to the next naturally aligned address actually solves the
conflict.

take care,
  Gerd

Re: [PATCH v2 4/4] OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB

[Laszlo Ersek]

On 1/11/23 09:26, Gerd Hoffmann wrote:
>   Hi,
> 
>>> +/**
>>> +  Check whenever the 64bit PCI MMIO window overlaps with a reservation
>>> +  from qemu.  If so move down the MMIO window to resolve the conflict.
>>> +
>>> +  This happens on (virtal) AMD machines with 1TB address space,
>>> +  because the AMD IOMMU uses an address window just below 1GB.
>>
>> (3) Same two typos, I think, as in the commit message.
> 
> Duplicated by the power of cut + paste.
> 
>>> +  NewBase = (PlatformInfoHob->PcdPciMmio64Base -
>>> +             PlatformInfoHob->PcdPciMmio64Size);
>>
>> (6) This appears a typo; we'll want
>>
>>   NewBase + PcdPciMmio64Size == E820Entry->BaseAddr
> 
> But then NewBase is not aligned.  The assignment above moves it down
> while maintaining the existing alignment.

Ah, good point. I totally missed the idea here.

The starting predicate is, apparently, that the base is aligned by size
("naturally aligned"), so by subtracting size from the aperture, we get
a new aperture that conforms to both properties below:

- still naturally aligned (original predicate preserved),

- empty intersection with the original aperture (because the new
aperture will end where the old one just started).

Now, here's one thought: just because the new (sunken) aperture does not
intersect the pre-move aperture, is it guaranteed to also steer clear of
the E820 Entry either?

I don't think that's certain. If the E820 Entry overlaps the bottom of
the original aperture, but goes "deeper" than that, then it will overlap
the top of the sunken aperture too.

So I think we might want to do a two step process here, in case the
original aperture overlaps the E820 Entry:

- push down the aperture (same size) so its exclusive end equal the
inclusive start of the E820 Entry

- align *down* (truncate) the base of the aperture, while keeping its
size unchanged.

Something like:

  NewBase = E820Entry->BaseAddr - PcdPciMmio64Size;

  ASSERT (PcdPciMmio64Size != 0);
  ASSERT ((PcdPciMmio64Size & (PcdPciMmio64Size - 1)) == 0);
  NewBase = NewBase & ~(PcdPciMmio64Size - 1);

> 
>> (9) Do we need any other checks or maybe assertions that we're only
>> conflicting with a reserved area, and/or that the subtraction for
>> NewBase does not underflow?
>>
>> I don't think we can "armor" this very well, I'm just pondering if there
>> are any egregious misunderstandings between QEMU and the firmware that
>> we might want to catch here. If not, that's OK of course.
> 
> Yes, it's hard to design something which can handle all reservations
> qemu might do in the future correctly.  And, yes, the code above works
> because we know the qemu reservation is smaller than the mmio window, so
> moving down to the next naturally aligned address actually solves the
> conflict.

Not only smaller, but also *not encompassing* the lower boundary of the
MMIO window.

Anyway, up to you -- if you want to stick with the logic shown in this
patch, I'm OK, but then please add some comments, or maybe even some
ASSERTs.

Thanks!
Laszlo