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 0864E21A09130 for ; Mon, 26 Nov 2018 02:06:32 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 87A3AC049596; Mon, 26 Nov 2018 10:06:31 +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 42E885DD7D; Mon, 26 Nov 2018 10:06:26 +0000 (UTC) From: Laszlo Ersek To: Ard Biesheuvel , edk2-devel@lists.01.org Cc: Andrew Jones , Eric Auger References: <20181123121431.22353-1-ard.biesheuvel@linaro.org> <20181123121431.22353-3-ard.biesheuvel@linaro.org> <8dbc755f-1e89-3c59-f3e3-731d16ea8ab2@redhat.com> Message-ID: <3fb36d63-7cb1-53a1-f1c6-6dff1910e978@redhat.com> Date: Mon, 26 Nov 2018 11:06:25 +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: <8dbc755f-1e89-3c59-f3e3-731d16ea8ab2@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 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:06:31 +0000 (UTC) Subject: Re: [PATCH 2/5] ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account 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:06:32 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/26/18 10:42, Laszlo Ersek wrote: > On 11/23/18 13:14, Ard Biesheuvel wrote: >> In preparation of permitting the virt code to define a larger PA space >> size via gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize than what the >> CPU actually supports, take the CPU's capabilities into account when >> setting up the page tables. This is necessary because KVM will shortly >> support variable PA space sizes, and to support running the same UEFI >> binaries regardless of that limit, PcdPrePiCpuMemorySize needs to be >> treated as an upper bound rather than a fixed size. >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Ard Biesheuvel >> --- >> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c >> index 4b62ecb6a476..a4fde9b59383 100644 >> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c >> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c >> @@ -593,6 +593,7 @@ ArmConfigureMmu ( >> { >> VOID* TranslationTable; >> UINT32 TranslationTableAttribute; >> + UINTN MaxAddressBits; >> UINT64 MaxAddress; >> UINTN T0SZ; >> UINTN RootTableEntryCount; >> @@ -605,7 +606,9 @@ ArmConfigureMmu ( >> } >> >> // Cover the entire GCD memory space >> - MaxAddress = (1UL << PcdGet8 (PcdPrePiCpuMemorySize)) - 1; >> + MaxAddressBits = MIN (ArmGetPhysicalAddressBits (), >> + PcdGet8 (PcdPrePiCpuMemorySize)); > > (1) Superficial comment: sticking to the letter of MIN() in > "MdePkg/Include/Base.h", the arguments should be of the exact same type. > Currently they aren't. (It's a different question whether that > requirement makes any sense for MIN().) > > (2) Why does it make sense to use MIN() here? Why not just disregard > PcdPrePiCpuMemorySize and map what the CPU is capable of? (This ties in > with my lack of understanding of PcdPrePiCpuMemorySize.) > > I mean, not going above ArmGetPhysicalAddressBits() makes total sense. > What's the point of staying below it though? And if so, how much exactly > do we want to stay below it? (I.e., how is a platform supposed to set > PcdPrePiCpuMemorySize, in order to clamp down ArmGetPhysicalAddressBits()?) Ugh, okay. (Maybe you've responded already, I'm still in write-only mode, sorry.) I think I'm getting the idea here. This is just being done to keep the series bisectable / regression-free in the middle too. Right? Thanks Laszlo > >> + MaxAddress = (1UL << MaxAddressBits) - 1; > > (3) I understand this just reworks the original code, but the original > code isn't stellar. If we are left-shifting a UINT32 or UINTN value, > then the result is the same, and the << operator is OK. However: > > - Here we intend to accommodate a UINT64 result, judged from the type of > MaxAddress (UINT64). > > - For that, we should likely left-shift 1ULL, not 1 U;, which in turn > requires LShiftU64() from BaseLib. > > (If we indeed want to use UINTN, then I think we should change the type > of MaxAddress, plus write "(UINTN)1", not 1UL.) > >> >> // Lookup the Table Level to get the information >> LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount); >> > > Thanks, > Laszlo >