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

["Gerd Hoffmann" <kraxel@redhat.com>]

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