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: [PATCH v5 4/6] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
Date: Wed, 14 Mar 2018 15:57:14 +0800 [thread overview]
Message-ID: <9f3e6241-e7fd-1178-ddc2-5b1cdd1b1153@Intel.com> (raw)
In-Reply-To: <20180307060143.GB91936@SZX1000114654>
On 3/7/2018 2:01 PM, Guo Heyi wrote:
> Thanks. Please let me know if any further changes are needed.
>
> Regards,
>
> Heyi
>
> On Wed, Mar 07, 2018 at 12:30:59PM +0800, Ni, Ruiyu wrote:
>> On 3/6/2018 10:44 AM, Guo Heyi wrote:
>>> Hi Ray,
>>>
>>> Any comments for v5?
>>
>> Heyi,
>> Some backward compatibility concerns were received from internal production
>> teams. Current change will cause silent failure on old platforms because
>> TranslationOffset might be random if uninitialized.
>> I will solve the concern and then send out updates to you, hopefully by end
>> of next week.
>>
>>>
>>> Regards,
>>>
>>> Heyi
>>>
>>> On Thu, Mar 01, 2018 at 02:57:22PM +0800, 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 in protocol interfaces and internal
>>>> implementations:
>>>>
>>>> 1. *Only* the following protocol interfaces assume Address is Device
>>>> Address:
>>>> (1). PciHostBridgeResourceAllocation.GetProposedResources()
>>>> Otherwise PCI bus driver cannot set correct address into PCI
>>>> BARs.
>>>> (2). PciRootBridgeIo.Mem.Read() and PciRootBridgeIo.Mem.Write()
>>>> (3). PciRootBridgeIo.CopyMem()
>>>> UEFI and PI spec have clear statements for all other protocol
>>>> interfaces about the address type.
>>>>
>>>> 2. Library interfaces and internal implementation:
>>>> (1). Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
>>>> It is easy to check whether the address is below 4G or above 4G.
>>>> (2). Addresses in PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode are host
>>>> address, for they are allocated from GCD.
>>>> (3). Address passed to PciHostBridgeResourceConflict is host address,
>>>> for it comes from PCI_ROOT_BRIDGE_INSTANCE.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.
>>>>
>>>> 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:
>>>> v5:
>>>> - Add check for the alignment of Translation against the BAR alignment
>>>> [Ray]
>>>> - Keep coding style of comments consistent with the context [Ray]
>>>> - Comment change for Base in PCI_RES_NODE [Ray]
>>>> - Add macros of TO_HOST_ADDRESS and TO_DEVICE_ADDRESS for address type
>>>> conversion (After that we can also simply the comments about the
>>>> calculation [Ray]
>>>> - Add check for bus translation offset in CreateRootBridge(), making
>>>> sure it is zero, and unify code logic for all types of resource
>>>> after that [Ray]
>>>> - Use GetTranslationByResourceType() to simplify the code in
>>>> RootBridgeIoConfiguration() (also fix a bug in previous patch
>>>> version of missing a break after case TypePMem64) [Ray]
>>>> - Commit message refinement [Ray]
>>>> 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
>>>> 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/PciHostBridge.h | 21 ++++
>>>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h | 3 +
>>>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 129 +++++++++++++++++---
>>>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 118 ++++++++++++++++--
>>>> 4 files changed, 245 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h
>>>> index 9a8ca21f4819..c2791ea5c2a4 100644
>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h
>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h
>>>> @@ -38,6 +38,13 @@ typedef struct {
>>>> #define PCI_HOST_BRIDGE_FROM_THIS(a) CR (a, PCI_HOST_BRIDGE_INSTANCE, ResAlloc, PCI_HOST_BRIDGE_SIGNATURE)
>>>> //
>>>> +// Macros to translate device address to host address and vice versa. According
>>>> +// to UEFI 2.7, device address = host address + translation offset.
>>>> +//
>>>> +#define TO_HOST_ADDRESS(DeviceAddress,TranslationOffset) ((DeviceAddress) - (TranslationOffset))
>>>> +#define TO_DEVICE_ADDRESS(HostAddress,TranslationOffset) ((HostAddress) + (TranslationOffset))
>>>> +
>>>> +//
>>>> // Driver Entry Point
>>>> //
>>>> /**
>>>> @@ -247,6 +254,20 @@ ResourceConflict (
>>>> IN PCI_HOST_BRIDGE_INSTANCE *HostBridge
>>>> );
>>>> +/**
>>>> + This routine gets translation offset from a root bridge instance by resource type.
>>>> +
>>>> + @param RootBridge The Root Bridge Instance for the resources.
>>>> + @param ResourceType The Resource Type of the translation offset.
>>>> +
>>>> + @retval The Translation Offset of the specified resource.
>>>> +**/
>>>> +UINT64
>>>> +GetTranslationByResourceType (
>>>> + IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge,
>>>> + IN PCI_RESOURCE_TYPE ResourceType
>>>> + );
>>>> +
>>>> extern EFI_METRONOME_ARCH_PROTOCOL *mMetronome;
>>>> extern EFI_CPU_IO2_PROTOCOL *mCpuIo;
>>>> #endif
>>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
>>>> index 8612c0c3251b..a6c3739db368 100644
>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
>>>> @@ -38,6 +38,9 @@ typedef enum {
>>>> typedef struct {
>>>> PCI_RESOURCE_TYPE Type;
>>>> + //
>>>> + // Base is a host address
>>>> + //
>>>> UINT64 Base;
>>>> UINT64 Length;
>>>> UINT64 Alignment;
>>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>> index 1494848c3e8c..42ded2855c71 100644
>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>> @@ -33,6 +33,39 @@ EFI_EVENT mIoMmuEvent;
>>>> VOID *mIoMmuRegistration;
>>>> /**
>>>> + This routine gets translation offset from a root bridge instance by resource type.
>>>> +
>>>> + @param RootBridge The Root Bridge Instance for the resources.
>>>> + @param ResourceType The Resource Type of the translation offset.
>>>> +
>>>> + @retval The Translation Offset of the specified resource.
>>>> +**/
>>>> +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;
>>>> + case TypeBus:
>>>> + return RootBridge->Bus.Translation;
>>>> + default:
>>>> + ASSERT (FALSE);
>>>> + return 0;
>>>> + }
>>>> +}
>>>> +
>>>> +/**
>>>> Ensure the compatibility of an IO space descriptor with the IO aperture.
>>>> The IO space descriptor can come from the GCD IO space map, or it can
>>>> @@ -366,6 +399,7 @@ InitializePciHostBridge (
>>>> UINTN MemApertureIndex;
>>>> BOOLEAN ResourceAssigned;
>>>> LIST_ENTRY *Link;
>>>> + UINT64 HostAddress;
>>>> RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount);
>>>> if ((RootBridges == NULL) || (RootBridgeCount == 0)) {
>>>> @@ -411,8 +445,15 @@ InitializePciHostBridge (
>>>> }
>>>> if (RootBridges[Index].Io.Base <= RootBridges[Index].Io.Limit) {
>>>> + //
>>>> + // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
>>>> + // For GCD resource manipulation, we need to use host address.
>>>> + //
>>>> + HostAddress = TO_HOST_ADDRESS (RootBridges[Index].Io.Base,
>>>> + RootBridges[Index].Io.Translation);
>>>> +
>>>> Status = AddIoSpace (
>>>> - RootBridges[Index].Io.Base,
>>>> + HostAddress,
>>>> RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1
>>>> );
>>>> ASSERT_EFI_ERROR (Status);
>>>> @@ -422,7 +463,7 @@ InitializePciHostBridge (
>>>> EfiGcdIoTypeIo,
>>>> 0,
>>>> RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1,
>>>> - &RootBridges[Index].Io.Base,
>>>> + &HostAddress,
>>>> gImageHandle,
>>>> NULL
>>>> );
>>>> @@ -443,14 +484,20 @@ 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.
>>>> + // For GCD resource manipulation, we need to use host address.
>>>> + //
>>>> + HostAddress = TO_HOST_ADDRESS (MemApertures[MemApertureIndex]->Base,
>>>> + MemApertures[MemApertureIndex]->Translation);
>>>> Status = AddMemoryMappedIoSpace (
>>>> - MemApertures[MemApertureIndex]->Base,
>>>> + HostAddress,
>>>> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
>>>> EFI_MEMORY_UC
>>>> );
>>>> ASSERT_EFI_ERROR (Status);
>>>> Status = gDS->SetMemorySpaceAttributes (
>>>> - MemApertures[MemApertureIndex]->Base,
>>>> + HostAddress,
>>>> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
>>>> EFI_MEMORY_UC
>>>> );
>>>> @@ -463,7 +510,7 @@ InitializePciHostBridge (
>>>> EfiGcdMemoryTypeMemoryMappedIo,
>>>> 0,
>>>> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
>>>> - &MemApertures[MemApertureIndex]->Base,
>>>> + &HostAddress,
>>>> gImageHandle,
>>>> NULL
>>>> );
>>>> @@ -654,6 +701,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));
>>>> @@ -721,6 +773,7 @@ NotifyPhase (
>>>> PCI_RESOURCE_TYPE Index2;
>>>> BOOLEAN ResNodeHandled[TypeMax];
>>>> UINT64 MaxAlignment;
>>>> + UINT64 Translation;
>>>> HostBridge = PCI_HOST_BRIDGE_FROM_THIS (This);
>>>> @@ -822,14 +875,43 @@ NotifyPhase (
>>>> BitsOfAlignment = LowBitSet64 (Alignment + 1);
>>>> BaseAddress = MAX_UINT64;
>>>> + //
>>>> + // 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.
>>>> + //
>>>> + Translation = GetTranslationByResourceType (RootBridge, Index);
>>>> + if ((Translation & Alignment) != 0) {
>>>> + DEBUG ((DEBUG_ERROR, "[%a:%d] Translation %lx is not aligned to %lx!\n",
>>>> + __FUNCTION__, __LINE__, Translation, Alignment
>>>> + ));
>>>> + ASSERT (FALSE);
1. Cool! But can you replace ASSERT(FALSE) with
ASSERT ((Translation & Alignment) == 0)?
>>>> + //
>>>> + // This may be caused by too large alignment or too small
>>>> + // Translation; pick the 1st possibility and return out of resource,
>>>> + // which can also go thru the same process for out of resource
>>>> + // outside the loop.
>>>> + //
>>>> + ReturnStatus = EFI_OUT_OF_RESOURCES;
>>>> + continue;
>>>> + }
>>>> +
>>>> switch (Index) {
>>>> case TypeIo:
>>>> + //
>>>> + // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
>>>> + // For AllocateResource is manipulating GCD resource, we need to use
>>>> + // host address here.
>>>> + //
>>>> BaseAddress = AllocateResource (
>>>> FALSE,
>>>> RootBridge->ResAllocNode[Index].Length,
>>>> MIN (15, BitsOfAlignment),
>>>> - ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1),
>>>> - RootBridge->Io.Limit
>>>> + TO_HOST_ADDRESS (ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1),
>>>> + RootBridge->Io.Translation),
>>>> + TO_HOST_ADDRESS (RootBridge->Io.Limit,
>>>> + RootBridge->Io.Translation)
>>>> );
>>>> break;
>>>> @@ -838,8 +920,10 @@ NotifyPhase (
>>>> TRUE,
>>>> RootBridge->ResAllocNode[Index].Length,
>>>> MIN (63, BitsOfAlignment),
>>>> - ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1),
>>>> - RootBridge->MemAbove4G.Limit
>>>> + TO_HOST_ADDRESS (ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1),
>>>> + RootBridge->MemAbove4G.Translation),
>>>> + TO_HOST_ADDRESS (RootBridge->MemAbove4G.Limit,
>>>> + RootBridge->MemAbove4G.Translation)
>>>> );
>>>> if (BaseAddress != MAX_UINT64) {
>>>> break;
>>>> @@ -853,8 +937,10 @@ NotifyPhase (
>>>> TRUE,
>>>> RootBridge->ResAllocNode[Index].Length,
>>>> MIN (31, BitsOfAlignment),
>>>> - ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1),
>>>> - RootBridge->Mem.Limit
>>>> + TO_HOST_ADDRESS (ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1),
>>>> + RootBridge->Mem.Translation),
>>>> + TO_HOST_ADDRESS (RootBridge->Mem.Limit,
>>>> + RootBridge->Mem.Translation)
>>>> );
>>>> break;
>>>> @@ -863,8 +949,10 @@ NotifyPhase (
>>>> TRUE,
>>>> RootBridge->ResAllocNode[Index].Length,
>>>> MIN (63, BitsOfAlignment),
>>>> - ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1),
>>>> - RootBridge->PMemAbove4G.Limit
>>>> + TO_HOST_ADDRESS (ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1),
>>>> + RootBridge->PMemAbove4G.Translation),
>>>> + TO_HOST_ADDRESS (RootBridge->PMemAbove4G.Limit,
>>>> + RootBridge->PMemAbove4G.Translation)
>>>> );
>>>> if (BaseAddress != MAX_UINT64) {
>>>> break;
>>>> @@ -877,8 +965,10 @@ NotifyPhase (
>>>> TRUE,
>>>> RootBridge->ResAllocNode[Index].Length,
>>>> MIN (31, BitsOfAlignment),
>>>> - ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1),
>>>> - RootBridge->PMem.Limit
>>>> + TO_HOST_ADDRESS (ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1),
>>>> + RootBridge->PMem.Translation),
>>>> + TO_HOST_ADDRESS (RootBridge->PMem.Limit,
>>>> + RootBridge->PMem.Translation)
>>>> );
>>>> break;
>>>> @@ -1421,7 +1511,14 @@ 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 conversion is needed.
>>>> + //
>>>> + Descriptor->AddrRangeMin = TO_DEVICE_ADDRESS (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/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>>> index dc06c16dc038..5dd9d257d46d 100644
>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>>> @@ -86,12 +86,35 @@ 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""
>>>> ));
>>>> + //
>>>> + // Translation for bus is not supported.
>>>> + //
>>>> 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));
>>>> + ASSERT (Bridge->Bus.Translation == 0);
>>>> + if (Bridge->Bus.Translation != 0) {
>>>> + return NULL;
>>>> + }
2. Can you please use the same debug message format for Bus range? I
mean to print the Translation for Bus as well. Then in the end of debug
message, use assertion and if-check. This way developer can know what
the Bus.Translation is from the debug log.
>>>> +
>>>> + 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 +229,12 @@ 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.
>>>> + //
>>>> + RootBridge->ResAllocNode[Index].Base = TO_HOST_ADDRESS (Aperture->Base,
>>>> + Aperture->Translation);
>>>> RootBridge->ResAllocNode[Index].Length = Aperture->Limit - Aperture->Base + 1;
>>>> RootBridge->ResAllocNode[Index].Status = ResAllocated;
>>>> } else {
>>>> @@ -403,6 +431,28 @@ RootBridgeIoCheckParameter (
>>>> return EFI_SUCCESS;
>>>> }
3. Please add the missing function header comments.
>>>> +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 +708,25 @@ 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 needs to be a host address instead of
>>>> + // device address.
>>>> + return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width,
>>>> + TO_HOST_ADDRESS (Address, Translation), Count, Buffer);
>>>> }
>>>> /**
>>>> @@ -705,13 +767,25 @@ 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 needs to be a host address instead of
>>>> + // device address.
>>>> + return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width,
>>>> + TO_HOST_ADDRESS (Address, Translation), Count, Buffer);
>>>> }
>>>> /**
>>>> @@ -746,6 +820,8 @@ RootBridgeIoIoRead (
>>>> )
>>>> {
>>>> EFI_STATUS Status;
>>>> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
>>>> +
>>>> Status = RootBridgeIoCheckParameter (
>>>> This, IoOperation, Width,
>>>> Address, Count, Buffer
>>>> @@ -753,7 +829,13 @@ 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 needs to be a host address instead of
>>>> + // device address.
>>>> + return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width,
>>>> + TO_HOST_ADDRESS (Address, RootBridge->Io.Translation), Count, Buffer);
>>>> }
>>>> /**
>>>> @@ -788,6 +870,8 @@ RootBridgeIoIoWrite (
>>>> )
>>>> {
>>>> EFI_STATUS Status;
>>>> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
>>>> +
>>>> Status = RootBridgeIoCheckParameter (
>>>> This, IoOperation, Width,
>>>> Address, Count, Buffer
>>>> @@ -795,7 +879,13 @@ 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 needs to be a host address instead of
>>>> + // device address.
>>>> + return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width,
>>>> + TO_HOST_ADDRESS (Address, RootBridge->Io.Translation), Count, Buffer);
>>>> }
>>>> /**
>>>> @@ -1615,9 +1705,17 @@ 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;
>>>> + Descriptor->AddrTranslationOffset = GetTranslationByResourceType (
>>>> + RootBridge,
>>>> + ResAllocNode->Type
>>>> + );
>>>> +
>>>> switch (ResAllocNode->Type) {
>>>> case TypeIo:
>>>> --
>>>> 2.7.4
>>>>
>>
>>
>> --
>> Thanks,
>> Ray
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
Please modify code according to the above 3 comments and make sure
the changes pass ECC check:
BaseTools/Source/Python/Ecc/Ecc.py -t -s \
MdeModulePkg/Pci/PciHostBridgeDxe
> _______________________________________________
> 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-03-14 7:50 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-01 6:57 [PATCH v5 0/6] Add translation support to generic PciHostBridge Heyi Guo
2018-03-01 6:57 ` [PATCH v5 1/6] CorebootPayloadPkg/PciHostBridgeLib: Init PCI aperture to 0 Heyi Guo
2018-03-14 11:24 ` Ard Biesheuvel
2018-03-01 6:57 ` [PATCH v5 2/6] OvmfPkg/PciHostBridgeLib: " Heyi Guo
2018-03-01 10:17 ` Laszlo Ersek
2018-03-01 10:48 ` Guo Heyi
2018-03-02 10:19 ` Laszlo Ersek
2018-03-05 8:23 ` Guo Heyi
2018-03-01 10:20 ` Laszlo Ersek
2018-03-01 10:25 ` Guo Heyi
2018-03-01 12:03 ` Ni, Ruiyu
2018-03-02 10:08 ` Laszlo Ersek
2018-03-01 6:57 ` [PATCH v5 3/6] MdeModulePkg/PciHostBridgeLib.h: add address Translation Heyi Guo
2018-03-01 6:57 ` [PATCH v5 4/6] MdeModulePkg/PciHostBridgeDxe: Add support for address translation Heyi Guo
2018-03-06 2:44 ` Guo Heyi
2018-03-07 4:30 ` Ni, Ruiyu
2018-03-07 6:01 ` Guo Heyi
2018-03-14 7:57 ` Ni, Ruiyu [this message]
2018-03-14 11:25 ` Ard Biesheuvel
2018-03-15 2:57 ` Guo Heyi
2018-03-15 3:25 ` Ni, Ruiyu
2018-03-12 17:18 ` Ard Biesheuvel
2018-03-13 3:00 ` Ni, Ruiyu
2018-03-13 7:31 ` Guo Heyi
2018-03-13 8:04 ` Ard Biesheuvel
2018-03-01 6:57 ` [PATCH v5 5/6] MdeModulePkg/PciBus: convert host address to device address Heyi Guo
2018-03-01 6:57 ` [PATCH v5 6/6] MdeModulePkg/PciBus: return CPU address for GetBarAttributes Heyi Guo
2018-03-01 8:28 ` [PATCH v5 0/6] Add translation support to generic PciHostBridge Ard Biesheuvel
2018-03-15 1:07 ` Ni, Ruiyu
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=9f3e6241-e7fd-1178-ddc2-5b1cdd1b1153@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