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 A4DAE20954BAE for ; Wed, 14 Mar 2018 00:50:54 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Mar 2018 00:57:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,469,1515484800"; d="scan'208";a="208071185" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.4]) ([10.239.9.4]) by orsmga005.jf.intel.com with ESMTP; 14 Mar 2018 00:57:14 -0700 To: Guo Heyi Cc: Eric Dong , Ard Biesheuvel , edk2-devel@lists.01.org, Michael D Kinney , Laszlo Ersek , Star Zeng References: <1519887444-75510-1-git-send-email-heyi.guo@linaro.org> <1519887444-75510-5-git-send-email-heyi.guo@linaro.org> <20180306024440.GA90780@SZX1000114654> <71a21699-a1b3-155a-e743-ee5e6288ce20@Intel.com> <20180307060143.GB91936@SZX1000114654> From: "Ni, Ruiyu" Message-ID: <9f3e6241-e7fd-1178-ddc2-5b1cdd1b1153@Intel.com> Date: Wed, 14 Mar 2018 15:57:14 +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: <20180307060143.GB91936@SZX1000114654> Subject: Re: [PATCH v5 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: Wed, 14 Mar 2018 07:50:55 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 3/7/2018 2:01 PM, Guo Heyi wrote: > Thanks. Please let me know if any further changes are needed. > > Regards, > > Heyi > > On Wed, Mar 07, 2018 at 12:30:59PM +0800, Ni, Ruiyu wrote: >> On 3/6/2018 10:44 AM, Guo Heyi wrote: >>> Hi Ray, >>> >>> Any comments for v5? >> >> Heyi, >> Some backward compatibility concerns were received from internal production >> teams. Current change will cause silent failure on old platforms because >> TranslationOffset might be random if uninitialized. >> I will solve the concern and then send out updates to you, hopefully by end >> of next week. >> >>> >>> Regards, >>> >>> Heyi >>> >>> On Thu, Mar 01, 2018 at 02:57:22PM +0800, 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 >>>> Cc: Ruiyu Ni >>>> Cc: Ard Biesheuvel >>>> Cc: Star Zeng >>>> Cc: Eric Dong >>>> Cc: Laszlo Ersek >>>> Cc: Michael D Kinney >>>> --- >>>> >>>> Notes: >>>> 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 | 118 ++++++++++++++++-- >>>> 4 files changed, 245 insertions(+), 26 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..42ded2855c71 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); >>>> + if ((Translation & Alignment) != 0) { >>>> + DEBUG ((DEBUG_ERROR, "[%a:%d] Translation %lx is not aligned to %lx!\n", >>>> + __FUNCTION__, __LINE__, Translation, Alignment >>>> + )); >>>> + ASSERT (FALSE); 1. Cool! But can you replace ASSERT(FALSE) with ASSERT ((Translation & Alignment) == 0)? >>>> + // >>>> + // 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; >>>> + } >>>> + >>>> switch (Index) { >>>> case TypeIo: >>>> + // >>>> + // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address. >>>> + // For AllocateResource is manipulating GCD resource, we need to use >>>> + // host address here. >>>> + // >>>> BaseAddress = AllocateResource ( >>>> FALSE, >>>> RootBridge->ResAllocNode[Index].Length, >>>> MIN (15, BitsOfAlignment), >>>> - ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1), >>>> - RootBridge->Io.Limit >>>> + TO_HOST_ADDRESS (ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1), >>>> + RootBridge->Io.Translation), >>>> + TO_HOST_ADDRESS (RootBridge->Io.Limit, >>>> + RootBridge->Io.Translation) >>>> ); >>>> break; >>>> @@ -838,8 +920,10 @@ NotifyPhase ( >>>> TRUE, >>>> RootBridge->ResAllocNode[Index].Length, >>>> MIN (63, BitsOfAlignment), >>>> - ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1), >>>> - RootBridge->MemAbove4G.Limit >>>> + TO_HOST_ADDRESS (ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1), >>>> + RootBridge->MemAbove4G.Translation), >>>> + TO_HOST_ADDRESS (RootBridge->MemAbove4G.Limit, >>>> + RootBridge->MemAbove4G.Translation) >>>> ); >>>> if (BaseAddress != MAX_UINT64) { >>>> break; >>>> @@ -853,8 +937,10 @@ NotifyPhase ( >>>> TRUE, >>>> RootBridge->ResAllocNode[Index].Length, >>>> MIN (31, BitsOfAlignment), >>>> - ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1), >>>> - RootBridge->Mem.Limit >>>> + TO_HOST_ADDRESS (ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1), >>>> + RootBridge->Mem.Translation), >>>> + TO_HOST_ADDRESS (RootBridge->Mem.Limit, >>>> + RootBridge->Mem.Translation) >>>> ); >>>> break; >>>> @@ -863,8 +949,10 @@ NotifyPhase ( >>>> TRUE, >>>> RootBridge->ResAllocNode[Index].Length, >>>> MIN (63, BitsOfAlignment), >>>> - ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1), >>>> - RootBridge->PMemAbove4G.Limit >>>> + TO_HOST_ADDRESS (ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1), >>>> + RootBridge->PMemAbove4G.Translation), >>>> + TO_HOST_ADDRESS (RootBridge->PMemAbove4G.Limit, >>>> + RootBridge->PMemAbove4G.Translation) >>>> ); >>>> if (BaseAddress != MAX_UINT64) { >>>> break; >>>> @@ -877,8 +965,10 @@ NotifyPhase ( >>>> TRUE, >>>> RootBridge->ResAllocNode[Index].Length, >>>> MIN (31, BitsOfAlignment), >>>> - ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1), >>>> - RootBridge->PMem.Limit >>>> + TO_HOST_ADDRESS (ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1), >>>> + RootBridge->PMem.Translation), >>>> + TO_HOST_ADDRESS (RootBridge->PMem.Limit, >>>> + RootBridge->PMem.Translation) >>>> ); >>>> break; >>>> @@ -1421,7 +1511,14 @@ GetProposedResources ( >>>> Descriptor->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR; >>>> Descriptor->Len = sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3;; >>>> Descriptor->GenFlag = 0; >>>> - Descriptor->AddrRangeMin = RootBridge->ResAllocNode[Index].Base; >>>> + // >>>> + // AddrRangeMin in Resource Descriptor here should be device address >>>> + // instead of host address, or else PCI bus driver cannot set correct >>>> + // address into PCI BAR registers. >>>> + // Base in ResAllocNode is a host address, so conversion is needed. >>>> + // >>>> + Descriptor->AddrRangeMin = TO_DEVICE_ADDRESS (RootBridge->ResAllocNode[Index].Base, >>>> + GetTranslationByResourceType (RootBridge, Index)); >>>> Descriptor->AddrRangeMax = 0; >>>> Descriptor->AddrTranslationOffset = (ResStatus == ResAllocated) ? EFI_RESOURCE_SATISFIED : PCI_RESOURCE_LESS; >>>> Descriptor->AddrLen = RootBridge->ResAllocNode[Index].Length; >>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c >>>> index dc06c16dc038..5dd9d257d46d 100644 >>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c >>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c >>>> @@ -86,12 +86,35 @@ CreateRootBridge ( >>>> (Bridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) != 0 ? L"CombineMemPMem " : L"", >>>> (Bridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_MEM64_DECODE) != 0 ? L"Mem64Decode" : L"" >>>> )); >>>> + // >>>> + // Translation for bus is not supported. >>>> + // >>>> DEBUG ((EFI_D_INFO, " Bus: %lx - %lx\n", Bridge->Bus.Base, Bridge->Bus.Limit)); >>>> - DEBUG ((EFI_D_INFO, " Io: %lx - %lx\n", Bridge->Io.Base, Bridge->Io.Limit)); >>>> - DEBUG ((EFI_D_INFO, " Mem: %lx - %lx\n", Bridge->Mem.Base, Bridge->Mem.Limit)); >>>> - DEBUG ((EFI_D_INFO, " MemAbove4G: %lx - %lx\n", Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit)); >>>> - DEBUG ((EFI_D_INFO, " PMem: %lx - %lx\n", Bridge->PMem.Base, Bridge->PMem.Limit)); >>>> - DEBUG ((EFI_D_INFO, " PMemAbove4G: %lx - %lx\n", Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit)); >>>> + ASSERT (Bridge->Bus.Translation == 0); >>>> + if (Bridge->Bus.Translation != 0) { >>>> + return NULL; >>>> + } 2. Can you please use the same debug message format for Bus range? I mean to print the Translation for Bus as well. Then in the end of debug message, use assertion and if-check. This way developer can know what the Bus.Translation is from the debug log. >>>> + >>>> + DEBUG (( >>>> + DEBUG_INFO, " Io: %lx - %lx Translation=%lx\n", >>>> + Bridge->Io.Base, Bridge->Io.Limit, Bridge->Io.Translation >>>> + )); >>>> + DEBUG (( >>>> + DEBUG_INFO, " Mem: %lx - %lx Translation=%lx\n", >>>> + Bridge->Mem.Base, Bridge->Mem.Limit, Bridge->Mem.Translation >>>> + )); >>>> + DEBUG (( >>>> + DEBUG_INFO, " MemAbove4G: %lx - %lx Translation=%lx\n", >>>> + Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit, Bridge->MemAbove4G.Translation >>>> + )); >>>> + DEBUG (( >>>> + DEBUG_INFO, " PMem: %lx - %lx Translation=%lx\n", >>>> + Bridge->PMem.Base, Bridge->PMem.Limit, Bridge->PMem.Translation >>>> + )); >>>> + DEBUG (( >>>> + DEBUG_INFO, " PMemAbove4G: %lx - %lx Translation=%lx\n", >>>> + Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit, Bridge->PMemAbove4G.Translation >>>> + )); >>>> // >>>> // Make sure Mem and MemAbove4G apertures are valid >>>> @@ -206,7 +229,12 @@ CreateRootBridge ( >>>> } >>>> RootBridge->ResAllocNode[Index].Type = Index; >>>> if (Bridge->ResourceAssigned && (Aperture->Limit >= Aperture->Base)) { >>>> - RootBridge->ResAllocNode[Index].Base = Aperture->Base; >>>> + // >>>> + // Base in ResAllocNode is a host address, while Base in Aperture is a >>>> + // device address. >>>> + // >>>> + RootBridge->ResAllocNode[Index].Base = TO_HOST_ADDRESS (Aperture->Base, >>>> + Aperture->Translation); >>>> RootBridge->ResAllocNode[Index].Length = Aperture->Limit - Aperture->Base + 1; >>>> RootBridge->ResAllocNode[Index].Status = ResAllocated; >>>> } else { >>>> @@ -403,6 +431,28 @@ RootBridgeIoCheckParameter ( >>>> return EFI_SUCCESS; >>>> } 3. Please add the missing function header comments. >>>> +EFI_STATUS >>>> +RootBridgeIoGetMemTranslationByAddress ( >>>> + IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge, >>>> + IN UINT64 Address, >>>> + IN OUT UINT64 *Translation >>>> + ) >>>> +{ >>>> + if (Address >= RootBridge->Mem.Base && Address <= RootBridge->Mem.Limit) { >>>> + *Translation = RootBridge->Mem.Translation; >>>> + } else if (Address >= RootBridge->PMem.Base && Address <= RootBridge->PMem.Limit) { >>>> + *Translation = RootBridge->PMem.Translation; >>>> + } else if (Address >= RootBridge->MemAbove4G.Base && Address <= RootBridge->MemAbove4G.Limit) { >>>> + *Translation = RootBridge->MemAbove4G.Translation; >>>> + } else if (Address >= RootBridge->PMemAbove4G.Base && Address <= RootBridge->PMemAbove4G.Limit) { >>>> + *Translation = RootBridge->PMemAbove4G.Translation; >>>> + } else { >>>> + return EFI_INVALID_PARAMETER; >>>> + } >>>> + >>>> + return EFI_SUCCESS; >>>> +} >>>> + >>>> /** >>>> Polls an address in memory mapped I/O space until an exit condition is met, >>>> or a timeout occurs. >>>> @@ -658,13 +708,25 @@ RootBridgeIoMemRead ( >>>> ) >>>> { >>>> EFI_STATUS Status; >>>> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge; >>>> + UINT64 Translation; >>>> Status = RootBridgeIoCheckParameter (This, MemOperation, Width, Address, >>>> Count, Buffer); >>>> if (EFI_ERROR (Status)) { >>>> return Status; >>>> } >>>> - return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer); >>>> + >>>> + RootBridge = ROOT_BRIDGE_FROM_THIS (This); >>>> + Status = RootBridgeIoGetMemTranslationByAddress (RootBridge, Address, &Translation); >>>> + if (EFI_ERROR (Status)) { >>>> + return Status; >>>> + } >>>> + >>>> + // Address passed to CpuIo->Mem.Read needs to be a host address instead of >>>> + // device address. >>>> + return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, >>>> + TO_HOST_ADDRESS (Address, Translation), Count, Buffer); >>>> } >>>> /** >>>> @@ -705,13 +767,25 @@ RootBridgeIoMemWrite ( >>>> ) >>>> { >>>> EFI_STATUS Status; >>>> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge; >>>> + UINT64 Translation; >>>> Status = RootBridgeIoCheckParameter (This, MemOperation, Width, Address, >>>> Count, Buffer); >>>> if (EFI_ERROR (Status)) { >>>> return Status; >>>> } >>>> - return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer); >>>> + >>>> + RootBridge = ROOT_BRIDGE_FROM_THIS (This); >>>> + Status = RootBridgeIoGetMemTranslationByAddress (RootBridge, Address, &Translation); >>>> + if (EFI_ERROR (Status)) { >>>> + return Status; >>>> + } >>>> + >>>> + // Address passed to CpuIo->Mem.Write needs to be a host address instead of >>>> + // device address. >>>> + return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, >>>> + TO_HOST_ADDRESS (Address, Translation), Count, Buffer); >>>> } >>>> /** >>>> @@ -746,6 +820,8 @@ RootBridgeIoIoRead ( >>>> ) >>>> { >>>> EFI_STATUS Status; >>>> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge; >>>> + >>>> Status = RootBridgeIoCheckParameter ( >>>> This, IoOperation, Width, >>>> Address, Count, Buffer >>>> @@ -753,7 +829,13 @@ RootBridgeIoIoRead ( >>>> if (EFI_ERROR (Status)) { >>>> return Status; >>>> } >>>> - return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer); >>>> + >>>> + RootBridge = ROOT_BRIDGE_FROM_THIS (This); >>>> + >>>> + // Address passed to CpuIo->Io.Read needs to be a host address instead of >>>> + // device address. >>>> + return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, >>>> + TO_HOST_ADDRESS (Address, RootBridge->Io.Translation), Count, Buffer); >>>> } >>>> /** >>>> @@ -788,6 +870,8 @@ RootBridgeIoIoWrite ( >>>> ) >>>> { >>>> EFI_STATUS Status; >>>> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge; >>>> + >>>> Status = RootBridgeIoCheckParameter ( >>>> This, IoOperation, Width, >>>> Address, Count, Buffer >>>> @@ -795,7 +879,13 @@ RootBridgeIoIoWrite ( >>>> if (EFI_ERROR (Status)) { >>>> return Status; >>>> } >>>> - return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer); >>>> + >>>> + RootBridge = ROOT_BRIDGE_FROM_THIS (This); >>>> + >>>> + // Address passed to CpuIo->Io.Write needs to be a host address instead of >>>> + // device address. >>>> + return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, >>>> + TO_HOST_ADDRESS (Address, RootBridge->Io.Translation), Count, Buffer); >>>> } >>>> /** >>>> @@ -1615,9 +1705,17 @@ RootBridgeIoConfiguration ( >>>> Descriptor->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR; >>>> Descriptor->Len = sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3; >>>> + // According to UEFI 2.7, RootBridgeIo->Configuration should return address >>>> + // range in CPU view (host address), and ResAllocNode->Base is already a CPU >>>> + // view address (host address). >>>> Descriptor->AddrRangeMin = ResAllocNode->Base; >>>> Descriptor->AddrRangeMax = ResAllocNode->Base + ResAllocNode->Length - 1; >>>> Descriptor->AddrLen = ResAllocNode->Length; >>>> + Descriptor->AddrTranslationOffset = GetTranslationByResourceType ( >>>> + RootBridge, >>>> + ResAllocNode->Type >>>> + ); >>>> + >>>> switch (ResAllocNode->Type) { >>>> case TypeIo: >>>> -- >>>> 2.7.4 >>>> >> >> >> -- >> Thanks, >> Ray Reviewed-by: Ruiyu Ni Please modify code according to the above 3 comments and make sure the changes pass ECC check: BaseTools/Source/Python/Ecc/Ecc.py -t -s \ MdeModulePkg/Pci/PciHostBridgeDxe > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > -- Thanks, Ray