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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 23 February 2018 at 15:05, Laszlo Ersek <lersek@redhat.com> 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?
>

Yes. Although DMA and MMIO space are different in this regard (and
this series does not cover the former), the purpose of having 32-bit
addressable BAR space and/or 32-bit addressable memory for DMA is to
ensure that the addresses can be generated/decoded by the PCI device.

So it is appropriate for the distinction between 'below' and 'above' 4
GB to be made based on the PCI address.

> ... 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 <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"?
>

+1

>> +      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.
>   //
>

Quite the opposite, in fact. Search for 'horror vacui' in the coding
style document.

That does not mean I agree with that document, and I am perfectly fine
with both styles.

>>        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?
>
> ... 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".
>

+1

>
> I've made some comments, but I admit my understanding is quite limited;
> sorry about that.
>
> Thanks
> Laszlo