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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Mon, 26 Nov 2018 at 11:00, Laszlo Ersek <lersek@redhat.com> wrote:
>
> 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.
>

OK

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

Indeed. I will reorder this with #2

Thanks