From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.43; helo=mga05.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (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 260D32202E4A2 for ; Wed, 14 Mar 2018 23:44:33 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Mar 2018 23:50:57 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,308,1517904000"; d="scan'208";a="24684599" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.4]) ([10.239.9.4]) by fmsmga008.fm.intel.com with ESMTP; 14 Mar 2018 23:50:55 -0700 To: Guo Heyi Cc: edk2-devel@lists.01.org, Yi Li , Ard Biesheuvel , Star Zeng , Eric Dong , Laszlo Ersek , Michael D Kinney 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> <20180315053304.GB108227@SZX1000114654> From: "Ni, Ruiyu" Message-ID: <702e7e0d-33dc-fe48-129d-eed6805ccec6@Intel.com> Date: Thu, 15 Mar 2018 14:50:55 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180315053304.GB108227@SZX1000114654> 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 06:44:34 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 3/15/2018 1:33 PM, Guo Heyi wrote: > 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 :)There is no such requirement I think. Easy debugging is more important:) It's not easy to directly attach a debugger and check the variable value in firmware world. > > 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; >>> + } >>> + -- Thanks, Ray