From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.136; helo=mga12.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id CB47D22352287 for ; Tue, 27 Feb 2018 23:18:58 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Feb 2018 23:25:05 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,404,1515484800"; d="scan'208";a="34219943" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.13]) ([10.239.9.13]) by fmsmga001.fm.intel.com with ESMTP; 27 Feb 2018 23:25:03 -0800 From: "Ni, Ruiyu" To: Guo Heyi Cc: Eric Dong , Ard Biesheuvel , edk2-devel@lists.01.org, Michael D Kinney , Laszlo Ersek , Star Zeng 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> Message-ID: <17035d2f-3a5d-b3be-149d-d7179f173677@Intel.com> Date: Wed, 28 Feb 2018 15:25:03 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <55c50dc9-4ce4-60a6-342c-35cd2f8d37cd@Intel.com> 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:18:59 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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? -- Thanks, Ray