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

["Laszlo Ersek" <lersek@redhat.com>]

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