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 8556821194860 for ; Tue, 27 Nov 2018 09:53:06 -0800 (PST) Received: by mail-it1-x144.google.com with SMTP id z7so18020905iti.0 for ; Tue, 27 Nov 2018 09:53:06 -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:content-transfer-encoding; bh=++q9O+MUMqbmfwcDMYWQjvf5120SC0NO5uGC589J/2I=; b=Nw/isz9ufhpcy/uDpOrgTN5sdoajZ1dciIqNBtU68+1FBXRaUGm0SiGErzEU4uirDT coO6WzJxq4WBVIUjpnLSein7itt2CqKv83/JFPLuw/g1XnD8ioVHQKX1Tx4tkk2M9Ti1 Ofg2uQvTuEmipFCf8DjsZIE/DEsEg6cLmX10o= 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:content-transfer-encoding; bh=++q9O+MUMqbmfwcDMYWQjvf5120SC0NO5uGC589J/2I=; b=Lb4Ly93kVmOFb6bnp0iDpnwsqy7V3nRLEsORp5EoOS4qpR3AyV4zc8Qiof+hUegUj9 tRabKx2rcKiCXHY4fSOOOXk3Nwjp/Kf6F9yOg2MvpFGgfD+zkzOpmwvnDhVcXi4DfvKz q9fslZa64WaK1L1QPJDh2jvtxzg6USLUHXCG7Cyvn7oN8KV1E0TgJ9HB11PAJpym3FXV ImaXA13bR16ZHEDTYESpvUuY90hJ8UieApnuTC3uIK/mHFObjKFooabJOHHeiMVPp5NQ sq5dmBWQGIHMlV+DWDSsBxgXfHV3imFVASLpr+7BVOO6991WapYpvfaLDhMMTx4RRPAd Cnow== X-Gm-Message-State: AGRZ1gLvnNBspax3I1J7FRJaYlKLC1dtlrk1u9H0wTpIsyEUVbfVDm7T T2YnCCo108yk/vNkvPnTibfovvbGgHFUgCKirxYbQw== X-Google-Smtp-Source: AFSGD/UuXe8QEPsM8vJcI19uTEPXMB8z3DDLl+CZfJnq6JYacAX9T4Kk0yRs3I6cATEqU+0dv5wZFvMLnbQbUbABQTo= X-Received: by 2002:a24:710:: with SMTP id f16mr25479863itf.121.1543341185793; Tue, 27 Nov 2018 09:53:05 -0800 (PST) MIME-Version: 1.0 References: <20181127145418.11992-1-ard.biesheuvel@linaro.org> <20181127145418.11992-3-ard.biesheuvel@linaro.org> <761d65bc-2e63-b061-258a-d7f6915371cf@redhat.com> In-Reply-To: <761d65bc-2e63-b061-258a-d7f6915371cf@redhat.com> From: Ard Biesheuvel Date: Tue, 27 Nov 2018 18:52:53 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Andrew Jones , Auger Eric Subject: Re: [PATCH v2 2/2] ArmVirtPkg/QemuVirtMemInfoLib: remove 1:1 mapping of top of PA range 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: Tue, 27 Nov 2018 17:53:06 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 27 Nov 2018 at 18:26, Laszlo Ersek wrote: > > On 11/27/18 15:54, Ard Biesheuvel wrote: > > Currently, we map DRAM as EFI_MEMORY_WB, and the remainder of the > > entire virtual address space is mapped with EFI_MEMORY_UC attributes, > > regardless of whether any devices actually reside there. > > > > Now that we are relaxing the address space limit to more than 40 bits, > > mapping all that address space actually takes up more space in page > > tables than we have so far made available as temporary RAM. So let's > > get rid of the mapping rather than increasing the available RAM, given > > that the mapping is not particularly useful anyway. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel > > --- > > ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 17 +++++-= ----------- > > 1 file changed, 5 insertions(+), 12 deletions(-) > > > > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c= b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c > > index 815ca145b644..70863abb2e7b 100644 > > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c > > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c > > @@ -73,21 +73,14 @@ ArmVirtGetMemoryMap ( > > VirtualMemoryTable[1].Length =3D VirtualMemoryTable[0].Physica= lBase; > > VirtualMemoryTable[1].Attributes =3D ARM_MEMORY_REGION_ATTRIBUTE_D= EVICE; > > > > - // Peripheral space after DRAM > > - VirtualMemoryTable[2].PhysicalBase =3D VirtualMemoryTable[0].Length = + VirtualMemoryTable[1].Length; > > - VirtualMemoryTable[2].VirtualBase =3D VirtualMemoryTable[2].Physica= lBase; > > - VirtualMemoryTable[2].Length =3D TopOfAddressSpace - > > - VirtualMemoryTable[2].PhysicalB= ase; > > - VirtualMemoryTable[2].Attributes =3D ARM_MEMORY_REGION_ATTRIBUTE_D= EVICE; > > - > > // Remap the FD region as normal executable memory > > - VirtualMemoryTable[3].PhysicalBase =3D PcdGet64 (PcdFdBaseAddress); > > - VirtualMemoryTable[3].VirtualBase =3D VirtualMemoryTable[3].Physica= lBase; > > - VirtualMemoryTable[3].Length =3D FixedPcdGet32 (PcdFdSize); > > - VirtualMemoryTable[3].Attributes =3D ARM_MEMORY_REGION_ATTRIBUTE_W= RITE_BACK; > > + VirtualMemoryTable[2].PhysicalBase =3D PcdGet64 (PcdFdBaseAddress); > > + VirtualMemoryTable[2].VirtualBase =3D VirtualMemoryTable[2].Physica= lBase; > > + VirtualMemoryTable[2].Length =3D FixedPcdGet32 (PcdFdSize); > > + VirtualMemoryTable[2].Attributes =3D ARM_MEMORY_REGION_ATTRIBUTE_W= RITE_BACK; > > > > // End of Table > > - ZeroMem (&VirtualMemoryTable[4], sizeof (ARM_MEMORY_REGION_DESCRIPTO= R)); > > + ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTO= R)); > > > > *VirtualMemoryMap =3D VirtualMemoryTable; > > } > > > > (1) This supplants your other series "[PATCH v2 00/13] ArmPkg, ArmVirtPkg= : lift 40-bit IPA space limit" minimally due to a contextual conflict; is t= hat right? > Not quite. It complements it, in the sense that is should fix the issue reported by Eric when mapping the entire address 48-bit address space. > (2) Regarding the patch itself. Currently we have: > > - VirtualMemoryTable[0]: "System DRAM" > - VirtualMemoryTable[1]: "Peripheral space before DRAM" > - VirtualMemoryTable[2]: "Peripheral space after DRAM" > - VirtualMemoryTable[3]: "Remap the FD region as normal executable > memory" > > Let's see what is affected, from the physical map in QEMU's "hw/arm/virt.= c", if we evict VirtualMemoryTable[2]: > > /* Additional 64 MB redist region (can contain up to 512 redistributo= rs) */ > [VIRT_GIC_REDIST2] =3D { 0x4000000000ULL, 0x4000000 }, > [VIRT_PCIE_ECAM_HIGH] =3D { 0x4010000000ULL, 0x10000000 }, > /* Second PCIe window, 512GB wide at the 512GB boundary */ > [VIRT_PCIE_MMIO_HIGH] =3D { 0x8000000000ULL, 0x8000000000ULL }, > > I have no idea about VIRT_GIC_REDIST2, but, given that in ArmVirtQemu we = do uniprocessor only, it doesn't seem worrisome. > The GICv3 architecture permits redistributors (one for each CPU) to be non-contiguous in physical memory. Some multi-socket systems make use of this. In ArmVirtQemu (or actually, in EDK2 in general) we assume that the boot CPU's redistributor is in the primary redistributor region, so this region can indeed be disregarded. > VIRT_PCIE_ECAM_HIGH should be handled by patch #1. (VIRT_PCIE_ECAM_HIGH *= replaces* [VIRT_PCIE_ECAM] =3D { 0x3f000000, 0x01000000 }, if memory serves= .) > > VIRT_PCIE_MMIO_HIGH is *in addition* to [VIRT_PCIE_MMIO] =3D { 0x10000000= , 0x2eff0000 }, but we need not do anything about that specifically, becaus= e we advertize it to PciHostBridgeDxe via our FdtPciHostBridgeLib instance,= and PciHostBridgeDxe handles the GCD aspects for the range automatically. > > So, together with patch #1, I think this is safe. If we catch a data abor= t anyway, we'll have to clean up the GCD handling in other drivers. > > Reviewed-by: Laszlo Ersek > Thanks Eric, mind applying this to double check that it fixes your issue? (and doesn't break anything else?)