Re: [PATCH 2/5] ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Nov 23, 2018 at 01:14:28PM +0100, Ard Biesheuvel wrote:
> In preparation of permitting the virt code to define a larger PA space
> size via gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize than what the
> CPU actually supports, take the CPU's capabilities into account when
> setting up the page tables. This is necessary because KVM will shortly
> support variable PA space sizes, and to support running the same UEFI
> binaries regardless of that limit, PcdPrePiCpuMemorySize needs to be
> treated as an upper bound rather than a fixed size.

Why do we keep PcdPrePiCpuMemorySize at all? (I.e., rather than just
using the probed value?
Mainly for the purpose of being able to restrict ourselves to 32/48
bits?

If we keep it, should we rename
PcdPrePiCpuMemorySize -> PcdPrePiCpuMemoryBits
and
PcdPrePiCpuIoSize -> PcdPrePiCpuIoBits
?

Argument against this would be that later consumers still refer to
the value extracted from the HOB as SizeOf*Space, and happily shifts
1s around by it :|

/
    Leif

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index 4b62ecb6a476..a4fde9b59383 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -593,6 +593,7 @@ ArmConfigureMmu (
>  {
>    VOID*                         TranslationTable;
>    UINT32                        TranslationTableAttribute;
> +  UINTN                         MaxAddressBits;
>    UINT64                        MaxAddress;
>    UINTN                         T0SZ;
>    UINTN                         RootTableEntryCount;
> @@ -605,7 +606,9 @@ ArmConfigureMmu (
>    }
>  
>    // Cover the entire GCD memory space
> -  MaxAddress = (1UL << PcdGet8 (PcdPrePiCpuMemorySize)) - 1;
> +  MaxAddressBits = MIN (ArmGetPhysicalAddressBits (),
> +                        PcdGet8 (PcdPrePiCpuMemorySize));
> +  MaxAddress = (1UL << MaxAddressBits) - 1;
>  
>    // Lookup the Table Level to get the information
>    LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);
> -- 
> 2.17.1
>