Hi Ray,
I've one question about (9); please see it below:
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....@linaro.org>
> >Cc: Ruiyu Ni <ruiyu...@intel.com>
> >Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> >Cc: Star Zeng <star.z...@intel.com>
> >Cc: Eric Dong <eric.d...@intel.com>
> >Cc: Laszlo Ersek <ler...@redhat.com>
> >Cc: Michael D Kinney <michael.d.kin...@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.
The comment is for the below code, not for AddrRangeMax, so I'm not quite
understand the comments.
Descriptor->AddrTranslationOffset = 0;
Heyi
> > 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
