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

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

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 :)

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;
> >+          }
> >+