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 C3F582117FD54 for ; Tue, 27 Nov 2018 09:47:52 -0800 (PST) Received: by mail-it1-x144.google.com with SMTP id h193so35320555ita.5 for ; Tue, 27 Nov 2018 09:47:52 -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=PYTjsu+AM+Evnw/cGH0aO8oI7bAA/RXNWlukS94s6QI=; b=dBxJ8uHy/kbIg2lwQvt0i7mWBHsuFN9oc8V7//Kvfzktstl4j7GjopG9OhZ3TR037q ZkSC5oG/4RjNiENCu4ORpBfx9cbrflOXHtpvrDam0j+hIk+wkQZEP6r35+v3QsH7fEHK FY0pwKZlbfO6pG0nVx7n5CyQ959JgOVWB8eLg= 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=PYTjsu+AM+Evnw/cGH0aO8oI7bAA/RXNWlukS94s6QI=; b=iWcXPDKJN9IFsXhnmPysZi+rbQO2peS0NSwFhe4aQEhNPgeuc1ylw7o3P/t0CSA65q xpzlACQ0Z/p+0WKttzkgDe7VsIHobw9E23jsd0NR00PWPBZXwfaY5bwVlGvfJVXbOzpv bzeJvHhjNL4aboZOirkJqK4v9SQ1t0lRI21XSNOfNygEAbSa4vXWtM+IL2X3UeymtRzu Pgo2S4Ttlh6VxXhlgjY3aJ1fG1b8Q6drJhLwuzwySzWQ3X1lzJNWW054xm+zowgd6xQ1 gUjZT65h7EKx8eBuojspZn0irnAK0vf3BtdT4QptD21huIfcrbLR5FsnjUvOABJHAFKm rSXA== X-Gm-Message-State: AGRZ1gInZKTIjZTLw5FukirJwM6qGMgLM2eRT/fg2J2oCdWMBKz4D3Wu vbLCYqghaBcWwdinztmc1FmSmFe07kENDlQPIsWEOA== X-Google-Smtp-Source: AFSGD/VkckmF/5SGTHpILNNkyzGQPhhHok5u+nkWokXUz1rV9Y0WJXPG129IFtV7i1KJi5IzsrqgHFXFx1Hs9bp04nU= X-Received: by 2002:a24:edc4:: with SMTP id r187mr30936123ith.158.1543340871668; Tue, 27 Nov 2018 09:47:51 -0800 (PST) MIME-Version: 1.0 References: <20181127145418.11992-1-ard.biesheuvel@linaro.org> <20181127145418.11992-2-ard.biesheuvel@linaro.org> <8ec067c0-bde9-e3f0-14dd-8565515f9cd7@redhat.com> In-Reply-To: <8ec067c0-bde9-e3f0-14dd-8565515f9cd7@redhat.com> From: Ard Biesheuvel Date: Tue, 27 Nov 2018 18:47:37 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Auger Eric , Andrew Jones , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= Subject: Re: [PATCH v2 1/2] ArmVirtPkg/FdtPciHostBridgeLib: map ECAM and I/O spaces in GCD memory map 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:47:53 -0000 Content-Type: text/plain; charset="UTF-8" On Tue, 27 Nov 2018 at 18:40, Laszlo Ersek wrote: > > On 11/27/18 15:54, Ard Biesheuvel wrote: > > Up until now, we have been getting away with not declaring the ECAM > > and translated I/O spaces at all in the GCD memory map, simply because > > we map the entire address space with device attributes in the early PEI > > code, and so these regions will be mapped wherever they end up. > > > > Now that we are about to make changes to how ArmVirtQemu reasons > > about the size of the address space, it would be better to get rid > > of this mapping of the entire address space, since it can get > > arbitrarily large without real benefit. > > > > So start by mapping the ECAM and translated I/O spaces explicitly, > > instead of relying on the early PEI mapping. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel > > --- > > ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 1 + > > ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 46 +++++++++++++++++++- > > 2 files changed, 46 insertions(+), 1 deletion(-) > > > > diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf > > index 0995f4b7a156..4011336a353b 100644 > > --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf > > +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf > > @@ -42,6 +42,7 @@ [Packages] > > [LibraryClasses] > > DebugLib > > DevicePathLib > > + DxeServicesTableLib > > MemoryAllocationLib > > PciPcdProducerLib > > > > diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c > > index 5b9c887db35d..ba177353122e 100644 > > --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c > > +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c > > @@ -17,6 +17,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -82,6 +83,33 @@ typedef struct { > > #define DTB_PCI_HOST_RANGE_IO BIT24 > > #define DTB_PCI_HOST_RANGE_TYPEMASK (BIT31 | BIT30 | BIT29 | BIT25 | BIT24) > > > > +STATIC > > +EFI_STATUS > > +MapGcdMmioSpace ( > > + IN UINT64 Base, > > + IN UINT64 Size > > + ) > > +{ > > + EFI_STATUS Status; > > + > > + Status = gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo, Base, Size, > > + EFI_MEMORY_UC); > > + if (EFI_ERROR (Status)) { > > + DEBUG ((DEBUG_WARN, > > + "%a: failed to add GCD memory space for region [0x%Lx+0x%Lx)\n", > > + __FUNCTION__, Base, Size)); > > + return Status; > > + } > > + > > + Status = gDS->SetMemorySpaceAttributes (Base, Size, EFI_MEMORY_UC); > > + if (EFI_ERROR (Status)) { > > + DEBUG ((DEBUG_WARN, > > + "%a: failed to set memory space attributes for region [0x%Lx+0x%Lx)\n", > > + __FUNCTION__, Base, Size)); > > + } > > + return Status; > > +} > > (1) Given that these failures will quite directly trigger assertions > (which print messages even in such builds that filter out everything > except DEBUG_ERROR), I wonder if we should use DEBUG_ERROR here. > > Anyway, just an idea, up to you. > Yeah that makes sense > > + > > STATIC > > EFI_STATUS > > ProcessPciHost ( > > @@ -266,7 +294,23 @@ ProcessPciHost ( > > "Io[0x%Lx+0x%Lx)@0x%Lx Mem32[0x%Lx+0x%Lx)@0x0 Mem64[0x%Lx+0x%Lx)@0x0\n", > > __FUNCTION__, ConfigBase, ConfigSize, *BusMin, *BusMax, *IoBase, *IoSize, > > IoTranslation, *Mmio32Base, *Mmio32Size, *Mmio64Base, *Mmio64Size)); > > - return EFI_SUCCESS; > > + > > + // Map the ECAM space in the GCD memory map > > + Status = MapGcdMmioSpace (ConfigBase, ConfigSize); > > + ASSERT_EFI_ERROR (Status); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + > > + // > > + // Map the MMIO window that provides I/O access - the PCI host bridge code > > + // is not aware of this translation and so it will only map the I/O view > > + // in the GCD I/O map. > > + // > > + Status = MapGcdMmioSpace (IoTranslation, *IoSize); > > (2) I think using IoTranslation as base address is incorrect here. I > reviewed the explanation in "ArmPkg/ArmPkg.dec", and also the rest of > the code in this function (which matches the DEC's specification). Thus, > I think you need, for the CPU view, (*IoBase + IoTranslation), as first > argument. > > (I do agree that the DEBUG message right at the end of the function > could be misleading; it prints > > Io[0x%Lx+0x%Lx)@0x%Lx > > from > > *IoBase, *IoSize, IoTranslation > ) > Indeed. Well spotted. > > + ASSERT_EFI_ERROR (Status); > > + > > + return Status; > > } > > > > STATIC PCI_ROOT_BRIDGE mRootBridge; > > > > With (2) fixed: > > Reviewed-by: Laszlo Ersek > Thanks.