Re: [PATCH 3/5] ArmVirtPkg: refactor reading of the physical address space size

[Laszlo Ersek <lersek@redhat.com>]

On 11/23/18 13:14, Ard Biesheuvel wrote:
> In preparation of dropping the hardcoded 40-bit limit on the physical
> address space when running under virtualization, let's refactor the
> code a bit so we read the hardware limit in a single place.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h                       |  1 +
>  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c |  4 +-
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S          | 39 --------------------
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S              | 24 ------------
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c           |  3 +-
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf         |  6 ---
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf      |  6 ---
>  ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S           | 39 --------------------
>  ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S               | 24 ------------
>  ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c             |  8 +---
>  ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf           |  6 ---
>  11 files changed, 8 insertions(+), 152 deletions(-)

OK so we got:

- one declaration for ArmVirtGetMemoryMap(), namely in
"ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h";

- two implementations (in
"ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c" and
"ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c");

- and one call site (in
"ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c").

> 
> diff --git a/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
> index bdf1c513bc6d..15562b35c730 100644
> --- a/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
> +++ b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
> @@ -35,6 +35,7 @@
>  VOID
>  EFIAPI
>  ArmVirtGetMemoryMap (
> +  IN  EFI_PHYSICAL_ADDRESS            TopOfAddressSpace,

Good parameter name; in OvmfPkg/PlatformPei, I call the same thing
"FirstNonAddress". :)

This handles the declaration of the API.

>    OUT ARM_MEMORY_REGION_DESCRIPTOR    **VirtualMemoryMap
>    );
>  
> diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> index 05afd1282422..3f0e9b3a0579 100644
> --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> @@ -15,6 +15,7 @@
>  
>  #include <PiPei.h>
>  
> +#include <Library/ArmLib.h>

Right, the INF file already spells out the ArmLib class, but the header
was never included. (Has the lib class been superfluous all this time?
Who knows. We do need it now.)

>  #include <Library/ArmMmuLib.h>
>  #include <Library/ArmVirtMemInfoLib.h>
>  #include <Library/DebugLib.h>
> @@ -39,7 +40,8 @@ InitMmu (
>    RETURN_STATUS                 Status;
>  
>    // Get Virtual Memory Map from the Platform Library
> -  ArmVirtGetMemoryMap (&MemoryTable);
> +  ArmVirtGetMemoryMap (LShiftU64 (1ULL, ArmGetPhysicalAddressBits ()),
> +    &MemoryTable);
>  
>    //Note: Because we called PeiServicesInstallPeiMemory() before to call InitMmu() the MMU Page Table resides in
>    //      DRAM (even at the top of DRAM as it is the first permanent memory allocation)

Right. I think I understand the use of ArmGetPhysicalAddressBits() here,
and I agree about the LShiftU64() too.

So this covers the call site. OK.

> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S b/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
> deleted file mode 100644
> index a1f6a194d59b..000000000000
> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
> +++ /dev/null
> @@ -1,39 +0,0 @@
> -#
> -#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> -#  Copyright (c) 2016-2017, Linaro Limited. All rights reserved.
> -#
> -#  This program and the accompanying materials
> -#  are licensed and made available under the terms and conditions of the BSD License
> -#  which accompanies this distribution.  The full text of the license may be found at
> -#  http://opensource.org/licenses/bsd-license.php
> -#
> -#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> -#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> -#
> -#
> -
> -#include <AsmMacroIoLibV8.h>
> -
> -//EFI_PHYSICAL_ADDRESS
> -//GetPhysAddrTop (
> -//  VOID
> -//  );
> -ASM_FUNC(ArmGetPhysAddrTop)
> -  mrs   x0, id_aa64mmfr0_el1
> -  adr   x1, .LPARanges
> -  and   x0, x0, #7
> -  ldrb  w1, [x1, x0]
> -  mov   x0, #1
> -  lsl   x0, x0, x1
> -  ret
> -
> -//
> -// Bits 0..2 of the AA64MFR0_EL1 system register encode the size of the
> -// physical address space support on this CPU:
> -// 0 == 32 bits, 1 == 36 bits, etc etc
> -// 6 and 7 are reserved
> -//
> -.LPARanges:
> -  .byte 32, 36, 40, 42, 44, 48, -1, -1
> -
> -ASM_FUNCTION_REMOVE_IF_UNREFERENCED
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S b/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
> deleted file mode 100644
> index 9cd81529fb3d..000000000000
> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -#
> -#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> -#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> -#
> -#  This program and the accompanying materials
> -#  are licensed and made available under the terms and conditions of the BSD License
> -#  which accompanies this distribution.  The full text of the license may be found at
> -#  http://opensource.org/licenses/bsd-license.php
> -#
> -#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> -#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> -#
> -#
> -
> -#include <AsmMacroIoLib.h>
> -
> -//EFI_PHYSICAL_ADDRESS
> -//GetPhysAddrTop (
> -//  VOID
> -//  );
> -ASM_FUNC(ArmGetPhysAddrTop)
> -  mov   r0, #0x00000000
> -  mov   r1, #0x10000
> -  bx    lr
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> index 760bcc169cf4..4eca9b895354 100644
> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> @@ -41,6 +41,7 @@ ArmGetPhysAddrTop (
>  **/
>  VOID
>  ArmVirtGetMemoryMap (
> +  IN  EFI_PHYSICAL_ADDRESS           TopOfAddressSpace,
>    OUT ARM_MEMORY_REGION_DESCRIPTOR   **VirtualMemoryMap
>    )
>  {
> @@ -80,7 +81,7 @@ ArmVirtGetMemoryMap (
>  
>    // Peripheral space after DRAM
>    TopOfMemory = MIN (1ULL << FixedPcdGet8 (PcdPrePiCpuMemorySize),
> -                     ArmGetPhysAddrTop ());
> +                     TopOfAddressSpace);
>    VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
>    VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
>    VirtualMemoryTable[2].Length       = TopOfMemory -

This updates one of the implementations, replacing the internal (asm)
helper function with the external param. OK.

(1) Can you remove the declaration too, of the helper function
ArmGetPhysAddrTop(), in "QemuVirtMemInfoLib.c"? The 32-bit & 64-bit
definitions are removed with the assembly files, but the declaration is
currently left over.

> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> index c72a97f9e78a..f2c461e3b55a 100644
> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> @@ -24,12 +24,6 @@
>  [Sources]
>    QemuVirtMemInfoLib.c
>  
> -[Sources.ARM]
> -  Arm/PhysAddrTop.S
> -
> -[Sources.AARCH64]
> -  AArch64/PhysAddrTop.S
> -
>  [Packages]
>    ArmPkg/ArmPkg.dec
>    ArmVirtPkg/ArmVirtPkg.dec
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> index e4032d3efb53..f54fb51ee1d4 100644
> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> @@ -26,12 +26,6 @@
>    QemuVirtMemInfoLib.c
>    QemuVirtMemInfoPeiLibConstructor.c
>  
> -[Sources.ARM]
> -  Arm/PhysAddrTop.S
> -
> -[Sources.AARCH64]
> -  AArch64/PhysAddrTop.S
> -
>  [Packages]
>    ArmPkg/ArmPkg.dec
>    ArmVirtPkg/ArmVirtPkg.dec

These look good.

> diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S b/ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S
> deleted file mode 100644
> index a1f6a194d59b..000000000000
> --- a/ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S
> +++ /dev/null
> @@ -1,39 +0,0 @@
> -#
> -#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> -#  Copyright (c) 2016-2017, Linaro Limited. All rights reserved.
> -#
> -#  This program and the accompanying materials
> -#  are licensed and made available under the terms and conditions of the BSD License
> -#  which accompanies this distribution.  The full text of the license may be found at
> -#  http://opensource.org/licenses/bsd-license.php
> -#
> -#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> -#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> -#
> -#
> -
> -#include <AsmMacroIoLibV8.h>
> -
> -//EFI_PHYSICAL_ADDRESS
> -//GetPhysAddrTop (
> -//  VOID
> -//  );
> -ASM_FUNC(ArmGetPhysAddrTop)
> -  mrs   x0, id_aa64mmfr0_el1
> -  adr   x1, .LPARanges
> -  and   x0, x0, #7
> -  ldrb  w1, [x1, x0]
> -  mov   x0, #1
> -  lsl   x0, x0, x1
> -  ret
> -
> -//
> -// Bits 0..2 of the AA64MFR0_EL1 system register encode the size of the
> -// physical address space support on this CPU:
> -// 0 == 32 bits, 1 == 36 bits, etc etc
> -// 6 and 7 are reserved
> -//
> -.LPARanges:
> -  .byte 32, 36, 40, 42, 44, 48, -1, -1
> -
> -ASM_FUNCTION_REMOVE_IF_UNREFERENCED
> diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S b/ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S
> deleted file mode 100644
> index 9cd81529fb3d..000000000000
> --- a/ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -#
> -#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> -#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> -#
> -#  This program and the accompanying materials
> -#  are licensed and made available under the terms and conditions of the BSD License
> -#  which accompanies this distribution.  The full text of the license may be found at
> -#  http://opensource.org/licenses/bsd-license.php
> -#
> -#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> -#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> -#
> -#
> -
> -#include <AsmMacroIoLib.h>
> -
> -//EFI_PHYSICAL_ADDRESS
> -//GetPhysAddrTop (
> -//  VOID
> -//  );
> -ASM_FUNC(ArmGetPhysAddrTop)
> -  mov   r0, #0x00000000
> -  mov   r1, #0x10000
> -  bx    lr
> diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c
> index 88ff3167cbfd..3d4e3e38c3f1 100644
> --- a/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c
> +++ b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c
> @@ -18,11 +18,6 @@
>  
>  STATIC ARM_MEMORY_REGION_DESCRIPTOR  mVirtualMemoryTable[2];
>  
> -EFI_PHYSICAL_ADDRESS
> -ArmGetPhysAddrTop (
> -  VOID
> -  );
> -
>  /**
>    Return the Virtual Memory Map of your platform
>  

Aha, here you didn't miss the helper's declaration. :)

> @@ -39,6 +34,7 @@ ArmGetPhysAddrTop (
>  VOID
>  EFIAPI
>  ArmVirtGetMemoryMap (
> +  IN  EFI_PHYSICAL_ADDRESS           TopOfAddressSpace,
>    OUT ARM_MEMORY_REGION_DESCRIPTOR   **VirtualMemoryMap
>    )
>  {
> @@ -51,7 +47,7 @@ ArmVirtGetMemoryMap (
>    //
>    mVirtualMemoryTable[0].PhysicalBase = 0x0;
>    mVirtualMemoryTable[0].VirtualBase  = 0x0;
> -  mVirtualMemoryTable[0].Length       = ArmGetPhysAddrTop ();
> +  mVirtualMemoryTable[0].Length       = TopOfAddressSpace;
>    mVirtualMemoryTable[0].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
>  
>    mVirtualMemoryTable[1].PhysicalBase = 0x0;

And this covers implementation #2.

> diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf
> index cd4c805a4db9..ae107810e927 100644
> --- a/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf
> +++ b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf
> @@ -24,12 +24,6 @@
>  [Sources]
>    XenVirtMemInfoLib.c
>  
> -[Sources.ARM]
> -  Arm/PhysAddrTop.S
> -
> -[Sources.AARCH64]
> -  AArch64/PhysAddrTop.S
> -
>  [Packages]
>    ArmPkg/ArmPkg.dec
>    ArmVirtPkg/ArmVirtPkg.dec
> 

With (1) fixed:

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

This is a good clean-up (with the help of patch #1) even without the
specific use case.

Thanks!
Laszlo