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.88; helo=mga01.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (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 E9064209574EA for ; Tue, 27 Feb 2018 00:42:43 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Feb 2018 00:48:49 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,400,1515484800"; d="scan'208";a="33927286" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.13]) ([10.239.9.13]) by fmsmga001.fm.intel.com with ESMTP; 27 Feb 2018 00:48:48 -0800 To: Heyi Guo , edk2-devel@lists.01.org Cc: Ard Biesheuvel , Star Zeng , Eric Dong , Laszlo Ersek , Michael D Kinney References: <1519697389-3525-1-git-send-email-heyi.guo@linaro.org> <1519697389-3525-2-git-send-email-heyi.guo@linaro.org> From: "Ni, Ruiyu" Message-ID: <563e76b9-ebbc-be09-d306-8dcb86f12112@Intel.com> Date: Tue, 27 Feb 2018 16:48:47 +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: <1519697389-3525-2-git-send-email-heyi.guo@linaro.org> Subject: Re: [RFC v4 1/3] 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: Tue, 27 Feb 2018 08:42:44 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 2/27/2018 10:09 AM, 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: > > 1. Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address, for > it is easy to check whether the address is below 4G or above 4G. > > 2. Address returned by > EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL::GetProposedResources > is device address, or else PCI bus driver cannot set correct address > into PCI BAR registers. > > 3. Address returned by PciRootBridgeIo->Configuration is host address > per UEFI 2.7 definition. > > 4. Addresses used in GCD manipulation are host address. > > 5. Addresses in ResAllocNode of PCI_ROOT_BRIDGE_INSTANCE are host > address, for they are allocated from GCD. > > 6. Address passed to PciHostBridgeResourceConflict is host address, > for it comes from 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. (1). I'd like to see this restriction be reflected in code. Both assertion and if-check are needed to ensure debug firmware hang and release firmware return fail status from PciHostBridge driver. > > 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: > 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/PciHostResource.h | 2 + > MdeModulePkg/Include/Library/PciHostBridgeLib.h | 14 +++ > MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 85 +++++++++++--- > MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 122 +++++++++++++++++--- > 4 files changed, 194 insertions(+), 29 deletions(-) > > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h > index 8612c0c3251b..662c2dd59529 100644 > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h > @@ -38,6 +38,8 @@ typedef enum { > > typedef struct { > PCI_RESOURCE_TYPE Type; > + // Base is a host address instead of a device address when address translation > + // exists and host address != device address (2). Coding style issue. The comments need to be in style like: //[EMPTY] // xxxx //[EMPTY] And how about just simply say Base is a host address. No matter address translation exists or not, Base is a host address actually. > UINT64 Base; > UINT64 Length; > UINT64 Alignment; > diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h b/MdeModulePkg/Include/Library/PciHostBridgeLib.h > index d42e9ecdd763..c842a4152e85 100644 > --- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h > +++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h > @@ -20,8 +20,22 @@ > // (Base > Limit) indicates an aperture is not available. > // > typedef struct { > + // Base and Limit are the device address instead of host address when > + // Translation is not zero (3). Similar comments to (2). > UINT64 Base; > UINT64 Limit; > + // According to UEFI 2.7, Device Address = Host Address + Translation, > + // so Translation = Device Address - Host Address. > + // On platforms where Translation is not zero, the subtraction is probably to > + // be performed with UINT64 wrap-around semantics, for we may translate an > + // above-4G host address into a below-4G device address for legacy PCIe device > + // compatibility. > + // NOTE: The alignment of Translation is required to be larger than any BAR > + // alignment in the same root bridge, so that the same alignment can be > + // applied to both device address and host address, which simplifies the > + // situation and makes the current resource allocation code in generic PCI > + // host bridge driver still work. (4). Still I'd like to see the alignment requirement be reflected in code through assertion and if-check. > + UINT64 Translation; > } PCI_ROOT_BRIDGE_APERTURE; > > typedef struct { > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > index 1494848c3e8c..1e65faee9084 100644 > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > @@ -32,6 +32,30 @@ EDKII_IOMMU_PROTOCOL *mIoMmuProtocol; > EFI_EVENT mIoMmuEvent; > VOID *mIoMmuRegistration; > > +STATIC (5). Can you remove STATIC? personal preference:) > +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; > + default: > + ASSERT (FALSE); > + return 0; > + } > +} > + > /** > Ensure the compatibility of an IO space descriptor with the IO aperture. > > @@ -366,6 +390,7 @@ InitializePciHostBridge ( > UINTN MemApertureIndex; > BOOLEAN ResourceAssigned; > LIST_ENTRY *Link; > + UINT64 TempHostAddress; (6). Just use name HostAddress? > > RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount); > if ((RootBridges == NULL) || (RootBridgeCount == 0)) { > @@ -411,18 +436,24 @@ InitializePciHostBridge ( > } > > if (RootBridges[Index].Io.Base <= RootBridges[Index].Io.Limit) { > + // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address. > + // According to UEFI 2.7, device address = host address + Translation. > + // For GCD resource manipulation, we should use host address, so > + // Translation is subtracted from device address here. (7). Please adjust the comment style to have two EMPTY line of comment above and below. Same to all comments in the patch. > Status = AddIoSpace ( > - RootBridges[Index].Io.Base, > + RootBridges[Index].Io.Base - RootBridges[Index].Io.Translation, > RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1 > ); > ASSERT_EFI_ERROR (Status); > if (ResourceAssigned) { > + TempHostAddress = RootBridges[Index].Io.Base - > + RootBridges[Index].Io.Translation; > Status = gDS->AllocateIoSpace ( > EfiGcdAllocateAddress, > EfiGcdIoTypeIo, > 0, > RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1, > - &RootBridges[Index].Io.Base, > + &TempHostAddress, > gImageHandle, > NULL > ); > @@ -443,14 +474,18 @@ 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. > + // According to UEFI 2.7, device address = host address + Translation. > + // For GCD resource manipulation, we should use host address, so > + // Translation is subtracted from device address here. > Status = AddMemoryMappedIoSpace ( > - MemApertures[MemApertureIndex]->Base, > + MemApertures[MemApertureIndex]->Base - MemApertures[MemApertureIndex]->Translation, > MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1, > EFI_MEMORY_UC > ); > ASSERT_EFI_ERROR (Status); > Status = gDS->SetMemorySpaceAttributes ( > - MemApertures[MemApertureIndex]->Base, > + MemApertures[MemApertureIndex]->Base - MemApertures[MemApertureIndex]->Translation, > MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1, > EFI_MEMORY_UC > ); > @@ -458,12 +493,14 @@ InitializePciHostBridge ( > DEBUG ((DEBUG_WARN, "PciHostBridge driver failed to set EFI_MEMORY_UC to MMIO aperture - %r.\n", Status)); > } > if (ResourceAssigned) { > + TempHostAddress = MemApertures[MemApertureIndex]->Base - > + MemApertures[MemApertureIndex]->Translation; > Status = gDS->AllocateMemorySpace ( > EfiGcdAllocateAddress, > EfiGcdMemoryTypeMemoryMappedIo, > 0, > MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1, > - &MemApertures[MemApertureIndex]->Base, > + &TempHostAddress, > gImageHandle, > NULL > ); > @@ -654,6 +691,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)); > > @@ -824,12 +866,17 @@ NotifyPhase ( > > switch (Index) { > case TypeIo: > + // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address. > + // According to UEFI 2.7, device address = host address + Translation. > + // For AllocateResource is manipulating GCD resource, we should use > + // host address here, so Translation is subtracted from Base and > + // Limit. > BaseAddress = AllocateResource ( > FALSE, > RootBridge->ResAllocNode[Index].Length, > MIN (15, BitsOfAlignment), > - ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1), > - RootBridge->Io.Limit > + ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1) - RootBridge->Io.Translation, > + RootBridge->Io.Limit - RootBridge->Io.Translation > ); > break; > > @@ -838,8 +885,8 @@ NotifyPhase ( (8). Add assertion and if-check here to make sure alignment of Translation is no less than the Alignment. > TRUE, > RootBridge->ResAllocNode[Index].Length, > MIN (63, BitsOfAlignment), > - ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1), > - RootBridge->MemAbove4G.Limit > + ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1) - RootBridge->MemAbove4G.Translation, > + RootBridge->MemAbove4G.Limit - RootBridge->MemAbove4G.Translation > ); > if (BaseAddress != MAX_UINT64) { > break; > @@ -853,8 +900,8 @@ NotifyPhase ( > TRUE, > RootBridge->ResAllocNode[Index].Length, > MIN (31, BitsOfAlignment), > - ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1), > - RootBridge->Mem.Limit > + ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1) - RootBridge->Mem.Translation, > + RootBridge->Mem.Limit - RootBridge->Mem.Translation > ); > break; > > @@ -863,8 +910,8 @@ NotifyPhase ( > TRUE, > RootBridge->ResAllocNode[Index].Length, > MIN (63, BitsOfAlignment), > - ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1), > - RootBridge->PMemAbove4G.Limit > + ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1) - RootBridge->PMemAbove4G.Translation, > + RootBridge->PMemAbove4G.Limit - RootBridge->PMemAbove4G.Translation > ); > if (BaseAddress != MAX_UINT64) { > break; > @@ -877,8 +924,8 @@ NotifyPhase ( > TRUE, > RootBridge->ResAllocNode[Index].Length, > MIN (31, BitsOfAlignment), > - ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1), > - RootBridge->PMem.Limit > + ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1) - RootBridge->PMem.Translation, > + RootBridge->PMem.Limit - RootBridge->PMem.Translation > ); > break; > > @@ -1152,6 +1199,7 @@ StartBusEnumeration ( > Descriptor->AddrSpaceGranularity = 0; > Descriptor->AddrRangeMin = RootBridge->Bus.Base; > Descriptor->AddrRangeMax = 0; > + // Ignore translation offset for bus (9). Per PI spec on StartBusEnumeration, AddrRangeMax is ignored. So I don't think we need add comments here. > Descriptor->AddrTranslationOffset = 0; > Descriptor->AddrLen = RootBridge->Bus.Limit - RootBridge->Bus.Base + 1; > > @@ -1421,7 +1469,12 @@ 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 Translation is added. > + Descriptor->AddrRangeMin = RootBridge->ResAllocNode[Index].Base + > + GetTranslationByResourceType (RootBridge, Index); (10). Do you think defining a PciHostBridgeDxe internal macro TO_HOST_ADDRESS(DeviceAddress, Offset) and TO_DEVICE_ADDRESS(HostAddress, Offset) is better? > 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..edaa0d48a441 100644 > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > @@ -86,12 +86,28 @@ 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"" > )); > + // We don't see any scenario for bus translation, so translation for bus is just ignored. > 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)); > + 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 +222,15 @@ CreateRootBridge ( > } > RootBridge->ResAllocNode[Index].Type = Index; > if (Bridge->ResourceAssigned && (Aperture->Limit >= Aperture->Base)) { > - RootBridge->ResAllocNode[Index].Base = Aperture->Base; > + // Ignore translation for bus (11). How about we only make sure the TranslationOffset is 0 for TypeBus when getting the aperture from PciHostBridgeLib. So that later logic doesn't need to do special handling for the TypeBus. This way the code can be simpler. > + if (Index == TypeBus) { > + RootBridge->ResAllocNode[Index].Base = Aperture->Base; > + } else { > + // Base in ResAllocNode is a host address, while Base in Aperture is a > + // device address, so translation needs to be subtracted. > + RootBridge->ResAllocNode[Index].Base = Aperture->Base - > + Aperture->Translation; (12). Still. Do you think using TO_HOST_ADDRESS() is better? > + } > RootBridge->ResAllocNode[Index].Length = Aperture->Limit - Aperture->Base + 1; > RootBridge->ResAllocNode[Index].Status = ResAllocated; > } else { > @@ -403,6 +427,28 @@ RootBridgeIoCheckParameter ( > return EFI_SUCCESS; > } > > +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 +704,24 @@ 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); (13). Why do you think the Address is a Device Address? PciIo.GetBarAttributes() returns a HOST Address and the Translation Offset. I didn't see you change the PciIoMemRead() implementation. So it means the same HOST Address is passed into the this function. > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + // Address passed to CpuIo->Mem.Read should be a host address instead of > + // device address, so Translation should be subtracted. > + return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - Translation, Count, Buffer); > } > > /** > @@ -705,13 +762,24 @@ 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 should be a host address instead of > + // device address, so Translation should be subtracted. > + return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - Translation, Count, Buffer); (14). Same as (13) > } > > /** > @@ -746,6 +814,8 @@ RootBridgeIoIoRead ( > ) > { > EFI_STATUS Status; > + PCI_ROOT_BRIDGE_INSTANCE *RootBridge; > + > Status = RootBridgeIoCheckParameter ( > This, IoOperation, Width, > Address, Count, Buffer > @@ -753,7 +823,12 @@ 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 should be a host address instead of > + // device address, so Translation should be subtracted. > + return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - RootBridge->Io.Translation, Count, Buffer); (15). Same as (13) > } > > /** > @@ -788,6 +863,8 @@ RootBridgeIoIoWrite ( > ) > { > EFI_STATUS Status; > + PCI_ROOT_BRIDGE_INSTANCE *RootBridge; > + > Status = RootBridgeIoCheckParameter ( > This, IoOperation, Width, > Address, Count, Buffer > @@ -795,7 +872,12 @@ 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 should be a host address instead of > + // device address, so Translation should be subtracted. > + return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - RootBridge->Io.Translation, Count, Buffer); (16). Same as (13) > } > > /** > @@ -1615,25 +1697,39 @@ 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; > switch (ResAllocNode->Type) { > > case TypeIo: > - Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO; > + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO; > + Descriptor->AddrTranslationOffset = RootBridge->Io.Translation; (17). Can GetTranslationByResourceType() be used to simplify the code? > break; > > case TypePMem32: > - Descriptor->SpecificFlag = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE; > + Descriptor->SpecificFlag = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE; > + Descriptor->AddrTranslationOffset = RootBridge->PMem.Translation; > + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; > + Descriptor->AddrSpaceGranularity = 32; > + break; > + > case TypeMem32: > + Descriptor->AddrTranslationOffset = RootBridge->Mem.Translation; > Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; > Descriptor->AddrSpaceGranularity = 32; > break; > > case TypePMem64: > - Descriptor->SpecificFlag = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE; > + Descriptor->SpecificFlag = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE; > + Descriptor->AddrTranslationOffset = RootBridge->PMemAbove4G.Translation; > + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; > + Descriptor->AddrSpaceGranularity = 64; > case TypeMem64: > + Descriptor->AddrTranslationOffset = RootBridge->MemAbove4G.Translation; > Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; > Descriptor->AddrSpaceGranularity = 64; > break; > (18). Please remember to run BaseTools/Source/Python/Ecc/Ecc.py on the PciHostBridgeDxe driver to make sure coding style matches the requirement. -- Thanks, Ray