From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::241; helo=mail-it0-x241.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x241.google.com (mail-it0-x241.google.com [IPv6:2607:f8b0:4001:c0b::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4E06B22352296 for ; Tue, 27 Feb 2018 23:47:46 -0800 (PST) Received: by mail-it0-x241.google.com with SMTP id j7so2403005ita.3 for ; Tue, 27 Feb 2018 23:53:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:date:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=8gszHQA+cb+oF137cHqj+vN8+QaUpj8nBvkDXMPjReE=; b=eChmftDJD7bhvV9OX7u4OL9ljDnYY0z3W2bGpyCJbjfChKBLKNb889QrTf1PN7xpio s5J6VJL0UC/XMhMDeuDyn0ppkGj6kXPnYF+JB2v+fvdRuw+QMuPbKRA5Wf48ojhtU22R MNkcE2VVyIDKonUFb6dgP4oUc9u7PcNA6XQJU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=8gszHQA+cb+oF137cHqj+vN8+QaUpj8nBvkDXMPjReE=; b=dQOkmF9g5VqI+J8VHdiOWdi9eN8/KUFklTO4ORYSnTh+UxtRzYjhoD5S96Lxvi/U+V m/+fXLsI1RdLiz4RLXEN/Qh4+FFXoGzj5rXmhI5oAVIaC5IWO/zctPmJjph29WP1Ygxt /OWpgQtvSOBjZvc00ZgX0cGgXiGp+Vd+Ztmc7OoqsziJqelN5xVIlgXKm8CURtuOLnPx MbEh2VnoBrEkHgWkeZ2rjksTXK+lhcpcEaa4LKRfg9k10qHYpGIf+WjHx926ZRLBCRFv FgC/VTqn3jfu10A1IHlkxpHtoVfwqaYFabXklD7FYK9/pa8YPTyAk/6JkaEZAvFyX5du yn6A== X-Gm-Message-State: APf1xPCJl55kUXYVg6feDXkDb5ZVJkVIbKXsgIy9e+TViZPn4hoFS31U mknlZqQENFN4O9XeIm7lkjy0sg== X-Google-Smtp-Source: AH8x226C+TMy5QTvVb6VFW3oTdzTjX5x9H8/HHU31loxa57cBlIyi0DK/FQCK1YBXo8xRTM1kjCIHA== X-Received: by 10.36.118.142 with SMTP id z136mr19155800itb.61.1519804432914; Tue, 27 Feb 2018 23:53:52 -0800 (PST) Received: from SZX1000114654 ([45.56.152.115]) by smtp.gmail.com with ESMTPSA id l42sm928500ioi.53.2018.02.27.23.53.50 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 27 Feb 2018 23:53:52 -0800 (PST) From: Guo Heyi X-Google-Original-From: Guo Heyi Date: Wed, 28 Feb 2018 15:53:48 +0800 To: "Ni, Ruiyu" Cc: Guo Heyi , Eric Dong , Ard Biesheuvel , edk2-devel@lists.01.org, Michael D Kinney , Laszlo Ersek , Star Zeng Message-ID: <20180228075348.GB27903@SZX1000114654> References: <1519697389-3525-1-git-send-email-heyi.guo@linaro.org> <1519697389-3525-2-git-send-email-heyi.guo@linaro.org> <563e76b9-ebbc-be09-d306-8dcb86f12112@Intel.com> <20180227093334.GA3918@SZX1000114654> <19deadb3-4d26-914c-c9f8-c6c6716746fa@Intel.com> <20180227104501.GD3918@SZX1000114654> <20180227114435.GH3918@SZX1000114654> <55c50dc9-4ce4-60a6-342c-35cd2f8d37cd@Intel.com> <17035d2f-3a5d-b3be-149d-d7179f173677@Intel.com> MIME-Version: 1.0 In-Reply-To: <17035d2f-3a5d-b3be-149d-d7179f173677@Intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) Subject: Re: [RFC v4 1/3] MdeModulePkg/PciHostBridgeDxe: Add support for address translation X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Feb 2018 07:47:47 -0000 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit On Wed, Feb 28, 2018 at 03:25:03PM +0800, Ni, Ruiyu wrote: > On 2/28/2018 10:29 AM, Ni, Ruiyu wrote: > >On 2/27/2018 7:44 PM, Guo Heyi wrote: > >>Hi all, > >> > >>I believe we have come to conclusion for major parts, so I'm going to > >>send out > >>formal patches instead of RFC from now on, as well as applying changes to > >>the existing implementations of PciHostBridgeLib in the tree. > >> > >>Thanks for your comments and advice. > > > >Heyi, > >I saw you had a very good summary about where the Device Address is used, > >where the Host Address is used. > >I think it would be better if you could split them into two categories: > >Protocol interfaces and internal implementations. > > > >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. > > > >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. > > > >Thanks, > >Ray > > > >> > >>Heyi > >> > >>On Tue, Feb 27, 2018 at 06:45:01PM +0800, Guo Heyi wrote: > >>>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 > >>>>>>>Cc: Ruiyu Ni > >>>>>>>Cc: Ard Biesheuvel > >>>>>>>Cc: Star Zeng > >>>>>>>Cc: Eric Dong > >>>>>>>Cc: Laszlo Ersek > >>>>>>>Cc: Michael D Kinney > >>>>>>>--- > >>>>>>> > >>>>>>>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 > > > > > Heyi, > My understanding is this whole change is backward compatible. > Which means an old version of PciHostBridgeLib + new PciHostBridgeLib.h + > new PciHostBridgeDxe driver won't cause build failure. > right? Not really; as Laszlo indicated in one mail, some implementations of PciHostBridgeLib uses temporary PCI_ROOT_BRIDGE_APERTURE variables and only initialize Base and Limit fields of the variables, like the code in https://github.com/tianocore/edk2/blob/master/CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c#L315 and https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c#L216 and https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c#L189 I'm also preparing the patches for these PciHostBridgeLib instances in edk2 tree, which are expected to be committed after PciHostBridgeLib.h change and before PciHostBridge driver change. Thanks, Heyi > > -- > Thanks, > Ray