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



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

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



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

> > +  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



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

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



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

> > 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



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

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



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

  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



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

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



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