> > + 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
