Re: [PATCH 4/5] ArmVirtPkg: disregard PcdPrePiCpuMemorySize PCD when sizing the GCD space

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Mon, 26 Nov 2018 at 11:47, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 11/23/18 13:14, Ard Biesheuvel wrote:
> > Move the call to BuildCpuHob () into ArmVirtMemoryInitPeiLib (which is
> > shared between all the ArmVirtPkg targets), and drop the inclusion of
> > CpuPei.inf [which calls it on ArmVirtQemu] and the BuildCpuHob () call
> > from ArmVirtPrePiUniCoreRelocatable [for the other targets].
>
> (1) Can you please confirm, in the commit message, that we don't need --
> or that we don't exercise anyway -- the other things that
> "ArmPkg/Drivers/CpuPei/CpuPei.inf" does, namely:
>
> - ArmEnableBranchPrediction()

This does nothing on AArch64, and even on ARM, this is something that
is enabled by the bare metal boot code, not per-guest.

> - building gArmMpCoreInfoGuid
>

That is for the MPCore versions of PrePi and PrePeiCore, and we only
use UniCore versions in ArmVirtPkg

I will add some clarification to the commit log.

> >
> > This makes the size of the GCD address space and page table mappings
> > depend only on the size of the physical address space as exposed by
> > the CPU system registers.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  ArmVirtPkg/ArmVirtQemu.dsc                                             | 1 -
> >  ArmVirtPkg/ArmVirtQemu.fdf                                             | 1 -
> >  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c   | 3 +++
> >  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf | 1 +
> >  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c             | 5 +----
> >  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf           | 1 -
> >  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf        | 1 -
> >  ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf                    | 3 ---
> >  ArmVirtPkg/PrePi/PrePi.c                                               | 3 ---
> >  9 files changed, 5 insertions(+), 14 deletions(-)
> >
> > diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> > index 885c6b14b844..cb59c790afcc 100644
> > --- a/ArmVirtPkg/ArmVirtQemu.dsc
> > +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> > @@ -226,7 +226,6 @@
> >    }
> >    ArmPlatformPkg/PlatformPei/PlatformPeim.inf
> >    ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
> > -  ArmPkg/Drivers/CpuPei/CpuPei.inf
> >
> >    MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
> >
> > diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
> > index c6a22dc018f3..12bc570c4cb3 100644
> > --- a/ArmVirtPkg/ArmVirtQemu.fdf
> > +++ b/ArmVirtPkg/ArmVirtQemu.fdf
> > @@ -106,7 +106,6 @@ READ_LOCK_STATUS   = TRUE
> >    INF MdeModulePkg/Core/Pei/PeiMain.inf
> >    INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf
> >    INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
> > -  INF ArmPkg/Drivers/CpuPei/CpuPei.inf
> >    INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
> >    INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
> >    INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>
> OK, so this removes the previous CPU HOB building in ArmVirtQemu.
>
> Quoting out of order:
>
> > diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> > index 1587bd92f206..e04bd1b7c497 100755
> > --- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> > +++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> > @@ -85,9 +85,6 @@
> >
> >    gArmPlatformTokenSpaceGuid.PcdCoreCount
> >
> > -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
> > -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize
> > -
> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
> > diff --git a/ArmVirtPkg/PrePi/PrePi.c b/ArmVirtPkg/PrePi/PrePi.c
> > index f6abe2f2016b..ecaaac1545c4 100755
> > --- a/ArmVirtPkg/PrePi/PrePi.c
> > +++ b/ArmVirtPkg/PrePi/PrePi.c
> > @@ -79,9 +79,6 @@ PrePiMain (
> >    StacksSize = PcdGet32 (PcdCPUCorePrimaryStackSize);
> >    BuildStackHob (StacksBase, StacksSize);
> >
> > -  //TODO: Call CpuPei as a library
> > -  BuildCpuHob (PcdGet8 (PcdPrePiCpuMemorySize), PcdGet8 (PcdPrePiCpuIoSize));
> > -
>
> Best way to solve a TODO is to delete it. :)
>
> >    // Set the Boot Mode
> >    SetBootMode (BOOT_WITH_FULL_CONFIGURATION);
> >
> >
>
> OK, this removes the previous CPU HOB building in ArmVirtQemuKernel and
> ArmVirtXen.
>
>
> > diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> > index 3f0e9b3a0579..3d86d31ab50e 100644
> > --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> > +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> > @@ -116,5 +116,8 @@ MemoryPeim (
> >      BuildMemoryTypeInformationHob ();
> >    }
> >
> > +  // Publish the CPU memory and io spaces sizes
> > +  BuildCpuHob (ArmGetPhysicalAddressBits (), FixedPcdGet8 (PcdPrePiCpuIoSize));
> > +
> >    return EFI_SUCCESS;
> >  }
>
> Argh, it took me a while to see that this will indeed supplement the CPU
> HOB on all ArmVirtPlatforms. The build report file(s) helped. I didn't
> think of ArmPlatformPkg originally. :)
>
> So, yes, this is fine; it's OK to produce the CPU HOB anywhere in the
> PEI phase.
>
> > diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
> > index 54879d590a8a..d0e39df84b20 100644
> > --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
> > +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
> > @@ -59,6 +59,7 @@
> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesData
> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderCode
> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderData
> > +  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize
> >
> >  [Pcd]
> >    gArmTokenSpaceGuid.PcdSystemMemoryBase
>
> OK, expected.
>
> > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> > index 4eca9b895354..ded87d604f4f 100644
> > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> > @@ -46,7 +46,6 @@ ArmVirtGetMemoryMap (
> >    )
> >  {
> >    ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;
> > -  UINT64                        TopOfMemory;
> >
> >    ASSERT (VirtualMemoryMap != NULL);
> >
> > @@ -80,11 +79,9 @@ ArmVirtGetMemoryMap (
> >    VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >
> >    // Peripheral space after DRAM
> > -  TopOfMemory = MIN (1ULL << FixedPcdGet8 (PcdPrePiCpuMemorySize),
> > -                     TopOfAddressSpace);
> >    VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
> >    VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
> > -  VirtualMemoryTable[2].Length       = TopOfMemory -
> > +  VirtualMemoryTable[2].Length       = TopOfAddressSpace -
> >                                         VirtualMemoryTable[2].PhysicalBase;
> >    VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >
> > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> > index f2c461e3b55a..5c5b841051ad 100644
> > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> > @@ -45,4 +45,3 @@
> >
> >  [FixedPcd]
> >    gArmTokenSpaceGuid.PcdFdSize
> > -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
> > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> > index f54fb51ee1d4..d12089760b22 100644
> > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> > @@ -49,4 +49,3 @@
> >  [FixedPcd]
> >    gArmTokenSpaceGuid.PcdFdSize
> >    gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
> > -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
>
> This last part, for QemuVirtMemInfoLib, is functionally valid, in my
> opinion. We are eliminating the clamping to PcdPrePiCpuMemorySize, which
> was one of my earlier points, so what remains is "TopOfMemory =
> TopOfAddressSpace"; good.
>

Now, I'm not so sure anymore whether this hunk is correct.

This describes the extent of DRAM to the generic MMU code, and we only
want to map what
a) the CPU can support and
b) what we actually describe as being populated by anything

So if your guest supports 52 bits of PA space in hardware, we want the
GCD space to accommodate that (so we can report memory as residing
there). However, we cannot map all of that memory in UEFI anyway, and
so capping the mapping (this hunk) to the PcdPrePiCpuMemorySize size
actually makes sense.


> However: this change does not relate to the GCD memory space. The
> ArmVirtGetMemoryMap() function partakes in the MMU configuration / page
> table setup. Its leading comment says,
>
>   This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU
>   on your platform.
>
> (2) Therefore, it should be split to a separate patch. Earlier, I
> suggested a slight restructuring for the patch series, so let me refine
> that:
>
> - new patch #1: current patch #1 (introduce helper)
>
> - new patch #2: current patch #3 (refactor ArmVirtPkg)
>
> - new patch #3: current patch #2 (consider CPU capabilities in generic
>                 MMU setup code)
>
> - new patch #4: this specific hunk, from current patch #4 (consider CPU
>                 capabilities in specific MMU setup code)
>
> - new patch #5: the rest of current patch #4 (consider CPU caps in GCD /
>                 CPU HOB)
>
> - new patch #6: kill PcdPrePiCpuMemorySize in ArmVirtPkg DSC files
>
> Does this sound acceptable?
>

Works for me.