From: "Ni, Ruiyu" <ruiyu.ni@Intel.com>
To: Guo Heyi <heyi.guo@linaro.org>
Cc: Laszlo Ersek <lersek@redhat.com>,
edk2-devel@lists.01.org, Eric Dong <eric.dong@intel.com>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
Michael D Kinney <michael.d.kinney@intel.com>,
Star Zeng <star.zeng@intel.com>
Subject: Re: [RFC v3 1/3] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
Date: Sat, 24 Feb 2018 16:30:42 +0800 [thread overview]
Message-ID: <4cac156f-feab-2746-53ee-2b009c5b56d0@Intel.com> (raw)
In-Reply-To: <20180224035914.GD111587@SZX1000114654>
On 2/24/2018 11:59 AM, Guo Heyi wrote:
> On Sat, Feb 24, 2018 at 11:11:35AM +0800, Ni, Ruiyu wrote:
>> On 2/24/2018 9:51 AM, Guo Heyi wrote:
>>> On Fri, Feb 23, 2018 at 04:05:17PM +0100, Laszlo Ersek 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?
>>>>
>>>> ... 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.)
>> DmaAbove4G in PCI_ROOT_BRIDGE_APERTURE indicates the capability of
>> the root bridge HW, whether it's capable to do DMA on device address
>> above 4GB.
>>
>>>>
>>>>>
>>>>> 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 <heyi.guo@linaro.org>
>>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>> Cc: Star Zeng <star.zeng@intel.com>
>>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>>>> ---
>>>>> .../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"?
>>>>
>>>>> + 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.
>>>> //
>>>>
>>>>> 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?
>>>
>>> I believe the alignment requirement is from PCI view, not the CPU view. This
>>> also implies alignment in allocating GCD resources becomes meaningless.
>> I agree. It's an issue we need to think about.
>>
>> All address spaces in GCD are added by gDS->AddXXX().
>> So it means for a given range of address [HostBase, HostLimit) in GCD,
>> the corresponding device address is
>> [HostBase + Offset, HostLimit + Offset).
>> E.g.: GCD entry = [0xFFD, 0x3FFD), Offset = 0x3
>> The corresponding device address range is [0x1000, 0x4000)
>> If we want to allocate 3 page-aligned pages of MMIO from GCD,
>> current AllocateResource() implementation will fail to allocate.
>>
>> What will the Offset be in real world?
>> If it's quite small (smaller than device required alignment),
>> thing becomes complicated. I even cannot think of any solution.
>> If it's quite large (larger than device required alignment),
>> thing becomes easy. Today's implementation can handle it well.
>
> I believe the Offset is normally large, so that we may place some assumption or
> restriction here, to make things easier. You can see the translation on our platform:
> https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Hisilicon/Hi1616/D05AcpiTables/Dsdt/D05Pci.asl#L204
>
> How about we assume ATO alignment >= the whole region length (denoted as
> L)? Then there are two possibilities:
This assumption is a bit strict I think.
Can we assume ATO alignment >= RootBridge->ResAllocNode[i].Alignment?
RootBridge->ResAllocNode[i].Alignment carries the alignment requirement
from the specific resource type.
Supposing there are three 32bit BARs from 2 PCI devices, each requires
alignment/length 0x100, 0x2000, 0x400,
RootBridge->ResAllocNode[i].Alignment equals to 0x2000.
RootBridge->ResAllocNode[i].Length equals to 0x2000 + 0x400 + 0x100.
We only require ATO alignment >= 0x2000 in this case, but don't require
ATO alignment >= 0x2500.
Agree?
>
> 1. BAR alignment <= L, so BAR alignment <= ATO alignment, so
> (each host address % BAR alignment) == (each device address % BAR alignment),
> and the current code can handle this.
>
> This should cover most of cases, since BAR alignment <= BAR length, and the
> other possibility is actually out of resource.
>
> 2. BAR alignment > L, e.g.
> BAR alignment = 1MB
> L = 4KB
> ATO = 4KB or -4KB
>
> 1) If device address = 0, host address = 4KB, then AllocateResource will
> return -1 and result to OUT_OF_RESOURCES;
> 2) If device address = 4KB, host address = 0, then AllocateResource will also
> return -1
> Anyway we will get the expected OUT_OF_RESOURCES result by current code.
>
> And this assumption is not a very strict one.
>
> Any comments?
>
> Thanks,
>
> Gary (Heyi Guo)
>
>
>>
>>
>>
>>>
>>> Thanks,
>>>
>>> Gary (Heyi Guo)
>>>
>>>
>>>>
>>>> ... 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".
>>>>
>>>>
>>>> I've made some comments, but I admit my understanding is quite limited;
>>>> sorry about that.
>>>>
>>>> Thanks
>>>> Laszlo
>>
>>
>> --
>> Thanks,
>> Ray
--
Thanks,
Ray
next prev parent reply other threads:[~2018-02-24 8:24 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-23 8:53 [RFC v3 0/3] Add translation support to generic PciHostBridge Heyi Guo
2018-02-23 8:53 ` [RFC v3 1/3] MdeModulePkg/PciHostBridgeDxe: Add support for address translation Heyi Guo
2018-02-23 15:05 ` Laszlo Ersek
2018-02-23 15:17 ` Ard Biesheuvel
2018-02-23 15:26 ` Laszlo Ersek
2018-02-24 1:10 ` Guo Heyi
2018-02-24 1:51 ` Guo Heyi
2018-02-24 3:11 ` Ni, Ruiyu
2018-02-24 3:59 ` Guo Heyi
2018-02-24 8:30 ` Ni, Ruiyu [this message]
2018-02-24 9:15 ` Guo Heyi
2018-02-24 8:14 ` Ard Biesheuvel
2018-02-27 2:19 ` Guo Heyi
2018-02-23 8:53 ` [RFC v3 2/3] MdeModulePkg/PciBus: convert host address to device address Heyi Guo
2018-02-23 8:53 ` [RFC v3 3/3] MdeModulePkg/PciBus: return CPU address for GetBarAttributes Heyi Guo
2018-02-23 15:08 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4cac156f-feab-2746-53ee-2b009c5b56d0@Intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox