public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Guo Heyi <heyi.guo@linaro.org>
To: "Ni, Ruiyu" <ruiyu.ni@Intel.com>
Cc: Guo Heyi <heyi.guo@linaro.org>, Eric Dong <eric.dong@intel.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	edk2-devel@lists.01.org,
	Michael D Kinney <michael.d.kinney@intel.com>,
	Laszlo Ersek <lersek@redhat.com>, Star Zeng <star.zeng@intel.com>
Subject: Re: [RFC v4 1/3] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
Date: Tue, 27 Feb 2018 18:45:01 +0800	[thread overview]
Message-ID: <20180227104501.GD3918@SZX1000114654> (raw)
In-Reply-To: <19deadb3-4d26-914c-c9f8-c6c6716746fa@Intel.com>

On Tue, Feb 27, 2018 at 05:59:48PM +0800, Ni, Ruiyu wrote:
> On 2/27/2018 5:33 PM, Guo Heyi wrote:
> >Hi Ray,
> >
> >Thanks for your comments. It seems comments 10 and 12 are the major issue; let's
> >discuss that first.
> >
> >1. In current implementation, namely PciBusDxe and PciHostBridgeDxe both in
> >MdeModulePkg, Address parameter passed to RootBridgeIoMemRead comes from
> >PciIoMemRead().
> >2. In PciIoMemRead(), Offset is only an offset within selected BAR; then
> >PciIoDevice->PciBar[BarIndex].BaseAddress is added to Offset in
> >PciIoVerifyBarAccess().
> Agree. I thought PciIoMemRead() gets the BAR base address from
> GetBarAttributes().
> 
> >3. So whether the "Address" passed to RootBridgeIoMemRead() is host address or
> >device address depends on the property of
> >PciIoDevice->PciBar[BarIndex].BaseAddress.
> RootBridgeIoMemRead() can choose to accept HOST address or Device address.
> Either can be implemented.
> I am fine to your proposal to use Device Address.
> 
> >4. In PciBusDxe, we can see PciIoDevice->PciBar[BarIndex].BaseAddress simply
> >comes from the value read from BAR register, which is absolutely a device
> >address.
> >5. What we do in patch 3/3 is also to convert the device address in
> >PciBar[BarIndex].BaseAddress to host address before returning to the caller of
> >PciIoGetBarAttributes().
> >
> >Please let me know your comments.
> >
> >For other comments, I have no strong opinion and I can follow the conclusion of
> >the community.
> 
> So the issue (13) (not 12) is closed.
> But what's your opinion to my comment (1): Explicitly add assertion and
> if-check to make sure the alignment meets requirement.

Ah, I thought it is obviously a good advice to place the restriction in the code
rather than in documentation only, so I didn't reply it separately :)

Thanks,

Heyi

> 
> Ard,
> I provided the detailed comments because I felt the code is almost
> finalized.
> So the detailed comments can avoid creating more versions of patches.
> 
> For the EMPTY comments and STATIC modifier, I created the first version of
> this driver, so I'd like to make the coding style in this driver is
> consistent.
> But if you insist to use a different style, I am fine as long as the coding
> style rule allows.
> 
> Thanks,
> Ray
> 
> >
> >Thanks and regards,
> >
> >Heyi
> >
> >On Tue, Feb 27, 2018 at 04:48:47PM +0800, Ni, Ruiyu wrote:
> >>On 2/27/2018 10:09 AM, Heyi Guo wrote:
> >>>PCI address translation is necessary for some non-x86 platforms. On
> >>>such platforms, address value (denoted as "device address" or "address
> >>>in PCI view") set to PCI BAR registers in configuration space might be
> >>>different from the address which is used by CPU to access the
> >>>registers in memory BAR or IO BAR spaces (denoted as "host address" or
> >>>"address in CPU view"). The difference between the two addresses is
> >>>called "Address Translation Offset" or simply "translation", and can
> >>>be represented by "Address Translation Offset" in ACPI QWORD Address
> >>>Space Descriptor (Offset 0x1E). However UEFI and ACPI differs on the
> >>>definitions of QWORD Address Space Descriptor, and we will follow UEFI
> >>>definition on UEFI protocols, such as PCI root bridge IO protocol and
> >>>PCI IO protocol. In UEFI 2.7, "Address Translation Offset" is "Offset
> >>>to apply to the Starting address to convert it to a PCI address". This
> >>>means:
> >>>
> >>>1. Translation = device address - host address.
> >>>
> >>>2. PciRootBridgeIo->Configuration should return CPU view address, as
> >>>well as PciIo->GetBarAttributes.
> >>>
> >>>Summary of addresses used:
> >>>
> >>>1. Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address, for
> >>>it is easy to check whether the address is below 4G or above 4G.
> >>>
> >>>2. Address returned by
> >>>EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL::GetProposedResources
> >>>is device address, or else PCI bus driver cannot set correct address
> >>>into PCI BAR registers.
> >>>
> >>>3. Address returned by PciRootBridgeIo->Configuration is host address
> >>>per UEFI 2.7 definition.
> >>>
> >>>4. Addresses used in GCD manipulation are host address.
> >>>
> >>>5. Addresses in ResAllocNode of PCI_ROOT_BRIDGE_INSTANCE are host
> >>>address, for they are allocated from GCD.
> >>>
> >>>6. Address passed to PciHostBridgeResourceConflict is host address,
> >>>for it comes from ResAllocNode.
> >>>
> >>>RESTRICTION: to simplify the situation, we require the alignment of
> >>>Translation must be larger than any BAR alignment in the same root
> >>>bridge, so that resource allocation alignment can be applied to both
> >>>device address and host address.
> >>(1). I'd like to see this restriction be reflected in code.
> >>Both assertion and if-check are needed to ensure debug firmware hang
> >>and release firmware return fail status from PciHostBridge driver.
> >>
> >>>
> >>>Contributed-under: TianoCore Contribution Agreement 1.1
> >>>Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> >>>Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >>>Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>>Cc: Star Zeng <star.zeng@intel.com>
> >>>Cc: Eric Dong <eric.dong@intel.com>
> >>>Cc: Laszlo Ersek <lersek@redhat.com>
> >>>Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >>>---
> >>>
> >>>Notes:
> >>>     v4:
> >>>     - Add ASSERT (FALSE) to default branch in GetTranslationByResourceType
> >>>       [Laszlo]
> >>>     - Fix bug when passing BaseAddress to gDS->AllocateIoSpace and
> >>>       gDS->AllocateMemorySpace [Laszlo]
> >>>     - Add comment for applying alignment to both device address and host
> >>>       address, and add NOTE for the alignment requirement of Translation,
> >>>       as well as in commit message [Laszlo][Ray]
> >>>     - Improve indention for the code in CreateRootBridge [Laszlo]
> >>>     - Improve comment for Translation in PCI_ROOT_BRIDGE_APERTURE
> >>>       definition [Laszlo]
> >>>     - Ignore translation of bus in CreateRootBridge
> >>>
> >>>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h |   2 +
> >>>  MdeModulePkg/Include/Library/PciHostBridgeLib.h         |  14 +++
> >>>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c   |  85 +++++++++++---
> >>>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 122 +++++++++++++++++---
> >>>  4 files changed, 194 insertions(+), 29 deletions(-)
> >>>
> >>>diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
> >>>index 8612c0c3251b..662c2dd59529 100644
> >>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
> >>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
> >>>@@ -38,6 +38,8 @@ typedef enum {
> >>>  typedef struct {
> >>>    PCI_RESOURCE_TYPE Type;
> >>>+  // Base is a host address instead of a device address when address translation
> >>>+  // exists and host address != device address
> >>(2). Coding style issue. The comments need to be in style like:
> >>//[EMPTY]
> >>// xxxx
> >>//[EMPTY]
> >>And how about just simply say Base is a host address. No matter address
> >>translation exists or not, Base is a host address actually.
> >>
> >>>    UINT64            Base;
> >>>    UINT64            Length;
> >>>    UINT64            Alignment;
> >>>diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> >>>index d42e9ecdd763..c842a4152e85 100644
> >>>--- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> >>>+++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> >>>@@ -20,8 +20,22 @@
> >>>  // (Base > Limit) indicates an aperture is not available.
> >>>  //
> >>>  typedef struct {
> >>>+  // Base and Limit are the device address instead of host address when
> >>>+  // Translation is not zero
> >>(3). Similar comments to (2).
> >>>    UINT64 Base;
> >>>    UINT64 Limit;
> >>>+  // According to UEFI 2.7, Device Address = Host Address + Translation,
> >>>+  // so Translation = Device Address - Host Address.
> >>>+  // On platforms where Translation is not zero, the subtraction is probably to
> >>>+  // be performed with UINT64 wrap-around semantics, for we may translate an
> >>>+  // above-4G host address into a below-4G device address for legacy PCIe device
> >>>+  // compatibility.
> >>>+  // NOTE: The alignment of Translation is required to be larger than any BAR
> >>>+  // alignment in the same root bridge, so that the same alignment can be
> >>>+  // applied to both device address and host address, which simplifies the
> >>>+  // situation and makes the current resource allocation code in generic PCI
> >>>+  // host bridge driver still work.
> >>(4). Still I'd like to see the alignment requirement be reflected in code
> >>through assertion and if-check.
> >>
> >>>+  UINT64 Translation;
> >>>  } PCI_ROOT_BRIDGE_APERTURE;
> >>>  typedef struct {
> >>>diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>index 1494848c3e8c..1e65faee9084 100644
> >>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>@@ -32,6 +32,30 @@ EDKII_IOMMU_PROTOCOL        *mIoMmuProtocol;
> >>>  EFI_EVENT                   mIoMmuEvent;
> >>>  VOID                        *mIoMmuRegistration;
> >>>+STATIC
> >>(5). Can you remove STATIC? personal preference:)
> >>>+UINT64
> >>>+GetTranslationByResourceType (
> >>>+  IN  PCI_ROOT_BRIDGE_INSTANCE     *RootBridge,
> >>>+  IN  PCI_RESOURCE_TYPE            ResourceType
> >>>+  )
> >>>+{
> >>>+  switch (ResourceType) {
> >>>+    case TypeIo:
> >>>+      return RootBridge->Io.Translation;
> >>>+    case TypeMem32:
> >>>+      return RootBridge->Mem.Translation;
> >>>+    case TypePMem32:
> >>>+      return RootBridge->PMem.Translation;
> >>>+    case TypeMem64:
> >>>+      return RootBridge->MemAbove4G.Translation;
> >>>+    case TypePMem64:
> >>>+      return RootBridge->PMemAbove4G.Translation;
> >>>+    default:
> >>>+      ASSERT (FALSE);
> >>>+      return 0;
> >>>+  }
> >>>+}
> >>>+
> >>>  /**
> >>>    Ensure the compatibility of an IO space descriptor with the IO aperture.
> >>>@@ -366,6 +390,7 @@ InitializePciHostBridge (
> >>>    UINTN                       MemApertureIndex;
> >>>    BOOLEAN                     ResourceAssigned;
> >>>    LIST_ENTRY                  *Link;
> >>>+  UINT64                      TempHostAddress;
> >>(6). Just use name HostAddress?
> >>>    RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount);
> >>>    if ((RootBridges == NULL) || (RootBridgeCount == 0)) {
> >>>@@ -411,18 +436,24 @@ InitializePciHostBridge (
> >>>      }
> >>>      if (RootBridges[Index].Io.Base <= RootBridges[Index].Io.Limit) {
> >>>+      // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
> >>>+      // According to UEFI 2.7, device address = host address + Translation.
> >>>+      // For GCD resource manipulation, we should use host address, so
> >>>+      // Translation is subtracted from device address here.
> >>(7). Please adjust the comment style to have two EMPTY line of comment above
> >>and below. Same to all comments in the patch.
> >>>        Status = AddIoSpace (
> >>>-                 RootBridges[Index].Io.Base,
> >>>+                 RootBridges[Index].Io.Base - RootBridges[Index].Io.Translation,
> >>>                   RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1
> >>>                   );
> >>>        ASSERT_EFI_ERROR (Status);
> >>>        if (ResourceAssigned) {
> >>>+        TempHostAddress = RootBridges[Index].Io.Base -
> >>>+          RootBridges[Index].Io.Translation;
> >>>          Status = gDS->AllocateIoSpace (
> >>>                          EfiGcdAllocateAddress,
> >>>                          EfiGcdIoTypeIo,
> >>>                          0,
> >>>                          RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1,
> >>>-                        &RootBridges[Index].Io.Base,
> >>>+                        &TempHostAddress,
> >>>                          gImageHandle,
> >>>                          NULL
> >>>                          );
> >>>@@ -443,14 +474,18 @@ InitializePciHostBridge (
> >>>      for (MemApertureIndex = 0; MemApertureIndex < ARRAY_SIZE (MemApertures); MemApertureIndex++) {
> >>>        if (MemApertures[MemApertureIndex]->Base <= MemApertures[MemApertureIndex]->Limit) {
> >>>+        // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
> >>>+        // According to UEFI 2.7, device address = host address + Translation.
> >>>+        // For GCD resource manipulation, we should use host address, so
> >>>+        // Translation is subtracted from device address here.
> >>>          Status = AddMemoryMappedIoSpace (
> >>>-                   MemApertures[MemApertureIndex]->Base,
> >>>+                   MemApertures[MemApertureIndex]->Base - MemApertures[MemApertureIndex]->Translation,
> >>>                     MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
> >>>                     EFI_MEMORY_UC
> >>>                     );
> >>>          ASSERT_EFI_ERROR (Status);
> >>>          Status = gDS->SetMemorySpaceAttributes (
> >>>-                        MemApertures[MemApertureIndex]->Base,
> >>>+                        MemApertures[MemApertureIndex]->Base - MemApertures[MemApertureIndex]->Translation,
> >>>                          MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
> >>>                          EFI_MEMORY_UC
> >>>                          );
> >>>@@ -458,12 +493,14 @@ InitializePciHostBridge (
> >>>            DEBUG ((DEBUG_WARN, "PciHostBridge driver failed to set EFI_MEMORY_UC to MMIO aperture - %r.\n", Status));
> >>>          }
> >>>          if (ResourceAssigned) {
> >>>+          TempHostAddress = MemApertures[MemApertureIndex]->Base -
> >>>+            MemApertures[MemApertureIndex]->Translation;
> >>>            Status = gDS->AllocateMemorySpace (
> >>>                            EfiGcdAllocateAddress,
> >>>                            EfiGcdMemoryTypeMemoryMappedIo,
> >>>                            0,
> >>>                            MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
> >>>-                          &MemApertures[MemApertureIndex]->Base,
> >>>+                          &TempHostAddress,
> >>>                            gImageHandle,
> >>>                            NULL
> >>>                            );
> >>>@@ -654,6 +691,11 @@ AllocateResource (
> >>>    if (BaseAddress < Limit) {
> >>>      //
> >>>      // Have to make sure Aligment is handled since we are doing direct address allocation
> >>>+    // Strictly speaking, alignment requirement should be applied to device
> >>>+    // address instead of host address which is used in GCD manipulation below,
> >>>+    // but as we restrict the alignment of Translation to be larger than any BAR
> >>>+    // alignment in the root bridge, we can simplify the situation and consider
> >>>+    // the same alignment requirement is also applied to host address.
> >>>      //
> >>>      BaseAddress = ALIGN_VALUE (BaseAddress, LShiftU64 (1, BitsOfAlignment));
> >>>@@ -824,12 +866,17 @@ NotifyPhase (
> >>>            switch (Index) {
> >>>            case TypeIo:
> >>>+            // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
> >>>+            // According to UEFI 2.7, device address = host address + Translation.
> >>>+            // For AllocateResource is manipulating GCD resource, we should use
> >>>+            // host address here, so Translation is subtracted from Base and
> >>>+            // Limit.
> >>>              BaseAddress = AllocateResource (
> >>>                              FALSE,
> >>>                              RootBridge->ResAllocNode[Index].Length,
> >>>                              MIN (15, BitsOfAlignment),
> >>>-                            ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1),
> >>>-                            RootBridge->Io.Limit
> >>>+                            ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1) - RootBridge->Io.Translation,
> >>>+                            RootBridge->Io.Limit - RootBridge->Io.Translation
> >>>                              );
> >>>              break;
> >>>@@ -838,8 +885,8 @@ NotifyPhase (
> >>(8). Add assertion and if-check here to make sure alignment of Translation
> >>is no less than the Alignment.
> >>>                              TRUE,
> >>>                              RootBridge->ResAllocNode[Index].Length,
> >>>                              MIN (63, BitsOfAlignment),
> >>>-                            ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1),
> >>>-                            RootBridge->MemAbove4G.Limit
> >>>+                            ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1) - RootBridge->MemAbove4G.Translation,
> >>>+                            RootBridge->MemAbove4G.Limit - RootBridge->MemAbove4G.Translation
> >>>                              );
> >>>              if (BaseAddress != MAX_UINT64) {
> >>>                break;
> >>>@@ -853,8 +900,8 @@ NotifyPhase (
> >>>                              TRUE,
> >>>                              RootBridge->ResAllocNode[Index].Length,
> >>>                              MIN (31, BitsOfAlignment),
> >>>-                            ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1),
> >>>-                            RootBridge->Mem.Limit
> >>>+                            ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1) - RootBridge->Mem.Translation,
> >>>+                            RootBridge->Mem.Limit - RootBridge->Mem.Translation
> >>>                              );
> >>>              break;
> >>>@@ -863,8 +910,8 @@ NotifyPhase (
> >>>                              TRUE,
> >>>                              RootBridge->ResAllocNode[Index].Length,
> >>>                              MIN (63, BitsOfAlignment),
> >>>-                            ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1),
> >>>-                            RootBridge->PMemAbove4G.Limit
> >>>+                            ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1) - RootBridge->PMemAbove4G.Translation,
> >>>+                            RootBridge->PMemAbove4G.Limit - RootBridge->PMemAbove4G.Translation
> >>>                              );
> >>>              if (BaseAddress != MAX_UINT64) {
> >>>                break;
> >>>@@ -877,8 +924,8 @@ NotifyPhase (
> >>>                              TRUE,
> >>>                              RootBridge->ResAllocNode[Index].Length,
> >>>                              MIN (31, BitsOfAlignment),
> >>>-                            ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1),
> >>>-                            RootBridge->PMem.Limit
> >>>+                            ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1) - RootBridge->PMem.Translation,
> >>>+                            RootBridge->PMem.Limit - RootBridge->PMem.Translation
> >>>                              );
> >>>              break;
> >>>@@ -1152,6 +1199,7 @@ StartBusEnumeration (
> >>>        Descriptor->AddrSpaceGranularity  = 0;
> >>>        Descriptor->AddrRangeMin          = RootBridge->Bus.Base;
> >>>        Descriptor->AddrRangeMax          = 0;
> >>>+      // Ignore translation offset for bus
> >>(9). Per PI spec on StartBusEnumeration, AddrRangeMax is ignored. So I don't
> >>think we need add comments here.
> >>>        Descriptor->AddrTranslationOffset = 0;
> >>>        Descriptor->AddrLen               = RootBridge->Bus.Limit - RootBridge->Bus.Base + 1;
> >>>@@ -1421,7 +1469,12 @@ GetProposedResources (
> >>>            Descriptor->Desc                  = ACPI_ADDRESS_SPACE_DESCRIPTOR;
> >>>            Descriptor->Len                   = sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3;;
> >>>            Descriptor->GenFlag               = 0;
> >>>-          Descriptor->AddrRangeMin          = RootBridge->ResAllocNode[Index].Base;
> >>>+          // AddrRangeMin in Resource Descriptor here should be device address
> >>>+          // instead of host address, or else PCI bus driver cannot set correct
> >>>+          // address into PCI BAR registers.
> >>>+          // Base in ResAllocNode is a host address, so Translation is added.
> >>>+          Descriptor->AddrRangeMin          = RootBridge->ResAllocNode[Index].Base +
> >>>+              GetTranslationByResourceType (RootBridge, Index);
> >>(10). Do you think defining a PciHostBridgeDxe internal macro
> >>TO_HOST_ADDRESS(DeviceAddress, Offset) and TO_DEVICE_ADDRESS(HostAddress,
> >>Offset) is better?
> >>
> >>>            Descriptor->AddrRangeMax          = 0;
> >>>            Descriptor->AddrTranslationOffset = (ResStatus == ResAllocated) ? EFI_RESOURCE_SATISFIED : PCI_RESOURCE_LESS;
> >>>            Descriptor->AddrLen               = RootBridge->ResAllocNode[Index].Length;
> >>>diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>>index dc06c16dc038..edaa0d48a441 100644
> >>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>>@@ -86,12 +86,28 @@ CreateRootBridge (
> >>>            (Bridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) != 0 ? L"CombineMemPMem " : L"",
> >>>            (Bridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_MEM64_DECODE) != 0 ? L"Mem64Decode" : L""
> >>>            ));
> >>>+  // We don't see any scenario for bus translation, so translation for bus is just ignored.
> >>>    DEBUG ((EFI_D_INFO, "           Bus: %lx - %lx\n", Bridge->Bus.Base, Bridge->Bus.Limit));
> >>>-  DEBUG ((EFI_D_INFO, "            Io: %lx - %lx\n", Bridge->Io.Base, Bridge->Io.Limit));
> >>>-  DEBUG ((EFI_D_INFO, "           Mem: %lx - %lx\n", Bridge->Mem.Base, Bridge->Mem.Limit));
> >>>-  DEBUG ((EFI_D_INFO, "    MemAbove4G: %lx - %lx\n", Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit));
> >>>-  DEBUG ((EFI_D_INFO, "          PMem: %lx - %lx\n", Bridge->PMem.Base, Bridge->PMem.Limit));
> >>>-  DEBUG ((EFI_D_INFO, "   PMemAbove4G: %lx - %lx\n", Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit));
> >>>+  DEBUG ((
> >>>+    DEBUG_INFO, "            Io: %lx - %lx Translation=%lx\n",
> >>>+    Bridge->Io.Base, Bridge->Io.Limit, Bridge->Io.Translation
> >>>+    ));
> >>>+  DEBUG ((
> >>>+    DEBUG_INFO, "           Mem: %lx - %lx Translation=%lx\n",
> >>>+    Bridge->Mem.Base, Bridge->Mem.Limit, Bridge->Mem.Translation
> >>>+    ));
> >>>+  DEBUG ((
> >>>+    DEBUG_INFO, "    MemAbove4G: %lx - %lx Translation=%lx\n",
> >>>+    Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit, Bridge->MemAbove4G.Translation
> >>>+    ));
> >>>+  DEBUG ((
> >>>+    DEBUG_INFO, "          PMem: %lx - %lx Translation=%lx\n",
> >>>+    Bridge->PMem.Base, Bridge->PMem.Limit, Bridge->PMem.Translation
> >>>+    ));
> >>>+  DEBUG ((
> >>>+    DEBUG_INFO, "   PMemAbove4G: %lx - %lx Translation=%lx\n",
> >>>+    Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit, Bridge->PMemAbove4G.Translation
> >>>+    ));
> >>>    //
> >>>    // Make sure Mem and MemAbove4G apertures are valid
> >>>@@ -206,7 +222,15 @@ CreateRootBridge (
> >>>      }
> >>>      RootBridge->ResAllocNode[Index].Type     = Index;
> >>>      if (Bridge->ResourceAssigned && (Aperture->Limit >= Aperture->Base)) {
> >>>-      RootBridge->ResAllocNode[Index].Base   = Aperture->Base;
> >>>+      // Ignore translation for bus
> >>(11). How about we only make sure the TranslationOffset is 0 for TypeBus
> >>when getting the aperture from PciHostBridgeLib.
> >>So that later logic doesn't need to do special handling for the TypeBus.
> >>This way the code can be simpler.
> >>>+      if (Index == TypeBus) {
> >>>+        RootBridge->ResAllocNode[Index].Base   = Aperture->Base;
> >>>+      } else {
> >>>+        // Base in ResAllocNode is a host address, while Base in Aperture is a
> >>>+        // device address, so translation needs to be subtracted.
> >>>+        RootBridge->ResAllocNode[Index].Base   = Aperture->Base -
> >>>+          Aperture->Translation;
> >>(12). Still. Do you think using TO_HOST_ADDRESS() is better?
> >>>+      }
> >>>        RootBridge->ResAllocNode[Index].Length = Aperture->Limit - Aperture->Base + 1;
> >>>        RootBridge->ResAllocNode[Index].Status = ResAllocated;
> >>>      } else {
> >>>@@ -403,6 +427,28 @@ RootBridgeIoCheckParameter (
> >>>    return EFI_SUCCESS;
> >>>  }
> >>>+EFI_STATUS
> >>>+RootBridgeIoGetMemTranslationByAddress (
> >>>+  IN PCI_ROOT_BRIDGE_INSTANCE               *RootBridge,
> >>>+  IN UINT64                                 Address,
> >>>+  IN OUT UINT64                             *Translation
> >>>+  )
> >>>+{
> >>>+  if (Address >= RootBridge->Mem.Base && Address <= RootBridge->Mem.Limit) {
> >>>+    *Translation = RootBridge->Mem.Translation;
> >>>+  } else if (Address >= RootBridge->PMem.Base && Address <= RootBridge->PMem.Limit) {
> >>>+    *Translation = RootBridge->PMem.Translation;
> >>>+  } else if (Address >= RootBridge->MemAbove4G.Base && Address <= RootBridge->MemAbove4G.Limit) {
> >>>+    *Translation = RootBridge->MemAbove4G.Translation;
> >>>+  } else if (Address >= RootBridge->PMemAbove4G.Base && Address <= RootBridge->PMemAbove4G.Limit) {
> >>>+    *Translation = RootBridge->PMemAbove4G.Translation;
> >>>+  } else {
> >>>+    return EFI_INVALID_PARAMETER;
> >>>+  }
> >>>+
> >>>+  return EFI_SUCCESS;
> >>>+}
> >>>+
> >>>  /**
> >>>    Polls an address in memory mapped I/O space until an exit condition is met,
> >>>    or a timeout occurs.
> >>>@@ -658,13 +704,24 @@ RootBridgeIoMemRead (
> >>>    )
> >>>  {
> >>>    EFI_STATUS                             Status;
> >>>+  PCI_ROOT_BRIDGE_INSTANCE               *RootBridge;
> >>>+  UINT64                                 Translation;
> >>>    Status = RootBridgeIoCheckParameter (This, MemOperation, Width, Address,
> >>>                                         Count, Buffer);
> >>>    if (EFI_ERROR (Status)) {
> >>>      return Status;
> >>>    }
> >>>-  return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer);
> >>>+
> >>>+  RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> >>>+  Status = RootBridgeIoGetMemTranslationByAddress (RootBridge, Address, &Translation);
> >>(13). Why do you think the Address is a Device Address?
> >>PciIo.GetBarAttributes() returns a HOST Address and the Translation Offset.
> >>I didn't see you change the PciIoMemRead() implementation.
> >>So it means the same HOST Address is passed into the this function.
> >>
> >>>+  if (EFI_ERROR (Status)) {
> >>>+    return Status;
> >>>+  }
> >>>+
> >>>+  // Address passed to CpuIo->Mem.Read should be a host address instead of
> >>>+  // device address, so Translation should be subtracted.
> >>>+  return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - Translation, Count, Buffer);
> >>>  }
> >>>  /**
> >>>@@ -705,13 +762,24 @@ RootBridgeIoMemWrite (
> >>>    )
> >>>  {
> >>>    EFI_STATUS                             Status;
> >>>+  PCI_ROOT_BRIDGE_INSTANCE               *RootBridge;
> >>>+  UINT64                                 Translation;
> >>>    Status = RootBridgeIoCheckParameter (This, MemOperation, Width, Address,
> >>>                                         Count, Buffer);
> >>>    if (EFI_ERROR (Status)) {
> >>>      return Status;
> >>>    }
> >>>-  return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer);
> >>>+
> >>>+  RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> >>>+  Status = RootBridgeIoGetMemTranslationByAddress (RootBridge, Address, &Translation);
> >>>+  if (EFI_ERROR (Status)) {
> >>>+    return Status;
> >>>+  }
> >>>+
> >>>+  // Address passed to CpuIo->Mem.Write should be a host address instead of
> >>>+  // device address, so Translation should be subtracted.
> >>>+  return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - Translation, Count, Buffer);
> >>(14). Same as (13)
> >>>  }
> >>>  /**
> >>>@@ -746,6 +814,8 @@ RootBridgeIoIoRead (
> >>>    )
> >>>  {
> >>>    EFI_STATUS                                    Status;
> >>>+  PCI_ROOT_BRIDGE_INSTANCE                      *RootBridge;
> >>>+
> >>>    Status = RootBridgeIoCheckParameter (
> >>>               This, IoOperation, Width,
> >>>               Address, Count, Buffer
> >>>@@ -753,7 +823,12 @@ RootBridgeIoIoRead (
> >>>    if (EFI_ERROR (Status)) {
> >>>      return Status;
> >>>    }
> >>>-  return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer);
> >>>+
> >>>+  RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> >>>+
> >>>+  // Address passed to CpuIo->Io.Read should be a host address instead of
> >>>+  // device address, so Translation should be subtracted.
> >>>+  return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - RootBridge->Io.Translation, Count, Buffer);
> >>(15). Same as (13)
> >>>  }
> >>>  /**
> >>>@@ -788,6 +863,8 @@ RootBridgeIoIoWrite (
> >>>    )
> >>>  {
> >>>    EFI_STATUS                                    Status;
> >>>+  PCI_ROOT_BRIDGE_INSTANCE                      *RootBridge;
> >>>+
> >>>    Status = RootBridgeIoCheckParameter (
> >>>               This, IoOperation, Width,
> >>>               Address, Count, Buffer
> >>>@@ -795,7 +872,12 @@ RootBridgeIoIoWrite (
> >>>    if (EFI_ERROR (Status)) {
> >>>      return Status;
> >>>    }
> >>>-  return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer);
> >>>+
> >>>+  RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> >>>+
> >>>+  // Address passed to CpuIo->Io.Write should be a host address instead of
> >>>+  // device address, so Translation should be subtracted.
> >>>+  return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - RootBridge->Io.Translation, Count, Buffer);
> >>(16). Same as (13)
> >>>  }
> >>>  /**
> >>>@@ -1615,25 +1697,39 @@ RootBridgeIoConfiguration (
> >>>      Descriptor->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR;
> >>>      Descriptor->Len  = sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3;
> >>>+    // According to UEFI 2.7, RootBridgeIo->Configuration should return address
> >>>+    // range in CPU view (host address), and ResAllocNode->Base is already a CPU
> >>>+    // view address (host address).
> >>>      Descriptor->AddrRangeMin  = ResAllocNode->Base;
> >>>      Descriptor->AddrRangeMax  = ResAllocNode->Base + ResAllocNode->Length - 1;
> >>>      Descriptor->AddrLen       = ResAllocNode->Length;
> >>>      switch (ResAllocNode->Type) {
> >>>      case TypeIo:
> >>>-      Descriptor->ResType       = ACPI_ADDRESS_SPACE_TYPE_IO;
> >>>+      Descriptor->ResType               = ACPI_ADDRESS_SPACE_TYPE_IO;
> >>>+      Descriptor->AddrTranslationOffset = RootBridge->Io.Translation;
> >>(17). Can GetTranslationByResourceType() be used to simplify the code?
> >>>        break;
> >>>      case TypePMem32:
> >>>-      Descriptor->SpecificFlag = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
> >>>+      Descriptor->SpecificFlag          = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
> >>>+      Descriptor->AddrTranslationOffset = RootBridge->PMem.Translation;
> >>>+      Descriptor->ResType               = ACPI_ADDRESS_SPACE_TYPE_MEM;
> >>>+      Descriptor->AddrSpaceGranularity  = 32;
> >>>+      break;
> >>>+
> >>>      case TypeMem32:
> >>>+      Descriptor->AddrTranslationOffset = RootBridge->Mem.Translation;
> >>>        Descriptor->ResType               = ACPI_ADDRESS_SPACE_TYPE_MEM;
> >>>        Descriptor->AddrSpaceGranularity  = 32;
> >>>        break;
> >>>      case TypePMem64:
> >>>-      Descriptor->SpecificFlag = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
> >>>+      Descriptor->SpecificFlag          = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
> >>>+      Descriptor->AddrTranslationOffset = RootBridge->PMemAbove4G.Translation;
> >>>+      Descriptor->ResType               = ACPI_ADDRESS_SPACE_TYPE_MEM;
> >>>+      Descriptor->AddrSpaceGranularity  = 64;
> >>>      case TypeMem64:
> >>>+      Descriptor->AddrTranslationOffset = RootBridge->MemAbove4G.Translation;
> >>>        Descriptor->ResType               = ACPI_ADDRESS_SPACE_TYPE_MEM;
> >>>        Descriptor->AddrSpaceGranularity  = 64;
> >>>        break;
> >>>
> >>
> >>(18). Please remember to run BaseTools/Source/Python/Ecc/Ecc.py on the
> >>PciHostBridgeDxe driver to make sure coding style matches the requirement.
> >>
> >>-- 
> >>Thanks,
> >>Ray
> >_______________________________________________
> >edk2-devel mailing list
> >edk2-devel@lists.01.org
> >https://lists.01.org/mailman/listinfo/edk2-devel
> >
> 
> 
> -- 
> Thanks,
> Ray


  parent reply	other threads:[~2018-02-27 10:39 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-27  2:09 [RFC v4 0/3] Add translation support to generic PciHostBridge Heyi Guo
2018-02-27  2:09 ` [RFC v4 1/3] MdeModulePkg/PciHostBridgeDxe: Add support for address translation Heyi Guo
2018-02-27  8:48   ` Ni, Ruiyu
2018-02-27  8:55     ` Ard Biesheuvel
2018-02-27  9:33     ` Guo Heyi
2018-02-27  9:59       ` Ni, Ruiyu
2018-02-27 10:14         ` Ard Biesheuvel
2018-02-27 10:45         ` Guo Heyi [this message]
2018-02-27 11:44           ` Guo Heyi
2018-02-28  2:29             ` Ni, Ruiyu
2018-02-28  7:25               ` Ni, Ruiyu
2018-02-28  7:53                 ` Guo Heyi
2018-02-28  9:39                   ` Laszlo Ersek
2018-02-28 23:31                     ` Guo Heyi
2018-03-01  3:56     ` Guo Heyi
2018-03-01  4:44       ` Ni, Ruiyu
2018-02-27  2:09 ` [RFC v4 2/3] MdeModulePkg/PciBus: convert host address to device address Heyi Guo
2018-02-27  2:09 ` [RFC v4 3/3] MdeModulePkg/PciBus: return CPU address for GetBarAttributes Heyi Guo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180227104501.GD3918@SZX1000114654 \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox