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

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

On 3/15/2018 1:33 PM, Guo Heyi wrote:
> On Thu, Mar 15, 2018 at 01:26:04PM +0800, Ni, Ruiyu wrote:
>> On 3/15/2018 12:00 PM, 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>
>>> Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
>>> 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:
>>>      v6:
>>>      - Change ASSERT(FALSE) to ASSERT ((Translation & Alignment) == 0) and
>>>        move ASSERT before if-check [Ray]
>>>      - Print translation of bus and then assert [Ray]
>>>      - Add function header for RootBridgeIoGetMemTranslationByAddress()
>>>        [Ray]
>>>      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 | 135 ++++++++++++++++++--
>>>   4 files changed, 261 insertions(+), 27 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..5342efd89275 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);
>>> +          ASSERT ((Translation & Alignment) == 0);
>>
>> Can you move the assertion to the body of if below, and below the DEBUG
>> message? So that when assertion happens, developer can know the actual value
>> of Translation and Alignment.
>>
>>
>>> +          if ((Translation & Alignment) != 0) {
>>> +            DEBUG ((DEBUG_ERROR, "[%a:%d] Translation %lx is not aligned to %lx!\n",
>>> +              __FUNCTION__, __LINE__, Translation, Alignment
>>> +              ));
>>
>> ASSERT ((Translation & Alignment) == 0); // Move to here.
> 
> No problem. I just found we often place ASSERT before if-check in other places
> and supposed it is the common style :)There is no such requirement I think.
Easy debugging is more important:)
It's not easy to directly attach a debugger and check the variable value 
in firmware world.

> 
> Thanks,
> Heyi
> 
>>
>> With the above change, Reviewed-by: Ruiyu Ni <ruiyu.ni@Intel.com>
>>
>>> +            //
>>> +            // 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;
>>> +          }
>>> +


-- 
Thanks,
Ray