From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.126; helo=mga18.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) (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 8AD25222A334B for ; Sat, 24 Feb 2018 00:24:42 -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 orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Feb 2018 00:30:44 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,386,1515484800"; d="scan'208";a="33259611" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.13]) ([10.239.9.13]) by fmsmga001.fm.intel.com with ESMTP; 24 Feb 2018 00:30:43 -0800 To: Guo Heyi Cc: Laszlo Ersek , edk2-devel@lists.01.org, Eric Dong , Ard Biesheuvel , Michael D Kinney , Star Zeng References: <1519376008-110662-1-git-send-email-heyi.guo@linaro.org> <1519376008-110662-2-git-send-email-heyi.guo@linaro.org> <20180224015139.GB111587@SZX1000114654> <56a50ebe-c27e-1a0b-1d96-30b0443fee59@Intel.com> <20180224035914.GD111587@SZX1000114654> From: "Ni, Ruiyu" Message-ID: <4cac156f-feab-2746-53ee-2b009c5b56d0@Intel.com> Date: Sat, 24 Feb 2018 16:30:42 +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: <20180224035914.GD111587@SZX1000114654> Subject: Re: [RFC v3 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: Sat, 24 Feb 2018 08:24:42 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 2/24/2018 11:59 AM, Guo Heyi wrote: > On Sat, Feb 24, 2018 at 11:11:35AM +0800, Ni, Ruiyu wrote: >> On 2/24/2018 9:51 AM, Guo Heyi wrote: >>> On Fri, Feb 23, 2018 at 04:05:17PM +0100, Laszlo Ersek wrote: >>>> Thanks for writing up the details! >>>> >>>> Comments: >>>> >>>> On 02/23/18 09:53, 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. >>>> >>>> OK, this looks like a sensible choice to me. It means that the "apply" >>>> term used in the UEFI spec means "add", which (I think) is the "natural" >>>> interpretation. >>>> >>>>> 2. PciRootBridgeIo->Configuration should return CPU view address, as >>>>> well as PciIo->GetBarAttributes. >>>> >>>> OK. >>>> >>>>> >>>>> 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. >>>> >>>> I agree that PciHostBridgeLib instances that do not set a nonzero >>>> Translation need not care about the difference. >>>> >>>> (1) Still, can you confirm this is a "natural" choice? Were the >>>> DmaAbove4G, MemAbove4G and PMemAbove4G fields originally introduced with >>>> the device (PCI) view in mind? >>>> >>>> ... I also meant to raise a concern about the InitializePciHostBridge() >>>> function where we call AddIoSpace() and AddMemoryMappedIoSpace() -- >>>> which end up manipulating GCD -- with data from >>>> PCI_ROOT_BRIDGE_APERTURE. I can see you modify those call sites in the >>>> patch, so that's good. (I do have more comments on the actual >>>> implementation.) >> DmaAbove4G in PCI_ROOT_BRIDGE_APERTURE indicates the capability of >> the root bridge HW, whether it's capable to do DMA on device address >> above 4GB. >> >>>> >>>>> >>>>> 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. >>>>> >>>>> 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 >>>>> --- >>>>> .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 74 +++++++++++--- >>>>> .../Bus/Pci/PciHostBridgeDxe/PciHostResource.h | 2 + >>>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 112 ++++++++++++++++++--- >>>>> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 8 ++ >>>>> 4 files changed, 167 insertions(+), 29 deletions(-) >>>>> >>>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >>>>> index 1494848..e8979eb 100644 >>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >>>>> @@ -32,6 +32,29 @@ EDKII_IOMMU_PROTOCOL *mIoMmuProtocol; >>>>> EFI_EVENT mIoMmuEvent; >>>>> VOID *mIoMmuRegistration; >>>>> >>>>> +STATIC >>>>> +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: >>>> >>>> (2) How about we return zero for TypeBus, explicitly, and then >>>> ASSERT(FALSE) and return zero for "default"? >>>> >>>>> + return 0; >>>>> + } >>>>> +} >>>>> + >>>>> /** >>>>> Ensure the compatibility of an IO space descriptor with the IO aperture. >>>>> >>>>> @@ -411,8 +434,12 @@ 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. >>>> >>>> (3) In general, the comment style requires comments like this: >>>> >>>> // >>>> // Add empty comment lines both before and after. >>>> // >>>> >>>>> 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); >>>> >>>> This looks fine. >>>> >>>>> @@ -422,7 +449,7 @@ InitializePciHostBridge ( >>>>> EfiGcdIoTypeIo, >>>>> 0, >>>>> RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1, >>>>> - &RootBridges[Index].Io.Base, >>>>> + &RootBridges[Index].Io.Base - RootBridges[Index].Io.Translation, >>>>> gImageHandle, >>>>> NULL >>>>> ); >>>> >>>> (4) But this is wrong (operating under the assumption that >>>> PCI_ROOT_BRIDGE_APERTURE providing device addresses). Namely, 5th >>>> parameter of gDS->AllocateIoSpace() is an in/out parameter: >>>> >>>> [MdeModulePkg/Core/Dxe/Gcd/Gcd.c] >>>> >>>> EFI_STATUS >>>> EFIAPI >>>> CoreAllocateIoSpace ( >>>> IN EFI_GCD_ALLOCATE_TYPE GcdAllocateType, >>>> IN EFI_GCD_IO_TYPE GcdIoType, >>>> IN UINTN Alignment, >>>> IN UINT64 Length, >>>> IN OUT EFI_PHYSICAL_ADDRESS *BaseAddress, >>>> IN EFI_HANDLE ImageHandle, >>>> IN EFI_HANDLE DeviceHandle OPTIONAL >>>> ) >>>> >>>> The patch, as written, takes the address of the Base field, and then >>>> decreases that address by Translation * sizeof Base. The resultant >>>> pointer is passed to the function as 5th argument. >>>> >>>> I don't think that's what you meant :) >>>> >>>> Instead, you have to translate the device address to host address first >>>> (using a temporary variable), and pass that to the function. (Updating >>>> the Base field back to the resultant / output value shouldn't be >>>> necessary, because GcdAllocateType==EfiGcdAllocateAddress, so the >>>> allocation is required to succeed at the exact address requested.) >>>> >>>>> @@ -443,14 +470,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 >>>>> ); >>>>> @@ -463,7 +494,7 @@ InitializePciHostBridge ( >>>>> EfiGcdMemoryTypeMemoryMappedIo, >>>>> 0, >>>>> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1, >>>>> - &MemApertures[MemApertureIndex]->Base, >>>>> + &MemApertures[MemApertureIndex]->Base - MemApertures[MemApertureIndex]->Translation, >>>>> gImageHandle, >>>>> NULL >>>>> ); >>>> >>>> (5) Same comment as (4) applies, to the 5th argument. >>>> >>>>> @@ -824,12 +855,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 +874,8 @@ NotifyPhase ( >>>>> 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 +889,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 +899,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 +913,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; >>>> >>>> (6) For all of these, I believe that we have to think about the corner >>>> case when the Translation value is not a multiple of (Alignment + 1). >>>> >>>> "Alignment" comes from the PCI BAR in question, whereas Base comes from >>>> the platform PciHostBridgeLib. I think these are fairly independent (you >>>> can plug a 3rd party, discrete PCI card into a board). I find it >>>> plausible that the card has such a huge MMIO BAR (and alignment) that >>>> the platform's Translation offset is not a multiple thereof. >>>> >>>> So, which "view" has to be aligned here? The PCI (device) view, or the >>>> host (CPU) view? >>> >>> I believe the alignment requirement is from PCI view, not the CPU view. This >>> also implies alignment in allocating GCD resources becomes meaningless. >> I agree. It's an issue we need to think about. >> >> All address spaces in GCD are added by gDS->AddXXX(). >> So it means for a given range of address [HostBase, HostLimit) in GCD, >> the corresponding device address is >> [HostBase + Offset, HostLimit + Offset). >> E.g.: GCD entry = [0xFFD, 0x3FFD), Offset = 0x3 >> The corresponding device address range is [0x1000, 0x4000) >> If we want to allocate 3 page-aligned pages of MMIO from GCD, >> current AllocateResource() implementation will fail to allocate. >> >> What will the Offset be in real world? >> If it's quite small (smaller than device required alignment), >> thing becomes complicated. I even cannot think of any solution. >> If it's quite large (larger than device required alignment), >> thing becomes easy. Today's implementation can handle it well. > > I believe the Offset is normally large, so that we may place some assumption or > restriction here, to make things easier. You can see the translation on our platform: > https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Hisilicon/Hi1616/D05AcpiTables/Dsdt/D05Pci.asl#L204 > > How about we assume ATO alignment >= the whole region length (denoted as > L)? Then there are two possibilities: This assumption is a bit strict I think. Can we assume ATO alignment >= RootBridge->ResAllocNode[i].Alignment? RootBridge->ResAllocNode[i].Alignment carries the alignment requirement from the specific resource type. Supposing there are three 32bit BARs from 2 PCI devices, each requires alignment/length 0x100, 0x2000, 0x400, RootBridge->ResAllocNode[i].Alignment equals to 0x2000. RootBridge->ResAllocNode[i].Length equals to 0x2000 + 0x400 + 0x100. We only require ATO alignment >= 0x2000 in this case, but don't require ATO alignment >= 0x2500. Agree? > > 1. BAR alignment <= L, so BAR alignment <= ATO alignment, so > (each host address % BAR alignment) == (each device address % BAR alignment), > and the current code can handle this. > > This should cover most of cases, since BAR alignment <= BAR length, and the > other possibility is actually out of resource. > > 2. BAR alignment > L, e.g. > BAR alignment = 1MB > L = 4KB > ATO = 4KB or -4KB > > 1) If device address = 0, host address = 4KB, then AllocateResource will > return -1 and result to OUT_OF_RESOURCES; > 2) If device address = 4KB, host address = 0, then AllocateResource will also > return -1 > Anyway we will get the expected OUT_OF_RESOURCES result by current code. > > And this assumption is not a very strict one. > > Any comments? > > Thanks, > > Gary (Heyi Guo) > > >> >> >> >>> >>> Thanks, >>> >>> Gary (Heyi Guo) >>> >>> >>>> >>>> ... Hmmm wait, the AllocateResource() function aligns BaseAddress >>>> internally anyway. So, first, I don't understand why the pre-patch code >>>> uses ALIGN_VALUE at the call site as well; second, due to the alignment >>>> internal to AllocateResource(), we couldn't align *just* the PCI view >>>> even if we wanted to. >>>> >>>> So I guess this code is correct (due to the alignment internal to >>>> AllocateResource()), but could we perhaps simplify it by passing >>>> >>>> RootBridge->Xxx.Base - RootBridge->Xxx.Translation >>>> >>>> i.e., by dropping the outer alignment? >>>> >>>> (To be clear, dropping the "outer" alignment, if it's a valid change, >>>> should be done as a separate patch, before the translation is >>>> introduced. I hope Ray can comment on this.) >>>> >>>>> >>>>> @@ -1152,6 +1188,7 @@ StartBusEnumeration ( >>>>> Descriptor->AddrSpaceGranularity = 0; >>>>> Descriptor->AddrRangeMin = RootBridge->Bus.Base; >>>>> Descriptor->AddrRangeMax = 0; >>>>> + // Ignore translation offset for bus >>>>> Descriptor->AddrTranslationOffset = 0; >>>>> Descriptor->AddrLen = RootBridge->Bus.Limit - RootBridge->Bus.Base + 1; >>>>> >>>> >>>> (7) Ah, in this case, adding TypeBus under (2) is not necessary, just >>>> ASSERT(FALSE) under the currently proposed "default" label. >>>> >>>> >>>>> @@ -1421,7 +1458,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); >>>>> 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/PciHostResource.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h >>>>> index 8612c0c..662c2dd 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 >>>>> UINT64 Base; >>>>> UINT64 Length; >>>>> UINT64 Alignment; >>>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c >>>>> index dc06c16..04ed411 100644 >>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c >>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c >>>>> @@ -86,12 +86,23 @@ 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 >>>>> + )); >>>> >>>> (8) I suggest capitalizing "Translation" in the debug output. >>>> >>>> (9) The indentation is not idiomatic, it should be >>>> >>>> DEBUG (( >>>> ... >>>> ... >>>> )); >>>> >>>>> >>>>> // >>>>> // Make sure Mem and MemAbove4G apertures are valid >>>>> @@ -206,7 +217,10 @@ 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, so translation needs to be subtracted. >>>>> + RootBridge->ResAllocNode[Index].Base = Aperture->Base - >>>>> + Aperture->Translation; >>>>> RootBridge->ResAllocNode[Index].Length = Aperture->Limit - Aperture->Base + 1; >>>>> RootBridge->ResAllocNode[Index].Status = ResAllocated; >>>>> } else { >>>>> @@ -403,6 +417,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 +694,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); >>>>> + 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 +752,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); >>>>> } >>>>> >>>>> /** >>>>> @@ -746,6 +804,8 @@ RootBridgeIoIoRead ( >>>>> ) >>>>> { >>>>> EFI_STATUS Status; >>>>> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge; >>>>> + >>>>> Status = RootBridgeIoCheckParameter ( >>>>> This, IoOperation, Width, >>>>> Address, Count, Buffer >>>>> @@ -753,7 +813,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); >>>>> } >>>>> >>>>> /** >>>>> @@ -788,6 +853,8 @@ RootBridgeIoIoWrite ( >>>>> ) >>>>> { >>>>> EFI_STATUS Status; >>>>> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge; >>>>> + >>>>> Status = RootBridgeIoCheckParameter ( >>>>> This, IoOperation, Width, >>>>> Address, Count, Buffer >>>>> @@ -795,7 +862,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); >>>>> } >>>>> >>>>> /** >>>>> @@ -1615,25 +1687,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; >>>>> 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; >>>>> diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h b/MdeModulePkg/Include/Library/PciHostBridgeLib.h >>>>> index d42e9ec..21ee8cd 100644 >>>>> --- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h >>>>> +++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h >>>>> @@ -20,8 +20,16 @@ >>>>> // (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 >>>>> 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, Translation is probably >>>>> + // negative for we may translate an above-4G host address into a below-4G >>>>> + // device address for legacy PCIe device compatibility. >>>>> + UINT64 Translation; >>>>> } PCI_ROOT_BRIDGE_APERTURE; >>>>> >>>>> typedef struct { >>>>> >>>> >>>> (10) I suggest replacing the "negative" language with "the subtraction >>>> is to be performed with UINT64 wrap-around semantics". >>>> >>>> >>>> I've made some comments, but I admit my understanding is quite limited; >>>> sorry about that. >>>> >>>> Thanks >>>> Laszlo >> >> >> -- >> Thanks, >> Ray -- Thanks, Ray