From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::242; helo=mail-it0-x242.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x242.google.com (mail-it0-x242.google.com [IPv6:2607:f8b0:4001:c0b::242]) (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 2EAAB20956061 for ; Wed, 14 Mar 2018 22:26:46 -0700 (PDT) Received: by mail-it0-x242.google.com with SMTP id g7-v6so1698381itf.1 for ; Wed, 14 Mar 2018 22:33:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:date:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=WYHVbGrPgK/kw4yHNJW26vJsJer+4bkfYbd74SHsj4E=; b=KZh+KrFBbfS30dRoMl/FUlaPStV8xNSlLB7vruGrzEiKHHFrm44puk2z4eX4BLU7F3 bBpKIOc+YZE1iRAb2HZVnWSAEO2UV8Lkvq504dNwQVL6nrh3Z5jMQTFQ0NVi5687LeNU Y651NCZemMUE0d6H6Dost8FnRwzLenWI+f5vU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=WYHVbGrPgK/kw4yHNJW26vJsJer+4bkfYbd74SHsj4E=; b=YqSb3qI+zrS1GomQEQgDx6HfjqRII+nM8WGHa5y662/MJZvnkqzLc/0r6widhFmgnP QhKji0BjYzmhBKTzc5HrA5Bpbt+ctmkfoTTt0RfflJKpSTYLQw5ObZY6UslF54lzEr0f EgZqzBO870PpFuL3TnQ5J0efMTjGpUwy3qH4Tlk7IaPFhEJ4apuP4Ka4OLHtDSnq4FSQ xJVGki5/f1dotEDVCO2mOWc3rlGNHZpvQ0YhrLg8HQ/SzBct/FR5Q5PUCXtx4c8upGtL fsRHtTkx6hNyKJ6jyAu+I+mSkwpJkxTBOo2/IXjWnXyl0EDFTkL21egDLJdjWENjPgqh pJsg== X-Gm-Message-State: AElRT7GehlQAKmx7O+WR6QukGa4Tdd8se+1UJvhCbWDrGcDx6O+EV+JQ XFj49oQWRvqamjTUCYnZD2rdtw== X-Google-Smtp-Source: AG47ELuyJadJfyISz5aiHBEsUo1eAn9U58zSgKjg1oNBGdjaFVwakQm8dBc8rETQDgjFjT4EnS2blQ== X-Received: by 2002:a24:5ad4:: with SMTP id v203-v6mr5132422ita.150.1521091989740; Wed, 14 Mar 2018 22:33:09 -0700 (PDT) Received: from SZX1000114654 ([45.56.152.100]) by smtp.gmail.com with ESMTPSA id t64sm2663530ioe.58.2018.03.14.22.33.06 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 14 Mar 2018 22:33:09 -0700 (PDT) From: Guo Heyi X-Google-Original-From: Guo Heyi Date: Thu, 15 Mar 2018 13:33:04 +0800 To: "Ni, Ruiyu" Cc: Heyi Guo , edk2-devel@lists.01.org, Yi Li , Ard Biesheuvel , Star Zeng , Eric Dong , Laszlo Ersek , Michael D Kinney Message-ID: <20180315053304.GB108227@SZX1000114654> References: <1521086424-113954-1-git-send-email-heyi.guo@linaro.org> <1521086424-113954-5-git-send-email-heyi.guo@linaro.org> <26c26ca9-1d40-f206-ff17-7c445d5caefa@Intel.com> MIME-Version: 1.0 In-Reply-To: <26c26ca9-1d40-f206-ff17-7c445d5caefa@Intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) Subject: Re: [PATCH v6 4/6] MdeModulePkg/PciHostBridgeDxe: Add support for address translation X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Mar 2018 05:26:47 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Mar 15, 2018 at 01:26:04PM +0800, Ni, Ruiyu wrote: > On 3/15/2018 12:00 PM, Heyi Guo wrote: > >PCI address translation is necessary for some non-x86 platforms. On > >such platforms, address value (denoted as "device address" or "address > >in PCI view") set to PCI BAR registers in configuration space might be > >different from the address which is used by CPU to access the > >registers in memory BAR or IO BAR spaces (denoted as "host address" or > >"address in CPU view"). The difference between the two addresses is > >called "Address Translation Offset" or simply "translation", and can > >be represented by "Address Translation Offset" in ACPI QWORD Address > >Space Descriptor (Offset 0x1E). However UEFI and ACPI differs on the > >definitions of QWORD Address Space Descriptor, and we will follow UEFI > >definition on UEFI protocols, such as PCI root bridge IO protocol and > >PCI IO protocol. In UEFI 2.7, "Address Translation Offset" is "Offset > >to apply to the Starting address to convert it to a PCI address". This > >means: > > > >1. Translation = device address - host address. > > > >2. PciRootBridgeIo->Configuration should return CPU view address, as > >well as PciIo->GetBarAttributes. > > > >Summary of addresses used in protocol interfaces and internal > >implementations: > > > >1. *Only* the following protocol interfaces assume Address is Device > > Address: > >(1). PciHostBridgeResourceAllocation.GetProposedResources() > > Otherwise PCI bus driver cannot set correct address into PCI > > BARs. > >(2). PciRootBridgeIo.Mem.Read() and PciRootBridgeIo.Mem.Write() > >(3). PciRootBridgeIo.CopyMem() > >UEFI and PI spec have clear statements for all other protocol > >interfaces about the address type. > > > >2. Library interfaces and internal implementation: > >(1). Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address. > > It is easy to check whether the address is below 4G or above 4G. > >(2). Addresses in PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode are host > > address, for they are allocated from GCD. > >(3). Address passed to PciHostBridgeResourceConflict is host address, > > for it comes from PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode. > > > >RESTRICTION: to simplify the situation, we require the alignment of > >Translation must be larger than any BAR alignment in the same root > >bridge, so that resource allocation alignment can be applied to both > >device address and host address. > > > >Contributed-under: TianoCore Contribution Agreement 1.1 > >Signed-off-by: Heyi Guo > >Signed-off-by: Yi Li > >Cc: Ruiyu Ni > >Cc: Ard Biesheuvel > >Cc: Star Zeng > >Cc: Eric Dong > >Cc: Laszlo Ersek > >Cc: Michael D Kinney > >--- > > > >Notes: > > v6: > > - Change ASSERT(FALSE) to ASSERT ((Translation & Alignment) == 0) and > > move ASSERT before if-check [Ray] > > - Print translation of bus and then assert [Ray] > > - Add function header for RootBridgeIoGetMemTranslationByAddress() > > [Ray] > > v5: > > - Add check for the alignment of Translation against the BAR alignment > > [Ray] > > - Keep coding style of comments consistent with the context [Ray] > > - Comment change for Base in PCI_RES_NODE [Ray] > > - Add macros of TO_HOST_ADDRESS and TO_DEVICE_ADDRESS for address type > > conversion (After that we can also simply the comments about the > > calculation [Ray] > > - Add check for bus translation offset in CreateRootBridge(), making > > sure it is zero, and unify code logic for all types of resource > > after that [Ray] > > - Use GetTranslationByResourceType() to simplify the code in > > RootBridgeIoConfiguration() (also fix a bug in previous patch > > version of missing a break after case TypePMem64) [Ray] > > - Commit message refinement [Ray] > > v4: > > - Add ASSERT (FALSE) to default branch in GetTranslationByResourceType > > [Laszlo] > > - Fix bug when passing BaseAddress to gDS->AllocateIoSpace and > > gDS->AllocateMemorySpace [Laszlo] > > - Add comment for applying alignment to both device address and host > > address, and add NOTE for the alignment requirement of Translation, > > as well as in commit message [Laszlo][Ray] > > - Improve indention for the code in CreateRootBridge [Laszlo] > > - Improve comment for Translation in PCI_ROOT_BRIDGE_APERTURE > > definition [Laszlo] > > - Ignore translation of bus in CreateRootBridge > > v4: > > - Add ASSERT (FALSE) to default branch in GetTranslationByResourceType > > [Laszlo] > > - Fix bug when passing BaseAddress to gDS->AllocateIoSpace and > > gDS->AllocateMemorySpace [Laszlo] > > - Add comment for applying alignment to both device address and host > > address, and add NOTE for the alignment requirement of Translation, > > as well as in commit message [Laszlo][Ray] > > - Improve indention for the code in CreateRootBridge [Laszlo] > > - Improve comment for Translation in PCI_ROOT_BRIDGE_APERTURE > > definition [Laszlo] > > - Ignore translation of bus in CreateRootBridge > > > > MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h | 21 +++ > > MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h | 3 + > > MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 129 ++++++++++++++++--- > > MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 135 ++++++++++++++++++-- > > 4 files changed, 261 insertions(+), 27 deletions(-) > > > >diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h > >index 9a8ca21f4819..c2791ea5c2a4 100644 > >--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h > >+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h > >@@ -38,6 +38,13 @@ typedef struct { > > #define PCI_HOST_BRIDGE_FROM_THIS(a) CR (a, PCI_HOST_BRIDGE_INSTANCE, ResAlloc, PCI_HOST_BRIDGE_SIGNATURE) > > // > >+// Macros to translate device address to host address and vice versa. According > >+// to UEFI 2.7, device address = host address + translation offset. > >+// > >+#define TO_HOST_ADDRESS(DeviceAddress,TranslationOffset) ((DeviceAddress) - (TranslationOffset)) > >+#define TO_DEVICE_ADDRESS(HostAddress,TranslationOffset) ((HostAddress) + (TranslationOffset)) > >+ > >+// > > // Driver Entry Point > > // > > /** > >@@ -247,6 +254,20 @@ ResourceConflict ( > > IN PCI_HOST_BRIDGE_INSTANCE *HostBridge > > ); > >+/** > >+ This routine gets translation offset from a root bridge instance by resource type. > >+ > >+ @param RootBridge The Root Bridge Instance for the resources. > >+ @param ResourceType The Resource Type of the translation offset. > >+ > >+ @retval The Translation Offset of the specified resource. > >+**/ > >+UINT64 > >+GetTranslationByResourceType ( > >+ IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge, > >+ IN PCI_RESOURCE_TYPE ResourceType > >+ ); > >+ > > extern EFI_METRONOME_ARCH_PROTOCOL *mMetronome; > > extern EFI_CPU_IO2_PROTOCOL *mCpuIo; > > #endif > >diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h > >index 8612c0c3251b..a6c3739db368 100644 > >--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h > >+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h > >@@ -38,6 +38,9 @@ typedef enum { > > typedef struct { > > PCI_RESOURCE_TYPE Type; > >+ // > >+ // Base is a host address > >+ // > > UINT64 Base; > > UINT64 Length; > > UINT64 Alignment; > >diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > >index 1494848c3e8c..5342efd89275 100644 > >--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > >+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > >@@ -33,6 +33,39 @@ EFI_EVENT mIoMmuEvent; > > VOID *mIoMmuRegistration; > > /** > >+ This routine gets translation offset from a root bridge instance by resource type. > >+ > >+ @param RootBridge The Root Bridge Instance for the resources. > >+ @param ResourceType The Resource Type of the translation offset. > >+ > >+ @retval The Translation Offset of the specified resource. > >+**/ > >+UINT64 > >+GetTranslationByResourceType ( > >+ IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge, > >+ IN PCI_RESOURCE_TYPE ResourceType > >+ ) > >+{ > >+ switch (ResourceType) { > >+ case TypeIo: > >+ return RootBridge->Io.Translation; > >+ case TypeMem32: > >+ return RootBridge->Mem.Translation; > >+ case TypePMem32: > >+ return RootBridge->PMem.Translation; > >+ case TypeMem64: > >+ return RootBridge->MemAbove4G.Translation; > >+ case TypePMem64: > >+ return RootBridge->PMemAbove4G.Translation; > >+ case TypeBus: > >+ return RootBridge->Bus.Translation; > >+ default: > >+ ASSERT (FALSE); > >+ return 0; > >+ } > >+} > >+ > >+/** > > Ensure the compatibility of an IO space descriptor with the IO aperture. > > The IO space descriptor can come from the GCD IO space map, or it can > >@@ -366,6 +399,7 @@ InitializePciHostBridge ( > > UINTN MemApertureIndex; > > BOOLEAN ResourceAssigned; > > LIST_ENTRY *Link; > >+ UINT64 HostAddress; > > RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount); > > if ((RootBridges == NULL) || (RootBridgeCount == 0)) { > >@@ -411,8 +445,15 @@ InitializePciHostBridge ( > > } > > if (RootBridges[Index].Io.Base <= RootBridges[Index].Io.Limit) { > >+ // > >+ // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address. > >+ // For GCD resource manipulation, we need to use host address. > >+ // > >+ HostAddress = TO_HOST_ADDRESS (RootBridges[Index].Io.Base, > >+ RootBridges[Index].Io.Translation); > >+ > > Status = AddIoSpace ( > >- RootBridges[Index].Io.Base, > >+ HostAddress, > > RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1 > > ); > > ASSERT_EFI_ERROR (Status); > >@@ -422,7 +463,7 @@ InitializePciHostBridge ( > > EfiGcdIoTypeIo, > > 0, > > RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1, > >- &RootBridges[Index].Io.Base, > >+ &HostAddress, > > gImageHandle, > > NULL > > ); > >@@ -443,14 +484,20 @@ InitializePciHostBridge ( > > for (MemApertureIndex = 0; MemApertureIndex < ARRAY_SIZE (MemApertures); MemApertureIndex++) { > > if (MemApertures[MemApertureIndex]->Base <= MemApertures[MemApertureIndex]->Limit) { > >+ // > >+ // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address. > >+ // For GCD resource manipulation, we need to use host address. > >+ // > >+ HostAddress = TO_HOST_ADDRESS (MemApertures[MemApertureIndex]->Base, > >+ MemApertures[MemApertureIndex]->Translation); > > Status = AddMemoryMappedIoSpace ( > >- MemApertures[MemApertureIndex]->Base, > >+ HostAddress, > > MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1, > > EFI_MEMORY_UC > > ); > > ASSERT_EFI_ERROR (Status); > > Status = gDS->SetMemorySpaceAttributes ( > >- MemApertures[MemApertureIndex]->Base, > >+ HostAddress, > > MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1, > > EFI_MEMORY_UC > > ); > >@@ -463,7 +510,7 @@ InitializePciHostBridge ( > > EfiGcdMemoryTypeMemoryMappedIo, > > 0, > > MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1, > >- &MemApertures[MemApertureIndex]->Base, > >+ &HostAddress, > > gImageHandle, > > NULL > > ); > >@@ -654,6 +701,11 @@ AllocateResource ( > > if (BaseAddress < Limit) { > > // > > // Have to make sure Aligment is handled since we are doing direct address allocation > >+ // Strictly speaking, alignment requirement should be applied to device > >+ // address instead of host address which is used in GCD manipulation below, > >+ // but as we restrict the alignment of Translation to be larger than any BAR > >+ // alignment in the root bridge, we can simplify the situation and consider > >+ // the same alignment requirement is also applied to host address. > > // > > BaseAddress = ALIGN_VALUE (BaseAddress, LShiftU64 (1, BitsOfAlignment)); > >@@ -721,6 +773,7 @@ NotifyPhase ( > > PCI_RESOURCE_TYPE Index2; > > BOOLEAN ResNodeHandled[TypeMax]; > > UINT64 MaxAlignment; > >+ UINT64 Translation; > > HostBridge = PCI_HOST_BRIDGE_FROM_THIS (This); > >@@ -822,14 +875,43 @@ NotifyPhase ( > > BitsOfAlignment = LowBitSet64 (Alignment + 1); > > BaseAddress = MAX_UINT64; > >+ // > >+ // RESTRICTION: To simplify the situation, we require the alignment of > >+ // Translation must be larger than any BAR alignment in the same root > >+ // bridge, so that resource allocation alignment can be applied to > >+ // both device address and host address. > >+ // > >+ Translation = GetTranslationByResourceType (RootBridge, Index); > >+ ASSERT ((Translation & Alignment) == 0); > > Can you move the assertion to the body of if below, and below the DEBUG > message? So that when assertion happens, developer can know the actual value > of Translation and Alignment. > > > >+ if ((Translation & Alignment) != 0) { > >+ DEBUG ((DEBUG_ERROR, "[%a:%d] Translation %lx is not aligned to %lx!\n", > >+ __FUNCTION__, __LINE__, Translation, Alignment > >+ )); > > ASSERT ((Translation & Alignment) == 0); // Move to here. No problem. I just found we often place ASSERT before if-check in other places and supposed it is the common style :) Thanks, Heyi > > With the above change, Reviewed-by: Ruiyu Ni > > >+ // > >+ // This may be caused by too large alignment or too small > >+ // Translation; pick the 1st possibility and return out of resource, > >+ // which can also go thru the same process for out of resource > >+ // outside the loop. > >+ // > >+ ReturnStatus = EFI_OUT_OF_RESOURCES; > >+ continue; > >+ } > >+