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 0BB8C21193581 for ; Mon, 26 Nov 2018 01:42:14 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 945CB30BF460; Mon, 26 Nov 2018 09:42:13 +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 885CC608FA; Mon, 26 Nov 2018 09:42:09 +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-3-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <8dbc755f-1e89-3c59-f3e3-731d16ea8ab2@redhat.com> Date: Mon, 26 Nov 2018 10:42:09 +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-3-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Mon, 26 Nov 2018 09:42:13 +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 09:42:14 -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 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()?) > + 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