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 3CEA721164EF2 for ; Mon, 26 Nov 2018 02:22:30 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 65435C049583; Mon, 26 Nov 2018 10:22:29 +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 6F6C6600CC; Mon, 26 Nov 2018 10:22:21 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org Cc: Leif Lindholm , Eric Auger , Andrew Jones , Philippe Mathieu-Daude , Julien Grall References: <20181123121431.22353-1-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <060a1e19-26b5-e62b-ca13-03cf6db95517@redhat.com> Date: Mon, 26 Nov 2018 11:22:20 +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-1-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Mon, 26 Nov 2018 10:22:29 +0000 (UTC) Subject: Re: [PATCH 0/5] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit 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:22:30 -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: > The ArmVirtQemu targets currently limit the size of the IPA space to > 40 bits because that is all what KVM supports. However, this is about > to change, and so we need to update the code if we want to ensure that > our UEFI firmware builds can keep running on systems that set values > other than 40 (which could be > 40 or < 40) > > So add a helper to ArmLib to read the number of supported address bits (#1) > and take this into account in the page table code (#2), which allows > PcdPrePiCpuMemorySize to assume a value that exceeds the capabilities of > the CPU. > > Patch #3 is mostly a cleanup patch, to switch to the new helper added in > patch #1. No functional changes intended. > > Patch #4 builds the CPU hob (and thus declares the size of the GCD memory > space) based on the CPU capabilities rather than the value of > PcdPrePiCpuMemorySize, to prevent any potential regressions in memory > utilization when we bump PcdPrePiCpuMemorySize back to 48. > > Patch #5 drops the definitions of PcdPrePiCpuMemorySize, reverting its > value back to the default 48. Staring a bit more at this, I wonder if it were helpful to reorder the patches like this (just thinking loudly, I'm not even suggesting it, I'm just curious about your opinion): - patch #1: keep in place (introduce new helper) - patch #2: current patch #3 (ArmVirtPkg refactoring), to benefit from the new helper at once, without any relation to the feature that's the end goal here. - patch #3: current patch #2, build page tables with CPU PA limits accounted for - patch #4: current patch #4, build GCD memory space map with CPU PA limits accounted for - patch #5: remove the traces of the now useless PCD from ArmVirt So basically, swap around #2 and #3. It's not really important; the reason I'm thinking of it is the following though: while removing the 40-bit limitation, my first thought is, "what are the current consumers, what needs to be updated". The current structuring of the series, with patch #3 in the middle, suggests that ArmVirtMemInfoLib instances are consumers. That's not the case however, they already fetch the CPU PA limits dynamically. So they need no functional updates, just a cleanup / centralization. That's why I'd find it helpful to handle ArmVirtMemInfoLib right after the introduction of the helper. The actual consumers (in need of functional updates) are the page tables and the GCD memory space map (two concepts, not three). If you think this would be an improvement, please consider the reordering. No need to repost just for this. Thanks! Laszlo > > Cc: Laszlo Ersek > Cc: Leif Lindholm > Cc: Eric Auger > Cc: Andrew Jones > Cc: Philippe Mathieu-Daude > Cc: Julien Grall > > Ard Biesheuvel (5): > ArmPkg/ArmLib: add support for reading the max physical address space > size > ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account > ArmVirtPkg: refactor reading of the physical address space size > ArmVirtPkg: disregard PcdPrePiCpuMemorySize PCD when sizing the GCD > space > ArmVirtPkg: revert PcdPrePiCpuMemorySize to is default value of 48 > > ArmPkg/Include/Library/ArmLib.h | 6 +++ > ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S | 16 ++++++++ > ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S | 8 ++++ > .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 5 ++- > ArmVirtPkg/ArmVirtQemu.dsc | 5 --- > ArmVirtPkg/ArmVirtQemu.fdf | 1 - > ArmVirtPkg/ArmVirtQemuKernel.dsc | 4 -- > .../Include/Library/ArmVirtMemInfoLib.h | 1 + > .../ArmVirtMemoryInitPeiLib.c | 7 +++- > .../ArmVirtMemoryInitPeiLib.inf | 1 + > .../QemuVirtMemInfoLib/AArch64/PhysAddrTop.S | 39 ------------------- > .../QemuVirtMemInfoLib/Arm/PhysAddrTop.S | 24 ------------ > .../QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 6 +-- > .../QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf | 7 ---- > .../QemuVirtMemInfoPeiLib.inf | 7 ---- > .../XenVirtMemInfoLib/AArch64/PhysAddrTop.S | 39 ------------------- > .../XenVirtMemInfoLib/Arm/PhysAddrTop.S | 24 ------------ > .../XenVirtMemInfoLib/XenVirtMemInfoLib.c | 8 +--- > .../XenVirtMemInfoLib/XenVirtMemInfoLib.inf | 6 --- > .../PrePi/ArmVirtPrePiUniCoreRelocatable.inf | 3 -- > ArmVirtPkg/PrePi/PrePi.c | 3 -- > 21 files changed, 46 insertions(+), 174 deletions(-) > delete mode 100644 ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S > delete mode 100644 ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S > delete mode 100644 ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S > delete mode 100644 ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S >