From: "Ni, Ruiyu" <ruiyu.ni@Intel.com>
To: Guo Heyi <heyi.guo@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
edk2-devel@lists.01.org,
Michael D Kinney <michael.d.kinney@intel.com>,
Laszlo Ersek <lersek@redhat.com>, Star Zeng <star.zeng@intel.com>
Subject: Re: [RFC v4 1/3] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
Date: Tue, 27 Feb 2018 17:59:48 +0800 [thread overview]
Message-ID: <19deadb3-4d26-914c-c9f8-c6c6716746fa@Intel.com> (raw)
In-Reply-To: <20180227093334.GA3918@SZX1000114654>
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 <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]
>> 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
next prev parent reply other threads:[~2018-02-27 9:53 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
2018-02-27 9:33 ` Guo Heyi
2018-02-27 9:59 ` Ni, Ruiyu [this message]
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=19deadb3-4d26-914c-c9f8-c6c6716746fa@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