Re: [PATCH v5 4/6] MdeModulePkg/PciHostBridgeDxe: Add support for address translation

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

On 14 March 2018 at 07:57, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> On 3/7/2018 2:01 PM, Guo Heyi wrote:
>>
>> Thanks. Please let me know if any further changes are needed.
>>
>> Regards,
>>
>> Heyi
>>
>> On Wed, Mar 07, 2018 at 12:30:59PM +0800, Ni, Ruiyu wrote:
>>>
>>> On 3/6/2018 10:44 AM, Guo Heyi wrote:
>>>>
>>>> Hi Ray,
>>>>
>>>> Any comments for v5?
>>>
>>>
>>> Heyi,
>>> Some backward compatibility concerns were received from internal
>>> production
>>> teams. Current change will cause silent failure on old platforms because
>>> TranslationOffset might be random if uninitialized.
>>> I will solve the concern and then send out updates to you, hopefully by
>>> end
>>> of next week.
>>>
>>>>
>>>> Regards,
>>>>
>>>> Heyi
>>>>
>>>> On Thu, Mar 01, 2018 at 02:57:22PM +0800, 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 in protocol interfaces and internal
>>>>> implementations:
>>>>>
>>>>> 1. *Only* the following protocol interfaces assume Address is Device
>>>>>     Address:
>>>>> (1). PciHostBridgeResourceAllocation.GetProposedResources()
>>>>>       Otherwise PCI bus driver cannot set correct address into PCI
>>>>>       BARs.
>>>>> (2). PciRootBridgeIo.Mem.Read() and PciRootBridgeIo.Mem.Write()
>>>>> (3). PciRootBridgeIo.CopyMem()
>>>>> UEFI and PI spec have clear statements for all other protocol
>>>>> interfaces about the address type.
>>>>>
>>>>> 2. Library interfaces and internal implementation:
>>>>> (1). Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
>>>>>       It is easy to check whether the address is below 4G or above 4G.
>>>>> (2). Addresses in PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode are host
>>>>>       address, for they are allocated from GCD.
>>>>> (3). Address passed to PciHostBridgeResourceConflict is host address,
>>>>>       for it comes from PCI_ROOT_BRIDGE_INSTANCE.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.
>>>>>
>>>>> 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:
>>>>>      v5:
>>>>>      - Add check for the alignment of Translation against the BAR
>>>>> alignment
>>>>>        [Ray]
>>>>>      - Keep coding style of comments consistent with the context [Ray]
>>>>>      - Comment change for Base in PCI_RES_NODE [Ray]
>>>>>      - Add macros of TO_HOST_ADDRESS and TO_DEVICE_ADDRESS for address
>>>>> type
>>>>>        conversion (After that we can also simply the comments about the
>>>>>        calculation [Ray]
>>>>>      - Add check for bus translation offset in CreateRootBridge(),
>>>>> making
>>>>>        sure it is zero, and unify code logic for all types of resource
>>>>>        after that [Ray]
>>>>>      - Use GetTranslationByResourceType() to simplify the code in
>>>>>        RootBridgeIoConfiguration() (also fix a bug in previous patch
>>>>>        version of missing a break after case TypePMem64) [Ray]
>>>>>      - Commit message refinement [Ray]
>>>>>      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
>>>>>      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/PciHostBridge.h   |  21 ++++
>>>>>   MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h |   3 +
>>>>>   MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c   | 129
>>>>> +++++++++++++++++---
>>>>>   MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 118
>>>>> ++++++++++++++++--
>>>>>   4 files changed, 245 insertions(+), 26 deletions(-)
>>>>>
>>>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h
>>>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h
>>>>> index 9a8ca21f4819..c2791ea5c2a4 100644
>>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h
>>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h
>>>>> @@ -38,6 +38,13 @@ typedef struct {
>>>>>   #define PCI_HOST_BRIDGE_FROM_THIS(a) CR (a, PCI_HOST_BRIDGE_INSTANCE,
>>>>> ResAlloc, PCI_HOST_BRIDGE_SIGNATURE)
>>>>>   //
>>>>> +// Macros to translate device address to host address and vice versa.
>>>>> According
>>>>> +// to UEFI 2.7, device address = host address + translation offset.
>>>>> +//
>>>>> +#define TO_HOST_ADDRESS(DeviceAddress,TranslationOffset)
>>>>> ((DeviceAddress) - (TranslationOffset))
>>>>> +#define TO_DEVICE_ADDRESS(HostAddress,TranslationOffset)
>>>>> ((HostAddress) + (TranslationOffset))
>>>>> +
>>>>> +//
>>>>>   // Driver Entry Point
>>>>>   //
>>>>>   /**
>>>>> @@ -247,6 +254,20 @@ ResourceConflict (
>>>>>     IN  PCI_HOST_BRIDGE_INSTANCE *HostBridge
>>>>>     );
>>>>> +/**
>>>>> +  This routine gets translation offset from a root bridge instance by
>>>>> resource type.
>>>>> +
>>>>> +  @param RootBridge The Root Bridge Instance for the resources.
>>>>> +  @param ResourceType The Resource Type of the translation offset.
>>>>> +
>>>>> +  @retval The Translation Offset of the specified resource.
>>>>> +**/
>>>>> +UINT64
>>>>> +GetTranslationByResourceType (
>>>>> +  IN  PCI_ROOT_BRIDGE_INSTANCE     *RootBridge,
>>>>> +  IN  PCI_RESOURCE_TYPE            ResourceType
>>>>> +  );
>>>>> +
>>>>>   extern EFI_METRONOME_ARCH_PROTOCOL *mMetronome;
>>>>>   extern EFI_CPU_IO2_PROTOCOL        *mCpuIo;
>>>>>   #endif
>>>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
>>>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
>>>>> index 8612c0c3251b..a6c3739db368 100644
>>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
>>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
>>>>> @@ -38,6 +38,9 @@ typedef enum {
>>>>>   typedef struct {
>>>>>     PCI_RESOURCE_TYPE Type;
>>>>> +  //
>>>>> +  // Base is a host address
>>>>> +  //
>>>>>     UINT64            Base;
>>>>>     UINT64            Length;
>>>>>     UINT64            Alignment;
>>>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>> index 1494848c3e8c..42ded2855c71 100644
>>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>> @@ -33,6 +33,39 @@ EFI_EVENT                   mIoMmuEvent;
>>>>>   VOID                        *mIoMmuRegistration;
>>>>>   /**
>>>>> +  This routine gets translation offset from a root bridge instance by
>>>>> resource type.
>>>>> +
>>>>> +  @param RootBridge The Root Bridge Instance for the resources.
>>>>> +  @param ResourceType The Resource Type of the translation offset.
>>>>> +
>>>>> +  @retval The Translation Offset of the specified resource.
>>>>> +**/
>>>>> +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;
>>>>> +    case TypeBus:
>>>>> +      return RootBridge->Bus.Translation;
>>>>> +    default:
>>>>> +      ASSERT (FALSE);
>>>>> +      return 0;
>>>>> +  }
>>>>> +}
>>>>> +
>>>>> +/**
>>>>>     Ensure the compatibility of an IO space descriptor with the IO
>>>>> aperture.
>>>>>     The IO space descriptor can come from the GCD IO space map, or it
>>>>> can
>>>>> @@ -366,6 +399,7 @@ InitializePciHostBridge (
>>>>>     UINTN                       MemApertureIndex;
>>>>>     BOOLEAN                     ResourceAssigned;
>>>>>     LIST_ENTRY                  *Link;
>>>>> +  UINT64                      HostAddress;
>>>>>     RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount);
>>>>>     if ((RootBridges == NULL) || (RootBridgeCount == 0)) {
>>>>> @@ -411,8 +445,15 @@ InitializePciHostBridge (
>>>>>       }
>>>>>       if (RootBridges[Index].Io.Base <= RootBridges[Index].Io.Limit) {
>>>>> +      //
>>>>> +      // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device
>>>>> address.
>>>>> +      // For GCD resource manipulation, we need to use host address.
>>>>> +      //
>>>>> +      HostAddress = TO_HOST_ADDRESS (RootBridges[Index].Io.Base,
>>>>> +        RootBridges[Index].Io.Translation);
>>>>> +
>>>>>         Status = AddIoSpace (
>>>>> -                 RootBridges[Index].Io.Base,
>>>>> +                 HostAddress,
>>>>>                    RootBridges[Index].Io.Limit -
>>>>> RootBridges[Index].Io.Base + 1
>>>>>                    );
>>>>>         ASSERT_EFI_ERROR (Status);
>>>>> @@ -422,7 +463,7 @@ InitializePciHostBridge (
>>>>>                           EfiGcdIoTypeIo,
>>>>>                           0,
>>>>>                           RootBridges[Index].Io.Limit -
>>>>> RootBridges[Index].Io.Base + 1,
>>>>> -                        &RootBridges[Index].Io.Base,
>>>>> +                        &HostAddress,
>>>>>                           gImageHandle,
>>>>>                           NULL
>>>>>                           );
>>>>> @@ -443,14 +484,20 @@ 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.
>>>>> +        // For GCD resource manipulation, we need to use host address.
>>>>> +        //
>>>>> +        HostAddress = TO_HOST_ADDRESS
>>>>> (MemApertures[MemApertureIndex]->Base,
>>>>> +          MemApertures[MemApertureIndex]->Translation);
>>>>>           Status = AddMemoryMappedIoSpace (
>>>>> -                   MemApertures[MemApertureIndex]->Base,
>>>>> +                   HostAddress,
>>>>>                      MemApertures[MemApertureIndex]->Limit -
>>>>> MemApertures[MemApertureIndex]->Base + 1,
>>>>>                      EFI_MEMORY_UC
>>>>>                      );
>>>>>           ASSERT_EFI_ERROR (Status);
>>>>>           Status = gDS->SetMemorySpaceAttributes (
>>>>> -                        MemApertures[MemApertureIndex]->Base,
>>>>> +                        HostAddress,
>>>>>                           MemApertures[MemApertureIndex]->Limit -
>>>>> MemApertures[MemApertureIndex]->Base + 1,
>>>>>                           EFI_MEMORY_UC
>>>>>                           );
>>>>> @@ -463,7 +510,7 @@ InitializePciHostBridge (
>>>>>                             EfiGcdMemoryTypeMemoryMappedIo,
>>>>>                             0,
>>>>>                             MemApertures[MemApertureIndex]->Limit -
>>>>> MemApertures[MemApertureIndex]->Base + 1,
>>>>> -                          &MemApertures[MemApertureIndex]->Base,
>>>>> +                          &HostAddress,
>>>>>                             gImageHandle,
>>>>>                             NULL
>>>>>                             );
>>>>> @@ -654,6 +701,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));
>>>>> @@ -721,6 +773,7 @@ NotifyPhase (
>>>>>     PCI_RESOURCE_TYPE                     Index2;
>>>>>     BOOLEAN                               ResNodeHandled[TypeMax];
>>>>>     UINT64                                MaxAlignment;
>>>>> +  UINT64                                Translation;
>>>>>     HostBridge = PCI_HOST_BRIDGE_FROM_THIS (This);
>>>>> @@ -822,14 +875,43 @@ NotifyPhase (
>>>>>             BitsOfAlignment = LowBitSet64 (Alignment + 1);
>>>>>             BaseAddress = MAX_UINT64;
>>>>> +          //
>>>>> +          // 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.
>>>>> +          //
>>>>> +          Translation = GetTranslationByResourceType (RootBridge,
>>>>> Index);
>>>>> +          if ((Translation & Alignment) != 0) {
>>>>> +            DEBUG ((DEBUG_ERROR, "[%a:%d] Translation %lx is not
>>>>> aligned to %lx!\n",
>>>>> +              __FUNCTION__, __LINE__, Translation, Alignment
>>>>> +              ));
>>>>> +            ASSERT (FALSE);
>
> 1. Cool! But can you replace ASSERT(FALSE) with
>    ASSERT ((Translation & Alignment) == 0)?
>
>>>>> +            //
>>>>> +            // This may be caused by too large alignment or too small
>>>>> +            // Translation; pick the 1st possibility and return out of
>>>>> resource,
>>>>> +            // which can also go thru the same process for out of
>>>>> resource
>>>>> +            // outside the loop.
>>>>> +            //
>>>>> +            ReturnStatus = EFI_OUT_OF_RESOURCES;
>>>>> +            continue;
>>>>> +          }
>>>>> +
>>>>>             switch (Index) {
>>>>>             case TypeIo:
>>>>> +            //
>>>>> +            // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device
>>>>> address.
>>>>> +            // For AllocateResource is manipulating GCD resource, we
>>>>> need to use
>>>>> +            // host address here.
>>>>> +            //
>>>>>               BaseAddress = AllocateResource (
>>>>>                               FALSE,
>>>>>                               RootBridge->ResAllocNode[Index].Length,
>>>>>                               MIN (15, BitsOfAlignment),
>>>>> -                            ALIGN_VALUE (RootBridge->Io.Base,
>>>>> Alignment + 1),
>>>>> -                            RootBridge->Io.Limit
>>>>> +                            TO_HOST_ADDRESS (ALIGN_VALUE
>>>>> (RootBridge->Io.Base, Alignment + 1),
>>>>> +                              RootBridge->Io.Translation),
>>>>> +                            TO_HOST_ADDRESS (RootBridge->Io.Limit,
>>>>> +                              RootBridge->Io.Translation)
>>>>>                               );
>>>>>               break;
>>>>> @@ -838,8 +920,10 @@ NotifyPhase (
>>>>>                               TRUE,
>>>>>                               RootBridge->ResAllocNode[Index].Length,
>>>>>                               MIN (63, BitsOfAlignment),
>>>>> -                            ALIGN_VALUE (RootBridge->MemAbove4G.Base,
>>>>> Alignment + 1),
>>>>> -                            RootBridge->MemAbove4G.Limit
>>>>> +                            TO_HOST_ADDRESS (ALIGN_VALUE
>>>>> (RootBridge->MemAbove4G.Base, Alignment + 1),
>>>>> +                              RootBridge->MemAbove4G.Translation),
>>>>> +                            TO_HOST_ADDRESS
>>>>> (RootBridge->MemAbove4G.Limit,
>>>>> +                              RootBridge->MemAbove4G.Translation)
>>>>>                               );
>>>>>               if (BaseAddress != MAX_UINT64) {
>>>>>                 break;
>>>>> @@ -853,8 +937,10 @@ NotifyPhase (
>>>>>                               TRUE,
>>>>>                               RootBridge->ResAllocNode[Index].Length,
>>>>>                               MIN (31, BitsOfAlignment),
>>>>> -                            ALIGN_VALUE (RootBridge->Mem.Base,
>>>>> Alignment + 1),
>>>>> -                            RootBridge->Mem.Limit
>>>>> +                            TO_HOST_ADDRESS (ALIGN_VALUE
>>>>> (RootBridge->Mem.Base, Alignment + 1),
>>>>> +                              RootBridge->Mem.Translation),
>>>>> +                            TO_HOST_ADDRESS (RootBridge->Mem.Limit,
>>>>> +                              RootBridge->Mem.Translation)
>>>>>                               );
>>>>>               break;
>>>>> @@ -863,8 +949,10 @@ NotifyPhase (
>>>>>                               TRUE,
>>>>>                               RootBridge->ResAllocNode[Index].Length,
>>>>>                               MIN (63, BitsOfAlignment),
>>>>> -                            ALIGN_VALUE (RootBridge->PMemAbove4G.Base,
>>>>> Alignment + 1),
>>>>> -                            RootBridge->PMemAbove4G.Limit
>>>>> +                            TO_HOST_ADDRESS (ALIGN_VALUE
>>>>> (RootBridge->PMemAbove4G.Base, Alignment + 1),
>>>>> +                              RootBridge->PMemAbove4G.Translation),
>>>>> +                            TO_HOST_ADDRESS
>>>>> (RootBridge->PMemAbove4G.Limit,
>>>>> +                              RootBridge->PMemAbove4G.Translation)
>>>>>                               );
>>>>>               if (BaseAddress != MAX_UINT64) {
>>>>>                 break;
>>>>> @@ -877,8 +965,10 @@ NotifyPhase (
>>>>>                               TRUE,
>>>>>                               RootBridge->ResAllocNode[Index].Length,
>>>>>                               MIN (31, BitsOfAlignment),
>>>>> -                            ALIGN_VALUE (RootBridge->PMem.Base,
>>>>> Alignment + 1),
>>>>> -                            RootBridge->PMem.Limit
>>>>> +                            TO_HOST_ADDRESS (ALIGN_VALUE
>>>>> (RootBridge->PMem.Base, Alignment + 1),
>>>>> +                              RootBridge->PMem.Translation),
>>>>> +                            TO_HOST_ADDRESS (RootBridge->PMem.Limit,
>>>>> +                              RootBridge->PMem.Translation)
>>>>>                               );
>>>>>               break;
>>>>> @@ -1421,7 +1511,14 @@ 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 conversion is
>>>>> needed.
>>>>> +          //
>>>>> +          Descriptor->AddrRangeMin          = TO_DEVICE_ADDRESS
>>>>> (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/PciRootBridgeIo.c
>>>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>>>> index dc06c16dc038..5dd9d257d46d 100644
>>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>>>> @@ -86,12 +86,35 @@ 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""
>>>>>             ));
>>>>> +  //
>>>>> +  // Translation for bus is not supported.
>>>>> +  //
>>>>>     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));
>>>>> +  ASSERT (Bridge->Bus.Translation == 0);
>>>>> +  if (Bridge->Bus.Translation != 0) {
>>>>> +    return NULL;
>>>>> +  }
>
>
> 2. Can you please use the same debug message format for Bus range? I mean to
> print the Translation for Bus as well. Then in the end of debug message, use
> assertion and if-check. This way developer can know what the Bus.Translation
> is from the debug log.
>
>
>>>>> +
>>>>> +  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 +229,12 @@ 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.
>>>>> +      //
>>>>> +      RootBridge->ResAllocNode[Index].Base   = TO_HOST_ADDRESS
>>>>> (Aperture->Base,
>>>>> +        Aperture->Translation);
>>>>>         RootBridge->ResAllocNode[Index].Length = Aperture->Limit -
>>>>> Aperture->Base + 1;
>>>>>         RootBridge->ResAllocNode[Index].Status = ResAllocated;
>>>>>       } else {
>>>>> @@ -403,6 +431,28 @@ RootBridgeIoCheckParameter (
>>>>>     return EFI_SUCCESS;
>>>>>   }
>
>
> 3. Please add the missing function header comments.
>
>
>>>>> +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 +708,25 @@ 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 needs to be a host address
>>>>> instead of
>>>>> +  // device address.
>>>>> +  return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width,
>>>>> +      TO_HOST_ADDRESS (Address, Translation), Count, Buffer);
>>>>>   }
>>>>>   /**
>>>>> @@ -705,13 +767,25 @@ 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 needs to be a host address
>>>>> instead of
>>>>> +  // device address.
>>>>> +  return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width,
>>>>> +      TO_HOST_ADDRESS (Address, Translation), Count, Buffer);
>>>>>   }
>>>>>   /**
>>>>> @@ -746,6 +820,8 @@ RootBridgeIoIoRead (
>>>>>     )
>>>>>   {
>>>>>     EFI_STATUS                                    Status;
>>>>> +  PCI_ROOT_BRIDGE_INSTANCE                      *RootBridge;
>>>>> +
>>>>>     Status = RootBridgeIoCheckParameter (
>>>>>                This, IoOperation, Width,
>>>>>                Address, Count, Buffer
>>>>> @@ -753,7 +829,13 @@ 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 needs to be a host address
>>>>> instead of
>>>>> +  // device address.
>>>>> +  return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width,
>>>>> +      TO_HOST_ADDRESS (Address, RootBridge->Io.Translation), Count,
>>>>> Buffer);
>>>>>   }
>>>>>   /**
>>>>> @@ -788,6 +870,8 @@ RootBridgeIoIoWrite (
>>>>>     )
>>>>>   {
>>>>>     EFI_STATUS                                    Status;
>>>>> +  PCI_ROOT_BRIDGE_INSTANCE                      *RootBridge;
>>>>> +
>>>>>     Status = RootBridgeIoCheckParameter (
>>>>>                This, IoOperation, Width,
>>>>>                Address, Count, Buffer
>>>>> @@ -795,7 +879,13 @@ 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 needs to be a host address
>>>>> instead of
>>>>> +  // device address.
>>>>> +  return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width,
>>>>> +      TO_HOST_ADDRESS (Address, RootBridge->Io.Translation), Count,
>>>>> Buffer);
>>>>>   }
>>>>>   /**
>>>>> @@ -1615,9 +1705,17 @@ 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;
>>>>> +    Descriptor->AddrTranslationOffset = GetTranslationByResourceType (
>>>>> +      RootBridge,
>>>>> +      ResAllocNode->Type
>>>>> +      );
>>>>> +
>>>>>       switch (ResAllocNode->Type) {
>>>>>       case TypeIo:
>>>>> --
>>>>> 2.7.4
>>>>>
>>>
>>>
>>> --
>>> Thanks,
>>> Ray
>
>
>
> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Please modify code according to the above 3 comments and make sure
> the changes pass ECC check:
> BaseTools/Source/Python/Ecc/Ecc.py -t -s \
>  MdeModulePkg/Pci/PciHostBridgeDxe
>

Thanks Ray

Does that apply to the whole series?