Re: [RFC v3 1/3] MdeModulePkg/PciHostBridgeDxe: Add support for address translation

[Guo Heyi <heyi.guo@linaro.org>]

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 <heyi.guo@linaro.org>
> >>>Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >>>Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>>Cc: Star Zeng <star.zeng@intel.com>
> >>>Cc: Eric Dong <eric.dong@intel.com>
> >>>Cc: Laszlo Ersek <lersek@redhat.com>
> >>>Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >>>---
> >>>  .../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