On Mon, 26 Nov 2018 at 11:00, Laszlo Ersek <[email protected]> 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 <[email protected]> > > --- > > 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 <[email protected]> > > 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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
