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

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 2/27/2018 10:09 AM, Heyi Guo wrote:
> PCI address translation is necessary for some non-x86 platforms. On
> such platforms, address value (denoted as "device address" or "address
> in PCI view") set to PCI BAR registers in configuration space might be
> different from the address which is used by CPU to access the
> registers in memory BAR or IO BAR spaces (denoted as "host address" or
> "address in CPU view"). The difference between the two addresses is
> called "Address Translation Offset" or simply "translation", and can
> be represented by "Address Translation Offset" in ACPI QWORD Address
> Space Descriptor (Offset 0x1E). However UEFI and ACPI differs on the
> definitions of QWORD Address Space Descriptor, and we will follow UEFI
> definition on UEFI protocols, such as PCI root bridge IO protocol and
> PCI IO protocol. In UEFI 2.7, "Address Translation Offset" is "Offset
> to apply to the Starting address to convert it to a PCI address". This
> means:
> 
> 1. Translation = device address - host address.
> 
> 2. PciRootBridgeIo->Configuration should return CPU view address, as
> well as PciIo->GetBarAttributes.
> 
> Summary of addresses used:
> 
> 1. Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address, for
> it is easy to check whether the address is below 4G or above 4G.
> 
> 2. Address returned by
> EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL::GetProposedResources
> is device address, or else PCI bus driver cannot set correct address
> into PCI BAR registers.
> 
> 3. Address returned by PciRootBridgeIo->Configuration is host address
> per UEFI 2.7 definition.
> 
> 4. Addresses used in GCD manipulation are host address.
> 
> 5. Addresses in ResAllocNode of PCI_ROOT_BRIDGE_INSTANCE are host
> address, for they are allocated from GCD.
> 
> 6. Address passed to PciHostBridgeResourceConflict is host address,
> for it comes from ResAllocNode.
> 
> RESTRICTION: to simplify the situation, we require the alignment of
> Translation must be larger than any BAR alignment in the same root
> bridge, so that resource allocation alignment can be applied to both
> device address and host address.
(1). I'd like to see this restriction be reflected in code.
Both assertion and if-check are needed to ensure debug firmware hang
and release firmware return fail status from PciHostBridge driver.

> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Heyi Guo <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>
> ---
> 
> Notes:
>      v4:
>      - Add ASSERT (FALSE) to default branch in GetTranslationByResourceType
>        [Laszlo]
>      - Fix bug when passing BaseAddress to gDS->AllocateIoSpace and
>        gDS->AllocateMemorySpace [Laszlo]
>      - Add comment for applying alignment to both device address and host
>        address, and add NOTE for the alignment requirement of Translation,
>        as well as in commit message [Laszlo][Ray]
>      - Improve indention for the code in CreateRootBridge [Laszlo]
>      - Improve comment for Translation in PCI_ROOT_BRIDGE_APERTURE
>        definition [Laszlo]
>      - Ignore translation of bus in CreateRootBridge
> 
>   MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h |   2 +
>   MdeModulePkg/Include/Library/PciHostBridgeLib.h         |  14 +++
>   MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c   |  85 +++++++++++---
>   MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 122 +++++++++++++++++---
>   4 files changed, 194 insertions(+), 29 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
> index 8612c0c3251b..662c2dd59529 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
> @@ -38,6 +38,8 @@ typedef enum {
>   
>   typedef struct {
>     PCI_RESOURCE_TYPE Type;
> +  // Base is a host address instead of a device address when address translation
> +  // exists and host address != device address
(2). Coding style issue. The comments need to be in style like:
//[EMPTY]
// xxxx
//[EMPTY]
And how about just simply say Base is a host address. No matter address
translation exists or not, Base is a host address actually.

>     UINT64            Base;
>     UINT64            Length;
>     UINT64            Alignment;
> diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> index d42e9ecdd763..c842a4152e85 100644
> --- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> +++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> @@ -20,8 +20,22 @@
>   // (Base > Limit) indicates an aperture is not available.
>   //
>   typedef struct {
> +  // Base and Limit are the device address instead of host address when
> +  // Translation is not zero
(3). Similar comments to (2).
>     UINT64 Base;
>     UINT64 Limit;
> +  // According to UEFI 2.7, Device Address = Host Address + Translation,
> +  // so Translation = Device Address - Host Address.
> +  // On platforms where Translation is not zero, the subtraction is probably to
> +  // be performed with UINT64 wrap-around semantics, for we may translate an
> +  // above-4G host address into a below-4G device address for legacy PCIe device
> +  // compatibility.
> +  // NOTE: The alignment of Translation is required to be larger than any BAR
> +  // alignment in the same root bridge, so that the same alignment can be
> +  // applied to both device address and host address, which simplifies the
> +  // situation and makes the current resource allocation code in generic PCI
> +  // host bridge driver still work.
(4). Still I'd like to see the alignment requirement be reflected in 
code through assertion and if-check.

> +  UINT64 Translation;
>   } PCI_ROOT_BRIDGE_APERTURE;
>   
>   typedef struct {
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> index 1494848c3e8c..1e65faee9084 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> @@ -32,6 +32,30 @@ EDKII_IOMMU_PROTOCOL        *mIoMmuProtocol;
>   EFI_EVENT                   mIoMmuEvent;
>   VOID                        *mIoMmuRegistration;
>   
> +STATIC
(5). Can you remove STATIC? personal preference:)
> +UINT64
> +GetTranslationByResourceType (
> +  IN  PCI_ROOT_BRIDGE_INSTANCE     *RootBridge,
> +  IN  PCI_RESOURCE_TYPE            ResourceType
> +  )
> +{
> +  switch (ResourceType) {
> +    case TypeIo:
> +      return RootBridge->Io.Translation;
> +    case TypeMem32:
> +      return RootBridge->Mem.Translation;
> +    case TypePMem32:
> +      return RootBridge->PMem.Translation;
> +    case TypeMem64:
> +      return RootBridge->MemAbove4G.Translation;
> +    case TypePMem64:
> +      return RootBridge->PMemAbove4G.Translation;
> +    default:
> +      ASSERT (FALSE);
> +      return 0;
> +  }
> +}
> +
>   /**
>     Ensure the compatibility of an IO space descriptor with the IO aperture.
>   
> @@ -366,6 +390,7 @@ InitializePciHostBridge (
>     UINTN                       MemApertureIndex;
>     BOOLEAN                     ResourceAssigned;
>     LIST_ENTRY                  *Link;
> +  UINT64                      TempHostAddress;
(6). Just use name HostAddress?
>   
>     RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount);
>     if ((RootBridges == NULL) || (RootBridgeCount == 0)) {
> @@ -411,18 +436,24 @@ InitializePciHostBridge (
>       }
>   
>       if (RootBridges[Index].Io.Base <= RootBridges[Index].Io.Limit) {
> +      // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
> +      // According to UEFI 2.7, device address = host address + Translation.
> +      // For GCD resource manipulation, we should use host address, so
> +      // Translation is subtracted from device address here.
(7). Please adjust the comment style to have two EMPTY line of comment 
above and below. Same to all comments in the patch.
>         Status = AddIoSpace (
> -                 RootBridges[Index].Io.Base,
> +                 RootBridges[Index].Io.Base - RootBridges[Index].Io.Translation,
>                    RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1
>                    );
>         ASSERT_EFI_ERROR (Status);
>         if (ResourceAssigned) {
> +        TempHostAddress = RootBridges[Index].Io.Base -
> +          RootBridges[Index].Io.Translation;
>           Status = gDS->AllocateIoSpace (
>                           EfiGcdAllocateAddress,
>                           EfiGcdIoTypeIo,
>                           0,
>                           RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1,
> -                        &RootBridges[Index].Io.Base,
> +                        &TempHostAddress,
>                           gImageHandle,
>                           NULL
>                           );
> @@ -443,14 +474,18 @@ InitializePciHostBridge (
>   
>       for (MemApertureIndex = 0; MemApertureIndex < ARRAY_SIZE (MemApertures); MemApertureIndex++) {
>         if (MemApertures[MemApertureIndex]->Base <= MemApertures[MemApertureIndex]->Limit) {
> +        // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
> +        // According to UEFI 2.7, device address = host address + Translation.
> +        // For GCD resource manipulation, we should use host address, so
> +        // Translation is subtracted from device address here.
>           Status = AddMemoryMappedIoSpace (
> -                   MemApertures[MemApertureIndex]->Base,
> +                   MemApertures[MemApertureIndex]->Base - MemApertures[MemApertureIndex]->Translation,
>                      MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
>                      EFI_MEMORY_UC
>                      );
>           ASSERT_EFI_ERROR (Status);
>           Status = gDS->SetMemorySpaceAttributes (
> -                        MemApertures[MemApertureIndex]->Base,
> +                        MemApertures[MemApertureIndex]->Base - MemApertures[MemApertureIndex]->Translation,
>                           MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
>                           EFI_MEMORY_UC
>                           );
> @@ -458,12 +493,14 @@ InitializePciHostBridge (
>             DEBUG ((DEBUG_WARN, "PciHostBridge driver failed to set EFI_MEMORY_UC to MMIO aperture - %r.\n", Status));
>           }
>           if (ResourceAssigned) {
> +          TempHostAddress = MemApertures[MemApertureIndex]->Base -
> +            MemApertures[MemApertureIndex]->Translation;
>             Status = gDS->AllocateMemorySpace (
>                             EfiGcdAllocateAddress,
>                             EfiGcdMemoryTypeMemoryMappedIo,
>                             0,
>                             MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
> -                          &MemApertures[MemApertureIndex]->Base,
> +                          &TempHostAddress,
>                             gImageHandle,
>                             NULL
>                             );
> @@ -654,6 +691,11 @@ AllocateResource (
>     if (BaseAddress < Limit) {
>       //
>       // Have to make sure Aligment is handled since we are doing direct address allocation
> +    // Strictly speaking, alignment requirement should be applied to device
> +    // address instead of host address which is used in GCD manipulation below,
> +    // but as we restrict the alignment of Translation to be larger than any BAR
> +    // alignment in the root bridge, we can simplify the situation and consider
> +    // the same alignment requirement is also applied to host address.
>       //
>       BaseAddress = ALIGN_VALUE (BaseAddress, LShiftU64 (1, BitsOfAlignment));
>   
> @@ -824,12 +866,17 @@ NotifyPhase (
>   
>             switch (Index) {
>             case TypeIo:
> +            // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
> +            // According to UEFI 2.7, device address = host address + Translation.
> +            // For AllocateResource is manipulating GCD resource, we should use
> +            // host address here, so Translation is subtracted from Base and
> +            // Limit.
>               BaseAddress = AllocateResource (
>                               FALSE,
>                               RootBridge->ResAllocNode[Index].Length,
>                               MIN (15, BitsOfAlignment),
> -                            ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1),
> -                            RootBridge->Io.Limit
> +                            ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1) - RootBridge->Io.Translation,
> +                            RootBridge->Io.Limit - RootBridge->Io.Translation
>                               );
>               break;
>   
> @@ -838,8 +885,8 @@ NotifyPhase (
(8). Add assertion and if-check here to make sure alignment of 
Translation is no less than the Alignment.
>                               TRUE,
>                               RootBridge->ResAllocNode[Index].Length,
>                               MIN (63, BitsOfAlignment),
> -                            ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1),
> -                            RootBridge->MemAbove4G.Limit
> +                            ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1) - RootBridge->MemAbove4G.Translation,
> +                            RootBridge->MemAbove4G.Limit - RootBridge->MemAbove4G.Translation
>                               );
>               if (BaseAddress != MAX_UINT64) {
>                 break;
> @@ -853,8 +900,8 @@ NotifyPhase (
>                               TRUE,
>                               RootBridge->ResAllocNode[Index].Length,
>                               MIN (31, BitsOfAlignment),
> -                            ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1),
> -                            RootBridge->Mem.Limit
> +                            ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1) - RootBridge->Mem.Translation,
> +                            RootBridge->Mem.Limit - RootBridge->Mem.Translation
>                               );
>               break;
>   
> @@ -863,8 +910,8 @@ NotifyPhase (
>                               TRUE,
>                               RootBridge->ResAllocNode[Index].Length,
>                               MIN (63, BitsOfAlignment),
> -                            ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1),
> -                            RootBridge->PMemAbove4G.Limit
> +                            ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1) - RootBridge->PMemAbove4G.Translation,
> +                            RootBridge->PMemAbove4G.Limit - RootBridge->PMemAbove4G.Translation
>                               );
>               if (BaseAddress != MAX_UINT64) {
>                 break;
> @@ -877,8 +924,8 @@ NotifyPhase (
>                               TRUE,
>                               RootBridge->ResAllocNode[Index].Length,
>                               MIN (31, BitsOfAlignment),
> -                            ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1),
> -                            RootBridge->PMem.Limit
> +                            ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1) - RootBridge->PMem.Translation,
> +                            RootBridge->PMem.Limit - RootBridge->PMem.Translation
>                               );
>               break;
>   
> @@ -1152,6 +1199,7 @@ StartBusEnumeration (
>         Descriptor->AddrSpaceGranularity  = 0;
>         Descriptor->AddrRangeMin          = RootBridge->Bus.Base;
>         Descriptor->AddrRangeMax          = 0;
> +      // Ignore translation offset for bus
(9). Per PI spec on StartBusEnumeration, AddrRangeMax is ignored. So I 
don't think we need add comments here.
>         Descriptor->AddrTranslationOffset = 0;
>         Descriptor->AddrLen               = RootBridge->Bus.Limit - RootBridge->Bus.Base + 1;
>   
> @@ -1421,7 +1469,12 @@ GetProposedResources (
>             Descriptor->Desc                  = ACPI_ADDRESS_SPACE_DESCRIPTOR;
>             Descriptor->Len                   = sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3;;
>             Descriptor->GenFlag               = 0;
> -          Descriptor->AddrRangeMin          = RootBridge->ResAllocNode[Index].Base;
> +          // AddrRangeMin in Resource Descriptor here should be device address
> +          // instead of host address, or else PCI bus driver cannot set correct
> +          // address into PCI BAR registers.
> +          // Base in ResAllocNode is a host address, so Translation is added.
> +          Descriptor->AddrRangeMin          = RootBridge->ResAllocNode[Index].Base +
> +              GetTranslationByResourceType (RootBridge, Index);
(10). Do you think defining a PciHostBridgeDxe internal macro 
TO_HOST_ADDRESS(DeviceAddress, Offset) and 
TO_DEVICE_ADDRESS(HostAddress, Offset) is better?

>             Descriptor->AddrRangeMax          = 0;
>             Descriptor->AddrTranslationOffset = (ResStatus == ResAllocated) ? EFI_RESOURCE_SATISFIED : PCI_RESOURCE_LESS;
>             Descriptor->AddrLen               = RootBridge->ResAllocNode[Index].Length;
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index dc06c16dc038..edaa0d48a441 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -86,12 +86,28 @@ CreateRootBridge (
>             (Bridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) != 0 ? L"CombineMemPMem " : L"",
>             (Bridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_MEM64_DECODE) != 0 ? L"Mem64Decode" : L""
>             ));
> +  // We don't see any scenario for bus translation, so translation for bus is just ignored.
>     DEBUG ((EFI_D_INFO, "           Bus: %lx - %lx\n", Bridge->Bus.Base, Bridge->Bus.Limit));
> -  DEBUG ((EFI_D_INFO, "            Io: %lx - %lx\n", Bridge->Io.Base, Bridge->Io.Limit));
> -  DEBUG ((EFI_D_INFO, "           Mem: %lx - %lx\n", Bridge->Mem.Base, Bridge->Mem.Limit));
> -  DEBUG ((EFI_D_INFO, "    MemAbove4G: %lx - %lx\n", Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit));
> -  DEBUG ((EFI_D_INFO, "          PMem: %lx - %lx\n", Bridge->PMem.Base, Bridge->PMem.Limit));
> -  DEBUG ((EFI_D_INFO, "   PMemAbove4G: %lx - %lx\n", Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit));
> +  DEBUG ((
> +    DEBUG_INFO, "            Io: %lx - %lx Translation=%lx\n",
> +    Bridge->Io.Base, Bridge->Io.Limit, Bridge->Io.Translation
> +    ));
> +  DEBUG ((
> +    DEBUG_INFO, "           Mem: %lx - %lx Translation=%lx\n",
> +    Bridge->Mem.Base, Bridge->Mem.Limit, Bridge->Mem.Translation
> +    ));
> +  DEBUG ((
> +    DEBUG_INFO, "    MemAbove4G: %lx - %lx Translation=%lx\n",
> +    Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit, Bridge->MemAbove4G.Translation
> +    ));
> +  DEBUG ((
> +    DEBUG_INFO, "          PMem: %lx - %lx Translation=%lx\n",
> +    Bridge->PMem.Base, Bridge->PMem.Limit, Bridge->PMem.Translation
> +    ));
> +  DEBUG ((
> +    DEBUG_INFO, "   PMemAbove4G: %lx - %lx Translation=%lx\n",
> +    Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit, Bridge->PMemAbove4G.Translation
> +    ));
>   
>     //
>     // Make sure Mem and MemAbove4G apertures are valid
> @@ -206,7 +222,15 @@ CreateRootBridge (
>       }
>       RootBridge->ResAllocNode[Index].Type     = Index;
>       if (Bridge->ResourceAssigned && (Aperture->Limit >= Aperture->Base)) {
> -      RootBridge->ResAllocNode[Index].Base   = Aperture->Base;
> +      // Ignore translation for bus
(11). How about we only make sure the TranslationOffset is 0 for TypeBus 
when getting the aperture from PciHostBridgeLib.
So that later logic doesn't need to do special handling for the TypeBus.
This way the code can be simpler.
> +      if (Index == TypeBus) {
> +        RootBridge->ResAllocNode[Index].Base   = Aperture->Base;
> +      } else {
> +        // Base in ResAllocNode is a host address, while Base in Aperture is a
> +        // device address, so translation needs to be subtracted.
> +        RootBridge->ResAllocNode[Index].Base   = Aperture->Base -
> +          Aperture->Translation;
(12). Still. Do you think using TO_HOST_ADDRESS() is better?
> +      }
>         RootBridge->ResAllocNode[Index].Length = Aperture->Limit - Aperture->Base + 1;
>         RootBridge->ResAllocNode[Index].Status = ResAllocated;
>       } else {
> @@ -403,6 +427,28 @@ RootBridgeIoCheckParameter (
>     return EFI_SUCCESS;
>   }
>   
> +EFI_STATUS
> +RootBridgeIoGetMemTranslationByAddress (
> +  IN PCI_ROOT_BRIDGE_INSTANCE               *RootBridge,
> +  IN UINT64                                 Address,
> +  IN OUT UINT64                             *Translation
> +  )
> +{
> +  if (Address >= RootBridge->Mem.Base && Address <= RootBridge->Mem.Limit) {
> +    *Translation = RootBridge->Mem.Translation;
> +  } else if (Address >= RootBridge->PMem.Base && Address <= RootBridge->PMem.Limit) {
> +    *Translation = RootBridge->PMem.Translation;
> +  } else if (Address >= RootBridge->MemAbove4G.Base && Address <= RootBridge->MemAbove4G.Limit) {
> +    *Translation = RootBridge->MemAbove4G.Translation;
> +  } else if (Address >= RootBridge->PMemAbove4G.Base && Address <= RootBridge->PMemAbove4G.Limit) {
> +    *Translation = RootBridge->PMemAbove4G.Translation;
> +  } else {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
>   /**
>     Polls an address in memory mapped I/O space until an exit condition is met,
>     or a timeout occurs.
> @@ -658,13 +704,24 @@ RootBridgeIoMemRead (
>     )
>   {
>     EFI_STATUS                             Status;
> +  PCI_ROOT_BRIDGE_INSTANCE               *RootBridge;
> +  UINT64                                 Translation;
>   
>     Status = RootBridgeIoCheckParameter (This, MemOperation, Width, Address,
>                                          Count, Buffer);
>     if (EFI_ERROR (Status)) {
>       return Status;
>     }
> -  return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer);
> +
> +  RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> +  Status = RootBridgeIoGetMemTranslationByAddress (RootBridge, Address, &Translation);
(13). Why do you think the Address is a Device Address?
PciIo.GetBarAttributes() returns a HOST Address and the Translation Offset.
I didn't see you change the PciIoMemRead() implementation.
So it means the same HOST Address is passed into the this function.

> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  // Address passed to CpuIo->Mem.Read should be a host address instead of
> +  // device address, so Translation should be subtracted.
> +  return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - Translation, Count, Buffer);
>   }
>   
>   /**
> @@ -705,13 +762,24 @@ RootBridgeIoMemWrite (
>     )
>   {
>     EFI_STATUS                             Status;
> +  PCI_ROOT_BRIDGE_INSTANCE               *RootBridge;
> +  UINT64                                 Translation;
>   
>     Status = RootBridgeIoCheckParameter (This, MemOperation, Width, Address,
>                                          Count, Buffer);
>     if (EFI_ERROR (Status)) {
>       return Status;
>     }
> -  return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer);
> +
> +  RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> +  Status = RootBridgeIoGetMemTranslationByAddress (RootBridge, Address, &Translation);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  // Address passed to CpuIo->Mem.Write should be a host address instead of
> +  // device address, so Translation should be subtracted.
> +  return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - Translation, Count, Buffer);
(14). Same as (13)
>   }
>   
>   /**
> @@ -746,6 +814,8 @@ RootBridgeIoIoRead (
>     )
>   {
>     EFI_STATUS                                    Status;
> +  PCI_ROOT_BRIDGE_INSTANCE                      *RootBridge;
> +
>     Status = RootBridgeIoCheckParameter (
>                This, IoOperation, Width,
>                Address, Count, Buffer
> @@ -753,7 +823,12 @@ RootBridgeIoIoRead (
>     if (EFI_ERROR (Status)) {
>       return Status;
>     }
> -  return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer);
> +
> +  RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> +
> +  // Address passed to CpuIo->Io.Read should be a host address instead of
> +  // device address, so Translation should be subtracted.
> +  return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - RootBridge->Io.Translation, Count, Buffer);
(15). Same as (13)
>   }
>   
>   /**
> @@ -788,6 +863,8 @@ RootBridgeIoIoWrite (
>     )
>   {
>     EFI_STATUS                                    Status;
> +  PCI_ROOT_BRIDGE_INSTANCE                      *RootBridge;
> +
>     Status = RootBridgeIoCheckParameter (
>                This, IoOperation, Width,
>                Address, Count, Buffer
> @@ -795,7 +872,12 @@ RootBridgeIoIoWrite (
>     if (EFI_ERROR (Status)) {
>       return Status;
>     }
> -  return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer);
> +
> +  RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> +
> +  // Address passed to CpuIo->Io.Write should be a host address instead of
> +  // device address, so Translation should be subtracted.
> +  return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - RootBridge->Io.Translation, Count, Buffer);
(16). Same as (13)
>   }
>   
>   /**
> @@ -1615,25 +1697,39 @@ RootBridgeIoConfiguration (
>   
>       Descriptor->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR;
>       Descriptor->Len  = sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3;
> +    // According to UEFI 2.7, RootBridgeIo->Configuration should return address
> +    // range in CPU view (host address), and ResAllocNode->Base is already a CPU
> +    // view address (host address).
>       Descriptor->AddrRangeMin  = ResAllocNode->Base;
>       Descriptor->AddrRangeMax  = ResAllocNode->Base + ResAllocNode->Length - 1;
>       Descriptor->AddrLen       = ResAllocNode->Length;
>       switch (ResAllocNode->Type) {
>   
>       case TypeIo:
> -      Descriptor->ResType       = ACPI_ADDRESS_SPACE_TYPE_IO;
> +      Descriptor->ResType               = ACPI_ADDRESS_SPACE_TYPE_IO;
> +      Descriptor->AddrTranslationOffset = RootBridge->Io.Translation;
(17). Can GetTranslationByResourceType() be used to simplify the code?
>         break;
>   
>       case TypePMem32:
> -      Descriptor->SpecificFlag = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
> +      Descriptor->SpecificFlag          = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
> +      Descriptor->AddrTranslationOffset = RootBridge->PMem.Translation;
> +      Descriptor->ResType               = ACPI_ADDRESS_SPACE_TYPE_MEM;
> +      Descriptor->AddrSpaceGranularity  = 32;
> +      break;
> +
>       case TypeMem32:
> +      Descriptor->AddrTranslationOffset = RootBridge->Mem.Translation;
>         Descriptor->ResType               = ACPI_ADDRESS_SPACE_TYPE_MEM;
>         Descriptor->AddrSpaceGranularity  = 32;
>         break;
>   
>       case TypePMem64:
> -      Descriptor->SpecificFlag = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
> +      Descriptor->SpecificFlag          = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
> +      Descriptor->AddrTranslationOffset = RootBridge->PMemAbove4G.Translation;
> +      Descriptor->ResType               = ACPI_ADDRESS_SPACE_TYPE_MEM;
> +      Descriptor->AddrSpaceGranularity  = 64;
>       case TypeMem64:
> +      Descriptor->AddrTranslationOffset = RootBridge->MemAbove4G.Translation;
>         Descriptor->ResType               = ACPI_ADDRESS_SPACE_TYPE_MEM;
>         Descriptor->AddrSpaceGranularity  = 64;
>         break;
> 

(18). Please remember to run BaseTools/Source/Python/Ecc/Ecc.py on the 
PciHostBridgeDxe driver to make sure coding style matches the requirement.

-- 
Thanks,
Ray