Re: [PATCH 0/5] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit

[Laszlo Ersek <lersek@redhat.com>]

On 11/23/18 13:14, Ard Biesheuvel wrote:
> The ArmVirtQemu targets currently limit the size of the IPA space to
> 40 bits because that is all what KVM supports. However, this is about
> to change, and so we need to update the code if we want to ensure that
> our UEFI firmware builds can keep running on systems that set values
> other than 40 (which could be > 40 or < 40)
> 
> So add a helper to ArmLib to read the number of supported address bits (#1)
> and take this into account in the page table code (#2), which allows
> PcdPrePiCpuMemorySize to assume a value that exceeds the capabilities of
> the CPU.
> 
> Patch #3 is mostly a cleanup patch, to switch to the new helper added in
> patch #1. No functional changes intended.
> 
> Patch #4 builds the CPU hob (and thus declares the size of the GCD memory
> space) based on the CPU capabilities rather than the value of 
> PcdPrePiCpuMemorySize, to prevent any potential regressions in memory
> utilization when we bump PcdPrePiCpuMemorySize back to 48.
> 
> Patch #5 drops the definitions of PcdPrePiCpuMemorySize, reverting its
> value back to the default 48.

Staring a bit more at this, I wonder if it were helpful to reorder the
patches like this (just thinking loudly, I'm not even suggesting it, I'm
just curious about your opinion):

- patch #1: keep in place (introduce new helper)
- patch #2: current patch #3 (ArmVirtPkg refactoring), to benefit from
  the new helper at once, without any relation to the feature that's the
  end goal here.
- patch #3: current patch #2, build page tables with CPU PA limits
  accounted for
- patch #4: current patch #4, build GCD memory space map with CPU PA
  limits accounted for
- patch #5: remove the traces of the now useless PCD from ArmVirt

So basically, swap around #2 and #3. It's not really important; the
reason I'm thinking of it is the following though: while removing the
40-bit limitation, my first thought is, "what are the current consumers,
what needs to be updated".

The current structuring of the series, with patch #3 in the middle,
suggests that ArmVirtMemInfoLib instances are consumers. That's not the
case however, they already fetch the CPU PA limits dynamically. So they
need no functional updates, just a cleanup / centralization. That's why
I'd find it helpful to handle ArmVirtMemInfoLib right after the
introduction of the helper.

The actual consumers (in need of functional updates) are the page tables
and the GCD memory space map (two concepts, not three).

If you think this would be an improvement, please consider the
reordering. No need to repost just for this.

Thanks!
Laszlo


> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Cc: Philippe Mathieu-Daude <philmd@redhat.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> 
> Ard Biesheuvel (5):
>   ArmPkg/ArmLib: add support for reading the max physical address space
>     size
>   ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account
>   ArmVirtPkg: refactor reading of the physical address space size
>   ArmVirtPkg: disregard PcdPrePiCpuMemorySize PCD when sizing the GCD
>     space
>   ArmVirtPkg: revert PcdPrePiCpuMemorySize to is default value of 48
> 
>  ArmPkg/Include/Library/ArmLib.h               |  6 +++
>  ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S | 16 ++++++++
>  ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S     |  8 ++++
>  .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  5 ++-
>  ArmVirtPkg/ArmVirtQemu.dsc                    |  5 ---
>  ArmVirtPkg/ArmVirtQemu.fdf                    |  1 -
>  ArmVirtPkg/ArmVirtQemuKernel.dsc              |  4 --
>  .../Include/Library/ArmVirtMemInfoLib.h       |  1 +
>  .../ArmVirtMemoryInitPeiLib.c                 |  7 +++-
>  .../ArmVirtMemoryInitPeiLib.inf               |  1 +
>  .../QemuVirtMemInfoLib/AArch64/PhysAddrTop.S  | 39 -------------------
>  .../QemuVirtMemInfoLib/Arm/PhysAddrTop.S      | 24 ------------
>  .../QemuVirtMemInfoLib/QemuVirtMemInfoLib.c   |  6 +--
>  .../QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf |  7 ----
>  .../QemuVirtMemInfoPeiLib.inf                 |  7 ----
>  .../XenVirtMemInfoLib/AArch64/PhysAddrTop.S   | 39 -------------------
>  .../XenVirtMemInfoLib/Arm/PhysAddrTop.S       | 24 ------------
>  .../XenVirtMemInfoLib/XenVirtMemInfoLib.c     |  8 +---
>  .../XenVirtMemInfoLib/XenVirtMemInfoLib.inf   |  6 ---
>  .../PrePi/ArmVirtPrePiUniCoreRelocatable.inf  |  3 --
>  ArmVirtPkg/PrePi/PrePi.c                      |  3 --
>  21 files changed, 46 insertions(+), 174 deletions(-)
>  delete mode 100644 ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
>  delete mode 100644 ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
>  delete mode 100644 ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S
>  delete mode 100644 ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S
>