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 BE68721191F49 for ; Mon, 26 Nov 2018 03:43:16 -0800 (PST) Received: by mail-it1-x144.google.com with SMTP id h193so27024738ita.5 for ; Mon, 26 Nov 2018 03:43:16 -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=5945E6K10xo8FRDxLrMfjKUN68AIe4OioBhA3UTg8EI=; b=jOZUaKtSwVnhsSUSY2JmFuIAewbTP3MXXFGJ7mxF7t8RGJyAUCNIcLh/uO8XJlviW3 8zxJqDrFUQC6x9HvJk+hl7EFEuBRpL/FWIwMxyJQtp84BfDh/x9OXp42EmVLecG1tUOP 5m3FL379HIgqqU8YCRuHowDaUNAiGdp9DOers= 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=5945E6K10xo8FRDxLrMfjKUN68AIe4OioBhA3UTg8EI=; b=tGEHWp/5MI4xBD7W8VBEuzP6Bu++1Jv4xHLw0awFkBaTZxSgWWlVoRl4JKLl60gxlD XP/fiXa7YcnubCJLAiGFJyk4m8brVl/Ata4Q6AqicqHMW7htByucypQ+lI/tIY+YJlK5 4E602XGM1e8htW81P4XYrDPWz65aO9o8BWeBtfsX7i8BD2qY5YnLGLEXQcS5OoQpitZ5 fNF88yD+/1CpdiMlnXvfUSEbcgDsthwVAokg5WBfeKNoT44cn4nxbvBVaDPPwtaL5Znu zjix75OgqQ9LSZkZrZw8NwbGS699JKM3qHsJxPqnGjM8F4O+CPvQ1U5IJlek08tI+V7f BfEQ== X-Gm-Message-State: AGRZ1gIjwCuWRs3H9qmGqVSmwbsWhFizfXRpTWYpxvhK91GCO2LNaqSj nIdRaWZJ9h5be0JpUu5ma9OGPzDRBv97ggj591uO9w== X-Google-Smtp-Source: AFSGD/XOD39DtheMAu51BfdZd+4/2kczyok1jSVr/+9KrEl3aPGCU9No5qyftZPUdZBWW0FIgChWk4Den7DfbZQj3Ao= X-Received: by 2002:a24:edc4:: with SMTP id r187mr24816441ith.158.1543232595649; Mon, 26 Nov 2018 03:43:15 -0800 (PST) MIME-Version: 1.0 References: <20181123121431.22353-1-ard.biesheuvel@linaro.org> <20181123121431.22353-3-ard.biesheuvel@linaro.org> <8dbc755f-1e89-3c59-f3e3-731d16ea8ab2@redhat.com> In-Reply-To: <8dbc755f-1e89-3c59-f3e3-731d16ea8ab2@redhat.com> From: Ard Biesheuvel Date: Mon, 26 Nov 2018 12:43:04 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Andrew Jones , Auger Eric 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 11:43:17 -0000 Content-Type: text/plain; charset="UTF-8" On Mon, 26 Nov 2018 at 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().) > OK > (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.) > PcdPrePiCpuMemorySize defines the size of the GCD memory space. PcdPrePiCpuIoSize defines the size of the GCD I/O space. However, we infer other quantities from it as well: - Up until now, it never made sense to make the GCD space larger than what the CPU is able to address, so we used it as an upper bound for the page table code; - On a given platform, the physical address space may not be entirely populated, and fewer PA bits means fewer levels of page tables, and so less memory used *. This means it might make sense to reduce PcdPrePiCpuMemorySize to a lower value than what the h/w supports. * this was briefly a concern when some (all?) levels of page tables were allocated from temporary RAM but that broke some platforms IIRC so that was reverted. > 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()?) > As explained above, staying below it gives you less overhead in the GCD memory map and the page tables. > > + 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. > This code currently only executes on AArch64, but for correctness (and in case we ever use this code for SMMUs on 32-bit ARM), it should indeed be fixed. > (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