From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.65; helo=mga03.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (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 C49EB22436923 for ; Tue, 27 Feb 2018 01:53:44 -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 orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Feb 2018 01:59:50 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,400,1515484800"; d="scan'208";a="33942641" 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 01:59:48 -0800 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> From: "Ni, Ruiyu" Message-ID: <19deadb3-4d26-914c-c9f8-c6c6716746fa@Intel.com> Date: Tue, 27 Feb 2018 17:59:48 +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: <20180227093334.GA3918@SZX1000114654> 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: Tue, 27 Feb 2018 09:53:45 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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. 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