Re: [PATCH v3 08/16] ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account

[Laszlo Ersek <lersek@redhat.com>]

On 11/28/18 15:33, Ard Biesheuvel wrote:
> In preparation of dropping PcdPrePiCpuMemorySize entirely, base the
> maximum size of the identity map on the capabilities of the CPU.
> Since that may exceed what is architecturally permitted when using
> 4 KB pages, take MAX_ADDRESS into account as well.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf       |  3 ---
>  ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf        |  3 ---
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 11 +++++++++--
>  3 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf b/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
> index b9f264de8d26..246963361e45 100644
> --- a/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
> +++ b/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
> @@ -40,8 +40,5 @@ [LibraryClasses]
>    CacheMaintenanceLib
>    MemoryAllocationLib
>
> -[Pcd.AARCH64]
> -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
> -
>  [Pcd.ARM]
>    gArmTokenSpaceGuid.PcdNormalMemoryNonshareableOverride
> diff --git a/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf b/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
> index ecf13f790734..f689c193b862 100644
> --- a/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
> +++ b/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
> @@ -35,6 +35,3 @@ [LibraryClasses]
>    ArmLib
>    CacheMaintenanceLib
>    MemoryAllocationLib
> -
> -[Pcd.AARCH64]
> -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize

OK

> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index 4b62ecb6a476..5403b8d4070e 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -604,8 +604,15 @@ ArmConfigureMmu (
>      return EFI_INVALID_PARAMETER;
>    }
>
> -  // Cover the entire GCD memory space
> -  MaxAddress = (1UL << PcdGet8 (PcdPrePiCpuMemorySize)) - 1;
> +  //
> +  // Limit the virtual address space to what we can actually use: UEFI
> +  // mandates a 1:1 mapping, so no point in making the virtual address
> +  // space larger than the physical address space. We also have to take
> +  // into account the architectural limitations that result from UEFI's
> +  // use of 4 KB pages.
> +  //
> +  MaxAddress = MIN (LShiftU64 (1ULL, ArmGetPhysicalAddressBits ()) - 1,
> +                    MAX_ADDRESS);
>
>    // Lookup the Table Level to get the information
>    LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);
>

This part of edk2 (or ArmVirtQemu) is where I *always* get lost. So let
me check the call tree.

  ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf [ArmVirtPkg/ArmVirtQemu.dsc]
    InitializeMemory()                            [ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
      PeiServicesInstallPeiMemory()               [...]
      // now we've got permanent PEI RAM
      MemoryPeim()                                [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
        InitMmu()                                 [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
          ArmVirtGetMemoryMap()                   [ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c]
          ArmConfigureMmu()                       [ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c]

In patch v3 04/16, we modify ArmVirtGetMemoryMap() so that the mapping
not cover the "Peripheral space after DRAM" (which is practically
"limitless"). Good.

In this patch, we have to modify ArmConfigureMmu() as well, because the
descriptor array that we pass it from ArmVirtGetMemoryMap() does not
contain all the information that is necessary for setting up "paging" in
general. Also considering mappings that could be added later during DXE,
such as MMIO ranges of platform drivers / devices, discontiguous RAM
ranges, and so on.

So we need a "maximum" here. The maximum that we choose doesn't
"seriously" translate to an increased memory footprint of paging
artifacts, *unlike* the size of the address space that we actually map
for access. Hence in patch v3 04/16, we are conservative, but in this
patch, when selecting the maximum, we go as high as we can -- either the
architecturally permitted limit (with 4KB page size), or, if the latter
is lower, the limit supported by the current CPU.

The GCD *could* go higher (even though we'd never attempt to map that in
UEFI), assuming the CPU is capable enough (using 64KB pages at OS
runtime).

This bends my brain. :) I'm now slowly comprehending your blurb. Indeed,
the rest of the series pertains to the GCD memory space map.

Nit: the second argument of the MIN() macro is not indented
idiomatically. With that fixed (no need to repost the series just
because of it):

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo