From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:400e:c05::232; helo=mail-pg0-x232.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pg0-x232.google.com (mail-pg0-x232.google.com [IPv6:2607:f8b0:400e:c05::232]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D8C152034D8FB for ; Fri, 23 Feb 2018 19:53:17 -0800 (PST) Received: by mail-pg0-x232.google.com with SMTP id e11so4099377pgq.12 for ; Fri, 23 Feb 2018 19:59:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:date:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=M0O3wkPJBU3ec6vlZmh7vmTd0JxMTbdtU9O8SmAr8lw=; b=Zfmk3xRONOUBGlr/j/MRrdGQZDrmRXrhMqQ2XjvFiWbnQRIcWJ+S+4qyYL8JMtxwm6 BSPPysLK+BM414BKpQkUuWWNPa/amGdCUSU6pD6BnOK5qIWbNkGGks5Hoo1SvMB8UKsu RaK5Q+Jv3Wraa1YIPZzBXWY6bJRuTJ+otVcA4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=M0O3wkPJBU3ec6vlZmh7vmTd0JxMTbdtU9O8SmAr8lw=; b=l51ZD4m5i/dkwpAvfVUKZNQFWXGqBp69Y/tqXnH1LlLanwP1RN515MwHx5C9igTptn +6Rxb6IL0+gKnF/ynQXlbOuXQwGgaTDV9u84erzKi5bVyr8ebFqn6vEn+ADscSqvRoDn /CHcBykDFFQDptyLP6Gj/TLXMWz80JiT5BCNNsnvjuQbZ9XdeGpfFgoyoHxWZq3DhjdG rKTDKraws5PtZIeI6DzKvgQu9TyACZ5TSaW3Gm9vrHGHldPl+f8p5Xjjxf9czLVQ4m6B d4FqbgPnUJy39ed6TNJ15EzoZvbhyTKwVa5mFWzYnVHPu6LgAM4SGm3yJdvLs6Qda77n fn5A== X-Gm-Message-State: APf1xPB7lziFkk+jEBWA5HVwrex3p4jVD/f5/rlIAKx9zA/nmj6wYHdd zu5rj2MDgxFutRQPWJl0JsIOiw== X-Google-Smtp-Source: AH8x2251nZ1JntYn989nSMCGVq2ZvEhZsV7ahcUXF2KyDcHxSSkTpm1B7FzeFT5ML20srn4ZNiY9QQ== X-Received: by 10.98.33.204 with SMTP id o73mr3860208pfj.54.1519444759327; Fri, 23 Feb 2018 19:59:19 -0800 (PST) Received: from SZX1000114654 ([104.237.91.49]) by smtp.gmail.com with ESMTPSA id 83sm7103065pfj.151.2018.02.23.19.59.16 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 23 Feb 2018 19:59:18 -0800 (PST) From: Guo Heyi X-Google-Original-From: Guo Heyi Date: Sat, 24 Feb 2018 11:59:14 +0800 To: "Ni, Ruiyu" Cc: Guo Heyi , Laszlo Ersek , edk2-devel@lists.01.org, Eric Dong , Ard Biesheuvel , Michael D Kinney , Star Zeng Message-ID: <20180224035914.GD111587@SZX1000114654> 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> MIME-Version: 1.0 In-Reply-To: <56a50ebe-c27e-1a0b-1d96-30b0443fee59@Intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) 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 03:53:18 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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: 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