From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 819D121106F21 for ; Mon, 26 Nov 2018 02:00:24 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0172A305FCB2; Mon, 26 Nov 2018 10:00:24 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-66.rdu2.redhat.com [10.10.121.66]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6BFC1261A7; Mon, 26 Nov 2018 10:00:19 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org Cc: Andrew Jones , Eric Auger References: <20181123121431.22353-1-ard.biesheuvel@linaro.org> <20181123121431.22353-4-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <011f1c04-24d3-59b4-8766-4f4170bacf83@redhat.com> Date: Mon, 26 Nov 2018 11:00:18 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181123121431.22353-4-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Mon, 26 Nov 2018 10:00:24 +0000 (UTC) Subject: Re: [PATCH 3/5] ArmVirtPkg: refactor reading of the physical address space size X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 26 Nov 2018 10:00:24 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > --- > 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 > > +#include 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 > #include > #include > @@ -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 > - > -//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 > - > -//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 > - > -//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 > - > -//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 This is a good clean-up (with the help of patch #1) even without the specific use case. Thanks! Laszlo