From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::144; helo=mail-it1-x144.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x144.google.com (mail-it1-x144.google.com [IPv6:2607:f8b0:4864:20::144]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 5C03021191F49 for ; Mon, 26 Nov 2018 03:44:38 -0800 (PST) Received: by mail-it1-x144.google.com with SMTP id h193so27031667ita.5 for ; Mon, 26 Nov 2018 03:44:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=BdldaOIm4gJqyH/3tIJr+Gx6n43Lk5nx0sYn/hto7hE=; b=Oxyhzyf8HjerZBhsipfIsJOtRMMxfpAWg3T0YjeAJ3IQKm3di2kFzfGQKiQtT3fMd4 DlqEg9SXHgV1u4htRv48Ux/KhVOKcGn3WaGWwjtxFQ9XjT6+E62FrJCsweCRN3Hhcjst hO2v3vWyr5EwEhGYCeooTBkucDZ0kUrmXyDzo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=BdldaOIm4gJqyH/3tIJr+Gx6n43Lk5nx0sYn/hto7hE=; b=rKCpoAflExy9RUJ6NMZ2WjmKeBEK6SHF5lteluL37A+wO5iNDxbeFnd8Xhq9Lhdz34 49+jboZde/9fCV8EPYjJ71ENPb8j3URlbKangEa1iFXUdc7ueLwnRhWU359B4SbA04Yg S7JKzZs2Zx/6+b2uP0uQMwbIvYAeh3kjEsUxwoQr1snJALF2ZloeEnWBjnPrdstEESnE bT1QzFn5STZ1owv7jBsbz0ha/4H0pt5iKFRjSGZYF5/va3IT+/R3nDJMAINk3P1w5Imr D0Kke2ao3RQBI0cX4tl8yd7Hx0WcYgFxj6de57heE4wUt+kNPV2cxlZbgQxYIU52MjPs NVug== X-Gm-Message-State: AGRZ1gJzLNrDp+F2TYV1KzTUD0hYMcPG0S+THWVutVFvCC0y6G05lfSy J5XwMNqkdP/zhDtDq4j6mGX9gs0ZVSp5zIawwltjaQ== X-Google-Smtp-Source: AFSGD/VC6sO5qKm4GtbhnW4sZfWlPTzqbnT4ramhVL7aSWtHNXd3iblZUJcdE9jCXMG23GWVjSb6KtWe0ZC5KMlm80Y= X-Received: by 2002:a24:edc4:: with SMTP id r187mr24820890ith.158.1543232677492; Mon, 26 Nov 2018 03:44:37 -0800 (PST) MIME-Version: 1.0 References: <20181123121431.22353-1-ard.biesheuvel@linaro.org> <20181123121431.22353-4-ard.biesheuvel@linaro.org> <011f1c04-24d3-59b4-8766-4f4170bacf83@redhat.com> In-Reply-To: <011f1c04-24d3-59b4-8766-4f4170bacf83@redhat.com> From: Ard Biesheuvel Date: Mon, 26 Nov 2018 12:44:26 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Andrew Jones , Auger Eric 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 11:44:38 -0000 Content-Type: text/plain; charset="UTF-8" On Mon, 26 Nov 2018 at 11:00, Laszlo Ersek 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 > > --- > > 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. > 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 > > - > > -//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. > Indeed. I will reorder this with #2 Thanks