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

[Laszlo Ersek <lersek@redhat.com>]

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()
- building gArmMpCoreInfoGuid

?

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

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?

Thanks!
Laszlo