From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: "Ni, Ruiyu" <ruiyu.ni@intel.com>
Cc: Heyi Guo <heyi.guo@linaro.org>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
Star Zeng <star.zeng@intel.com>, Eric Dong <eric.dong@intel.com>,
Laszlo Ersek <lersek@redhat.com>,
Michael D Kinney <michael.d.kinney@intel.com>
Subject: Re: [RFC v4 1/3] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
Date: Tue, 27 Feb 2018 08:55:29 +0000 [thread overview]
Message-ID: <CAKv+Gu8O4oerzp46XZjAkP7fdVhQXLxR7sfPW52pUs2uwxEO9w@mail.gmail.com> (raw)
In-Reply-To: <563e76b9-ebbc-be09-d306-8dcb86f12112@Intel.com>
On 27 February 2018 at 08:48, Ni, Ruiyu <ruiyu.ni@intel.com> 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 <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>
>> ---
>>
>> 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]
Please, can we stop bikheshedding about the comment style? This style
is the exact opposite of what the coding style document mandates, and
both flavours exist throughout the code.
> 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:)
>
Why?
>> +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.
>
Again, can we please focus on the code? This issue is complex and
controversial enough as it is.
>> 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
next prev parent reply other threads:[~2018-02-27 8:49 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-27 2:09 [RFC v4 0/3] Add translation support to generic PciHostBridge Heyi Guo
2018-02-27 2:09 ` [RFC v4 1/3] MdeModulePkg/PciHostBridgeDxe: Add support for address translation Heyi Guo
2018-02-27 8:48 ` Ni, Ruiyu
2018-02-27 8:55 ` Ard Biesheuvel [this message]
2018-02-27 9:33 ` Guo Heyi
2018-02-27 9:59 ` Ni, Ruiyu
2018-02-27 10:14 ` Ard Biesheuvel
2018-02-27 10:45 ` Guo Heyi
2018-02-27 11:44 ` Guo Heyi
2018-02-28 2:29 ` Ni, Ruiyu
2018-02-28 7:25 ` Ni, Ruiyu
2018-02-28 7:53 ` Guo Heyi
2018-02-28 9:39 ` Laszlo Ersek
2018-02-28 23:31 ` Guo Heyi
2018-03-01 3:56 ` Guo Heyi
2018-03-01 4:44 ` Ni, Ruiyu
2018-02-27 2:09 ` [RFC v4 2/3] MdeModulePkg/PciBus: convert host address to device address Heyi Guo
2018-02-27 2:09 ` [RFC v4 3/3] MdeModulePkg/PciBus: return CPU address for GetBarAttributes Heyi Guo
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=CAKv+Gu8O4oerzp46XZjAkP7fdVhQXLxR7sfPW52pUs2uwxEO9w@mail.gmail.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