On 23 February 2018 at 15:05, Laszlo Ersek <[email protected]> wrote: > Thanks for writing up the details! > > Comments: > > On 02/23/18 09:53, 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. > > OK, this looks like a sensible choice to me. It means that the "apply" > term used in the UEFI spec means "add", which (I think) is the "natural" > interpretation. > >> 2. PciRootBridgeIo->Configuration should return CPU view address, as >> well as PciIo->GetBarAttributes. > > OK. > >> >> 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. > > I agree that PciHostBridgeLib instances that do not set a nonzero > Translation need not care about the difference. > > (1) Still, can you confirm this is a "natural" choice? Were the > DmaAbove4G, MemAbove4G and PMemAbove4G fields originally introduced with > the device (PCI) view in mind? >
Yes. Although DMA and MMIO space are different in this regard (and this series does not cover the former), the purpose of having 32-bit addressable BAR space and/or 32-bit addressable memory for DMA is to ensure that the addresses can be generated/decoded by the PCI device. So it is appropriate for the distinction between 'below' and 'above' 4 GB to be made based on the PCI address. > ... I also meant to raise a concern about the InitializePciHostBridge() > function where we call AddIoSpace() and AddMemoryMappedIoSpace() -- > which end up manipulating GCD -- with data from > PCI_ROOT_BRIDGE_APERTURE. I can see you modify those call sites in the > patch, so that's good. (I do have more comments on the actual > implementation.) > >> >> 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. >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Heyi Guo <[email protected]> >> Cc: Ruiyu Ni <[email protected]> >> Cc: Ard Biesheuvel <[email protected]> >> Cc: Star Zeng <[email protected]> >> Cc: Eric Dong <[email protected]> >> Cc: Laszlo Ersek <[email protected]> >> Cc: Michael D Kinney <[email protected]> >> --- >> .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 74 +++++++++++--- >> .../Bus/Pci/PciHostBridgeDxe/PciHostResource.h | 2 + >> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 112 >> ++++++++++++++++++--- >> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 8 ++ >> 4 files changed, 167 insertions(+), 29 deletions(-) >> >> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >> index 1494848..e8979eb 100644 >> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >> @@ -32,6 +32,29 @@ EDKII_IOMMU_PROTOCOL *mIoMmuProtocol; >> EFI_EVENT mIoMmuEvent; >> VOID *mIoMmuRegistration; >> >> +STATIC >> +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: > > (2) How about we return zero for TypeBus, explicitly, and then > ASSERT(FALSE) and return zero for "default"? > +1 >> + return 0; >> + } >> +} >> + >> /** >> Ensure the compatibility of an IO space descriptor with the IO aperture. >> >> @@ -411,8 +434,12 @@ 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. > > (3) In general, the comment style requires comments like this: > > // > // Add empty comment lines both before and after. > // > Quite the opposite, in fact. Search for 'horror vacui' in the coding style document. That does not mean I agree with that document, and I am perfectly fine with both styles. >> 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); > > This looks fine. > >> @@ -422,7 +449,7 @@ InitializePciHostBridge ( >> EfiGcdIoTypeIo, >> 0, >> RootBridges[Index].Io.Limit - >> RootBridges[Index].Io.Base + 1, >> - &RootBridges[Index].Io.Base, >> + &RootBridges[Index].Io.Base - >> RootBridges[Index].Io.Translation, >> gImageHandle, >> NULL >> ); > > (4) But this is wrong (operating under the assumption that > PCI_ROOT_BRIDGE_APERTURE providing device addresses). Namely, 5th > parameter of gDS->AllocateIoSpace() is an in/out parameter: > > [MdeModulePkg/Core/Dxe/Gcd/Gcd.c] > > EFI_STATUS > EFIAPI > CoreAllocateIoSpace ( > IN EFI_GCD_ALLOCATE_TYPE GcdAllocateType, > IN EFI_GCD_IO_TYPE GcdIoType, > IN UINTN Alignment, > IN UINT64 Length, > IN OUT EFI_PHYSICAL_ADDRESS *BaseAddress, > IN EFI_HANDLE ImageHandle, > IN EFI_HANDLE DeviceHandle OPTIONAL > ) > > The patch, as written, takes the address of the Base field, and then > decreases that address by Translation * sizeof Base. The resultant > pointer is passed to the function as 5th argument. > > I don't think that's what you meant :) > > Instead, you have to translate the device address to host address first > (using a temporary variable), and pass that to the function. (Updating > the Base field back to the resultant / output value shouldn't be > necessary, because GcdAllocateType==EfiGcdAllocateAddress, so the > allocation is required to succeed at the exact address requested.) > >> @@ -443,14 +470,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 >> ); >> @@ -463,7 +494,7 @@ InitializePciHostBridge ( >> EfiGcdMemoryTypeMemoryMappedIo, >> 0, >> MemApertures[MemApertureIndex]->Limit - >> MemApertures[MemApertureIndex]->Base + 1, >> - &MemApertures[MemApertureIndex]->Base, >> + &MemApertures[MemApertureIndex]->Base - >> MemApertures[MemApertureIndex]->Translation, >> gImageHandle, >> NULL >> ); > > (5) Same comment as (4) applies, to the 5th argument. > >> @@ -824,12 +855,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 +874,8 @@ NotifyPhase ( >> 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 +889,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 +899,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 +913,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; > > (6) For all of these, I believe that we have to think about the corner > case when the Translation value is not a multiple of (Alignment + 1). > > "Alignment" comes from the PCI BAR in question, whereas Base comes from > the platform PciHostBridgeLib. I think these are fairly independent (you > can plug a 3rd party, discrete PCI card into a board). I find it > plausible that the card has such a huge MMIO BAR (and alignment) that > the platform's Translation offset is not a multiple thereof. > > So, which "view" has to be aligned here? The PCI (device) view, or the > host (CPU) view? > > ... Hmmm wait, the AllocateResource() function aligns BaseAddress > internally anyway. So, first, I don't understand why the pre-patch code > uses ALIGN_VALUE at the call site as well; second, due to the alignment > internal to AllocateResource(), we couldn't align *just* the PCI view > even if we wanted to. > > So I guess this code is correct (due to the alignment internal to > AllocateResource()), but could we perhaps simplify it by passing > > RootBridge->Xxx.Base - RootBridge->Xxx.Translation > > i.e., by dropping the outer alignment? > > (To be clear, dropping the "outer" alignment, if it's a valid change, > should be done as a separate patch, before the translation is > introduced. I hope Ray can comment on this.) > >> >> @@ -1152,6 +1188,7 @@ StartBusEnumeration ( >> Descriptor->AddrSpaceGranularity = 0; >> Descriptor->AddrRangeMin = RootBridge->Bus.Base; >> Descriptor->AddrRangeMax = 0; >> + // Ignore translation offset for bus >> Descriptor->AddrTranslationOffset = 0; >> Descriptor->AddrLen = RootBridge->Bus.Limit - >> RootBridge->Bus.Base + 1; >> > > (7) Ah, in this case, adding TypeBus under (2) is not necessary, just > ASSERT(FALSE) under the currently proposed "default" label. > > >> @@ -1421,7 +1458,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); >> 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/PciHostResource.h >> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h >> index 8612c0c..662c2dd 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 >> UINT64 Base; >> UINT64 Length; >> UINT64 Alignment; >> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c >> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c >> index dc06c16..04ed411 100644 >> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c >> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c >> @@ -86,12 +86,23 @@ 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 >> + )); > > (8) I suggest capitalizing "Translation" in the debug output. > > (9) The indentation is not idiomatic, it should be > > DEBUG (( > ... > ... > )); > >> >> // >> // Make sure Mem and MemAbove4G apertures are valid >> @@ -206,7 +217,10 @@ CreateRootBridge ( >> } >> RootBridge->ResAllocNode[Index].Type = Index; >> if (Bridge->ResourceAssigned && (Aperture->Limit >= Aperture->Base)) { >> - RootBridge->ResAllocNode[Index].Base = Aperture->Base; >> + // 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; >> RootBridge->ResAllocNode[Index].Length = Aperture->Limit - >> Aperture->Base + 1; >> RootBridge->ResAllocNode[Index].Status = ResAllocated; >> } else { >> @@ -403,6 +417,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 +694,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); >> + 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 +752,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); >> } >> >> /** >> @@ -746,6 +804,8 @@ RootBridgeIoIoRead ( >> ) >> { >> EFI_STATUS Status; >> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge; >> + >> Status = RootBridgeIoCheckParameter ( >> This, IoOperation, Width, >> Address, Count, Buffer >> @@ -753,7 +813,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); >> } >> >> /** >> @@ -788,6 +853,8 @@ RootBridgeIoIoWrite ( >> ) >> { >> EFI_STATUS Status; >> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge; >> + >> Status = RootBridgeIoCheckParameter ( >> This, IoOperation, Width, >> Address, Count, Buffer >> @@ -795,7 +862,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); >> } >> >> /** >> @@ -1615,25 +1687,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; >> 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; >> diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h >> b/MdeModulePkg/Include/Library/PciHostBridgeLib.h >> index d42e9ec..21ee8cd 100644 >> --- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h >> +++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h >> @@ -20,8 +20,16 @@ >> // (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 >> 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, Translation is probably >> + // negative for we may translate an above-4G host address into a below-4G >> + // device address for legacy PCIe device compatibility. >> + UINT64 Translation; >> } PCI_ROOT_BRIDGE_APERTURE; >> >> typedef struct { >> > > (10) I suggest replacing the "negative" language with "the subtraction > is to be performed with UINT64 wrap-around semantics". > +1 > > I've made some comments, but I admit my understanding is quite limited; > sorry about that. > > Thanks > Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
