... Hmmm wait, the AllocateResource() function aligns BaseAddress
internally anyway. So, first, I don't understand why the pre-patch code
uses ALIGN_VALUE at the call site as well; second, due to the alignment
internal to AllocateResource(), we couldn't align *just* the PCI view
even if we wanted to.
So I guess this code is correct (due to the alignment internal to
AllocateResource()), but could we perhaps simplify it by passing
RootBridge->Xxx.Base - RootBridge->Xxx.Translation
i.e., by dropping the outer alignment?
(To be clear, dropping the "outer" alignment, if it's a valid change,
should be done as a separate patch, before the translation is
introduced. I hope Ray can comment on this.)
@@ -1152,6 +1188,7 @@ StartBusEnumeration (
Descriptor->AddrSpaceGranularity = 0;
Descriptor->AddrRangeMin = RootBridge->Bus.Base;
Descriptor->AddrRangeMax = 0;
+ // Ignore translation offset for bus
Descriptor->AddrTranslationOffset = 0;
Descriptor->AddrLen = RootBridge->Bus.Limit -
RootBridge->Bus.Base + 1;
(7) Ah, in this case, adding TypeBus under (2) is not necessary, just
ASSERT(FALSE) under the currently proposed "default" label.
@@ -1421,7 +1458,12 @@ GetProposedResources (
Descriptor->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR;
Descriptor->Len = sizeof
(EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3;;
Descriptor->GenFlag = 0;
- Descriptor->AddrRangeMin =
RootBridge->ResAllocNode[Index].Base;
+ // AddrRangeMin in Resource Descriptor here should be device address
+ // instead of host address, or else PCI bus driver cannot set correct
+ // address into PCI BAR registers.
+ // Base in ResAllocNode is a host address, so Translation is added.
+ Descriptor->AddrRangeMin =
RootBridge->ResAllocNode[Index].Base +
+ GetTranslationByResourceType (RootBridge, Index);
Descriptor->AddrRangeMax = 0;
Descriptor->AddrTranslationOffset = (ResStatus == ResAllocated) ?
EFI_RESOURCE_SATISFIED : PCI_RESOURCE_LESS;
Descriptor->AddrLen =
RootBridge->ResAllocNode[Index].Length;
diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
index 8612c0c..662c2dd 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
@@ -38,6 +38,8 @@ typedef enum {
typedef struct {
PCI_RESOURCE_TYPE Type;
+ // Base is a host address instead of a device address when address
translation
+ // exists and host address != device address
UINT64 Base;
UINT64 Length;
UINT64 Alignment;
diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index dc06c16..04ed411 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -86,12 +86,23 @@ CreateRootBridge (
(Bridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) != 0 ?
L"CombineMemPMem " : L"",
(Bridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_MEM64_DECODE) != 0 ?
L"Mem64Decode" : L""
));
+ // We don't see any scenario for bus translation, so translation for bus is
just ignored.
DEBUG ((EFI_D_INFO, " Bus: %lx - %lx\n", Bridge->Bus.Base,
Bridge->Bus.Limit));
- DEBUG ((EFI_D_INFO, " Io: %lx - %lx\n", Bridge->Io.Base,
Bridge->Io.Limit));
- DEBUG ((EFI_D_INFO, " Mem: %lx - %lx\n", Bridge->Mem.Base,
Bridge->Mem.Limit));
- DEBUG ((EFI_D_INFO, " MemAbove4G: %lx - %lx\n", Bridge->MemAbove4G.Base,
Bridge->MemAbove4G.Limit));
- DEBUG ((EFI_D_INFO, " PMem: %lx - %lx\n", Bridge->PMem.Base,
Bridge->PMem.Limit));
- DEBUG ((EFI_D_INFO, " PMemAbove4G: %lx - %lx\n", Bridge->PMemAbove4G.Base,
Bridge->PMemAbove4G.Limit));
+ DEBUG ((DEBUG_INFO, " Io: %lx - %lx translation=%lx\n",
+ Bridge->Io.Base, Bridge->Io.Limit, Bridge->Io.Translation
+ ));
+ DEBUG ((DEBUG_INFO, " Mem: %lx - %lx translation=%lx\n",
+ Bridge->Mem.Base, Bridge->Mem.Limit, Bridge->Mem.Translation
+ ));
+ DEBUG ((DEBUG_INFO, " MemAbove4G: %lx - %lx translation=%lx\n",
+ Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit,
Bridge->MemAbove4G.Translation
+ ));
+ DEBUG ((DEBUG_INFO, " PMem: %lx - %lx translation=%lx\n",
+ Bridge->PMem.Base, Bridge->PMem.Limit, Bridge->PMem.Translation
+ ));
+ DEBUG ((DEBUG_INFO, " PMemAbove4G: %lx - %lx translation=%lx\n",
+ Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit,
Bridge->PMemAbove4G.Translation
+ ));
(8) I suggest capitalizing "Translation" in the debug output.
(9) The indentation is not idiomatic, it should be
DEBUG ((
...
...
));
//
// Make sure Mem and MemAbove4G apertures are valid
@@ -206,7 +217,10 @@ CreateRootBridge (
}
RootBridge->ResAllocNode[Index].Type = Index;
if (Bridge->ResourceAssigned && (Aperture->Limit >= Aperture->Base)) {
- RootBridge->ResAllocNode[Index].Base = Aperture->Base;
+ // Base in ResAllocNode is a host address, while Base in Aperture is a
+ // device address, so translation needs to be subtracted.
+ RootBridge->ResAllocNode[Index].Base = Aperture->Base -
+ Aperture->Translation;
RootBridge->ResAllocNode[Index].Length = Aperture->Limit -
Aperture->Base + 1;
RootBridge->ResAllocNode[Index].Status = ResAllocated;
} else {
@@ -403,6 +417,28 @@ RootBridgeIoCheckParameter (
return EFI_SUCCESS;
}
+EFI_STATUS
+RootBridgeIoGetMemTranslationByAddress (
+ IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge,
+ IN UINT64 Address,
+ IN OUT UINT64 *Translation
+ )
+{
+ if (Address >= RootBridge->Mem.Base && Address <= RootBridge->Mem.Limit) {
+ *Translation = RootBridge->Mem.Translation;
+ } else if (Address >= RootBridge->PMem.Base && Address <=
RootBridge->PMem.Limit) {
+ *Translation = RootBridge->PMem.Translation;
+ } else if (Address >= RootBridge->MemAbove4G.Base && Address <=
RootBridge->MemAbove4G.Limit) {
+ *Translation = RootBridge->MemAbove4G.Translation;
+ } else if (Address >= RootBridge->PMemAbove4G.Base && Address <=
RootBridge->PMemAbove4G.Limit) {
+ *Translation = RootBridge->PMemAbove4G.Translation;
+ } else {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ return EFI_SUCCESS;
+}
+
/**
Polls an address in memory mapped I/O space until an exit condition is met,
or a timeout occurs.
@@ -658,13 +694,24 @@ RootBridgeIoMemRead (
)
{
EFI_STATUS Status;
+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
+ UINT64 Translation;
Status = RootBridgeIoCheckParameter (This, MemOperation, Width, Address,
Count, Buffer);
if (EFI_ERROR (Status)) {
return Status;
}
- return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address,
Count, Buffer);
+
+ RootBridge = ROOT_BRIDGE_FROM_THIS (This);
+ Status = RootBridgeIoGetMemTranslationByAddress (RootBridge, Address,
&Translation);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ // Address passed to CpuIo->Mem.Read should be a host address instead of
+ // device address, so Translation should be subtracted.
+ return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address
- Translation, Count, Buffer);
}
/**
@@ -705,13 +752,24 @@ RootBridgeIoMemWrite (
)
{
EFI_STATUS Status;
+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
+ UINT64 Translation;
Status = RootBridgeIoCheckParameter (This, MemOperation, Width, Address,
Count, Buffer);
if (EFI_ERROR (Status)) {
return Status;
}
- return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width,
Address, Count, Buffer);
+
+ RootBridge = ROOT_BRIDGE_FROM_THIS (This);
+ Status = RootBridgeIoGetMemTranslationByAddress (RootBridge, Address,
&Translation);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ // Address passed to CpuIo->Mem.Write should be a host address instead of
+ // device address, so Translation should be subtracted.
+ return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address
- Translation, Count, Buffer);
}
/**
@@ -746,6 +804,8 @@ RootBridgeIoIoRead (
)
{
EFI_STATUS Status;
+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
+
Status = RootBridgeIoCheckParameter (
This, IoOperation, Width,
Address, Count, Buffer
@@ -753,7 +813,12 @@ RootBridgeIoIoRead (
if (EFI_ERROR (Status)) {
return Status;
}
- return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address,
Count, Buffer);
+
+ RootBridge = ROOT_BRIDGE_FROM_THIS (This);
+
+ // Address passed to CpuIo->Io.Read should be a host address instead of
+ // device address, so Translation should be subtracted.
+ return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address -
RootBridge->Io.Translation, Count, Buffer);
}
/**
@@ -788,6 +853,8 @@ RootBridgeIoIoWrite (
)
{
EFI_STATUS Status;
+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
+
Status = RootBridgeIoCheckParameter (
This, IoOperation, Width,
Address, Count, Buffer
@@ -795,7 +862,12 @@ RootBridgeIoIoWrite (
if (EFI_ERROR (Status)) {
return Status;
}
- return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address,
Count, Buffer);
+
+ RootBridge = ROOT_BRIDGE_FROM_THIS (This);
+
+ // Address passed to CpuIo->Io.Write should be a host address instead of
+ // device address, so Translation should be subtracted.
+ return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address -
RootBridge->Io.Translation, Count, Buffer);
}
/**
@@ -1615,25 +1687,39 @@ RootBridgeIoConfiguration (
Descriptor->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR;
Descriptor->Len = sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3;
+ // According to UEFI 2.7, RootBridgeIo->Configuration should return address
+ // range in CPU view (host address), and ResAllocNode->Base is already a
CPU
+ // view address (host address).
Descriptor->AddrRangeMin = ResAllocNode->Base;
Descriptor->AddrRangeMax = ResAllocNode->Base + ResAllocNode->Length - 1;
Descriptor->AddrLen = ResAllocNode->Length;
switch (ResAllocNode->Type) {
case TypeIo:
- Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
+ Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
+ Descriptor->AddrTranslationOffset = RootBridge->Io.Translation;
break;
case TypePMem32:
- Descriptor->SpecificFlag =
EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
+ Descriptor->SpecificFlag =
EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
+ Descriptor->AddrTranslationOffset = RootBridge->PMem.Translation;
+ Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
+ Descriptor->AddrSpaceGranularity = 32;
+ break;
+
case TypeMem32:
+ Descriptor->AddrTranslationOffset = RootBridge->Mem.Translation;
Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
Descriptor->AddrSpaceGranularity = 32;
break;
case TypePMem64:
- Descriptor->SpecificFlag =
EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
+ Descriptor->SpecificFlag =
EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
+ Descriptor->AddrTranslationOffset = RootBridge->PMemAbove4G.Translation;
+ Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
+ Descriptor->AddrSpaceGranularity = 64;
case TypeMem64:
+ Descriptor->AddrTranslationOffset = RootBridge->MemAbove4G.Translation;
Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
Descriptor->AddrSpaceGranularity = 64;
break;
diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
index d42e9ec..21ee8cd 100644
--- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
+++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
@@ -20,8 +20,16 @@
// (Base > Limit) indicates an aperture is not available.
//
typedef struct {
+ // Base and Limit are the device address instead of host address when
+ // Translation is not zero
UINT64 Base;
UINT64 Limit;
+ // According to UEFI 2.7, Device Address = Host Address + Translation,
+ // so Translation = Device Address - Host Address.
+ // On platforms where Translation is not zero, Translation is probably
+ // negative for we may translate an above-4G host address into a below-4G
+ // device address for legacy PCIe device compatibility.
+ UINT64 Translation;
} PCI_ROOT_BRIDGE_APERTURE;
typedef struct {
(10) I suggest replacing the "negative" language with "the subtraction
is to be performed with UINT64 wrap-around semantics".
I've made some comments, but I admit my understanding is quite limited;
sorry about that.
Thanks
Laszlo
