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:c01::244; helo=mail-pl0-x244.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pl0-x244.google.com (mail-pl0-x244.google.com [IPv6:2607:f8b0:400e:c01::244]) (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 4E13922436924 for ; Fri, 23 Feb 2018 17:04:51 -0800 (PST) Received: by mail-pl0-x244.google.com with SMTP id f23so5872672plr.10 for ; Fri, 23 Feb 2018 17:10:53 -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=KHsprfrqW/u5ErXKiV0Sd7GScsQ/cczZuE+hhUUdrXI=; b=RO9nspR6KxrBjqHm8RXxLHvadHQ5TQHAwlxDkFiiHAHxsrRZYN1EF8l5qSlgkU4GqA O9YNnpqaaps1TTZXLeyFWcunqxvj+82VNAJ62ymsFp1Zh2Gd4XKeRrS3bLxVC4G0u2F/ mCmMuhRn3QlCjyVQ7FeAdS0jay+1GVvulGN5M= 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=KHsprfrqW/u5ErXKiV0Sd7GScsQ/cczZuE+hhUUdrXI=; b=aCdRPjJYAliUzT6hKxdPrBIDqiouzzRzj6Vhe/h+/InI25hJxtdJ6J1mg7lmvmgy0B 0zsQUSZU3vFElsYoiXJ/MH/sED2EAFHqbAbdsabnRHdcYKDbcpMoyyQ0cjRf1XElvcmN Hkv4bRWgHQQ0qK8pXwUsDC8qXrDRWMchKmhqV9WmP+obfB5KJ5vKSH5URkGxme5zhK7u F76C8b6NVPiDflD1YCbks8VAJE6HVkPyhLXz3/n/4Sf+4pVeOrDvlfnZNd+iRxRZpDA3 Zbh18DRe2ary3s7YcLM3w6qSZrHrYc/Q93OnKMEi2CaiRdcctx+e1Ya180I9v2k9slyQ EE0Q== X-Gm-Message-State: APf1xPBI8Pna91IA2kOrU6GhJsKynVU0Tvq9iFEaJeHISR+LniMKAtpg G95HZoQqfQne69fJdJDfDR16+w== X-Google-Smtp-Source: AH8x2260lCcR2mkB+SWNjK0SnUh8cMWwPrXfxxClWGVPimTPx68zW2S7VsAMeJLYFNYroJQEI+WKvQ== X-Received: by 2002:a17:902:b7c1:: with SMTP id v1-v6mr3405912plz.315.1519434652996; Fri, 23 Feb 2018 17:10:52 -0800 (PST) Received: from SZX1000114654 ([104.237.91.49]) by smtp.gmail.com with ESMTPSA id t11sm6089034pfe.17.2018.02.23.17.10.50 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 23 Feb 2018 17:10:52 -0800 (PST) From: Guo Heyi X-Google-Original-From: Guo Heyi Date: Sat, 24 Feb 2018 09:10:48 +0800 To: Laszlo Ersek Cc: Heyi Guo , edk2-devel@lists.01.org, Ruiyu Ni , Eric Dong , Ard Biesheuvel , Michael D Kinney , Star Zeng Message-ID: <20180224011048.GA111587@SZX1000114654> References: <1519376008-110662-1-git-send-email-heyi.guo@linaro.org> <1519376008-110662-2-git-send-email-heyi.guo@linaro.org> MIME-Version: 1.0 In-Reply-To: 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 01:04:51 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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.) > > > > > 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.) That's really a bug. Nice catch :) Will consider the other comments and modify the code accordingly. Thanks, Gary (Heyi Guo) > > > @@ -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? > > ... 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