* [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
@ 2018-01-15 14:46 Heyi Guo
2018-01-17 14:08 ` Ard Biesheuvel
0 siblings, 1 reply; 12+ messages in thread
From: Heyi Guo @ 2018-01-15 14:46 UTC (permalink / raw)
To: linaro-uefi, edk2-devel
Cc: Heyi Guo, Ruiyu Ni, Ard Biesheuvel, Star Zeng, Eric Dong
This is the draft patch for the discussion posted in edk2-devel
mailing list:
https://lists.01.org/pipermail/edk2-devel/2017-December/019289.html
As discussed in the mailing list, we'd like to add support for PCI
address translation which is necessary for some non-x86 platforms. I
also want to minimize the changes to the generic host bridge driver
and platform PciHostBridgeLib implemetations, so additional two
interfaces are added to expose translation information of the
platform. To be generic, I add translation for each type of IO or
memory resources.
The patch is still a RFC, so I only passed the build for qemu64 and
the function has not been tested yet.
Please let me know your comments about it.
Thanks.
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>
---
.../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 19 ++++
.../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 53 ++++++++---
.../Bus/Pci/PciHostBridgeDxe/PciRootBridge.h | 8 +-
.../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 101 ++++++++++++++++++---
MdeModulePkg/Include/Library/PciHostBridgeLib.h | 36 ++++++++
5 files changed, 192 insertions(+), 25 deletions(-)
diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
index 5b9c887..0c8371a 100644
--- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
+++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
@@ -360,6 +360,16 @@ PciHostBridgeGetRootBridges (
return &mRootBridge;
}
+PCI_ROOT_BRIDGE_TRANSLATION *
+EFIAPI
+PciHostBridgeGetTranslations (
+ UINTN *Count
+ )
+{
+ *Count = 0;
+ return NULL;
+}
+
/**
Free the root bridge instances array returned from
PciHostBridgeGetRootBridges().
@@ -377,6 +387,15 @@ PciHostBridgeFreeRootBridges (
ASSERT (Count == 1);
}
+VOID
+EFIAPI
+PciHostBridgeFreeTranslations (
+ PCI_ROOT_BRIDGE_TRANSLATION *Translations,
+ UINTN Count
+ )
+{
+}
+
/**
Inform the platform that the resource conflict happens.
diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index 1494848..835e411 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -360,18 +360,38 @@ InitializePciHostBridge (
PCI_HOST_BRIDGE_INSTANCE *HostBridge;
PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
PCI_ROOT_BRIDGE *RootBridges;
+ PCI_ROOT_BRIDGE_TRANSLATION *Translations;
UINTN RootBridgeCount;
+ UINTN TranslationCount;
UINTN Index;
PCI_ROOT_BRIDGE_APERTURE *MemApertures[4];
+ UINT64 MemTranslation[4];
UINTN MemApertureIndex;
BOOLEAN ResourceAssigned;
LIST_ENTRY *Link;
+ UINT64 Trans;
RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount);
if ((RootBridges == NULL) || (RootBridgeCount == 0)) {
return EFI_UNSUPPORTED;
}
+ Translations = PciHostBridgeGetTranslations (&TranslationCount);
+ if (Translations == NULL || TranslationCount == 0) {
+ TranslationCount = 0;
+ Translations = AllocateZeroPool (RootBridgeCount * sizeof (*Translations));
+ if (Translations == NULL) {
+ PciHostBridgeFreeRootBridges (RootBridges, RootBridgeCount);
+ return EFI_OUT_OF_RESOURCES;
+ }
+ }
+
+ if (TranslationCount != 0 && TranslationCount != RootBridgeCount) {
+ DEBUG ((DEBUG_ERROR, "Error: count of root bridges (%d) and translation (%d) are different!\n",
+ RootBridgeCount, TranslationCount));
+ return EFI_INVALID_PARAMETER;
+ }
+
Status = gBS->LocateProtocol (&gEfiMetronomeArchProtocolGuid, NULL, (VOID **) &mMetronome);
ASSERT_EFI_ERROR (Status);
Status = gBS->LocateProtocol (&gEfiCpuIo2ProtocolGuid, NULL, (VOID **) &mCpuIo);
@@ -395,7 +415,7 @@ InitializePciHostBridge (
//
// Create Root Bridge Handle Instance
//
- RootBridge = CreateRootBridge (&RootBridges[Index]);
+ RootBridge = CreateRootBridge (&RootBridges[Index], &Translations[Index]);
ASSERT (RootBridge != NULL);
if (RootBridge == NULL) {
continue;
@@ -411,8 +431,9 @@ InitializePciHostBridge (
}
if (RootBridges[Index].Io.Base <= RootBridges[Index].Io.Limit) {
+ Trans = Translations[Index].IoTranslation;
Status = AddIoSpace (
- RootBridges[Index].Io.Base,
+ RootBridges[Index].Io.Base + Trans,
RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1
);
ASSERT_EFI_ERROR (Status);
@@ -422,7 +443,7 @@ InitializePciHostBridge (
EfiGcdIoTypeIo,
0,
RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1,
- &RootBridges[Index].Io.Base,
+ &RootBridges[Index].Io.Base + Trans,
gImageHandle,
NULL
);
@@ -437,20 +458,24 @@ InitializePciHostBridge (
// the MEM aperture in Mem
//
MemApertures[0] = &RootBridges[Index].Mem;
+ MemTranslation[0] = Translations[Index].MemTranslation;
MemApertures[1] = &RootBridges[Index].MemAbove4G;
+ MemTranslation[1] = Translations[Index].MemAbove4GTranslation;
MemApertures[2] = &RootBridges[Index].PMem;
+ MemTranslation[2] = Translations[Index].PMemTranslation;
MemApertures[3] = &RootBridges[Index].PMemAbove4G;
+ MemTranslation[3] = Translations[Index].PMemAbove4GTranslation;
for (MemApertureIndex = 0; MemApertureIndex < ARRAY_SIZE (MemApertures); MemApertureIndex++) {
if (MemApertures[MemApertureIndex]->Base <= MemApertures[MemApertureIndex]->Limit) {
Status = AddMemoryMappedIoSpace (
- MemApertures[MemApertureIndex]->Base,
+ MemApertures[MemApertureIndex]->Base + MemTranslation[MemApertureIndex],
MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
EFI_MEMORY_UC
);
ASSERT_EFI_ERROR (Status);
Status = gDS->SetMemorySpaceAttributes (
- MemApertures[MemApertureIndex]->Base,
+ MemApertures[MemApertureIndex]->Base + MemTranslation[MemApertureIndex],
MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
EFI_MEMORY_UC
);
@@ -463,7 +488,7 @@ InitializePciHostBridge (
EfiGcdMemoryTypeMemoryMappedIo,
0,
MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
- &MemApertures[MemApertureIndex]->Base,
+ &MemApertures[MemApertureIndex]->Base + MemTranslation[MemApertureIndex],
gImageHandle,
NULL
);
@@ -514,7 +539,13 @@ InitializePciHostBridge (
);
ASSERT_EFI_ERROR (Status);
}
+
PciHostBridgeFreeRootBridges (RootBridges, RootBridgeCount);
+ if (TranslationCount == 0) {
+ FreePool (Translations);
+ } else {
+ PciHostBridgeFreeTranslations (Translations, TranslationCount);
+ }
if (!EFI_ERROR (Status)) {
mIoMmuEvent = EfiCreateProtocolNotifyEvent (
@@ -828,7 +859,7 @@ NotifyPhase (
FALSE,
RootBridge->ResAllocNode[Index].Length,
MIN (15, BitsOfAlignment),
- ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1),
+ ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1) + RootBridge->IoTranslation,
RootBridge->Io.Limit
);
break;
@@ -838,7 +869,7 @@ NotifyPhase (
TRUE,
RootBridge->ResAllocNode[Index].Length,
MIN (63, BitsOfAlignment),
- ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1),
+ ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1) + RootBridge->MemAbove4GTranslation,
RootBridge->MemAbove4G.Limit
);
if (BaseAddress != MAX_UINT64) {
@@ -853,7 +884,7 @@ NotifyPhase (
TRUE,
RootBridge->ResAllocNode[Index].Length,
MIN (31, BitsOfAlignment),
- ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1),
+ ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1) + RootBridge->MemTranslation,
RootBridge->Mem.Limit
);
break;
@@ -863,7 +894,7 @@ NotifyPhase (
TRUE,
RootBridge->ResAllocNode[Index].Length,
MIN (63, BitsOfAlignment),
- ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1),
+ ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1) + RootBridge->PMemAbove4GTranslation,
RootBridge->PMemAbove4G.Limit
);
if (BaseAddress != MAX_UINT64) {
@@ -877,7 +908,7 @@ NotifyPhase (
TRUE,
RootBridge->ResAllocNode[Index].Length,
MIN (31, BitsOfAlignment),
- ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1),
+ ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1) + RootBridge->PMemTranslation,
RootBridge->PMem.Limit
);
break;
diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
index d3dfb57..449c4b3 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
@@ -73,6 +73,11 @@ typedef struct {
PCI_ROOT_BRIDGE_APERTURE PMem;
PCI_ROOT_BRIDGE_APERTURE MemAbove4G;
PCI_ROOT_BRIDGE_APERTURE PMemAbove4G;
+ UINT64 IoTranslation;
+ UINT64 MemTranslation;
+ UINT64 MemAbove4GTranslation;
+ UINT64 PMemTranslation;
+ UINT64 PMemAbove4GTranslation;
BOOLEAN DmaAbove4G;
BOOLEAN NoExtendedConfigSpace;
VOID *ConfigBuffer;
@@ -98,7 +103,8 @@ typedef struct {
**/
PCI_ROOT_BRIDGE_INSTANCE *
CreateRootBridge (
- IN PCI_ROOT_BRIDGE *Bridge
+ IN PCI_ROOT_BRIDGE *Bridge,
+ IN PCI_ROOT_BRIDGE_TRANSLATION *Translation
);
//
diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index dc06c16..84b2d5a 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -67,7 +67,8 @@ UINT8 mOutStride[] = {
**/
PCI_ROOT_BRIDGE_INSTANCE *
CreateRootBridge (
- IN PCI_ROOT_BRIDGE *Bridge
+ IN PCI_ROOT_BRIDGE *Bridge,
+ IN PCI_ROOT_BRIDGE_TRANSLATION *Translation
)
{
PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
@@ -87,11 +88,21 @@ CreateRootBridge (
(Bridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_MEM64_DECODE) != 0 ? L"Mem64Decode" : L""
));
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, Translation->IoTranslation
+ ));
+ DEBUG ((DEBUG_INFO, " Mem: %lx - %lx translation=%lx\n",
+ Bridge->Mem.Base, Bridge->Mem.Limit, Translation->MemTranslation
+ ));
+ DEBUG ((DEBUG_INFO, " MemAbove4G: %lx - %lx translation=%lx\n",
+ Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit, Translation->MemAbove4GTranslation
+ ));
+ DEBUG ((DEBUG_INFO, " PMem: %lx - %lx translation=%lx\n",
+ Bridge->PMem.Base, Bridge->PMem.Limit, Translation->PMemTranslation
+ ));
+ DEBUG ((DEBUG_INFO, " PMemAbove4G: %lx - %lx translation=%lx\n",
+ Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit, Translation->PMemAbove4GTranslation
+ ));
//
// Make sure Mem and MemAbove4G apertures are valid
@@ -174,10 +185,15 @@ CreateRootBridge (
CopyMem (&RootBridge->Bus, &Bridge->Bus, sizeof (PCI_ROOT_BRIDGE_APERTURE));
CopyMem (&RootBridge->Io, &Bridge->Io, sizeof (PCI_ROOT_BRIDGE_APERTURE));
+ RootBridge->IoTranslation = Translation->IoTranslation;
CopyMem (&RootBridge->Mem, &Bridge->Mem, sizeof (PCI_ROOT_BRIDGE_APERTURE));
+ RootBridge->MemTranslation = Translation->MemTranslation;
CopyMem (&RootBridge->MemAbove4G, &Bridge->MemAbove4G, sizeof (PCI_ROOT_BRIDGE_APERTURE));
+ RootBridge->MemAbove4GTranslation = Translation->MemAbove4GTranslation;
CopyMem (&RootBridge->PMem, &Bridge->PMem, sizeof (PCI_ROOT_BRIDGE_APERTURE));
+ RootBridge->PMemTranslation = Translation->PMemTranslation;
CopyMem (&RootBridge->PMemAbove4G, &Bridge->PMemAbove4G, sizeof (PCI_ROOT_BRIDGE_APERTURE));
+ RootBridge->PMemAbove4GTranslation = Translation->PMemAbove4GTranslation;
for (Index = TypeIo; Index < TypeMax; Index++) {
switch (Index) {
@@ -403,6 +419,28 @@ RootBridgeIoCheckParameter (
return EFI_SUCCESS;
}
+EFI_STATUS
+RootBridgeIoGetMemTranslation (
+ IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge,
+ IN UINT64 Address,
+ IN OUT UINT64 *Translation
+ )
+{
+ if (Address >= RootBridge->Mem.Base && Address <= RootBridge->Mem.Limit) {
+ *Translation = RootBridge->MemTranslation;
+ } else if (Address >= RootBridge->PMem.Base && Address <= RootBridge->PMem.Limit) {
+ *Translation = RootBridge->PMemTranslation;
+ } else if (Address >= RootBridge->MemAbove4G.Base && Address <= RootBridge->MemAbove4G.Limit) {
+ *Translation = RootBridge->MemAbove4GTranslation;
+ } else if (Address >= RootBridge->PMemAbove4G.Base && Address <= RootBridge->PMemAbove4G.Limit) {
+ *Translation = RootBridge->PMemAbove4GTranslation;
+ } 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 +696,22 @@ 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 = RootBridgeIoGetMemTranslation (RootBridge, Address, &Translation);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + Translation, Count, Buffer);
}
/**
@@ -705,13 +752,22 @@ 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 = RootBridgeIoGetMemTranslation (RootBridge, Address, &Translation);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + Translation, Count, Buffer);
}
/**
@@ -746,6 +802,8 @@ RootBridgeIoIoRead (
)
{
EFI_STATUS Status;
+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
+
Status = RootBridgeIoCheckParameter (
This, IoOperation, Width,
Address, Count, Buffer
@@ -753,7 +811,10 @@ 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);
+
+ return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + RootBridge->IoTranslation, Count, Buffer);
}
/**
@@ -788,6 +849,8 @@ RootBridgeIoIoWrite (
)
{
EFI_STATUS Status;
+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
+
Status = RootBridgeIoCheckParameter (
This, IoOperation, Width,
Address, Count, Buffer
@@ -795,7 +858,10 @@ 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);
+
+ return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + RootBridge->IoTranslation, Count, Buffer);
}
/**
@@ -1621,19 +1687,28 @@ RootBridgeIoConfiguration (
switch (ResAllocNode->Type) {
case TypeIo:
- Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
+ Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
+ Descriptor->AddrTranslationOffset = RootBridge->IoTranslation;
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->PMemTranslation;
+ Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
+ Descriptor->AddrSpaceGranularity = 32;
case TypeMem32:
+ Descriptor->AddrTranslationOffset = RootBridge->MemTranslation;
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->PMemAbove4GTranslation;
+ Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
+ Descriptor->AddrSpaceGranularity = 64;
case TypeMem64:
+ Descriptor->AddrTranslationOffset = RootBridge->MemAbove4GTranslation;
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..4c297fd 100644
--- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
+++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
@@ -53,6 +53,14 @@ typedef struct {
EFI_DEVICE_PATH_PROTOCOL *DevicePath; ///< Device path.
} PCI_ROOT_BRIDGE;
+typedef struct {
+ UINT64 IoTranslation;
+ UINT64 MemTranslation;
+ UINT64 MemAbove4GTranslation;
+ UINT64 PMemTranslation;
+ UINT64 PMemAbove4GTranslation;
+} PCI_ROOT_BRIDGE_TRANSLATION;
+
/**
Return all the root bridge instances in an array.
@@ -69,6 +77,21 @@ PciHostBridgeGetRootBridges (
);
/**
+ Return all the root bridge instances in an array.
+
+ @param Count Return the count of root bridge instances.
+
+ @return All the root bridge instances in an array.
+ The array should be passed into PciHostBridgeFreeRootBridges()
+ when it's not used.
+**/
+PCI_ROOT_BRIDGE_TRANSLATION *
+EFIAPI
+PciHostBridgeGetTranslations (
+ UINTN *Count
+ );
+
+/**
Free the root bridge instances array returned from PciHostBridgeGetRootBridges().
@param Bridges The root bridge instances array.
@@ -82,6 +105,19 @@ PciHostBridgeFreeRootBridges (
);
/**
+ Free the root bridge instances array returned from PciHostBridgeGetRootBridges().
+
+ @param Bridges The root bridge instances array.
+ @param Count The count of the array.
+**/
+VOID
+EFIAPI
+PciHostBridgeFreeTranslations (
+ PCI_ROOT_BRIDGE_TRANSLATION *Bridges,
+ UINTN Count
+ );
+
+/**
Inform the platform that the resource conflict happens.
@param HostBridgeHandle Handle of the Host Bridge.
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
2018-01-15 14:46 [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for address translation Heyi Guo
@ 2018-01-17 14:08 ` Ard Biesheuvel
2018-01-18 1:26 ` Guo Heyi
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2018-01-17 14:08 UTC (permalink / raw)
To: Heyi Guo
Cc: linaro-uefi, edk2-devel@lists.01.org, Ruiyu Ni, Star Zeng,
Eric Dong
On 15 January 2018 at 14:46, Heyi Guo <heyi.guo@linaro.org> wrote:
> This is the draft patch for the discussion posted in edk2-devel
> mailing list:
> https://lists.01.org/pipermail/edk2-devel/2017-December/019289.html
>
> As discussed in the mailing list, we'd like to add support for PCI
> address translation which is necessary for some non-x86 platforms. I
> also want to minimize the changes to the generic host bridge driver
> and platform PciHostBridgeLib implemetations, so additional two
> interfaces are added to expose translation information of the
> platform. To be generic, I add translation for each type of IO or
> memory resources.
>
> The patch is still a RFC, so I only passed the build for qemu64 and
> the function has not been tested yet.
>
> Please let me know your comments about it.
>
> Thanks.
>
> 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>
> ---
> .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 19 ++++
> .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 53 ++++++++---
> .../Bus/Pci/PciHostBridgeDxe/PciRootBridge.h | 8 +-
> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 101 ++++++++++++++++++---
> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 36 ++++++++
> 5 files changed, 192 insertions(+), 25 deletions(-)
>
> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> index 5b9c887..0c8371a 100644
> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> @@ -360,6 +360,16 @@ PciHostBridgeGetRootBridges (
> return &mRootBridge;
> }
>
> +PCI_ROOT_BRIDGE_TRANSLATION *
> +EFIAPI
> +PciHostBridgeGetTranslations (
> + UINTN *Count
> + )
> +{
> + *Count = 0;
> + return NULL;
> +}
> +
> /**
> Free the root bridge instances array returned from
> PciHostBridgeGetRootBridges().
> @@ -377,6 +387,15 @@ PciHostBridgeFreeRootBridges (
> ASSERT (Count == 1);
> }
>
> +VOID
> +EFIAPI
> +PciHostBridgeFreeTranslations (
> + PCI_ROOT_BRIDGE_TRANSLATION *Translations,
> + UINTN Count
> + )
> +{
> +}
> +
> /**
> Inform the platform that the resource conflict happens.
>
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> index 1494848..835e411 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> @@ -360,18 +360,38 @@ InitializePciHostBridge (
> PCI_HOST_BRIDGE_INSTANCE *HostBridge;
> PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> PCI_ROOT_BRIDGE *RootBridges;
> + PCI_ROOT_BRIDGE_TRANSLATION *Translations;
> UINTN RootBridgeCount;
> + UINTN TranslationCount;
> UINTN Index;
> PCI_ROOT_BRIDGE_APERTURE *MemApertures[4];
Wouldn't it be much better to add a 'translation' member to
PCI_ROOT_BRIDGE_APERTURE? That way, existing code just default to a
translation of 0, and all the handling of the separate array can be
dropped.
> + UINT64 MemTranslation[4];
> UINTN MemApertureIndex;
> BOOLEAN ResourceAssigned;
> LIST_ENTRY *Link;
> + UINT64 Trans;
>
> RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount);
> if ((RootBridges == NULL) || (RootBridgeCount == 0)) {
> return EFI_UNSUPPORTED;
> }
>
> + Translations = PciHostBridgeGetTranslations (&TranslationCount);
> + if (Translations == NULL || TranslationCount == 0) {
> + TranslationCount = 0;
> + Translations = AllocateZeroPool (RootBridgeCount * sizeof (*Translations));
> + if (Translations == NULL) {
> + PciHostBridgeFreeRootBridges (RootBridges, RootBridgeCount);
> + return EFI_OUT_OF_RESOURCES;
> + }
> + }
> +
> + if (TranslationCount != 0 && TranslationCount != RootBridgeCount) {
> + DEBUG ((DEBUG_ERROR, "Error: count of root bridges (%d) and translation (%d) are different!\n",
> + RootBridgeCount, TranslationCount));
> + return EFI_INVALID_PARAMETER;
> + }
> +
> Status = gBS->LocateProtocol (&gEfiMetronomeArchProtocolGuid, NULL, (VOID **) &mMetronome);
> ASSERT_EFI_ERROR (Status);
> Status = gBS->LocateProtocol (&gEfiCpuIo2ProtocolGuid, NULL, (VOID **) &mCpuIo);
> @@ -395,7 +415,7 @@ InitializePciHostBridge (
> //
> // Create Root Bridge Handle Instance
> //
> - RootBridge = CreateRootBridge (&RootBridges[Index]);
> + RootBridge = CreateRootBridge (&RootBridges[Index], &Translations[Index]);
> ASSERT (RootBridge != NULL);
> if (RootBridge == NULL) {
> continue;
> @@ -411,8 +431,9 @@ InitializePciHostBridge (
> }
>
> if (RootBridges[Index].Io.Base <= RootBridges[Index].Io.Limit) {
> + Trans = Translations[Index].IoTranslation;
> Status = AddIoSpace (
> - RootBridges[Index].Io.Base,
> + RootBridges[Index].Io.Base + Trans,
> RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1
> );
> ASSERT_EFI_ERROR (Status);
> @@ -422,7 +443,7 @@ InitializePciHostBridge (
> EfiGcdIoTypeIo,
> 0,
> RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1,
> - &RootBridges[Index].Io.Base,
> + &RootBridges[Index].Io.Base + Trans,
> gImageHandle,
> NULL
> );
> @@ -437,20 +458,24 @@ InitializePciHostBridge (
> // the MEM aperture in Mem
> //
> MemApertures[0] = &RootBridges[Index].Mem;
> + MemTranslation[0] = Translations[Index].MemTranslation;
> MemApertures[1] = &RootBridges[Index].MemAbove4G;
> + MemTranslation[1] = Translations[Index].MemAbove4GTranslation;
> MemApertures[2] = &RootBridges[Index].PMem;
> + MemTranslation[2] = Translations[Index].PMemTranslation;
> MemApertures[3] = &RootBridges[Index].PMemAbove4G;
> + MemTranslation[3] = Translations[Index].PMemAbove4GTranslation;
>
> for (MemApertureIndex = 0; MemApertureIndex < ARRAY_SIZE (MemApertures); MemApertureIndex++) {
> if (MemApertures[MemApertureIndex]->Base <= MemApertures[MemApertureIndex]->Limit) {
> Status = AddMemoryMappedIoSpace (
> - MemApertures[MemApertureIndex]->Base,
> + MemApertures[MemApertureIndex]->Base + MemTranslation[MemApertureIndex],
> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
> EFI_MEMORY_UC
> );
> ASSERT_EFI_ERROR (Status);
> Status = gDS->SetMemorySpaceAttributes (
> - MemApertures[MemApertureIndex]->Base,
> + MemApertures[MemApertureIndex]->Base + MemTranslation[MemApertureIndex],
> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
> EFI_MEMORY_UC
> );
> @@ -463,7 +488,7 @@ InitializePciHostBridge (
> EfiGcdMemoryTypeMemoryMappedIo,
> 0,
> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
> - &MemApertures[MemApertureIndex]->Base,
> + &MemApertures[MemApertureIndex]->Base + MemTranslation[MemApertureIndex],
> gImageHandle,
> NULL
> );
> @@ -514,7 +539,13 @@ InitializePciHostBridge (
> );
> ASSERT_EFI_ERROR (Status);
> }
> +
> PciHostBridgeFreeRootBridges (RootBridges, RootBridgeCount);
> + if (TranslationCount == 0) {
> + FreePool (Translations);
> + } else {
> + PciHostBridgeFreeTranslations (Translations, TranslationCount);
> + }
>
> if (!EFI_ERROR (Status)) {
> mIoMmuEvent = EfiCreateProtocolNotifyEvent (
> @@ -828,7 +859,7 @@ NotifyPhase (
> FALSE,
> RootBridge->ResAllocNode[Index].Length,
> MIN (15, BitsOfAlignment),
> - ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1),
> + ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1) + RootBridge->IoTranslation,
> RootBridge->Io.Limit
> );
> break;
> @@ -838,7 +869,7 @@ NotifyPhase (
> TRUE,
> RootBridge->ResAllocNode[Index].Length,
> MIN (63, BitsOfAlignment),
> - ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1),
> + ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1) + RootBridge->MemAbove4GTranslation,
> RootBridge->MemAbove4G.Limit
> );
> if (BaseAddress != MAX_UINT64) {
> @@ -853,7 +884,7 @@ NotifyPhase (
> TRUE,
> RootBridge->ResAllocNode[Index].Length,
> MIN (31, BitsOfAlignment),
> - ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1),
> + ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1) + RootBridge->MemTranslation,
> RootBridge->Mem.Limit
> );
> break;
> @@ -863,7 +894,7 @@ NotifyPhase (
> TRUE,
> RootBridge->ResAllocNode[Index].Length,
> MIN (63, BitsOfAlignment),
> - ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1),
> + ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1) + RootBridge->PMemAbove4GTranslation,
> RootBridge->PMemAbove4G.Limit
> );
> if (BaseAddress != MAX_UINT64) {
> @@ -877,7 +908,7 @@ NotifyPhase (
> TRUE,
> RootBridge->ResAllocNode[Index].Length,
> MIN (31, BitsOfAlignment),
> - ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1),
> + ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1) + RootBridge->PMemTranslation,
> RootBridge->PMem.Limit
> );
> break;
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> index d3dfb57..449c4b3 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> @@ -73,6 +73,11 @@ typedef struct {
> PCI_ROOT_BRIDGE_APERTURE PMem;
> PCI_ROOT_BRIDGE_APERTURE MemAbove4G;
> PCI_ROOT_BRIDGE_APERTURE PMemAbove4G;
> + UINT64 IoTranslation;
> + UINT64 MemTranslation;
> + UINT64 MemAbove4GTranslation;
> + UINT64 PMemTranslation;
> + UINT64 PMemAbove4GTranslation;
> BOOLEAN DmaAbove4G;
> BOOLEAN NoExtendedConfigSpace;
> VOID *ConfigBuffer;
> @@ -98,7 +103,8 @@ typedef struct {
> **/
> PCI_ROOT_BRIDGE_INSTANCE *
> CreateRootBridge (
> - IN PCI_ROOT_BRIDGE *Bridge
> + IN PCI_ROOT_BRIDGE *Bridge,
> + IN PCI_ROOT_BRIDGE_TRANSLATION *Translation
> );
>
> //
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index dc06c16..84b2d5a 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -67,7 +67,8 @@ UINT8 mOutStride[] = {
> **/
> PCI_ROOT_BRIDGE_INSTANCE *
> CreateRootBridge (
> - IN PCI_ROOT_BRIDGE *Bridge
> + IN PCI_ROOT_BRIDGE *Bridge,
> + IN PCI_ROOT_BRIDGE_TRANSLATION *Translation
> )
> {
> PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> @@ -87,11 +88,21 @@ CreateRootBridge (
> (Bridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_MEM64_DECODE) != 0 ? L"Mem64Decode" : L""
> ));
> 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, Translation->IoTranslation
> + ));
> + DEBUG ((DEBUG_INFO, " Mem: %lx - %lx translation=%lx\n",
> + Bridge->Mem.Base, Bridge->Mem.Limit, Translation->MemTranslation
> + ));
> + DEBUG ((DEBUG_INFO, " MemAbove4G: %lx - %lx translation=%lx\n",
> + Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit, Translation->MemAbove4GTranslation
> + ));
> + DEBUG ((DEBUG_INFO, " PMem: %lx - %lx translation=%lx\n",
> + Bridge->PMem.Base, Bridge->PMem.Limit, Translation->PMemTranslation
> + ));
> + DEBUG ((DEBUG_INFO, " PMemAbove4G: %lx - %lx translation=%lx\n",
> + Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit, Translation->PMemAbove4GTranslation
> + ));
>
> //
> // Make sure Mem and MemAbove4G apertures are valid
> @@ -174,10 +185,15 @@ CreateRootBridge (
>
> CopyMem (&RootBridge->Bus, &Bridge->Bus, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> CopyMem (&RootBridge->Io, &Bridge->Io, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> + RootBridge->IoTranslation = Translation->IoTranslation;
> CopyMem (&RootBridge->Mem, &Bridge->Mem, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> + RootBridge->MemTranslation = Translation->MemTranslation;
> CopyMem (&RootBridge->MemAbove4G, &Bridge->MemAbove4G, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> + RootBridge->MemAbove4GTranslation = Translation->MemAbove4GTranslation;
> CopyMem (&RootBridge->PMem, &Bridge->PMem, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> + RootBridge->PMemTranslation = Translation->PMemTranslation;
> CopyMem (&RootBridge->PMemAbove4G, &Bridge->PMemAbove4G, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> + RootBridge->PMemAbove4GTranslation = Translation->PMemAbove4GTranslation;
>
> for (Index = TypeIo; Index < TypeMax; Index++) {
> switch (Index) {
> @@ -403,6 +419,28 @@ RootBridgeIoCheckParameter (
> return EFI_SUCCESS;
> }
>
> +EFI_STATUS
> +RootBridgeIoGetMemTranslation (
> + IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge,
> + IN UINT64 Address,
> + IN OUT UINT64 *Translation
> + )
> +{
> + if (Address >= RootBridge->Mem.Base && Address <= RootBridge->Mem.Limit) {
> + *Translation = RootBridge->MemTranslation;
> + } else if (Address >= RootBridge->PMem.Base && Address <= RootBridge->PMem.Limit) {
> + *Translation = RootBridge->PMemTranslation;
> + } else if (Address >= RootBridge->MemAbove4G.Base && Address <= RootBridge->MemAbove4G.Limit) {
> + *Translation = RootBridge->MemAbove4GTranslation;
> + } else if (Address >= RootBridge->PMemAbove4G.Base && Address <= RootBridge->PMemAbove4G.Limit) {
> + *Translation = RootBridge->PMemAbove4GTranslation;
> + } 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 +696,22 @@ 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 = RootBridgeIoGetMemTranslation (RootBridge, Address, &Translation);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + Translation, Count, Buffer);
> }
>
> /**
> @@ -705,13 +752,22 @@ 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 = RootBridgeIoGetMemTranslation (RootBridge, Address, &Translation);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + Translation, Count, Buffer);
> }
>
> /**
> @@ -746,6 +802,8 @@ RootBridgeIoIoRead (
> )
> {
> EFI_STATUS Status;
> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> +
> Status = RootBridgeIoCheckParameter (
> This, IoOperation, Width,
> Address, Count, Buffer
> @@ -753,7 +811,10 @@ 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);
> +
> + return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + RootBridge->IoTranslation, Count, Buffer);
> }
>
> /**
> @@ -788,6 +849,8 @@ RootBridgeIoIoWrite (
> )
> {
> EFI_STATUS Status;
> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> +
> Status = RootBridgeIoCheckParameter (
> This, IoOperation, Width,
> Address, Count, Buffer
> @@ -795,7 +858,10 @@ 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);
> +
> + return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + RootBridge->IoTranslation, Count, Buffer);
> }
>
> /**
> @@ -1621,19 +1687,28 @@ RootBridgeIoConfiguration (
> switch (ResAllocNode->Type) {
>
> case TypeIo:
> - Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
> + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
> + Descriptor->AddrTranslationOffset = RootBridge->IoTranslation;
> 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->PMemTranslation;
> + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
> + Descriptor->AddrSpaceGranularity = 32;
> case TypeMem32:
> + Descriptor->AddrTranslationOffset = RootBridge->MemTranslation;
> 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->PMemAbove4GTranslation;
> + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
> + Descriptor->AddrSpaceGranularity = 64;
> case TypeMem64:
> + Descriptor->AddrTranslationOffset = RootBridge->MemAbove4GTranslation;
> 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..4c297fd 100644
> --- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> +++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> @@ -53,6 +53,14 @@ typedef struct {
> EFI_DEVICE_PATH_PROTOCOL *DevicePath; ///< Device path.
> } PCI_ROOT_BRIDGE;
>
> +typedef struct {
> + UINT64 IoTranslation;
> + UINT64 MemTranslation;
> + UINT64 MemAbove4GTranslation;
> + UINT64 PMemTranslation;
> + UINT64 PMemAbove4GTranslation;
> +} PCI_ROOT_BRIDGE_TRANSLATION;
> +
> /**
> Return all the root bridge instances in an array.
>
> @@ -69,6 +77,21 @@ PciHostBridgeGetRootBridges (
> );
>
> /**
> + Return all the root bridge instances in an array.
> +
> + @param Count Return the count of root bridge instances.
> +
> + @return All the root bridge instances in an array.
> + The array should be passed into PciHostBridgeFreeRootBridges()
> + when it's not used.
> +**/
> +PCI_ROOT_BRIDGE_TRANSLATION *
> +EFIAPI
> +PciHostBridgeGetTranslations (
> + UINTN *Count
> + );
> +
> +/**
> Free the root bridge instances array returned from PciHostBridgeGetRootBridges().
>
> @param Bridges The root bridge instances array.
> @@ -82,6 +105,19 @@ PciHostBridgeFreeRootBridges (
> );
>
> /**
> + Free the root bridge instances array returned from PciHostBridgeGetRootBridges().
> +
> + @param Bridges The root bridge instances array.
> + @param Count The count of the array.
> +**/
> +VOID
> +EFIAPI
> +PciHostBridgeFreeTranslations (
> + PCI_ROOT_BRIDGE_TRANSLATION *Bridges,
> + UINTN Count
> + );
> +
> +/**
> Inform the platform that the resource conflict happens.
>
> @param HostBridgeHandle Handle of the Host Bridge.
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
2018-01-17 14:08 ` Ard Biesheuvel
@ 2018-01-18 1:26 ` Guo Heyi
2018-01-22 3:36 ` Ni, Ruiyu
0 siblings, 1 reply; 12+ messages in thread
From: Guo Heyi @ 2018-01-18 1:26 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Heyi Guo, linaro-uefi, edk2-devel@lists.01.org, Ruiyu Ni,
Star Zeng, Eric Dong
On Wed, Jan 17, 2018 at 02:08:06PM +0000, Ard Biesheuvel wrote:
> On 15 January 2018 at 14:46, Heyi Guo <heyi.guo@linaro.org> wrote:
> > This is the draft patch for the discussion posted in edk2-devel
> > mailing list:
> > https://lists.01.org/pipermail/edk2-devel/2017-December/019289.html
> >
> > As discussed in the mailing list, we'd like to add support for PCI
> > address translation which is necessary for some non-x86 platforms. I
> > also want to minimize the changes to the generic host bridge driver
> > and platform PciHostBridgeLib implemetations, so additional two
> > interfaces are added to expose translation information of the
> > platform. To be generic, I add translation for each type of IO or
> > memory resources.
> >
> > The patch is still a RFC, so I only passed the build for qemu64 and
> > the function has not been tested yet.
> >
> > Please let me know your comments about it.
> >
> > Thanks.
> >
> > 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>
> > ---
> > .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 19 ++++
> > .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 53 ++++++++---
> > .../Bus/Pci/PciHostBridgeDxe/PciRootBridge.h | 8 +-
> > .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 101 ++++++++++++++++++---
> > MdeModulePkg/Include/Library/PciHostBridgeLib.h | 36 ++++++++
> > 5 files changed, 192 insertions(+), 25 deletions(-)
> >
> > diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> > index 5b9c887..0c8371a 100644
> > --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> > +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> > @@ -360,6 +360,16 @@ PciHostBridgeGetRootBridges (
> > return &mRootBridge;
> > }
> >
> > +PCI_ROOT_BRIDGE_TRANSLATION *
> > +EFIAPI
> > +PciHostBridgeGetTranslations (
> > + UINTN *Count
> > + )
> > +{
> > + *Count = 0;
> > + return NULL;
> > +}
> > +
> > /**
> > Free the root bridge instances array returned from
> > PciHostBridgeGetRootBridges().
> > @@ -377,6 +387,15 @@ PciHostBridgeFreeRootBridges (
> > ASSERT (Count == 1);
> > }
> >
> > +VOID
> > +EFIAPI
> > +PciHostBridgeFreeTranslations (
> > + PCI_ROOT_BRIDGE_TRANSLATION *Translations,
> > + UINTN Count
> > + )
> > +{
> > +}
> > +
> > /**
> > Inform the platform that the resource conflict happens.
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > index 1494848..835e411 100644
> > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > @@ -360,18 +360,38 @@ InitializePciHostBridge (
> > PCI_HOST_BRIDGE_INSTANCE *HostBridge;
> > PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> > PCI_ROOT_BRIDGE *RootBridges;
> > + PCI_ROOT_BRIDGE_TRANSLATION *Translations;
> > UINTN RootBridgeCount;
> > + UINTN TranslationCount;
> > UINTN Index;
> > PCI_ROOT_BRIDGE_APERTURE *MemApertures[4];
>
> Wouldn't it be much better to add a 'translation' member to
> PCI_ROOT_BRIDGE_APERTURE? That way, existing code just default to a
> translation of 0, and all the handling of the separate array can be
> dropped.
>
Actually my first idea was the same, but when I looked at the implementation of
PciHostBridgeLib of Ovmf, I found it a little tedious to change the
existing code in this way. We'll need to check everywhere
PCI_ROOT_BRIDGE_APERTURE or PCI_ROOT_BRIDGE is used, to make sure the
translation field is initialized to be zero, e.g. line 235~245.
What I did in this RFC is not so straightforward indeed. So I can change the
code if we all agree to add Translation field into PCI_ROOT_BRIDGE_APERTURE
directly.
Thanks,
Gary (Heyi Guo)
> > + UINT64 MemTranslation[4];
> > UINTN MemApertureIndex;
> > BOOLEAN ResourceAssigned;
> > LIST_ENTRY *Link;
> > + UINT64 Trans;
> >
> > RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount);
> > if ((RootBridges == NULL) || (RootBridgeCount == 0)) {
> > return EFI_UNSUPPORTED;
> > }
> >
> > + Translations = PciHostBridgeGetTranslations (&TranslationCount);
> > + if (Translations == NULL || TranslationCount == 0) {
> > + TranslationCount = 0;
> > + Translations = AllocateZeroPool (RootBridgeCount * sizeof (*Translations));
> > + if (Translations == NULL) {
> > + PciHostBridgeFreeRootBridges (RootBridges, RootBridgeCount);
> > + return EFI_OUT_OF_RESOURCES;
> > + }
> > + }
> > +
> > + if (TranslationCount != 0 && TranslationCount != RootBridgeCount) {
> > + DEBUG ((DEBUG_ERROR, "Error: count of root bridges (%d) and translation (%d) are different!\n",
> > + RootBridgeCount, TranslationCount));
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > Status = gBS->LocateProtocol (&gEfiMetronomeArchProtocolGuid, NULL, (VOID **) &mMetronome);
> > ASSERT_EFI_ERROR (Status);
> > Status = gBS->LocateProtocol (&gEfiCpuIo2ProtocolGuid, NULL, (VOID **) &mCpuIo);
> > @@ -395,7 +415,7 @@ InitializePciHostBridge (
> > //
> > // Create Root Bridge Handle Instance
> > //
> > - RootBridge = CreateRootBridge (&RootBridges[Index]);
> > + RootBridge = CreateRootBridge (&RootBridges[Index], &Translations[Index]);
> > ASSERT (RootBridge != NULL);
> > if (RootBridge == NULL) {
> > continue;
> > @@ -411,8 +431,9 @@ InitializePciHostBridge (
> > }
> >
> > if (RootBridges[Index].Io.Base <= RootBridges[Index].Io.Limit) {
> > + Trans = Translations[Index].IoTranslation;
> > Status = AddIoSpace (
> > - RootBridges[Index].Io.Base,
> > + RootBridges[Index].Io.Base + Trans,
> > RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1
> > );
> > ASSERT_EFI_ERROR (Status);
> > @@ -422,7 +443,7 @@ InitializePciHostBridge (
> > EfiGcdIoTypeIo,
> > 0,
> > RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1,
> > - &RootBridges[Index].Io.Base,
> > + &RootBridges[Index].Io.Base + Trans,
> > gImageHandle,
> > NULL
> > );
> > @@ -437,20 +458,24 @@ InitializePciHostBridge (
> > // the MEM aperture in Mem
> > //
> > MemApertures[0] = &RootBridges[Index].Mem;
> > + MemTranslation[0] = Translations[Index].MemTranslation;
> > MemApertures[1] = &RootBridges[Index].MemAbove4G;
> > + MemTranslation[1] = Translations[Index].MemAbove4GTranslation;
> > MemApertures[2] = &RootBridges[Index].PMem;
> > + MemTranslation[2] = Translations[Index].PMemTranslation;
> > MemApertures[3] = &RootBridges[Index].PMemAbove4G;
> > + MemTranslation[3] = Translations[Index].PMemAbove4GTranslation;
> >
> > for (MemApertureIndex = 0; MemApertureIndex < ARRAY_SIZE (MemApertures); MemApertureIndex++) {
> > if (MemApertures[MemApertureIndex]->Base <= MemApertures[MemApertureIndex]->Limit) {
> > Status = AddMemoryMappedIoSpace (
> > - MemApertures[MemApertureIndex]->Base,
> > + MemApertures[MemApertureIndex]->Base + MemTranslation[MemApertureIndex],
> > MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
> > EFI_MEMORY_UC
> > );
> > ASSERT_EFI_ERROR (Status);
> > Status = gDS->SetMemorySpaceAttributes (
> > - MemApertures[MemApertureIndex]->Base,
> > + MemApertures[MemApertureIndex]->Base + MemTranslation[MemApertureIndex],
> > MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
> > EFI_MEMORY_UC
> > );
> > @@ -463,7 +488,7 @@ InitializePciHostBridge (
> > EfiGcdMemoryTypeMemoryMappedIo,
> > 0,
> > MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
> > - &MemApertures[MemApertureIndex]->Base,
> > + &MemApertures[MemApertureIndex]->Base + MemTranslation[MemApertureIndex],
> > gImageHandle,
> > NULL
> > );
> > @@ -514,7 +539,13 @@ InitializePciHostBridge (
> > );
> > ASSERT_EFI_ERROR (Status);
> > }
> > +
> > PciHostBridgeFreeRootBridges (RootBridges, RootBridgeCount);
> > + if (TranslationCount == 0) {
> > + FreePool (Translations);
> > + } else {
> > + PciHostBridgeFreeTranslations (Translations, TranslationCount);
> > + }
> >
> > if (!EFI_ERROR (Status)) {
> > mIoMmuEvent = EfiCreateProtocolNotifyEvent (
> > @@ -828,7 +859,7 @@ NotifyPhase (
> > FALSE,
> > RootBridge->ResAllocNode[Index].Length,
> > MIN (15, BitsOfAlignment),
> > - ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1),
> > + ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1) + RootBridge->IoTranslation,
> > RootBridge->Io.Limit
> > );
> > break;
> > @@ -838,7 +869,7 @@ NotifyPhase (
> > TRUE,
> > RootBridge->ResAllocNode[Index].Length,
> > MIN (63, BitsOfAlignment),
> > - ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1),
> > + ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1) + RootBridge->MemAbove4GTranslation,
> > RootBridge->MemAbove4G.Limit
> > );
> > if (BaseAddress != MAX_UINT64) {
> > @@ -853,7 +884,7 @@ NotifyPhase (
> > TRUE,
> > RootBridge->ResAllocNode[Index].Length,
> > MIN (31, BitsOfAlignment),
> > - ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1),
> > + ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1) + RootBridge->MemTranslation,
> > RootBridge->Mem.Limit
> > );
> > break;
> > @@ -863,7 +894,7 @@ NotifyPhase (
> > TRUE,
> > RootBridge->ResAllocNode[Index].Length,
> > MIN (63, BitsOfAlignment),
> > - ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1),
> > + ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1) + RootBridge->PMemAbove4GTranslation,
> > RootBridge->PMemAbove4G.Limit
> > );
> > if (BaseAddress != MAX_UINT64) {
> > @@ -877,7 +908,7 @@ NotifyPhase (
> > TRUE,
> > RootBridge->ResAllocNode[Index].Length,
> > MIN (31, BitsOfAlignment),
> > - ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1),
> > + ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1) + RootBridge->PMemTranslation,
> > RootBridge->PMem.Limit
> > );
> > break;
> > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> > index d3dfb57..449c4b3 100644
> > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> > @@ -73,6 +73,11 @@ typedef struct {
> > PCI_ROOT_BRIDGE_APERTURE PMem;
> > PCI_ROOT_BRIDGE_APERTURE MemAbove4G;
> > PCI_ROOT_BRIDGE_APERTURE PMemAbove4G;
> > + UINT64 IoTranslation;
> > + UINT64 MemTranslation;
> > + UINT64 MemAbove4GTranslation;
> > + UINT64 PMemTranslation;
> > + UINT64 PMemAbove4GTranslation;
> > BOOLEAN DmaAbove4G;
> > BOOLEAN NoExtendedConfigSpace;
> > VOID *ConfigBuffer;
> > @@ -98,7 +103,8 @@ typedef struct {
> > **/
> > PCI_ROOT_BRIDGE_INSTANCE *
> > CreateRootBridge (
> > - IN PCI_ROOT_BRIDGE *Bridge
> > + IN PCI_ROOT_BRIDGE *Bridge,
> > + IN PCI_ROOT_BRIDGE_TRANSLATION *Translation
> > );
> >
> > //
> > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> > index dc06c16..84b2d5a 100644
> > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> > @@ -67,7 +67,8 @@ UINT8 mOutStride[] = {
> > **/
> > PCI_ROOT_BRIDGE_INSTANCE *
> > CreateRootBridge (
> > - IN PCI_ROOT_BRIDGE *Bridge
> > + IN PCI_ROOT_BRIDGE *Bridge,
> > + IN PCI_ROOT_BRIDGE_TRANSLATION *Translation
> > )
> > {
> > PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> > @@ -87,11 +88,21 @@ CreateRootBridge (
> > (Bridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_MEM64_DECODE) != 0 ? L"Mem64Decode" : L""
> > ));
> > 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, Translation->IoTranslation
> > + ));
> > + DEBUG ((DEBUG_INFO, " Mem: %lx - %lx translation=%lx\n",
> > + Bridge->Mem.Base, Bridge->Mem.Limit, Translation->MemTranslation
> > + ));
> > + DEBUG ((DEBUG_INFO, " MemAbove4G: %lx - %lx translation=%lx\n",
> > + Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit, Translation->MemAbove4GTranslation
> > + ));
> > + DEBUG ((DEBUG_INFO, " PMem: %lx - %lx translation=%lx\n",
> > + Bridge->PMem.Base, Bridge->PMem.Limit, Translation->PMemTranslation
> > + ));
> > + DEBUG ((DEBUG_INFO, " PMemAbove4G: %lx - %lx translation=%lx\n",
> > + Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit, Translation->PMemAbove4GTranslation
> > + ));
> >
> > //
> > // Make sure Mem and MemAbove4G apertures are valid
> > @@ -174,10 +185,15 @@ CreateRootBridge (
> >
> > CopyMem (&RootBridge->Bus, &Bridge->Bus, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> > CopyMem (&RootBridge->Io, &Bridge->Io, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> > + RootBridge->IoTranslation = Translation->IoTranslation;
> > CopyMem (&RootBridge->Mem, &Bridge->Mem, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> > + RootBridge->MemTranslation = Translation->MemTranslation;
> > CopyMem (&RootBridge->MemAbove4G, &Bridge->MemAbove4G, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> > + RootBridge->MemAbove4GTranslation = Translation->MemAbove4GTranslation;
> > CopyMem (&RootBridge->PMem, &Bridge->PMem, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> > + RootBridge->PMemTranslation = Translation->PMemTranslation;
> > CopyMem (&RootBridge->PMemAbove4G, &Bridge->PMemAbove4G, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> > + RootBridge->PMemAbove4GTranslation = Translation->PMemAbove4GTranslation;
> >
> > for (Index = TypeIo; Index < TypeMax; Index++) {
> > switch (Index) {
> > @@ -403,6 +419,28 @@ RootBridgeIoCheckParameter (
> > return EFI_SUCCESS;
> > }
> >
> > +EFI_STATUS
> > +RootBridgeIoGetMemTranslation (
> > + IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge,
> > + IN UINT64 Address,
> > + IN OUT UINT64 *Translation
> > + )
> > +{
> > + if (Address >= RootBridge->Mem.Base && Address <= RootBridge->Mem.Limit) {
> > + *Translation = RootBridge->MemTranslation;
> > + } else if (Address >= RootBridge->PMem.Base && Address <= RootBridge->PMem.Limit) {
> > + *Translation = RootBridge->PMemTranslation;
> > + } else if (Address >= RootBridge->MemAbove4G.Base && Address <= RootBridge->MemAbove4G.Limit) {
> > + *Translation = RootBridge->MemAbove4GTranslation;
> > + } else if (Address >= RootBridge->PMemAbove4G.Base && Address <= RootBridge->PMemAbove4G.Limit) {
> > + *Translation = RootBridge->PMemAbove4GTranslation;
> > + } 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 +696,22 @@ 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 = RootBridgeIoGetMemTranslation (RootBridge, Address, &Translation);
> > + if (EFI_ERROR (Status)) {
> > + return Status;
> > + }
> > +
> > + return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + Translation, Count, Buffer);
> > }
> >
> > /**
> > @@ -705,13 +752,22 @@ 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 = RootBridgeIoGetMemTranslation (RootBridge, Address, &Translation);
> > + if (EFI_ERROR (Status)) {
> > + return Status;
> > + }
> > +
> > + return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + Translation, Count, Buffer);
> > }
> >
> > /**
> > @@ -746,6 +802,8 @@ RootBridgeIoIoRead (
> > )
> > {
> > EFI_STATUS Status;
> > + PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> > +
> > Status = RootBridgeIoCheckParameter (
> > This, IoOperation, Width,
> > Address, Count, Buffer
> > @@ -753,7 +811,10 @@ 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);
> > +
> > + return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + RootBridge->IoTranslation, Count, Buffer);
> > }
> >
> > /**
> > @@ -788,6 +849,8 @@ RootBridgeIoIoWrite (
> > )
> > {
> > EFI_STATUS Status;
> > + PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> > +
> > Status = RootBridgeIoCheckParameter (
> > This, IoOperation, Width,
> > Address, Count, Buffer
> > @@ -795,7 +858,10 @@ 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);
> > +
> > + return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + RootBridge->IoTranslation, Count, Buffer);
> > }
> >
> > /**
> > @@ -1621,19 +1687,28 @@ RootBridgeIoConfiguration (
> > switch (ResAllocNode->Type) {
> >
> > case TypeIo:
> > - Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
> > + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
> > + Descriptor->AddrTranslationOffset = RootBridge->IoTranslation;
> > 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->PMemTranslation;
> > + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
> > + Descriptor->AddrSpaceGranularity = 32;
> > case TypeMem32:
> > + Descriptor->AddrTranslationOffset = RootBridge->MemTranslation;
> > 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->PMemAbove4GTranslation;
> > + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
> > + Descriptor->AddrSpaceGranularity = 64;
> > case TypeMem64:
> > + Descriptor->AddrTranslationOffset = RootBridge->MemAbove4GTranslation;
> > 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..4c297fd 100644
> > --- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> > +++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> > @@ -53,6 +53,14 @@ typedef struct {
> > EFI_DEVICE_PATH_PROTOCOL *DevicePath; ///< Device path.
> > } PCI_ROOT_BRIDGE;
> >
> > +typedef struct {
> > + UINT64 IoTranslation;
> > + UINT64 MemTranslation;
> > + UINT64 MemAbove4GTranslation;
> > + UINT64 PMemTranslation;
> > + UINT64 PMemAbove4GTranslation;
> > +} PCI_ROOT_BRIDGE_TRANSLATION;
> > +
> > /**
> > Return all the root bridge instances in an array.
> >
> > @@ -69,6 +77,21 @@ PciHostBridgeGetRootBridges (
> > );
> >
> > /**
> > + Return all the root bridge instances in an array.
> > +
> > + @param Count Return the count of root bridge instances.
> > +
> > + @return All the root bridge instances in an array.
> > + The array should be passed into PciHostBridgeFreeRootBridges()
> > + when it's not used.
> > +**/
> > +PCI_ROOT_BRIDGE_TRANSLATION *
> > +EFIAPI
> > +PciHostBridgeGetTranslations (
> > + UINTN *Count
> > + );
> > +
> > +/**
> > Free the root bridge instances array returned from PciHostBridgeGetRootBridges().
> >
> > @param Bridges The root bridge instances array.
> > @@ -82,6 +105,19 @@ PciHostBridgeFreeRootBridges (
> > );
> >
> > /**
> > + Free the root bridge instances array returned from PciHostBridgeGetRootBridges().
> > +
> > + @param Bridges The root bridge instances array.
> > + @param Count The count of the array.
> > +**/
> > +VOID
> > +EFIAPI
> > +PciHostBridgeFreeTranslations (
> > + PCI_ROOT_BRIDGE_TRANSLATION *Bridges,
> > + UINTN Count
> > + );
> > +
> > +/**
> > Inform the platform that the resource conflict happens.
> >
> > @param HostBridgeHandle Handle of the Host Bridge.
> > --
> > 2.7.4
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
2018-01-18 1:26 ` Guo Heyi
@ 2018-01-22 3:36 ` Ni, Ruiyu
2018-01-29 8:50 ` Guo Heyi
0 siblings, 1 reply; 12+ messages in thread
From: Ni, Ruiyu @ 2018-01-22 3:36 UTC (permalink / raw)
To: Guo Heyi, Michael D, Ard Biesheuvel
Cc: Eric Dong, edk2-devel@lists.01.org, linaro-uefi, Star Zeng
On 1/18/2018 9:26 AM, Guo Heyi wrote:
> On Wed, Jan 17, 2018 at 02:08:06PM +0000, Ard Biesheuvel wrote:
>> On 15 January 2018 at 14:46, Heyi Guo <heyi.guo@linaro.org> wrote:
>>> This is the draft patch for the discussion posted in edk2-devel
>>> mailing list:
>>> https://lists.01.org/pipermail/edk2-devel/2017-December/019289.html
>>>
>>> As discussed in the mailing list, we'd like to add support for PCI
>>> address translation which is necessary for some non-x86 platforms. I
>>> also want to minimize the changes to the generic host bridge driver
>>> and platform PciHostBridgeLib implemetations, so additional two
>>> interfaces are added to expose translation information of the
>>> platform. To be generic, I add translation for each type of IO or
>>> memory resources.
>>>
>>> The patch is still a RFC, so I only passed the build for qemu64 and
>>> the function has not been tested yet.
>>>
>>> Please let me know your comments about it.
>>>
>>> Thanks.
>>>
>>> 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>
>>> ---
>>> .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 19 ++++
>>> .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 53 ++++++++---
>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridge.h | 8 +-
>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 101 ++++++++++++++++++---
>>> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 36 ++++++++
>>> 5 files changed, 192 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>> index 5b9c887..0c8371a 100644
>>> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>> @@ -360,6 +360,16 @@ PciHostBridgeGetRootBridges (
>>> return &mRootBridge;
>>> }
>>>
>>> +PCI_ROOT_BRIDGE_TRANSLATION *
>>> +EFIAPI
>>> +PciHostBridgeGetTranslations (
>>> + UINTN *Count
>>> + )
>>> +{
>>> + *Count = 0;
>>> + return NULL;
>>> +}
>>> +
>>> /**
>>> Free the root bridge instances array returned from
>>> PciHostBridgeGetRootBridges().
>>> @@ -377,6 +387,15 @@ PciHostBridgeFreeRootBridges (
>>> ASSERT (Count == 1);
>>> }
>>>
>>> +VOID
>>> +EFIAPI
>>> +PciHostBridgeFreeTranslations (
>>> + PCI_ROOT_BRIDGE_TRANSLATION *Translations,
>>> + UINTN Count
>>> + )
>>> +{
>>> +}
>>> +
>>> /**
>>> Inform the platform that the resource conflict happens.
>>>
>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>> index 1494848..835e411 100644
>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>> @@ -360,18 +360,38 @@ InitializePciHostBridge (
>>> PCI_HOST_BRIDGE_INSTANCE *HostBridge;
>>> PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
>>> PCI_ROOT_BRIDGE *RootBridges;
>>> + PCI_ROOT_BRIDGE_TRANSLATION *Translations;
>>> UINTN RootBridgeCount;
>>> + UINTN TranslationCount;
>>> UINTN Index;
>>> PCI_ROOT_BRIDGE_APERTURE *MemApertures[4];
>>
>> Wouldn't it be much better to add a 'translation' member to
>> PCI_ROOT_BRIDGE_APERTURE? That way, existing code just default to a
>> translation of 0, and all the handling of the separate array can be
>> dropped.
>>
> Actually my first idea was the same, but when I looked at the implementation of
> PciHostBridgeLib of Ovmf, I found it a little tedious to change the
> existing code in this way. We'll need to check everywhere
> PCI_ROOT_BRIDGE_APERTURE or PCI_ROOT_BRIDGE is used, to make sure the
> translation field is initialized to be zero, e.g. line 235~245.
>
> What I did in this RFC is not so straightforward indeed. So I can change the
> code if we all agree to add Translation field into PCI_ROOT_BRIDGE_APERTURE
> directly.
>
> Thanks,
>
> Gary (Heyi Guo)
I also agree to put the translation fields into the
PCI_ROOT_BRIDGE_APERTURE.
But I think we may have different understandings to the address
translation.
My understanding to the whole translation:
1. PciHostBridge.GetProposedResources () returns resource information
for a single root bridge. It only carries the address range in PCI
view.
The address range/resource is reported/submitted by PciHostBridgeLib.
Before the change, CPU view equals to PCI view. So PciHostBridgeDxe
driver directly adds the resource to GCD.
In your change, PciHostBridgeDxe assumes the source is in PCI view
and it adds offset to convert to CPU view.
CPU-address = PCI-address + offset. <-- Equation #A
2. PciRootBridgeIo.Configuration() returns the resource information
for a single root bridge. It carries the address range in CPU view,
and the translation offset.
However, per UEFI spec, "Address Translation Offset. Offset to apply
to the Starting address of a BAR to convert it to a PCI address. "
PCI-address = CPU-address + offset. <-- Equation #B
You can see that equation #A and #B are conflict.
3. PciIo.GetBarAttributes() returns the resource information for a
single PCI BAR.
It carries the address range in CPU view, and the translation offset.
UEFI Spec content here is consistent to #2.
4. I didn't find spec requires the offset only applies to MEM address.
So I am ok to extend the offset to IO address.
So I suggest:
1. PciHostBridgeLib still reports the resource in CPU view because
#2 and #3 both report resource in CPU view.
It also reports the resource offset (= PCI-address - CPU-address).
2. PciHostBridge.GetProposedResources() returns resource for a single
root bridge in PCI view. Because per PI spec, the Offset field in
the ACPI descriptor carries the allocation status. If the resource
is in CPU view, PciBus driver doesn't know how to calculate the
PCI address and update the BAR.
3. PciRootBridgeIo.Configuration() returns offset value reported by
PciHostBridgeLib.
4. PciRootBridgeIo/PciIo.Mem.Read/Write(): I agree that the address is
a PCI-address. So a conversion is needed when use CpuIo to access.
But I think a spec clarification is needed to tell developers
that the conversion happens in PciRootBridgeIo layer, not in PciIo
layer.
>
>
>>> + UINT64 MemTranslation[4];
>>> UINTN MemApertureIndex;
>>> BOOLEAN ResourceAssigned;
>>> LIST_ENTRY *Link;
>>> + UINT64 Trans;
>>>
>>> RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount);
>>> if ((RootBridges == NULL) || (RootBridgeCount == 0)) {
>>> return EFI_UNSUPPORTED;
>>> }
>>>
>>> + Translations = PciHostBridgeGetTranslations (&TranslationCount);
>>> + if (Translations == NULL || TranslationCount == 0) {
>>> + TranslationCount = 0;
>>> + Translations = AllocateZeroPool (RootBridgeCount * sizeof (*Translations));
>>> + if (Translations == NULL) {
>>> + PciHostBridgeFreeRootBridges (RootBridges, RootBridgeCount);
>>> + return EFI_OUT_OF_RESOURCES;
>>> + }
>>> + }
>>> +
>>> + if (TranslationCount != 0 && TranslationCount != RootBridgeCount) {
>>> + DEBUG ((DEBUG_ERROR, "Error: count of root bridges (%d) and translation (%d) are different!\n",
>>> + RootBridgeCount, TranslationCount));
>>> + return EFI_INVALID_PARAMETER;
>>> + }
>>> +
>>> Status = gBS->LocateProtocol (&gEfiMetronomeArchProtocolGuid, NULL, (VOID **) &mMetronome);
>>> ASSERT_EFI_ERROR (Status);
>>> Status = gBS->LocateProtocol (&gEfiCpuIo2ProtocolGuid, NULL, (VOID **) &mCpuIo);
>>> @@ -395,7 +415,7 @@ InitializePciHostBridge (
>>> //
>>> // Create Root Bridge Handle Instance
>>> //
>>> - RootBridge = CreateRootBridge (&RootBridges[Index]);
>>> + RootBridge = CreateRootBridge (&RootBridges[Index], &Translations[Index]);
>>> ASSERT (RootBridge != NULL);
>>> if (RootBridge == NULL) {
>>> continue;
>>> @@ -411,8 +431,9 @@ InitializePciHostBridge (
>>> }
>>>
>>> if (RootBridges[Index].Io.Base <= RootBridges[Index].Io.Limit) {
>>> + Trans = Translations[Index].IoTranslation;
>>> Status = AddIoSpace (
>>> - RootBridges[Index].Io.Base,
>>> + RootBridges[Index].Io.Base + Trans,
>>> RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1
>>> );
>>> ASSERT_EFI_ERROR (Status);
>>> @@ -422,7 +443,7 @@ InitializePciHostBridge (
>>> EfiGcdIoTypeIo,
>>> 0,
>>> RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1,
>>> - &RootBridges[Index].Io.Base,
>>> + &RootBridges[Index].Io.Base + Trans,
>>> gImageHandle,
>>> NULL
>>> );
>>> @@ -437,20 +458,24 @@ InitializePciHostBridge (
>>> // the MEM aperture in Mem
>>> //
>>> MemApertures[0] = &RootBridges[Index].Mem;
>>> + MemTranslation[0] = Translations[Index].MemTranslation;
>>> MemApertures[1] = &RootBridges[Index].MemAbove4G;
>>> + MemTranslation[1] = Translations[Index].MemAbove4GTranslation;
>>> MemApertures[2] = &RootBridges[Index].PMem;
>>> + MemTranslation[2] = Translations[Index].PMemTranslation;
>>> MemApertures[3] = &RootBridges[Index].PMemAbove4G;
>>> + MemTranslation[3] = Translations[Index].PMemAbove4GTranslation;
>>>
>>> for (MemApertureIndex = 0; MemApertureIndex < ARRAY_SIZE (MemApertures); MemApertureIndex++) {
>>> if (MemApertures[MemApertureIndex]->Base <= MemApertures[MemApertureIndex]->Limit) {
>>> Status = AddMemoryMappedIoSpace (
>>> - MemApertures[MemApertureIndex]->Base,
>>> + MemApertures[MemApertureIndex]->Base + MemTranslation[MemApertureIndex],
>>> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
>>> EFI_MEMORY_UC
>>> );
>>> ASSERT_EFI_ERROR (Status);
>>> Status = gDS->SetMemorySpaceAttributes (
>>> - MemApertures[MemApertureIndex]->Base,
>>> + MemApertures[MemApertureIndex]->Base + MemTranslation[MemApertureIndex],
>>> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
>>> EFI_MEMORY_UC
>>> );
>>> @@ -463,7 +488,7 @@ InitializePciHostBridge (
>>> EfiGcdMemoryTypeMemoryMappedIo,
>>> 0,
>>> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
>>> - &MemApertures[MemApertureIndex]->Base,
>>> + &MemApertures[MemApertureIndex]->Base + MemTranslation[MemApertureIndex],
>>> gImageHandle,
>>> NULL
>>> );
>>> @@ -514,7 +539,13 @@ InitializePciHostBridge (
>>> );
>>> ASSERT_EFI_ERROR (Status);
>>> }
>>> +
>>> PciHostBridgeFreeRootBridges (RootBridges, RootBridgeCount);
>>> + if (TranslationCount == 0) {
>>> + FreePool (Translations);
>>> + } else {
>>> + PciHostBridgeFreeTranslations (Translations, TranslationCount);
>>> + }
>>>
>>> if (!EFI_ERROR (Status)) {
>>> mIoMmuEvent = EfiCreateProtocolNotifyEvent (
>>> @@ -828,7 +859,7 @@ NotifyPhase (
>>> FALSE,
>>> RootBridge->ResAllocNode[Index].Length,
>>> MIN (15, BitsOfAlignment),
>>> - ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1),
>>> + ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1) + RootBridge->IoTranslation,
>>> RootBridge->Io.Limit
>>> );
>>> break;
>>> @@ -838,7 +869,7 @@ NotifyPhase (
>>> TRUE,
>>> RootBridge->ResAllocNode[Index].Length,
>>> MIN (63, BitsOfAlignment),
>>> - ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1),
>>> + ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1) + RootBridge->MemAbove4GTranslation,
>>> RootBridge->MemAbove4G.Limit
>>> );
>>> if (BaseAddress != MAX_UINT64) {
>>> @@ -853,7 +884,7 @@ NotifyPhase (
>>> TRUE,
>>> RootBridge->ResAllocNode[Index].Length,
>>> MIN (31, BitsOfAlignment),
>>> - ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1),
>>> + ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1) + RootBridge->MemTranslation,
>>> RootBridge->Mem.Limit
>>> );
>>> break;
>>> @@ -863,7 +894,7 @@ NotifyPhase (
>>> TRUE,
>>> RootBridge->ResAllocNode[Index].Length,
>>> MIN (63, BitsOfAlignment),
>>> - ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1),
>>> + ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1) + RootBridge->PMemAbove4GTranslation,
>>> RootBridge->PMemAbove4G.Limit
>>> );
>>> if (BaseAddress != MAX_UINT64) {
>>> @@ -877,7 +908,7 @@ NotifyPhase (
>>> TRUE,
>>> RootBridge->ResAllocNode[Index].Length,
>>> MIN (31, BitsOfAlignment),
>>> - ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1),
>>> + ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1) + RootBridge->PMemTranslation,
>>> RootBridge->PMem.Limit
>>> );
>>> break;
>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
>>> index d3dfb57..449c4b3 100644
>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
>>> @@ -73,6 +73,11 @@ typedef struct {
>>> PCI_ROOT_BRIDGE_APERTURE PMem;
>>> PCI_ROOT_BRIDGE_APERTURE MemAbove4G;
>>> PCI_ROOT_BRIDGE_APERTURE PMemAbove4G;
>>> + UINT64 IoTranslation;
>>> + UINT64 MemTranslation;
>>> + UINT64 MemAbove4GTranslation;
>>> + UINT64 PMemTranslation;
>>> + UINT64 PMemAbove4GTranslation;
>>> BOOLEAN DmaAbove4G;
>>> BOOLEAN NoExtendedConfigSpace;
>>> VOID *ConfigBuffer;
>>> @@ -98,7 +103,8 @@ typedef struct {
>>> **/
>>> PCI_ROOT_BRIDGE_INSTANCE *
>>> CreateRootBridge (
>>> - IN PCI_ROOT_BRIDGE *Bridge
>>> + IN PCI_ROOT_BRIDGE *Bridge,
>>> + IN PCI_ROOT_BRIDGE_TRANSLATION *Translation
>>> );
>>>
>>> //
>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> index dc06c16..84b2d5a 100644
>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> @@ -67,7 +67,8 @@ UINT8 mOutStride[] = {
>>> **/
>>> PCI_ROOT_BRIDGE_INSTANCE *
>>> CreateRootBridge (
>>> - IN PCI_ROOT_BRIDGE *Bridge
>>> + IN PCI_ROOT_BRIDGE *Bridge,
>>> + IN PCI_ROOT_BRIDGE_TRANSLATION *Translation
>>> )
>>> {
>>> PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
>>> @@ -87,11 +88,21 @@ CreateRootBridge (
>>> (Bridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_MEM64_DECODE) != 0 ? L"Mem64Decode" : L""
>>> ));
>>> 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, Translation->IoTranslation
>>> + ));
>>> + DEBUG ((DEBUG_INFO, " Mem: %lx - %lx translation=%lx\n",
>>> + Bridge->Mem.Base, Bridge->Mem.Limit, Translation->MemTranslation
>>> + ));
>>> + DEBUG ((DEBUG_INFO, " MemAbove4G: %lx - %lx translation=%lx\n",
>>> + Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit, Translation->MemAbove4GTranslation
>>> + ));
>>> + DEBUG ((DEBUG_INFO, " PMem: %lx - %lx translation=%lx\n",
>>> + Bridge->PMem.Base, Bridge->PMem.Limit, Translation->PMemTranslation
>>> + ));
>>> + DEBUG ((DEBUG_INFO, " PMemAbove4G: %lx - %lx translation=%lx\n",
>>> + Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit, Translation->PMemAbove4GTranslation
>>> + ));
>>>
>>> //
>>> // Make sure Mem and MemAbove4G apertures are valid
>>> @@ -174,10 +185,15 @@ CreateRootBridge (
>>>
>>> CopyMem (&RootBridge->Bus, &Bridge->Bus, sizeof (PCI_ROOT_BRIDGE_APERTURE));
>>> CopyMem (&RootBridge->Io, &Bridge->Io, sizeof (PCI_ROOT_BRIDGE_APERTURE));
>>> + RootBridge->IoTranslation = Translation->IoTranslation;
>>> CopyMem (&RootBridge->Mem, &Bridge->Mem, sizeof (PCI_ROOT_BRIDGE_APERTURE));
>>> + RootBridge->MemTranslation = Translation->MemTranslation;
>>> CopyMem (&RootBridge->MemAbove4G, &Bridge->MemAbove4G, sizeof (PCI_ROOT_BRIDGE_APERTURE));
>>> + RootBridge->MemAbove4GTranslation = Translation->MemAbove4GTranslation;
>>> CopyMem (&RootBridge->PMem, &Bridge->PMem, sizeof (PCI_ROOT_BRIDGE_APERTURE));
>>> + RootBridge->PMemTranslation = Translation->PMemTranslation;
>>> CopyMem (&RootBridge->PMemAbove4G, &Bridge->PMemAbove4G, sizeof (PCI_ROOT_BRIDGE_APERTURE));
>>> + RootBridge->PMemAbove4GTranslation = Translation->PMemAbove4GTranslation;
>>>
>>> for (Index = TypeIo; Index < TypeMax; Index++) {
>>> switch (Index) {
>>> @@ -403,6 +419,28 @@ RootBridgeIoCheckParameter (
>>> return EFI_SUCCESS;
>>> }
>>>
>>> +EFI_STATUS
>>> +RootBridgeIoGetMemTranslation (
>>> + IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge,
>>> + IN UINT64 Address,
>>> + IN OUT UINT64 *Translation
>>> + )
>>> +{
>>> + if (Address >= RootBridge->Mem.Base && Address <= RootBridge->Mem.Limit) {
>>> + *Translation = RootBridge->MemTranslation;
>>> + } else if (Address >= RootBridge->PMem.Base && Address <= RootBridge->PMem.Limit) {
>>> + *Translation = RootBridge->PMemTranslation;
>>> + } else if (Address >= RootBridge->MemAbove4G.Base && Address <= RootBridge->MemAbove4G.Limit) {
>>> + *Translation = RootBridge->MemAbove4GTranslation;
>>> + } else if (Address >= RootBridge->PMemAbove4G.Base && Address <= RootBridge->PMemAbove4G.Limit) {
>>> + *Translation = RootBridge->PMemAbove4GTranslation;
>>> + } 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 +696,22 @@ 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 = RootBridgeIoGetMemTranslation (RootBridge, Address, &Translation);
>>> + if (EFI_ERROR (Status)) {
>>> + return Status;
>>> + }
>>> +
>>> + return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + Translation, Count, Buffer);
>>> }
>>>
>>> /**
>>> @@ -705,13 +752,22 @@ 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 = RootBridgeIoGetMemTranslation (RootBridge, Address, &Translation);
>>> + if (EFI_ERROR (Status)) {
>>> + return Status;
>>> + }
>>> +
>>> + return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + Translation, Count, Buffer);
>>> }
>>>
>>> /**
>>> @@ -746,6 +802,8 @@ RootBridgeIoIoRead (
>>> )
>>> {
>>> EFI_STATUS Status;
>>> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
>>> +
>>> Status = RootBridgeIoCheckParameter (
>>> This, IoOperation, Width,
>>> Address, Count, Buffer
>>> @@ -753,7 +811,10 @@ 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);
>>> +
>>> + return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + RootBridge->IoTranslation, Count, Buffer);
>>> }
>>>
>>> /**
>>> @@ -788,6 +849,8 @@ RootBridgeIoIoWrite (
>>> )
>>> {
>>> EFI_STATUS Status;
>>> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
>>> +
>>> Status = RootBridgeIoCheckParameter (
>>> This, IoOperation, Width,
>>> Address, Count, Buffer
>>> @@ -795,7 +858,10 @@ 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);
>>> +
>>> + return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + RootBridge->IoTranslation, Count, Buffer);
>>> }
>>>
>>> /**
>>> @@ -1621,19 +1687,28 @@ RootBridgeIoConfiguration (
>>> switch (ResAllocNode->Type) {
>>>
>>> case TypeIo:
>>> - Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
>>> + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
>>> + Descriptor->AddrTranslationOffset = RootBridge->IoTranslation;
>>> 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->PMemTranslation;
>>> + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
>>> + Descriptor->AddrSpaceGranularity = 32;
>>> case TypeMem32:
>>> + Descriptor->AddrTranslationOffset = RootBridge->MemTranslation;
>>> 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->PMemAbove4GTranslation;
>>> + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
>>> + Descriptor->AddrSpaceGranularity = 64;
>>> case TypeMem64:
>>> + Descriptor->AddrTranslationOffset = RootBridge->MemAbove4GTranslation;
>>> 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..4c297fd 100644
>>> --- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
>>> +++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
>>> @@ -53,6 +53,14 @@ typedef struct {
>>> EFI_DEVICE_PATH_PROTOCOL *DevicePath; ///< Device path.
>>> } PCI_ROOT_BRIDGE;
>>>
>>> +typedef struct {
>>> + UINT64 IoTranslation;
>>> + UINT64 MemTranslation;
>>> + UINT64 MemAbove4GTranslation;
>>> + UINT64 PMemTranslation;
>>> + UINT64 PMemAbove4GTranslation;
>>> +} PCI_ROOT_BRIDGE_TRANSLATION;
>>> +
>>> /**
>>> Return all the root bridge instances in an array.
>>>
>>> @@ -69,6 +77,21 @@ PciHostBridgeGetRootBridges (
>>> );
>>>
>>> /**
>>> + Return all the root bridge instances in an array.
>>> +
>>> + @param Count Return the count of root bridge instances.
>>> +
>>> + @return All the root bridge instances in an array.
>>> + The array should be passed into PciHostBridgeFreeRootBridges()
>>> + when it's not used.
>>> +**/
>>> +PCI_ROOT_BRIDGE_TRANSLATION *
>>> +EFIAPI
>>> +PciHostBridgeGetTranslations (
>>> + UINTN *Count
>>> + );
>>> +
>>> +/**
>>> Free the root bridge instances array returned from PciHostBridgeGetRootBridges().
>>>
>>> @param Bridges The root bridge instances array.
>>> @@ -82,6 +105,19 @@ PciHostBridgeFreeRootBridges (
>>> );
>>>
>>> /**
>>> + Free the root bridge instances array returned from PciHostBridgeGetRootBridges().
>>> +
>>> + @param Bridges The root bridge instances array.
>>> + @param Count The count of the array.
>>> +**/
>>> +VOID
>>> +EFIAPI
>>> +PciHostBridgeFreeTranslations (
>>> + PCI_ROOT_BRIDGE_TRANSLATION *Bridges,
>>> + UINTN Count
>>> + );
>>> +
>>> +/**
>>> Inform the platform that the resource conflict happens.
>>>
>>> @param HostBridgeHandle Handle of the Host Bridge.
>>> --
>>> 2.7.4
>>>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
2018-01-22 3:36 ` Ni, Ruiyu
@ 2018-01-29 8:50 ` Guo Heyi
[not found] ` <685b27a9-e620-e7e7-e0fb-8cebe16e7b1a@Intel.com>
0 siblings, 1 reply; 12+ messages in thread
From: Guo Heyi @ 2018-01-29 8:50 UTC (permalink / raw)
To: Ni, Ruiyu
Cc: Guo Heyi, Michael D, Ard Biesheuvel, Eric Dong,
edk2-devel@lists.01.org, linaro-uefi, Star Zeng
Sorry for the late; I caught cold and didn't work for several days last week :(
Please see my comments below:
On Mon, Jan 22, 2018 at 11:36:14AM +0800, Ni, Ruiyu wrote:
> On 1/18/2018 9:26 AM, Guo Heyi wrote:
> >On Wed, Jan 17, 2018 at 02:08:06PM +0000, Ard Biesheuvel wrote:
> >>On 15 January 2018 at 14:46, Heyi Guo <heyi.guo@linaro.org> wrote:
> >>>This is the draft patch for the discussion posted in edk2-devel
> >>>mailing list:
> >>>https://lists.01.org/pipermail/edk2-devel/2017-December/019289.html
> >>>
> >>>As discussed in the mailing list, we'd like to add support for PCI
> >>>address translation which is necessary for some non-x86 platforms. I
> >>>also want to minimize the changes to the generic host bridge driver
> >>>and platform PciHostBridgeLib implemetations, so additional two
> >>>interfaces are added to expose translation information of the
> >>>platform. To be generic, I add translation for each type of IO or
> >>>memory resources.
> >>>
> >>>The patch is still a RFC, so I only passed the build for qemu64 and
> >>>the function has not been tested yet.
> >>>
> >>>Please let me know your comments about it.
> >>>
> >>>Thanks.
> >>>
> >>>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>
> >>>---
> >>> .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 19 ++++
> >>> .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 53 ++++++++---
> >>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridge.h | 8 +-
> >>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 101 ++++++++++++++++++---
> >>> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 36 ++++++++
> >>> 5 files changed, 192 insertions(+), 25 deletions(-)
> >>>
> >>>diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> >>>index 5b9c887..0c8371a 100644
> >>>--- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> >>>+++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> >>>@@ -360,6 +360,16 @@ PciHostBridgeGetRootBridges (
> >>> return &mRootBridge;
> >>> }
> >>>
> >>>+PCI_ROOT_BRIDGE_TRANSLATION *
> >>>+EFIAPI
> >>>+PciHostBridgeGetTranslations (
> >>>+ UINTN *Count
> >>>+ )
> >>>+{
> >>>+ *Count = 0;
> >>>+ return NULL;
> >>>+}
> >>>+
> >>> /**
> >>> Free the root bridge instances array returned from
> >>> PciHostBridgeGetRootBridges().
> >>>@@ -377,6 +387,15 @@ PciHostBridgeFreeRootBridges (
> >>> ASSERT (Count == 1);
> >>> }
> >>>
> >>>+VOID
> >>>+EFIAPI
> >>>+PciHostBridgeFreeTranslations (
> >>>+ PCI_ROOT_BRIDGE_TRANSLATION *Translations,
> >>>+ UINTN Count
> >>>+ )
> >>>+{
> >>>+}
> >>>+
> >>> /**
> >>> Inform the platform that the resource conflict happens.
> >>>
> >>>diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>index 1494848..835e411 100644
> >>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>@@ -360,18 +360,38 @@ InitializePciHostBridge (
> >>> PCI_HOST_BRIDGE_INSTANCE *HostBridge;
> >>> PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> >>> PCI_ROOT_BRIDGE *RootBridges;
> >>>+ PCI_ROOT_BRIDGE_TRANSLATION *Translations;
> >>> UINTN RootBridgeCount;
> >>>+ UINTN TranslationCount;
> >>> UINTN Index;
> >>> PCI_ROOT_BRIDGE_APERTURE *MemApertures[4];
> >>
> >>Wouldn't it be much better to add a 'translation' member to
> >>PCI_ROOT_BRIDGE_APERTURE? That way, existing code just default to a
> >>translation of 0, and all the handling of the separate array can be
> >>dropped.
> >>
> >Actually my first idea was the same, but when I looked at the implementation of
> >PciHostBridgeLib of Ovmf, I found it a little tedious to change the
> >existing code in this way. We'll need to check everywhere
> >PCI_ROOT_BRIDGE_APERTURE or PCI_ROOT_BRIDGE is used, to make sure the
> >translation field is initialized to be zero, e.g. line 235~245.
> >
> >What I did in this RFC is not so straightforward indeed. So I can change the
> >code if we all agree to add Translation field into PCI_ROOT_BRIDGE_APERTURE
> >directly.
> >
> >Thanks,
> >
> >Gary (Heyi Guo)
>
> I also agree to put the translation fields into the
> PCI_ROOT_BRIDGE_APERTURE.
>
>
> But I think we may have different understandings to the address
> translation.
> My understanding to the whole translation:
> 1. PciHostBridge.GetProposedResources () returns resource information
> for a single root bridge. It only carries the address range in PCI
> view.
> The address range/resource is reported/submitted by PciHostBridgeLib.
> Before the change, CPU view equals to PCI view. So PciHostBridgeDxe
> driver directly adds the resource to GCD.
> In your change, PciHostBridgeDxe assumes the source is in PCI view
> and it adds offset to convert to CPU view.
> CPU-address = PCI-address + offset. <-- Equation #A
>
>
> 2. PciRootBridgeIo.Configuration() returns the resource information
> for a single root bridge. It carries the address range in CPU view,
> and the translation offset.
> However, per UEFI spec, "Address Translation Offset. Offset to apply
> to the Starting address of a BAR to convert it to a PCI address. "
> PCI-address = CPU-address + offset. <-- Equation #B
If we get Equation #B from the statement in UEFI spec, does it also imply
Starting address is "Address Range Minimum" and so it is CPU view address?
If we use Equation #B, can offset be a negative value? If it is not, then it
will make translation useless, since we cannot change CPU address above 4G into
PCI address under 4G for legacy compatibility.
Equation #B is also not consistent with how OS treats address translation;
please see the ACPI ASL code which works well for OS:
https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Hisilicon/Hi1616/D05AcpiTables/Dsdt/D05Pci.asl#L201
Thanks,
Gary (Heyi Guo)
>
> You can see that equation #A and #B are conflict.
>
>
> 3. PciIo.GetBarAttributes() returns the resource information for a
> single PCI BAR.
> It carries the address range in CPU view, and the translation offset.
> UEFI Spec content here is consistent to #2.
>
>
> 4. I didn't find spec requires the offset only applies to MEM address.
> So I am ok to extend the offset to IO address.
>
> So I suggest:
> 1. PciHostBridgeLib still reports the resource in CPU view because
> #2 and #3 both report resource in CPU view.
> It also reports the resource offset (= PCI-address - CPU-address).
>
> 2. PciHostBridge.GetProposedResources() returns resource for a single
> root bridge in PCI view. Because per PI spec, the Offset field in
> the ACPI descriptor carries the allocation status. If the resource
> is in CPU view, PciBus driver doesn't know how to calculate the
> PCI address and update the BAR.
>
> 3. PciRootBridgeIo.Configuration() returns offset value reported by
> PciHostBridgeLib.
>
> 4. PciRootBridgeIo/PciIo.Mem.Read/Write(): I agree that the address is
> a PCI-address. So a conversion is needed when use CpuIo to access.
> But I think a spec clarification is needed to tell developers
> that the conversion happens in PciRootBridgeIo layer, not in PciIo
> layer.
>
> >
> >
> >>>+ UINT64 MemTranslation[4];
> >>> UINTN MemApertureIndex;
> >>> BOOLEAN ResourceAssigned;
> >>> LIST_ENTRY *Link;
> >>>+ UINT64 Trans;
> >>>
> >>> RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount);
> >>> if ((RootBridges == NULL) || (RootBridgeCount == 0)) {
> >>> return EFI_UNSUPPORTED;
> >>> }
> >>>
> >>>+ Translations = PciHostBridgeGetTranslations (&TranslationCount);
> >>>+ if (Translations == NULL || TranslationCount == 0) {
> >>>+ TranslationCount = 0;
> >>>+ Translations = AllocateZeroPool (RootBridgeCount * sizeof (*Translations));
> >>>+ if (Translations == NULL) {
> >>>+ PciHostBridgeFreeRootBridges (RootBridges, RootBridgeCount);
> >>>+ return EFI_OUT_OF_RESOURCES;
> >>>+ }
> >>>+ }
> >>>+
> >>>+ if (TranslationCount != 0 && TranslationCount != RootBridgeCount) {
> >>>+ DEBUG ((DEBUG_ERROR, "Error: count of root bridges (%d) and translation (%d) are different!\n",
> >>>+ RootBridgeCount, TranslationCount));
> >>>+ return EFI_INVALID_PARAMETER;
> >>>+ }
> >>>+
> >>> Status = gBS->LocateProtocol (&gEfiMetronomeArchProtocolGuid, NULL, (VOID **) &mMetronome);
> >>> ASSERT_EFI_ERROR (Status);
> >>> Status = gBS->LocateProtocol (&gEfiCpuIo2ProtocolGuid, NULL, (VOID **) &mCpuIo);
> >>>@@ -395,7 +415,7 @@ InitializePciHostBridge (
> >>> //
> >>> // Create Root Bridge Handle Instance
> >>> //
> >>>- RootBridge = CreateRootBridge (&RootBridges[Index]);
> >>>+ RootBridge = CreateRootBridge (&RootBridges[Index], &Translations[Index]);
> >>> ASSERT (RootBridge != NULL);
> >>> if (RootBridge == NULL) {
> >>> continue;
> >>>@@ -411,8 +431,9 @@ InitializePciHostBridge (
> >>> }
> >>>
> >>> if (RootBridges[Index].Io.Base <= RootBridges[Index].Io.Limit) {
> >>>+ Trans = Translations[Index].IoTranslation;
> >>> Status = AddIoSpace (
> >>>- RootBridges[Index].Io.Base,
> >>>+ RootBridges[Index].Io.Base + Trans,
> >>> RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1
> >>> );
> >>> ASSERT_EFI_ERROR (Status);
> >>>@@ -422,7 +443,7 @@ InitializePciHostBridge (
> >>> EfiGcdIoTypeIo,
> >>> 0,
> >>> RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1,
> >>>- &RootBridges[Index].Io.Base,
> >>>+ &RootBridges[Index].Io.Base + Trans,
> >>> gImageHandle,
> >>> NULL
> >>> );
> >>>@@ -437,20 +458,24 @@ InitializePciHostBridge (
> >>> // the MEM aperture in Mem
> >>> //
> >>> MemApertures[0] = &RootBridges[Index].Mem;
> >>>+ MemTranslation[0] = Translations[Index].MemTranslation;
> >>> MemApertures[1] = &RootBridges[Index].MemAbove4G;
> >>>+ MemTranslation[1] = Translations[Index].MemAbove4GTranslation;
> >>> MemApertures[2] = &RootBridges[Index].PMem;
> >>>+ MemTranslation[2] = Translations[Index].PMemTranslation;
> >>> MemApertures[3] = &RootBridges[Index].PMemAbove4G;
> >>>+ MemTranslation[3] = Translations[Index].PMemAbove4GTranslation;
> >>>
> >>> for (MemApertureIndex = 0; MemApertureIndex < ARRAY_SIZE (MemApertures); MemApertureIndex++) {
> >>> if (MemApertures[MemApertureIndex]->Base <= MemApertures[MemApertureIndex]->Limit) {
> >>> Status = AddMemoryMappedIoSpace (
> >>>- MemApertures[MemApertureIndex]->Base,
> >>>+ MemApertures[MemApertureIndex]->Base + MemTranslation[MemApertureIndex],
> >>> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
> >>> EFI_MEMORY_UC
> >>> );
> >>> ASSERT_EFI_ERROR (Status);
> >>> Status = gDS->SetMemorySpaceAttributes (
> >>>- MemApertures[MemApertureIndex]->Base,
> >>>+ MemApertures[MemApertureIndex]->Base + MemTranslation[MemApertureIndex],
> >>> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
> >>> EFI_MEMORY_UC
> >>> );
> >>>@@ -463,7 +488,7 @@ InitializePciHostBridge (
> >>> EfiGcdMemoryTypeMemoryMappedIo,
> >>> 0,
> >>> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
> >>>- &MemApertures[MemApertureIndex]->Base,
> >>>+ &MemApertures[MemApertureIndex]->Base + MemTranslation[MemApertureIndex],
> >>> gImageHandle,
> >>> NULL
> >>> );
> >>>@@ -514,7 +539,13 @@ InitializePciHostBridge (
> >>> );
> >>> ASSERT_EFI_ERROR (Status);
> >>> }
> >>>+
> >>> PciHostBridgeFreeRootBridges (RootBridges, RootBridgeCount);
> >>>+ if (TranslationCount == 0) {
> >>>+ FreePool (Translations);
> >>>+ } else {
> >>>+ PciHostBridgeFreeTranslations (Translations, TranslationCount);
> >>>+ }
> >>>
> >>> if (!EFI_ERROR (Status)) {
> >>> mIoMmuEvent = EfiCreateProtocolNotifyEvent (
> >>>@@ -828,7 +859,7 @@ NotifyPhase (
> >>> FALSE,
> >>> RootBridge->ResAllocNode[Index].Length,
> >>> MIN (15, BitsOfAlignment),
> >>>- ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1),
> >>>+ ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1) + RootBridge->IoTranslation,
> >>> RootBridge->Io.Limit
> >>> );
> >>> break;
> >>>@@ -838,7 +869,7 @@ NotifyPhase (
> >>> TRUE,
> >>> RootBridge->ResAllocNode[Index].Length,
> >>> MIN (63, BitsOfAlignment),
> >>>- ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1),
> >>>+ ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1) + RootBridge->MemAbove4GTranslation,
> >>> RootBridge->MemAbove4G.Limit
> >>> );
> >>> if (BaseAddress != MAX_UINT64) {
> >>>@@ -853,7 +884,7 @@ NotifyPhase (
> >>> TRUE,
> >>> RootBridge->ResAllocNode[Index].Length,
> >>> MIN (31, BitsOfAlignment),
> >>>- ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1),
> >>>+ ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1) + RootBridge->MemTranslation,
> >>> RootBridge->Mem.Limit
> >>> );
> >>> break;
> >>>@@ -863,7 +894,7 @@ NotifyPhase (
> >>> TRUE,
> >>> RootBridge->ResAllocNode[Index].Length,
> >>> MIN (63, BitsOfAlignment),
> >>>- ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1),
> >>>+ ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1) + RootBridge->PMemAbove4GTranslation,
> >>> RootBridge->PMemAbove4G.Limit
> >>> );
> >>> if (BaseAddress != MAX_UINT64) {
> >>>@@ -877,7 +908,7 @@ NotifyPhase (
> >>> TRUE,
> >>> RootBridge->ResAllocNode[Index].Length,
> >>> MIN (31, BitsOfAlignment),
> >>>- ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1),
> >>>+ ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1) + RootBridge->PMemTranslation,
> >>> RootBridge->PMem.Limit
> >>> );
> >>> break;
> >>>diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> >>>index d3dfb57..449c4b3 100644
> >>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> >>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> >>>@@ -73,6 +73,11 @@ typedef struct {
> >>> PCI_ROOT_BRIDGE_APERTURE PMem;
> >>> PCI_ROOT_BRIDGE_APERTURE MemAbove4G;
> >>> PCI_ROOT_BRIDGE_APERTURE PMemAbove4G;
> >>>+ UINT64 IoTranslation;
> >>>+ UINT64 MemTranslation;
> >>>+ UINT64 MemAbove4GTranslation;
> >>>+ UINT64 PMemTranslation;
> >>>+ UINT64 PMemAbove4GTranslation;
> >>> BOOLEAN DmaAbove4G;
> >>> BOOLEAN NoExtendedConfigSpace;
> >>> VOID *ConfigBuffer;
> >>>@@ -98,7 +103,8 @@ typedef struct {
> >>> **/
> >>> PCI_ROOT_BRIDGE_INSTANCE *
> >>> CreateRootBridge (
> >>>- IN PCI_ROOT_BRIDGE *Bridge
> >>>+ IN PCI_ROOT_BRIDGE *Bridge,
> >>>+ IN PCI_ROOT_BRIDGE_TRANSLATION *Translation
> >>> );
> >>>
> >>> //
> >>>diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>>index dc06c16..84b2d5a 100644
> >>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>>@@ -67,7 +67,8 @@ UINT8 mOutStride[] = {
> >>> **/
> >>> PCI_ROOT_BRIDGE_INSTANCE *
> >>> CreateRootBridge (
> >>>- IN PCI_ROOT_BRIDGE *Bridge
> >>>+ IN PCI_ROOT_BRIDGE *Bridge,
> >>>+ IN PCI_ROOT_BRIDGE_TRANSLATION *Translation
> >>> )
> >>> {
> >>> PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> >>>@@ -87,11 +88,21 @@ CreateRootBridge (
> >>> (Bridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_MEM64_DECODE) != 0 ? L"Mem64Decode" : L""
> >>> ));
> >>> 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, Translation->IoTranslation
> >>>+ ));
> >>>+ DEBUG ((DEBUG_INFO, " Mem: %lx - %lx translation=%lx\n",
> >>>+ Bridge->Mem.Base, Bridge->Mem.Limit, Translation->MemTranslation
> >>>+ ));
> >>>+ DEBUG ((DEBUG_INFO, " MemAbove4G: %lx - %lx translation=%lx\n",
> >>>+ Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit, Translation->MemAbove4GTranslation
> >>>+ ));
> >>>+ DEBUG ((DEBUG_INFO, " PMem: %lx - %lx translation=%lx\n",
> >>>+ Bridge->PMem.Base, Bridge->PMem.Limit, Translation->PMemTranslation
> >>>+ ));
> >>>+ DEBUG ((DEBUG_INFO, " PMemAbove4G: %lx - %lx translation=%lx\n",
> >>>+ Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit, Translation->PMemAbove4GTranslation
> >>>+ ));
> >>>
> >>> //
> >>> // Make sure Mem and MemAbove4G apertures are valid
> >>>@@ -174,10 +185,15 @@ CreateRootBridge (
> >>>
> >>> CopyMem (&RootBridge->Bus, &Bridge->Bus, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> >>> CopyMem (&RootBridge->Io, &Bridge->Io, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> >>>+ RootBridge->IoTranslation = Translation->IoTranslation;
> >>> CopyMem (&RootBridge->Mem, &Bridge->Mem, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> >>>+ RootBridge->MemTranslation = Translation->MemTranslation;
> >>> CopyMem (&RootBridge->MemAbove4G, &Bridge->MemAbove4G, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> >>>+ RootBridge->MemAbove4GTranslation = Translation->MemAbove4GTranslation;
> >>> CopyMem (&RootBridge->PMem, &Bridge->PMem, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> >>>+ RootBridge->PMemTranslation = Translation->PMemTranslation;
> >>> CopyMem (&RootBridge->PMemAbove4G, &Bridge->PMemAbove4G, sizeof (PCI_ROOT_BRIDGE_APERTURE));
> >>>+ RootBridge->PMemAbove4GTranslation = Translation->PMemAbove4GTranslation;
> >>>
> >>> for (Index = TypeIo; Index < TypeMax; Index++) {
> >>> switch (Index) {
> >>>@@ -403,6 +419,28 @@ RootBridgeIoCheckParameter (
> >>> return EFI_SUCCESS;
> >>> }
> >>>
> >>>+EFI_STATUS
> >>>+RootBridgeIoGetMemTranslation (
> >>>+ IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge,
> >>>+ IN UINT64 Address,
> >>>+ IN OUT UINT64 *Translation
> >>>+ )
> >>>+{
> >>>+ if (Address >= RootBridge->Mem.Base && Address <= RootBridge->Mem.Limit) {
> >>>+ *Translation = RootBridge->MemTranslation;
> >>>+ } else if (Address >= RootBridge->PMem.Base && Address <= RootBridge->PMem.Limit) {
> >>>+ *Translation = RootBridge->PMemTranslation;
> >>>+ } else if (Address >= RootBridge->MemAbove4G.Base && Address <= RootBridge->MemAbove4G.Limit) {
> >>>+ *Translation = RootBridge->MemAbove4GTranslation;
> >>>+ } else if (Address >= RootBridge->PMemAbove4G.Base && Address <= RootBridge->PMemAbove4G.Limit) {
> >>>+ *Translation = RootBridge->PMemAbove4GTranslation;
> >>>+ } 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 +696,22 @@ 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 = RootBridgeIoGetMemTranslation (RootBridge, Address, &Translation);
> >>>+ if (EFI_ERROR (Status)) {
> >>>+ return Status;
> >>>+ }
> >>>+
> >>>+ return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + Translation, Count, Buffer);
> >>> }
> >>>
> >>> /**
> >>>@@ -705,13 +752,22 @@ 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 = RootBridgeIoGetMemTranslation (RootBridge, Address, &Translation);
> >>>+ if (EFI_ERROR (Status)) {
> >>>+ return Status;
> >>>+ }
> >>>+
> >>>+ return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + Translation, Count, Buffer);
> >>> }
> >>>
> >>> /**
> >>>@@ -746,6 +802,8 @@ RootBridgeIoIoRead (
> >>> )
> >>> {
> >>> EFI_STATUS Status;
> >>>+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> >>>+
> >>> Status = RootBridgeIoCheckParameter (
> >>> This, IoOperation, Width,
> >>> Address, Count, Buffer
> >>>@@ -753,7 +811,10 @@ 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);
> >>>+
> >>>+ return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + RootBridge->IoTranslation, Count, Buffer);
> >>> }
> >>>
> >>> /**
> >>>@@ -788,6 +849,8 @@ RootBridgeIoIoWrite (
> >>> )
> >>> {
> >>> EFI_STATUS Status;
> >>>+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> >>>+
> >>> Status = RootBridgeIoCheckParameter (
> >>> This, IoOperation, Width,
> >>> Address, Count, Buffer
> >>>@@ -795,7 +858,10 @@ 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);
> >>>+
> >>>+ return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + RootBridge->IoTranslation, Count, Buffer);
> >>> }
> >>>
> >>> /**
> >>>@@ -1621,19 +1687,28 @@ RootBridgeIoConfiguration (
> >>> switch (ResAllocNode->Type) {
> >>>
> >>> case TypeIo:
> >>>- Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
> >>>+ Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
> >>>+ Descriptor->AddrTranslationOffset = RootBridge->IoTranslation;
> >>> 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->PMemTranslation;
> >>>+ Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
> >>>+ Descriptor->AddrSpaceGranularity = 32;
> >>> case TypeMem32:
> >>>+ Descriptor->AddrTranslationOffset = RootBridge->MemTranslation;
> >>> 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->PMemAbove4GTranslation;
> >>>+ Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
> >>>+ Descriptor->AddrSpaceGranularity = 64;
> >>> case TypeMem64:
> >>>+ Descriptor->AddrTranslationOffset = RootBridge->MemAbove4GTranslation;
> >>> 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..4c297fd 100644
> >>>--- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> >>>+++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> >>>@@ -53,6 +53,14 @@ typedef struct {
> >>> EFI_DEVICE_PATH_PROTOCOL *DevicePath; ///< Device path.
> >>> } PCI_ROOT_BRIDGE;
> >>>
> >>>+typedef struct {
> >>>+ UINT64 IoTranslation;
> >>>+ UINT64 MemTranslation;
> >>>+ UINT64 MemAbove4GTranslation;
> >>>+ UINT64 PMemTranslation;
> >>>+ UINT64 PMemAbove4GTranslation;
> >>>+} PCI_ROOT_BRIDGE_TRANSLATION;
> >>>+
> >>> /**
> >>> Return all the root bridge instances in an array.
> >>>
> >>>@@ -69,6 +77,21 @@ PciHostBridgeGetRootBridges (
> >>> );
> >>>
> >>> /**
> >>>+ Return all the root bridge instances in an array.
> >>>+
> >>>+ @param Count Return the count of root bridge instances.
> >>>+
> >>>+ @return All the root bridge instances in an array.
> >>>+ The array should be passed into PciHostBridgeFreeRootBridges()
> >>>+ when it's not used.
> >>>+**/
> >>>+PCI_ROOT_BRIDGE_TRANSLATION *
> >>>+EFIAPI
> >>>+PciHostBridgeGetTranslations (
> >>>+ UINTN *Count
> >>>+ );
> >>>+
> >>>+/**
> >>> Free the root bridge instances array returned from PciHostBridgeGetRootBridges().
> >>>
> >>> @param Bridges The root bridge instances array.
> >>>@@ -82,6 +105,19 @@ PciHostBridgeFreeRootBridges (
> >>> );
> >>>
> >>> /**
> >>>+ Free the root bridge instances array returned from PciHostBridgeGetRootBridges().
> >>>+
> >>>+ @param Bridges The root bridge instances array.
> >>>+ @param Count The count of the array.
> >>>+**/
> >>>+VOID
> >>>+EFIAPI
> >>>+PciHostBridgeFreeTranslations (
> >>>+ PCI_ROOT_BRIDGE_TRANSLATION *Bridges,
> >>>+ UINTN Count
> >>>+ );
> >>>+
> >>>+/**
> >>> Inform the platform that the resource conflict happens.
> >>>
> >>> @param HostBridgeHandle Handle of the Host Bridge.
> >>>--
> >>>2.7.4
> >>>
> >_______________________________________________
> >edk2-devel mailing list
> >edk2-devel@lists.01.org
> >https://lists.01.org/mailman/listinfo/edk2-devel
> >
>
>
> --
> Thanks,
> Ray
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
[not found] ` <685b27a9-e620-e7e7-e0fb-8cebe16e7b1a@Intel.com>
@ 2018-02-01 17:22 ` Ard Biesheuvel
2018-02-02 0:34 ` Ni, Ruiyu
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2018-02-01 17:22 UTC (permalink / raw)
To: Ni, Ruiyu
Cc: Guo Heyi <heyi.guo@linaro.org>, Dong Wei, Eric Dong,
edk2-devel@lists.01.org, linaro-uefi, Michael D, Star Zeng
On 1 February 2018 at 05:03, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> On 1/29/2018 4:50 PM, Guo Heyi wrote:
>>
>> Sorry for the late; I caught cold and didn't work for several days last
>> week :(
>> Please see my comments below:
>>
>>
>> On Mon, Jan 22, 2018 at 11:36:14AM +0800, Ni, Ruiyu wrote:
>>>
>>> On 1/18/2018 9:26 AM, Guo Heyi wrote:
>>>>
>>>> On Wed, Jan 17, 2018 at 02:08:06PM +0000, Ard Biesheuvel wrote:
>>>>>
>>>>> On 15 January 2018 at 14:46, Heyi Guo <heyi.guo@linaro.org> wrote:
>>>>>>
>>>>>> This is the draft patch for the discussion posted in edk2-devel
>>>>>> mailing list:
>>>>>> https://lists.01.org/pipermail/edk2-devel/2017-December/019289.html
>>>>>>
>>>>>> As discussed in the mailing list, we'd like to add support for PCI
>>>>>> address translation which is necessary for some non-x86 platforms. I
>>>>>> also want to minimize the changes to the generic host bridge driver
>>>>>> and platform PciHostBridgeLib implemetations, so additional two
>>>>>> interfaces are added to expose translation information of the
>>>>>> platform. To be generic, I add translation for each type of IO or
>>>>>> memory resources.
>>>>>>
>>>>>> The patch is still a RFC, so I only passed the build for qemu64 and
>>>>>> the function has not been tested yet.
>>>>>>
>>>>>> Please let me know your comments about it.
>>>>>>
>>>>>> Thanks.
>>>>>>
>>>>>> 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>
>>>>>> ---
>>>>>> .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 19 ++++
>>>>>> .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 53 ++++++++---
>>>>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridge.h | 8 +-
>>>>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 101
>>>>>> ++++++++++++++++++---
>>>>>> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 36 ++++++++
>>>>>> 5 files changed, 192 insertions(+), 25 deletions(-)
>>>>>>
>>>>>> diff --git
>>>>>> a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>>>>> b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>>>>> index 5b9c887..0c8371a 100644
>>>>>> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>>>>> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>>>>> @@ -360,6 +360,16 @@ PciHostBridgeGetRootBridges (
>>>>>> return &mRootBridge;
>>>>>> }
>>>>>>
>>>>>> +PCI_ROOT_BRIDGE_TRANSLATION *
>>>>>> +EFIAPI
>>>>>> +PciHostBridgeGetTranslations (
>>>>>> + UINTN *Count
>>>>>> + )
>>>>>> +{
>>>>>> + *Count = 0;
>>>>>> + return NULL;
>>>>>> +}
>>>>>> +
>>>>>> /**
>>>>>> Free the root bridge instances array returned from
>>>>>> PciHostBridgeGetRootBridges().
>>>>>> @@ -377,6 +387,15 @@ PciHostBridgeFreeRootBridges (
>>>>>> ASSERT (Count == 1);
>>>>>> }
>>>>>>
>>>>>> +VOID
>>>>>> +EFIAPI
>>>>>> +PciHostBridgeFreeTranslations (
>>>>>> + PCI_ROOT_BRIDGE_TRANSLATION *Translations,
>>>>>> + UINTN Count
>>>>>> + )
>>>>>> +{
>>>>>> +}
>>>>>> +
>>>>>> /**
>>>>>> Inform the platform that the resource conflict happens.
>>>>>>
>>>>>> diff --git asame/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>>> index 1494848..835e411 100644
>>>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>>> @@ -360,18 +360,38 @@ InitializePciHostBridge (
>>>>>> PCI_HOST_BRIDGE_INSTANCE *HostBridge;
>>>>>> PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
>>>>>> PCI_ROOT_BRIDGE *RootBridges;
>>>>>> + PCI_ROOT_BRIDGE_TRANSLATION *Translations;
>>>>>> UINTN RootBridgeCount;
>>>>>> + UINTN TranslationCount;
>>>>>> UINTN Index;
>>>>>> PCI_ROOT_BRIDGE_APERTURE *MemApertures[4];
>>>>>
>>>>>
>>>>> Wouldn't it be much better to add a 'translation' member to
>>>>> PCI_ROOT_BRIDGE_APERTURE? That way, existing code just default to a
>>>>> translation of 0, and all the handling of the separate array can be
>>>>> dropped.
>>>>>
>>>> Actually my first idea was the same, but when I looked at the
>>>> implementation of
>>>> PciHostBridgeLib of Ovmf, I found it a little tedious to change the
>>>> existing code in this way. We'll need to check everywhere
>>>> PCI_ROOT_BRIDGE_APERTURE or PCI_ROOT_BRIDGE is used, to make sure the
>>>> translation field is initialized to be zero, e.g. line 235~245.
>>>>
>>>> What I did in this RFC is not so straightforward indeed. So I can change
>>>> the
>>>> code if we all agree to add Translation field into
>>>> PCI_ROOT_BRIDGE_APERTURE
>>>> directly.
>>>>
>>>> Thanks,
>>>>
>>>> Gary (Heyi Guo)
>>>
>>>
>>> I also agree to put the translation fields into the
>>> PCI_ROOT_BRIDGE_APERTURE.
>>>
>>>
>>> But I think we may have different understandings to the address
>>> translation.
>>> My understanding to the whole translation:
>>> 1. PciHostBridsamesamege.GetProposedResources () returns resource information
>>> for a single root bridge. It only carries the address range in PCI
>>> view.
>>> The address range/resource is reported/submitted by PciHostBridgeLib.
>>> Before the change, CPU view equals to PCI view. So PciHostBridgeDxe
>>> driver directly adds the resource to GCD.
>>> In your change, PciHostBridgeDxe assumes the source is in PCI view
>>> and it adds offset to convert to CPU view.
>>> CPU-address = PCI-address + offset. <-- Equation #A
>>>
>>>
>>> 2. PciRootBridgeIo.Configuration() returns the resource information
>>> for a single root bridge. It carries the address range in CPU view,
>>> and the translation offset.
>>> However, per UEFI spec, "Address Translation Offset. Offset to apply
>>> to the Starting address of a BAR to convert it to a PCI address. "
>>> PCI-address = CPU-address + offset. <-- Equation #B
>>
>>
>> If we get Equation #B from the statement in UEFI spec, does it also imply
>> Starting address is "Address Range Minimum" and so it is CPU view address?
>>
>> If we use Equation #B, can offset be a negative value? If it is not, then
>> it
>> will make translation useless, since we cannot change CPU address above 4G
>> into
>> PCI address under 4G for legacy compatibility.
>>
>> Equation #B is also not consistent with how OS treats address translation;
>> please see the ACPI ASL code which works well for OS:
>>
>> https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Hisilicon/Hi1616/D05AcpiTables/Dsdt/D05Pci.asl#L201
>
>
> I agree with your statement about the ASL code.
> I copied the following wordings from ACPI spec:
>
> Byte 14 Address range minimum,
> _MIN bits[7:0]
> For bridges that translate addresses, this is the address space on
> the secondary side of the bridge.
>
> Byte 30 Address Translation
> offset, _TRA bits[7:0]
> For bridges that translate addresses across the bridge, this is the
> offset that must be added to the address on the secondary side to
> obtain the address on the primary side. Non-bridge devices must
> list 0 for all Address Translation offset bits.
>
> It seems UEFI Spec conflicts with ACPI Spec.
> UEFI Spec: AddressRangeMin = CPU-view address
> ACPI Spec: AddressRangeMin = PCI-view address
>
> I remembered that this topic was discussed long time ago
> and re-discussed in year 2017.
> There is no conclusion.
>
> I tend to agree to align to ACPI spec.
> But I am not sure what impact it may cause.
>
> Actually this CPU/PCI address topic was discussed long time
> ago and re-discussed in patch mail:
> https://lists.01.org/pipermail/edk2-devel/2017-September/014425.html
> No conclusion so the patch was also not checked in.
>
> With your proposed patch (maybe refine or address translation logic
> updates are needed), I think it's a good opportunity for us to
> review the whole PCI resource allocation logic in PciHostBridge/PciBus
> /GCD, and propose a consistent/clear solution.
>
>
This has indeed been discussed many times, but the UEFI spec is quite
clear that its definition of the QWORD address space descriptor
supersedes the one in the ACPI spec.
Given that an offset can be both positive and negative, it is
reasonable to assume that this field may be interpreted as signed
and/or may be truncated on 64-bit overflow (which come down to the
same thing)
I would rather not park this discussion again. The UEFI spec may be
quirky in this regard, but it is perfectly clear.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
2018-02-01 17:22 ` Ard Biesheuvel
@ 2018-02-02 0:34 ` Ni, Ruiyu
2018-02-02 8:22 ` Ard Biesheuvel
0 siblings, 1 reply; 12+ messages in thread
From: Ni, Ruiyu @ 2018-02-02 0:34 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Guo Heyi <heyi.guo@linaro.org>, Dong Wei, Dong, Eric,
edk2-devel@lists.01.org, linaro-uefi, Kinney, Michael D,
Zeng, Star
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Friday, February 2, 2018 1:23 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>
> Cc: Guo Heyi <heyi.guo@linaro.org>,Dong Wei <Dong.Wei@arm.com>; Dong,
> Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; linaro-uefi <linaro-
> uefi@lists.linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng,
> Star <star.zeng@intel.com>
> Subject: Re: [edk2] [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for
> address translation
>
> On 1 February 2018 at 05:03, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> > On 1/29/2018 4:50 PM, Guo Heyi wrote:
> >>
> >> Sorry for the late; I caught cold and didn't work for several days
> >> last week :( Please see my comments below:
> >>
> >>
> >> On Mon, Jan 22, 2018 at 11:36:14AM +0800, Ni, Ruiyu wrote:
> >>>
> >>> On 1/18/2018 9:26 AM, Guo Heyi wrote:
> >>>>
> >>>> On Wed, Jan 17, 2018 at 02:08:06PM +0000, Ard Biesheuvel wrote:
> >>>>>
> >>>>> On 15 January 2018 at 14:46, Heyi Guo <heyi.guo@linaro.org> wrote:
> >>>>>>
> >>>>>> This is the draft patch for the discussion posted in edk2-devel
> >>>>>> mailing list:
> >>>>>> https://lists.01.org/pipermail/edk2-devel/2017-December/019289.ht
> >>>>>> ml
> >>>>>>
> >>>>>> As discussed in the mailing list, we'd like to add support for
> >>>>>> PCI address translation which is necessary for some non-x86
> >>>>>> platforms. I also want to minimize the changes to the generic
> >>>>>> host bridge driver and platform PciHostBridgeLib implemetations,
> >>>>>> so additional two interfaces are added to expose translation
> >>>>>> information of the platform. To be generic, I add translation for
> >>>>>> each type of IO or memory resources.
> >>>>>>
> >>>>>> The patch is still a RFC, so I only passed the build for qemu64
> >>>>>> and the function has not been tested yet.
> >>>>>>
> >>>>>> Please let me know your comments about it.
> >>>>>>
> >>>>>> Thanks.
> >>>>>>
> >>>>>> 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>
> >>>>>> ---
> >>>>>> .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 19 ++++
> >>>>>> .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 53 ++++++++---
> >>>>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridge.h | 8 +-
> >>>>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 101
> >>>>>> ++++++++++++++++++---
> >>>>>> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 36 ++++++++
> >>>>>> 5 files changed, 192 insertions(+), 25 deletions(-)
> >>>>>>
> >>>>>> diff --git
> >>>>>> a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> >>>>>> b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> >>>>>> index 5b9c887..0c8371a 100644
> >>>>>> ---
> >>>>>> a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> >>>>>> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.
> >>>>>> +++ c
> >>>>>> @@ -360,6 +360,16 @@ PciHostBridgeGetRootBridges (
> >>>>>> return &mRootBridge;
> >>>>>> }
> >>>>>>
> >>>>>> +PCI_ROOT_BRIDGE_TRANSLATION *
> >>>>>> +EFIAPI
> >>>>>> +PciHostBridgeGetTranslations (
> >>>>>> + UINTN *Count
> >>>>>> + )
> >>>>>> +{
> >>>>>> + *Count = 0;
> >>>>>> + return NULL;
> >>>>>> +}
> >>>>>> +
> >>>>>> /**
> >>>>>> Free the root bridge instances array returned from
> >>>>>> PciHostBridgeGetRootBridges().
> >>>>>> @@ -377,6 +387,15 @@ PciHostBridgeFreeRootBridges (
> >>>>>> ASSERT (Count == 1);
> >>>>>> }
> >>>>>>
> >>>>>> +VOID
> >>>>>> +EFIAPI
> >>>>>> +PciHostBridgeFreeTranslations (
> >>>>>> + PCI_ROOT_BRIDGE_TRANSLATION *Translations,
> >>>>>> + UINTN Count
> >>>>>> + )
> >>>>>> +{
> >>>>>> +}
> >>>>>> +
> >>>>>> /**
> >>>>>> Inform the platform that the resource conflict happens.
> >>>>>>
> >>>>>> diff --git
> >>>>>> asame/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>>>> index 1494848..835e411 100644
> >>>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>>>> @@ -360,18 +360,38 @@ InitializePciHostBridge (
> >>>>>> PCI_HOST_BRIDGE_INSTANCE *HostBridge;
> >>>>>> PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> >>>>>> PCI_ROOT_BRIDGE *RootBridges;
> >>>>>> + PCI_ROOT_BRIDGE_TRANSLATION *Translations;
> >>>>>> UINTN RootBridgeCount;
> >>>>>> + UINTN TranslationCount;
> >>>>>> UINTN Index;
> >>>>>> PCI_ROOT_BRIDGE_APERTURE *MemApertures[4];
> >>>>>
> >>>>>
> >>>>> Wouldn't it be much better to add a 'translation' member to
> >>>>> PCI_ROOT_BRIDGE_APERTURE? That way, existing code just default to
> >>>>> a translation of 0, and all the handling of the separate array can
> >>>>> be dropped.
> >>>>>
> >>>> Actually my first idea was the same, but when I looked at the
> >>>> implementation of PciHostBridgeLib of Ovmf, I found it a little
> >>>> tedious to change the existing code in this way. We'll need to
> >>>> check everywhere PCI_ROOT_BRIDGE_APERTURE or PCI_ROOT_BRIDGE is
> >>>> used, to make sure the translation field is initialized to be zero,
> >>>> e.g. line 235~245.
> >>>>
> >>>> What I did in this RFC is not so straightforward indeed. So I can
> >>>> change the code if we all agree to add Translation field into
> >>>> PCI_ROOT_BRIDGE_APERTURE directly.
> >>>>
> >>>> Thanks,
> >>>>
> >>>> Gary (Heyi Guo)
> >>>
> >>>
> >>> I also agree to put the translation fields into the
> >>> PCI_ROOT_BRIDGE_APERTURE.
> >>>
> >>>
> >>> But I think we may have different understandings to the address
> >>> translation.
> >>> My understanding to the whole translation:
> >>> 1. PciHostBridsamesamege.GetProposedResources () returns resource
> information
> >>> for a single root bridge. It only carries the address range in PCI
> >>> view.
> >>> The address range/resource is reported/submitted by PciHostBridgeLib.
> >>> Before the change, CPU view equals to PCI view. So PciHostBridgeDxe
> >>> driver directly adds the resource to GCD.
> >>> In your change, PciHostBridgeDxe assumes the source is in PCI view
> >>> and it adds offset to convert to CPU view.
> >>> CPU-address = PCI-address + offset. <-- Equation #A
> >>>
> >>>
> >>> 2. PciRootBridgeIo.Configuration() returns the resource information
> >>> for a single root bridge. It carries the address range in CPU view,
> >>> and the translation offset.
> >>> However, per UEFI spec, "Address Translation Offset. Offset to apply
> >>> to the Starting address of a BAR to convert it to a PCI address. "
> >>> PCI-address = CPU-address + offset. <-- Equation #B
> >>
> >>
> >> If we get Equation #B from the statement in UEFI spec, does it also
> >> imply Starting address is "Address Range Minimum" and so it is CPU view
> address?
> >>
> >> If we use Equation #B, can offset be a negative value? If it is not,
> >> then it will make translation useless, since we cannot change CPU
> >> address above 4G into PCI address under 4G for legacy compatibility.
> >>
> >> Equation #B is also not consistent with how OS treats address
> >> translation; please see the ACPI ASL code which works well for OS:
> >>
> >> https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Hisil
> >> icon/Hi1616/D05AcpiTables/Dsdt/D05Pci.asl#L201
> >
> >
> > I agree with your statement about the ASL code.
> > I copied the following wordings from ACPI spec:
> >
> > Byte 14 Address range minimum,
> > _MIN bits[7:0]
> > For bridges that translate addresses, this is the address space on the
> > secondary side of the bridge.
> >
> > Byte 30 Address Translation
> > offset, _TRA bits[7:0]
> > For bridges that translate addresses across the bridge, this is the
> > offset that must be added to the address on the secondary side to
> > obtain the address on the primary side. Non-bridge devices must list 0
> > for all Address Translation offset bits.
> >
> > It seems UEFI Spec conflicts with ACPI Spec.
> > UEFI Spec: AddressRangeMin = CPU-view address ACPI Spec:
> > AddressRangeMin = PCI-view address
> >
> > I remembered that this topic was discussed long time ago and
> > re-discussed in year 2017.
> > There is no conclusion.
> >
> > I tend to agree to align to ACPI spec.
> > But I am not sure what impact it may cause.
> >
> > Actually this CPU/PCI address topic was discussed long time ago and
> > re-discussed in patch mail:
> > https://lists.01.org/pipermail/edk2-devel/2017-September/014425.html
> > No conclusion so the patch was also not checked in.
> >
> > With your proposed patch (maybe refine or address translation logic
> > updates are needed), I think it's a good opportunity for us to review
> > the whole PCI resource allocation logic in PciHostBridge/PciBus /GCD,
> > and propose a consistent/clear solution.
> >
> >
>
> This has indeed been discussed many times, but the UEFI spec is quite clear that
> its definition of the QWORD address space descriptor supersedes the one in the
> ACPI spec.
>
> Given that an offset can be both positive and negative, it is reasonable to
> assume that this field may be interpreted as signed and/or may be truncated on
> 64-bit overflow (which come down to the same thing)
>
> I would rather not park this discussion again. The UEFI spec may be quirky in this
> regard, but it is perfectly clear.
Ard,
In my opinion, to align UEFI Spec to ACPI spec on this sounds like reasonable.
Do you see any real problem if we change the meeting of AddressRangeMin?
Thanks,
Ray
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
2018-02-02 0:34 ` Ni, Ruiyu
@ 2018-02-02 8:22 ` Ard Biesheuvel
2018-02-03 6:00 ` Ni, Ruiyu
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2018-02-02 8:22 UTC (permalink / raw)
To: Ni, Ruiyu
Cc: Guo Heyi <heyi.guo@linaro.org>, Dong Wei, Dong, Eric,
edk2-devel@lists.01.org, linaro-uefi, Kinney, Michael D,
Zeng, Star
On 2 February 2018 at 00:34, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Friday, February 2, 2018 1:23 AM
>> To: Ni, Ruiyu <ruiyu.ni@intel.com>
>> Cc: Guo Heyi <heyi.guo@linaro.org>,Dong Wei <Dong.Wei@arm.com>; Dong,
>> Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; linaro-uefi <linaro-
>> uefi@lists.linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng,
>> Star <star.zeng@intel.com>
>> Subject: Re: [edk2] [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for
>> address translation
>>
>> On 1 February 2018 at 05:03, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
>> > On 1/29/2018 4:50 PM, Guo Heyi wrote:
>> >>
>> >> Sorry for the late; I caught cold and didn't work for several days
>> >> last week :( Please see my comments below:
>> >>
>> >>
>> >> On Mon, Jan 22, 2018 at 11:36:14AM +0800, Ni, Ruiyu wrote:
>> >>>
>> >>> On 1/18/2018 9:26 AM, Guo Heyi wrote:
>> >>>>
>> >>>> On Wed, Jan 17, 2018 at 02:08:06PM +0000, Ard Biesheuvel wrote:
>> >>>>>
>> >>>>> On 15 January 2018 at 14:46, Heyi Guo <heyi.guo@linaro.org> wrote:
>> >>>>>>
>> >>>>>> This is the draft patch for the discussion posted in edk2-devel
>> >>>>>> mailing list:
>> >>>>>> https://lists.01.org/pipermail/edk2-devel/2017-December/019289.ht
>> >>>>>> ml
>> >>>>>>
>> >>>>>> As discussed in the mailing list, we'd like to add support for
>> >>>>>> PCI address translation which is necessary for some non-x86
>> >>>>>> platforms. I also want to minimize the changes to the generic
>> >>>>>> host bridge driver and platform PciHostBridgeLib implemetations,
>> >>>>>> so additional two interfaces are added to expose translation
>> >>>>>> information of the platform. To be generic, I add translation for
>> >>>>>> each type of IO or memory resources.
>> >>>>>>
>> >>>>>> The patch is still a RFC, so I only passed the build for qemu64
>> >>>>>> and the function has not been tested yet.
>> >>>>>>
>> >>>>>> Please let me know your comments about it.
>> >>>>>>
>> >>>>>> Thanks.
>> >>>>>>
>> >>>>>> 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>
>> >>>>>> ---
>> >>>>>> .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 19 ++++
>> >>>>>> .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 53 ++++++++---
>> >>>>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridge.h | 8 +-
>> >>>>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 101
>> >>>>>> ++++++++++++++++++---
>> >>>>>> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 36 ++++++++
>> >>>>>> 5 files changed, 192 insertions(+), 25 deletions(-)
>> >>>>>>
>> >>>>>> diff --git
>> >>>>>> a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>> >>>>>> b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>> >>>>>> index 5b9c887..0c8371a 100644
>> >>>>>> ---
>> >>>>>> a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>> >>>>>> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.
>> >>>>>> +++ c
>> >>>>>> @@ -360,6 +360,16 @@ PciHostBridgeGetRootBridges (
>> >>>>>> return &mRootBridge;
>> >>>>>> }
>> >>>>>>
>> >>>>>> +PCI_ROOT_BRIDGE_TRANSLATION *
>> >>>>>> +EFIAPI
>> >>>>>> +PciHostBridgeGetTranslations (
>> >>>>>> + UINTN *Count
>> >>>>>> + )
>> >>>>>> +{
>> >>>>>> + *Count = 0;
>> >>>>>> + return NULL;
>> >>>>>> +}
>> >>>>>> +
>> >>>>>> /**
>> >>>>>> Free the root bridge instances array returned from
>> >>>>>> PciHostBridgeGetRootBridges().
>> >>>>>> @@ -377,6 +387,15 @@ PciHostBridgeFreeRootBridges (
>> >>>>>> ASSERT (Count == 1);
>> >>>>>> }
>> >>>>>>
>> >>>>>> +VOID
>> >>>>>> +EFIAPI
>> >>>>>> +PciHostBridgeFreeTranslations (
>> >>>>>> + PCI_ROOT_BRIDGE_TRANSLATION *Translations,
>> >>>>>> + UINTN Count
>> >>>>>> + )
>> >>>>>> +{
>> >>>>>> +}
>> >>>>>> +
>> >>>>>> /**
>> >>>>>> Inform the platform that the resource conflict happens.
>> >>>>>>
>> >>>>>> diff --git
>> >>>>>> asame/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>> >>>>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>> >>>>>> index 1494848..835e411 100644
>> >>>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>> >>>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>> >>>>>> @@ -360,18 +360,38 @@ InitializePciHostBridge (
>> >>>>>> PCI_HOST_BRIDGE_INSTANCE *HostBridge;
>> >>>>>> PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
>> >>>>>> PCI_ROOT_BRIDGE *RootBridges;
>> >>>>>> + PCI_ROOT_BRIDGE_TRANSLATION *Translations;
>> >>>>>> UINTN RootBridgeCount;
>> >>>>>> + UINTN TranslationCount;
>> >>>>>> UINTN Index;
>> >>>>>> PCI_ROOT_BRIDGE_APERTURE *MemApertures[4];
>> >>>>>
>> >>>>>
>> >>>>> Wouldn't it be much better to add a 'translation' member to
>> >>>>> PCI_ROOT_BRIDGE_APERTURE? That way, existing code just default to
>> >>>>> a translation of 0, and all the handling of the separate array can
>> >>>>> be dropped.
>> >>>>>
>> >>>> Actually my first idea was the same, but when I looked at the
>> >>>> implementation of PciHostBridgeLib of Ovmf, I found it a little
>> >>>> tedious to change the existing code in this way. We'll need to
>> >>>> check everywhere PCI_ROOT_BRIDGE_APERTURE or PCI_ROOT_BRIDGE is
>> >>>> used, to make sure the translation field is initialized to be zero,
>> >>>> e.g. line 235~245.
>> >>>>
>> >>>> What I did in this RFC is not so straightforward indeed. So I can
>> >>>> change the code if we all agree to add Translation field into
>> >>>> PCI_ROOT_BRIDGE_APERTURE directly.
>> >>>>
>> >>>> Thanks,
>> >>>>
>> >>>> Gary (Heyi Guo)
>> >>>
>> >>>
>> >>> I also agree to put the translation fields into the
>> >>> PCI_ROOT_BRIDGE_APERTURE.
>> >>>
>> >>>
>> >>> But I think we may have different understandings to the address
>> >>> translation.
>> >>> My understanding to the whole translation:
>> >>> 1. PciHostBridsamesamege.GetProposedResources () returns resource
>> information
>> >>> for a single root bridge. It only carries the address range in PCI
>> >>> view.
>> >>> The address range/resource is reported/submitted by PciHostBridgeLib.
>> >>> Before the change, CPU view equals to PCI view. So PciHostBridgeDxe
>> >>> driver directly adds the resource to GCD.
>> >>> In your change, PciHostBridgeDxe assumes the source is in PCI view
>> >>> and it adds offset to convert to CPU view.
>> >>> CPU-address = PCI-address + offset. <-- Equation #A
>> >>>
>> >>>
>> >>> 2. PciRootBridgeIo.Configuration() returns the resource information
>> >>> for a single root bridge. It carries the address range in CPU view,
>> >>> and the translation offset.
>> >>> However, per UEFI spec, "Address Translation Offset. Offset to apply
>> >>> to the Starting address of a BAR to convert it to a PCI address. "
>> >>> PCI-address = CPU-address + offset. <-- Equation #B
>> >>
>> >>
>> >> If we get Equation #B from the statement in UEFI spec, does it also
>> >> imply Starting address is "Address Range Minimum" and so it is CPU view
>> address?
>> >>
>> >> If we use Equation #B, can offset be a negative value? If it is not,
>> >> then it will make translation useless, since we cannot change CPU
>> >> address above 4G into PCI address under 4G for legacy compatibility.
>> >>
>> >> Equation #B is also not consistent with how OS treats address
>> >> translation; please see the ACPI ASL code which works well for OS:
>> >>
>> >> https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Hisil
>> >> icon/Hi1616/D05AcpiTables/Dsdt/D05Pci.asl#L201
>> >
>> >
>> > I agree with your statement about the ASL code.
>> > I copied the following wordings from ACPI spec:
>> >
>> > Byte 14 Address range minimum,
>> > _MIN bits[7:0]
>> > For bridges that translate addresses, this is the address space on the
>> > secondary side of the bridge.
>> >
>> > Byte 30 Address Translation
>> > offset, _TRA bits[7:0]
>> > For bridges that translate addresses across the bridge, this is the
>> > offset that must be added to the address on the secondary side to
>> > obtain the address on the primary side. Non-bridge devices must list 0
>> > for all Address Translation offset bits.
>> >
>> > It seems UEFI Spec conflicts with ACPI Spec.
>> > UEFI Spec: AddressRangeMin = CPU-view address ACPI Spec:
>> > AddressRangeMin = PCI-view address
>> >
>> > I remembered that this topic was discussed long time ago and
>> > re-discussed in year 2017.
>> > There is no conclusion.
>> >
>> > I tend to agree to align to ACPI spec.
>> > But I am not sure what impact it may cause.
>> >
>> > Actually this CPU/PCI address topic was discussed long time ago and
>> > re-discussed in patch mail:
>> > https://lists.01.org/pipermail/edk2-devel/2017-September/014425.html
>> > No conclusion so the patch was also not checked in.
>> >
>> > With your proposed patch (maybe refine or address translation logic
>> > updates are needed), I think it's a good opportunity for us to review
>> > the whole PCI resource allocation logic in PciHostBridge/PciBus /GCD,
>> > and propose a consistent/clear solution.
>> >
>> >
>>
>> This has indeed been discussed many times, but the UEFI spec is quite clear that
>> its definition of the QWORD address space descriptor supersedes the one in the
>> ACPI spec.
>>
>> Given that an offset can be both positive and negative, it is reasonable to
>> assume that this field may be interpreted as signed and/or may be truncated on
>> 64-bit overflow (which come down to the same thing)
>>
>> I would rather not park this discussion again. The UEFI spec may be quirky in this
>> regard, but it is perfectly clear.
>
> Ard,
> In my opinion, to align UEFI Spec to ACPI spec on this sounds like reasonable.
> Do you see any real problem if we change the meeting of AddressRangeMin?
>
I agree it would be best to fix the spec, but only if it applies
retroactively, i.e., if it is incorporated as an erratum into the
current version.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
2018-02-02 8:22 ` Ard Biesheuvel
@ 2018-02-03 6:00 ` Ni, Ruiyu
2018-02-05 8:06 ` Ni, Ruiyu
0 siblings, 1 reply; 12+ messages in thread
From: Ni, Ruiyu @ 2018-02-03 6:00 UTC (permalink / raw)
To: Ard Biesheuvel, Rothman, Michael A
Cc: Guo Heyi <heyi.guo@linaro.org>, Dong Wei, Dong, Eric,
edk2-devel@lists.01.org, linaro-uefi, Kinney, Michael D,
Zeng, Star
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Friday, February 2, 2018 4:22 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>
> Cc: Guo Heyi <heyi.guo@linaro.org>,Dong Wei <Dong.Wei@arm.com>; Dong,
> Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; linaro-uefi <linaro-
> uefi@lists.linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng,
> Star <star.zeng@intel.com>
> Subject: Re: [edk2] [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for
> address translation
>
> On 2 February 2018 at 00:34, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >> Sent: Friday, February 2, 2018 1:23 AM
> >> To: Ni, Ruiyu <ruiyu.ni@intel.com>
> >> Cc: Guo Heyi <heyi.guo@linaro.org>,Dong Wei <Dong.Wei@arm.com>;
> Dong,
> >> Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; linaro-uefi
> >> <linaro- uefi@lists.linaro.org>; Kinney, Michael D
> >> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
> >> Subject: Re: [edk2] [RFC] MdeModulePkg/PciHostBridgeDxe: Add support
> >> for address translation
> >>
> >> On 1 February 2018 at 05:03, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> >> > On 1/29/2018 4:50 PM, Guo Heyi wrote:
> >> >>
> >> >> Sorry for the late; I caught cold and didn't work for several days
> >> >> last week :( Please see my comments below:
> >> >>
> >> >>
> >> >> On Mon, Jan 22, 2018 at 11:36:14AM +0800, Ni, Ruiyu wrote:
> >> >>>
> >> >>> On 1/18/2018 9:26 AM, Guo Heyi wrote:
> >> >>>>
> >> >>>> On Wed, Jan 17, 2018 at 02:08:06PM +0000, Ard Biesheuvel wrote:
> >> >>>>>
> >> >>>>> On 15 January 2018 at 14:46, Heyi Guo <heyi.guo@linaro.org> wrote:
> >> >>>>>>
> >> >>>>>> This is the draft patch for the discussion posted in
> >> >>>>>> edk2-devel mailing list:
> >> >>>>>> https://lists.01.org/pipermail/edk2-devel/2017-December/019289
> >> >>>>>> .ht
> >> >>>>>> ml
> >> >>>>>>
> >> >>>>>> As discussed in the mailing list, we'd like to add support for
> >> >>>>>> PCI address translation which is necessary for some non-x86
> >> >>>>>> platforms. I also want to minimize the changes to the generic
> >> >>>>>> host bridge driver and platform PciHostBridgeLib
> >> >>>>>> implemetations, so additional two interfaces are added to
> >> >>>>>> expose translation information of the platform. To be generic,
> >> >>>>>> I add translation for each type of IO or memory resources.
> >> >>>>>>
> >> >>>>>> The patch is still a RFC, so I only passed the build for
> >> >>>>>> qemu64 and the function has not been tested yet.
> >> >>>>>>
> >> >>>>>> Please let me know your comments about it.
> >> >>>>>>
> >> >>>>>> Thanks.
> >> >>>>>>
> >> >>>>>> 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>
> >> >>>>>> ---
> >> >>>>>> .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 19 ++++
> >> >>>>>> .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 53 ++++++++---
> >> >>>>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridge.h | 8 +-
> >> >>>>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 101
> >> >>>>>> ++++++++++++++++++---
> >> >>>>>> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 36
> ++++++++
> >> >>>>>> 5 files changed, 192 insertions(+), 25 deletions(-)
> >> >>>>>>
> >> >>>>>> diff --git
> >> >>>>>> a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> >> >>>>>> b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> >> >>>>>> index 5b9c887..0c8371a 100644
> >> >>>>>> ---
> >> >>>>>> a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> >> >>>>>> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.
> >> >>>>>> +++ c
> >> >>>>>> @@ -360,6 +360,16 @@ PciHostBridgeGetRootBridges (
> >> >>>>>> return &mRootBridge;
> >> >>>>>> }
> >> >>>>>>
> >> >>>>>> +PCI_ROOT_BRIDGE_TRANSLATION * EFIAPI
> >> >>>>>> +PciHostBridgeGetTranslations (
> >> >>>>>> + UINTN *Count
> >> >>>>>> + )
> >> >>>>>> +{
> >> >>>>>> + *Count = 0;
> >> >>>>>> + return NULL;
> >> >>>>>> +}
> >> >>>>>> +
> >> >>>>>> /**
> >> >>>>>> Free the root bridge instances array returned from
> >> >>>>>> PciHostBridgeGetRootBridges().
> >> >>>>>> @@ -377,6 +387,15 @@ PciHostBridgeFreeRootBridges (
> >> >>>>>> ASSERT (Count == 1);
> >> >>>>>> }
> >> >>>>>>
> >> >>>>>> +VOID
> >> >>>>>> +EFIAPI
> >> >>>>>> +PciHostBridgeFreeTranslations (
> >> >>>>>> + PCI_ROOT_BRIDGE_TRANSLATION *Translations,
> >> >>>>>> + UINTN Count
> >> >>>>>> + )
> >> >>>>>> +{
> >> >>>>>> +}
> >> >>>>>> +
> >> >>>>>> /**
> >> >>>>>> Inform the platform that the resource conflict happens.
> >> >>>>>>
> >> >>>>>> diff --git
> >> >>>>>> asame/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >> >>>>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >> >>>>>> index 1494848..835e411 100644
> >> >>>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >> >>>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >> >>>>>> @@ -360,18 +360,38 @@ InitializePciHostBridge (
> >> >>>>>> PCI_HOST_BRIDGE_INSTANCE *HostBridge;
> >> >>>>>> PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> >> >>>>>> PCI_ROOT_BRIDGE *RootBridges;
> >> >>>>>> + PCI_ROOT_BRIDGE_TRANSLATION *Translations;
> >> >>>>>> UINTN RootBridgeCount;
> >> >>>>>> + UINTN TranslationCount;
> >> >>>>>> UINTN Index;
> >> >>>>>> PCI_ROOT_BRIDGE_APERTURE *MemApertures[4];
> >> >>>>>
> >> >>>>>
> >> >>>>> Wouldn't it be much better to add a 'translation' member to
> >> >>>>> PCI_ROOT_BRIDGE_APERTURE? That way, existing code just default
> >> >>>>> to a translation of 0, and all the handling of the separate
> >> >>>>> array can be dropped.
> >> >>>>>
> >> >>>> Actually my first idea was the same, but when I looked at the
> >> >>>> implementation of PciHostBridgeLib of Ovmf, I found it a little
> >> >>>> tedious to change the existing code in this way. We'll need to
> >> >>>> check everywhere PCI_ROOT_BRIDGE_APERTURE or
> PCI_ROOT_BRIDGE is
> >> >>>> used, to make sure the translation field is initialized to be
> >> >>>> zero, e.g. line 235~245.
> >> >>>>
> >> >>>> What I did in this RFC is not so straightforward indeed. So I
> >> >>>> can change the code if we all agree to add Translation field
> >> >>>> into PCI_ROOT_BRIDGE_APERTURE directly.
> >> >>>>
> >> >>>> Thanks,
> >> >>>>
> >> >>>> Gary (Heyi Guo)
> >> >>>
> >> >>>
> >> >>> I also agree to put the translation fields into the
> >> >>> PCI_ROOT_BRIDGE_APERTURE.
> >> >>>
> >> >>>
> >> >>> But I think we may have different understandings to the address
> >> >>> translation.
> >> >>> My understanding to the whole translation:
> >> >>> 1. PciHostBridsamesamege.GetProposedResources () returns resource
> >> information
> >> >>> for a single root bridge. It only carries the address range in PCI
> >> >>> view.
> >> >>> The address range/resource is reported/submitted by PciHostBridgeLib.
> >> >>> Before the change, CPU view equals to PCI view. So PciHostBridgeDxe
> >> >>> driver directly adds the resource to GCD.
> >> >>> In your change, PciHostBridgeDxe assumes the source is in PCI view
> >> >>> and it adds offset to convert to CPU view.
> >> >>> CPU-address = PCI-address + offset. <-- Equation #A
> >> >>>
> >> >>>
> >> >>> 2. PciRootBridgeIo.Configuration() returns the resource information
> >> >>> for a single root bridge. It carries the address range in CPU view,
> >> >>> and the translation offset.
> >> >>> However, per UEFI spec, "Address Translation Offset. Offset to apply
> >> >>> to the Starting address of a BAR to convert it to a PCI address. "
> >> >>> PCI-address = CPU-address + offset. <-- Equation #B
> >> >>
> >> >>
> >> >> If we get Equation #B from the statement in UEFI spec, does it
> >> >> also imply Starting address is "Address Range Minimum" and so it
> >> >> is CPU view
> >> address?
> >> >>
> >> >> If we use Equation #B, can offset be a negative value? If it is
> >> >> not, then it will make translation useless, since we cannot change
> >> >> CPU address above 4G into PCI address under 4G for legacy compatibility.
> >> >>
> >> >> Equation #B is also not consistent with how OS treats address
> >> >> translation; please see the ACPI ASL code which works well for OS:
> >> >>
> >> >> https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Hi
> >> >> sil
> >> >> icon/Hi1616/D05AcpiTables/Dsdt/D05Pci.asl#L201
> >> >
> >> >
> >> > I agree with your statement about the ASL code.
> >> > I copied the following wordings from ACPI spec:
> >> >
> >> > Byte 14 Address range minimum,
> >> > _MIN bits[7:0]
> >> > For bridges that translate addresses, this is the address space on
> >> > the secondary side of the bridge.
> >> >
> >> > Byte 30 Address Translation
> >> > offset, _TRA bits[7:0]
> >> > For bridges that translate addresses across the bridge, this is the
> >> > offset that must be added to the address on the secondary side to
> >> > obtain the address on the primary side. Non-bridge devices must
> >> > list 0 for all Address Translation offset bits.
> >> >
> >> > It seems UEFI Spec conflicts with ACPI Spec.
> >> > UEFI Spec: AddressRangeMin = CPU-view address ACPI Spec:
> >> > AddressRangeMin = PCI-view address
> >> >
> >> > I remembered that this topic was discussed long time ago and
> >> > re-discussed in year 2017.
> >> > There is no conclusion.
> >> >
> >> > I tend to agree to align to ACPI spec.
> >> > But I am not sure what impact it may cause.
> >> >
> >> > Actually this CPU/PCI address topic was discussed long time ago and
> >> > re-discussed in patch mail:
> >> > https://lists.01.org/pipermail/edk2-devel/2017-September/014425.htm
> >> > l No conclusion so the patch was also not checked in.
> >> >
> >> > With your proposed patch (maybe refine or address translation logic
> >> > updates are needed), I think it's a good opportunity for us to
> >> > review the whole PCI resource allocation logic in
> >> > PciHostBridge/PciBus /GCD, and propose a consistent/clear solution.
> >> >
> >> >
> >>
> >> This has indeed been discussed many times, but the UEFI spec is quite
> >> clear that its definition of the QWORD address space descriptor
> >> supersedes the one in the ACPI spec.
> >>
> >> Given that an offset can be both positive and negative, it is
> >> reasonable to assume that this field may be interpreted as signed
> >> and/or may be truncated on 64-bit overflow (which come down to the
> >> same thing)
> >>
> >> I would rather not park this discussion again. The UEFI spec may be
> >> quirky in this regard, but it is perfectly clear.
> >
> > Ard,
> > In my opinion, to align UEFI Spec to ACPI spec on this sounds like reasonable.
> > Do you see any real problem if we change the meeting of AddressRangeMin?
> >
>
> I agree it would be best to fix the spec, but only if it applies retroactively, i.e., if
> it is incorporated as an erratum into the current version.
Copy Mike for opinions.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
2018-02-03 6:00 ` Ni, Ruiyu
@ 2018-02-05 8:06 ` Ni, Ruiyu
2018-02-05 9:11 ` Ard Biesheuvel
0 siblings, 1 reply; 12+ messages in thread
From: Ni, Ruiyu @ 2018-02-05 8:06 UTC (permalink / raw)
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>;Guo Heyi,
Rothman, Michael A
Cc: Dong, Eric, Guo Heyi, Dong Wei, linaro-uefi, Kinney, Michael D,
edk2-devel@lists.01.org, Zeng, Star
On 2/3/2018 2:00 PM, Ni, Ruiyu wrote:
>
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Friday, February 2, 2018 4:22 PM
>> To: Ni, Ruiyu <ruiyu.ni@intel.com>
>> Cc: Guo Heyi <heyi.guo@linaro.org>,Dong Wei <Dong.Wei@arm.com>; Dong,
>> Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; linaro-uefi <linaro-
>> uefi@lists.linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng,
>> Star <star.zeng@intel.com>
>> Subject: Re: [edk2] [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for
>> address translation
>>
>> On 2 February 2018 at 00:34, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>>>> Sent: Friday, February 2, 2018 1:23 AM
>>>> To: Ni, Ruiyu <ruiyu.ni@intel.com>
>>>> Cc: Guo Heyi <heyi.guo@linaro.org>,Dong Wei <Dong.Wei@arm.com>;
>> Dong,
>>>> Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; linaro-uefi
>>>> <linaro- uefi@lists.linaro.org>; Kinney, Michael D
>>>> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
>>>> Subject: Re: [edk2] [RFC] MdeModulePkg/PciHostBridgeDxe: Add support
>>>> for address translation
>>>>
>>>> On 1 February 2018 at 05:03, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
>>>>> On 1/29/2018 4:50 PM, Guo Heyi wrote:
>>>>>>
>>>>>> Sorry for the late; I caught cold and didn't work for several days
>>>>>> last week :( Please see my comments below:
>>>>>>
>>>>>>
>>>>>> On Mon, Jan 22, 2018 at 11:36:14AM +0800, Ni, Ruiyu wrote:
>>>>>>>
>>>>>>> On 1/18/2018 9:26 AM, Guo Heyi wrote:
>>>>>>>>
>>>>>>>> On Wed, Jan 17, 2018 at 02:08:06PM +0000, Ard Biesheuvel wrote:
>>>>>>>>>
>>>>>>>>> On 15 January 2018 at 14:46, Heyi Guo <heyi.guo@linaro.org> wrote:
>>>>>>>>>>
>>>>>>>>>> This is the draft patch for the discussion posted in
>>>>>>>>>> edk2-devel mailing list:
>>>>>>>>>> https://lists.01.org/pipermail/edk2-devel/2017-December/019289
>>>>>>>>>> .ht
>>>>>>>>>> ml
>>>>>>>>>>
>>>>>>>>>> As discussed in the mailing list, we'd like to add support for
>>>>>>>>>> PCI address translation which is necessary for some non-x86
>>>>>>>>>> platforms. I also want to minimize the changes to the generic
>>>>>>>>>> host bridge driver and platform PciHostBridgeLib
>>>>>>>>>> implemetations, so additional two interfaces are added to
>>>>>>>>>> expose translation information of the platform. To be generic,
>>>>>>>>>> I add translation for each type of IO or memory resources.
>>>>>>>>>>
>>>>>>>>>> The patch is still a RFC, so I only passed the build for
>>>>>>>>>> qemu64 and the function has not been tested yet.
>>>>>>>>>>
>>>>>>>>>> Please let me know your comments about it.
>>>>>>>>>>
>>>>>>>>>> Thanks.
>>>>>>>>>>
>>>>>>>>>> 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>
>>>>>>>>>> ---
>>>>>>>>>> .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 19 ++++
>>>>>>>>>> .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 53 ++++++++---
>>>>>>>>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridge.h | 8 +-
>>>>>>>>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 101
>>>>>>>>>> ++++++++++++++++++---
>>>>>>>>>> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 36
>> ++++++++
>>>>>>>>>> 5 files changed, 192 insertions(+), 25 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git
>>>>>>>>>> a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>>>>>>>>> b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>>>>>>>>> index 5b9c887..0c8371a 100644
>>>>>>>>>> ---
>>>>>>>>>> a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>>>>>>>>> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.
>>>>>>>>>> +++ c
>>>>>>>>>> @@ -360,6 +360,16 @@ PciHostBridgeGetRootBridges (
>>>>>>>>>> return &mRootBridge;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> +PCI_ROOT_BRIDGE_TRANSLATION * EFIAPI
>>>>>>>>>> +PciHostBridgeGetTranslations (
>>>>>>>>>> + UINTN *Count
>>>>>>>>>> + )
>>>>>>>>>> +{
>>>>>>>>>> + *Count = 0;
>>>>>>>>>> + return NULL;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> /**
>>>>>>>>>> Free the root bridge instances array returned from
>>>>>>>>>> PciHostBridgeGetRootBridges().
>>>>>>>>>> @@ -377,6 +387,15 @@ PciHostBridgeFreeRootBridges (
>>>>>>>>>> ASSERT (Count == 1);
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> +VOID
>>>>>>>>>> +EFIAPI
>>>>>>>>>> +PciHostBridgeFreeTranslations (
>>>>>>>>>> + PCI_ROOT_BRIDGE_TRANSLATION *Translations,
>>>>>>>>>> + UINTN Count
>>>>>>>>>> + )
>>>>>>>>>> +{
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> /**
>>>>>>>>>> Inform the platform that the resource conflict happens.
>>>>>>>>>>
>>>>>>>>>> diff --git
>>>>>>>>>> asame/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>>>>>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>>>>>>> index 1494848..835e411 100644
>>>>>>>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>>>>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>>>>>>> @@ -360,18 +360,38 @@ InitializePciHostBridge (
>>>>>>>>>> PCI_HOST_BRIDGE_INSTANCE *HostBridge;
>>>>>>>>>> PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
>>>>>>>>>> PCI_ROOT_BRIDGE *RootBridges;
>>>>>>>>>> + PCI_ROOT_BRIDGE_TRANSLATION *Translations;
>>>>>>>>>> UINTN RootBridgeCount;
>>>>>>>>>> + UINTN TranslationCount;
>>>>>>>>>> UINTN Index;
>>>>>>>>>> PCI_ROOT_BRIDGE_APERTURE *MemApertures[4];
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Wouldn't it be much better to add a 'translation' member to
>>>>>>>>> PCI_ROOT_BRIDGE_APERTURE? That way, existing code just default
>>>>>>>>> to a translation of 0, and all the handling of the separate
>>>>>>>>> array can be dropped.
>>>>>>>>>
>>>>>>>> Actually my first idea was the same, but when I looked at the
>>>>>>>> implementation of PciHostBridgeLib of Ovmf, I found it a little
>>>>>>>> tedious to change the existing code in this way. We'll need to
>>>>>>>> check everywhere PCI_ROOT_BRIDGE_APERTURE or
>> PCI_ROOT_BRIDGE is
>>>>>>>> used, to make sure the translation field is initialized to be
>>>>>>>> zero, e.g. line 235~245.
>>>>>>>>
>>>>>>>> What I did in this RFC is not so straightforward indeed. So I
>>>>>>>> can change the code if we all agree to add Translation field
>>>>>>>> into PCI_ROOT_BRIDGE_APERTURE directly.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Gary (Heyi Guo)
>>>>>>>
>>>>>>>
>>>>>>> I also agree to put the translation fields into the
>>>>>>> PCI_ROOT_BRIDGE_APERTURE.
>>>>>>>
>>>>>>>
>>>>>>> But I think we may have different understandings to the address
>>>>>>> translation.
>>>>>>> My understanding to the whole translation:
>>>>>>> 1. PciHostBridsamesamege.GetProposedResources () returns resource
>>>> information
>>>>>>> for a single root bridge. It only carries the address range in PCI
>>>>>>> view.
>>>>>>> The address range/resource is reported/submitted by PciHostBridgeLib.
>>>>>>> Before the change, CPU view equals to PCI view. So PciHostBridgeDxe
>>>>>>> driver directly adds the resource to GCD.
>>>>>>> In your change, PciHostBridgeDxe assumes the source is in PCI view
>>>>>>> and it adds offset to convert to CPU view.
>>>>>>> CPU-address = PCI-address + offset. <-- Equation #A
>>>>>>>
>>>>>>>
>>>>>>> 2. PciRootBridgeIo.Configuration() returns the resource information
>>>>>>> for a single root bridge. It carries the address range in CPU view,
>>>>>>> and the translation offset.
>>>>>>> However, per UEFI spec, "Address Translation Offset. Offset to apply
>>>>>>> to the Starting address of a BAR to convert it to a PCI address. "
>>>>>>> PCI-address = CPU-address + offset. <-- Equation #B
>>>>>>
>>>>>>
>>>>>> If we get Equation #B from the statement in UEFI spec, does it
>>>>>> also imply Starting address is "Address Range Minimum" and so it
>>>>>> is CPU view
>>>> address?
>>>>>>
>>>>>> If we use Equation #B, can offset be a negative value? If it is
>>>>>> not, then it will make translation useless, since we cannot change
>>>>>> CPU address above 4G into PCI address under 4G for legacy compatibility.
>>>>>>
>>>>>> Equation #B is also not consistent with how OS treats address
>>>>>> translation; please see the ACPI ASL code which works well for OS:
>>>>>>
>>>>>> https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Hi
>>>>>> sil
>>>>>> icon/Hi1616/D05AcpiTables/Dsdt/D05Pci.asl#L201
>>>>>
>>>>>
>>>>> I agree with your statement about the ASL code.
>>>>> I copied the following wordings from ACPI spec:
>>>>>
>>>>> Byte 14 Address range minimum,
>>>>> _MIN bits[7:0]
>>>>> For bridges that translate addresses, this is the address space on
>>>>> the secondary side of the bridge.
>>>>>
>>>>> Byte 30 Address Translation
>>>>> offset, _TRA bits[7:0]
>>>>> For bridges that translate addresses across the bridge, this is the
>>>>> offset that must be added to the address on the secondary side to
>>>>> obtain the address on the primary side. Non-bridge devices must
>>>>> list 0 for all Address Translation offset bits.
>>>>>
>>>>> It seems UEFI Spec conflicts with ACPI Spec.
>>>>> UEFI Spec: AddressRangeMin = CPU-view address ACPI Spec:
>>>>> AddressRangeMin = PCI-view address
>>>>>
>>>>> I remembered that this topic was discussed long time ago and
>>>>> re-discussed in year 2017.
>>>>> There is no conclusion.
>>>>>
>>>>> I tend to agree to align to ACPI spec.
>>>>> But I am not sure what impact it may cause.
>>>>>
>>>>> Actually this CPU/PCI address topic was discussed long time ago and
>>>>> re-discussed in patch mail:
>>>>> https://lists.01.org/pipermail/edk2-devel/2017-September/014425.htm
>>>>> l No conclusion so the patch was also not checked in.
>>>>>
>>>>> With your proposed patch (maybe refine or address translation logic
>>>>> updates are needed), I think it's a good opportunity for us to
>>>>> review the whole PCI resource allocation logic in
>>>>> PciHostBridge/PciBus /GCD, and propose a consistent/clear solution.
>>>>>
>>>>>
>>>>
>>>> This has indeed been discussed many times, but the UEFI spec is quite
>>>> clear that its definition of the QWORD address space descriptor
>>>> supersedes the one in the ACPI spec.
>>>>
>>>> Given that an offset can be both positive and negative, it is
>>>> reasonable to assume that this field may be interpreted as signed
>>>> and/or may be truncated on 64-bit overflow (which come down to the
>>>> same thing)
>>>>
>>>> I would rather not park this discussion again. The UEFI spec may be
>>>> quirky in this regard, but it is perfectly clear.
>>>
>>> Ard,
>>> In my opinion, to align UEFI Spec to ACPI spec on this sounds like reasonable.
>>> Do you see any real problem if we change the meeting of AddressRangeMin?
>>>
>>
>> I agree it would be best to fix the spec, but only if it applies retroactively, i.e., if
>> it is incorporated as an erratum into the current version.
>
> Copy Mike for opinions.
Ard, Heyi,
If there is no feedback from Mike. I prefer to not violate the UEFI
Spec, though UEFI spec and ACPI spec have differences on this.
What do you think?
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
2018-02-05 8:06 ` Ni, Ruiyu
@ 2018-02-05 9:11 ` Ard Biesheuvel
2018-02-06 1:12 ` Guo Heyi
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2018-02-05 9:11 UTC (permalink / raw)
To: Ni, Ruiyu
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>, Guo Heyi,
Rothman, Michael A, Dong, Eric, Dong Wei, linaro-uefi,
Kinney, Michael D, edk2-devel@lists.01.org, Zeng, Star
On 5 February 2018 at 09:06, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> On 2/3/2018 2:00 PM, Ni, Ruiyu wrote:
>>
>>
>>
>>> -----Original Message-----
>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>>> Sent: Friday, February 2, 2018 4:22 PM
>>> To: Ni, Ruiyu <ruiyu.ni@intel.com>
>>> Cc: Guo Heyi <heyi.guo@linaro.org>,Dong Wei <Dong.Wei@arm.com>; Dong,
>>> Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; linaro-uefi <linaro-
>>> uefi@lists.linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>;
>>> Zeng,
>>> Star <star.zeng@intel.com>
>>> Subject: Re: [edk2] [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for
>>> address translation
>>>
>>> On 2 February 2018 at 00:34, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>>>>> Sent: Friday, February 2, 2018 1:23 AM
>>>>> To: Ni, Ruiyu <ruiyu.ni@intel.com>
>>>>> Cc: Guo Heyi <heyi.guo@linaro.org>,Dong Wei <Dong.Wei@arm.com>;
>>>
>>> Dong,
>>>>>
>>>>> Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; linaro-uefi
>>>>> <linaro- uefi@lists.linaro.org>; Kinney, Michael D
>>>>> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
>>>>> Subject: Re: [edk2] [RFC] MdeModulePkg/PciHostBridgeDxe: Add support
>>>>> for address translation
>>>>>
>>>>> On 1 February 2018 at 05:03, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
>>>>>>
>>>>>> On 1/29/2018 4:50 PM, Guo Heyi wrote:
>>>>>>>
>>>>>>>
>>>>>>> Sorry for the late; I caught cold and didn't work for several days
>>>>>>> last week :( Please see my comments below:
>>>>>>>
>>>>>>>
>>>>>>> On Mon, Jan 22, 2018 at 11:36:14AM +0800, Ni, Ruiyu wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 1/18/2018 9:26 AM, Guo Heyi wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Wed, Jan 17, 2018 at 02:08:06PM +0000, Ard Biesheuvel wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 15 January 2018 at 14:46, Heyi Guo <heyi.guo@linaro.org> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> This is the draft patch for the discussion posted in
>>>>>>>>>>> edk2-devel mailing list:
>>>>>>>>>>> https://lists.01.org/pipermail/edk2-devel/2017-December/019289
>>>>>>>>>>> .ht
>>>>>>>>>>> ml
>>>>>>>>>>>
>>>>>>>>>>> As discussed in the mailing list, we'd like to add support for
>>>>>>>>>>> PCI address translation which is necessary for some non-x86
>>>>>>>>>>> platforms. I also want to minimize the changes to the generic
>>>>>>>>>>> host bridge driver and platform PciHostBridgeLib
>>>>>>>>>>> implemetations, so additional two interfaces are added to
>>>>>>>>>>> expose translation information of the platform. To be generic,
>>>>>>>>>>> I add translation for each type of IO or memory resources.
>>>>>>>>>>>
>>>>>>>>>>> The patch is still a RFC, so I only passed the build for
>>>>>>>>>>> qemu64 and the function has not been tested yet.
>>>>>>>>>>>
>>>>>>>>>>> Please let me know your comments about it.
>>>>>>>>>>>
>>>>>>>>>>> Thanks.
>>>>>>>>>>>
>>>>>>>>>>> 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>
>>>>>>>>>>> ---
>>>>>>>>>>> .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 19 ++++
>>>>>>>>>>> .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 53
>>>>>>>>>>> ++++++++---
>>>>>>>>>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridge.h | 8 +-
>>>>>>>>>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 101
>>>>>>>>>>> ++++++++++++++++++---
>>>>>>>>>>> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 36
>>>
>>> ++++++++
>>>>>>>>>>>
>>>>>>>>>>> 5 files changed, 192 insertions(+), 25 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git
>>>>>>>>>>> a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>>>>>>>>>> b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>>>>>>>>>> index 5b9c887..0c8371a 100644
>>>>>>>>>>> ---
>>>>>>>>>>> a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>>>>>>>>>> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.
>>>>>>>>>>> +++ c
>>>>>>>>>>> @@ -360,6 +360,16 @@ PciHostBridgeGetRootBridges (
>>>>>>>>>>> return &mRootBridge;
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> +PCI_ROOT_BRIDGE_TRANSLATION * EFIAPI
>>>>>>>>>>> +PciHostBridgeGetTranslations (
>>>>>>>>>>> + UINTN *Count
>>>>>>>>>>> + )
>>>>>>>>>>> +{
>>>>>>>>>>> + *Count = 0;
>>>>>>>>>>> + return NULL;
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> /**
>>>>>>>>>>> Free the root bridge instances array returned from
>>>>>>>>>>> PciHostBridgeGetRootBridges().
>>>>>>>>>>> @@ -377,6 +387,15 @@ PciHostBridgeFreeRootBridges (
>>>>>>>>>>> ASSERT (Count == 1);
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> +VOID
>>>>>>>>>>> +EFIAPI
>>>>>>>>>>> +PciHostBridgeFreeTranslations (
>>>>>>>>>>> + PCI_ROOT_BRIDGE_TRANSLATION *Translations,
>>>>>>>>>>> + UINTN Count
>>>>>>>>>>> + )
>>>>>>>>>>> +{
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> /**
>>>>>>>>>>> Inform the platform that the resource conflict happens.
>>>>>>>>>>>
>>>>>>>>>>> diff --git
>>>>>>>>>>> asame/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>>>>>>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>>>>>>>> index 1494848..835e411 100644
>>>>>>>>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>>>>>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>>>>>>>> @@ -360,18 +360,38 @@ InitializePciHostBridge (
>>>>>>>>>>> PCI_HOST_BRIDGE_INSTANCE *HostBridge;
>>>>>>>>>>> PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
>>>>>>>>>>> PCI_ROOT_BRIDGE *RootBridges;
>>>>>>>>>>> + PCI_ROOT_BRIDGE_TRANSLATION *Translations;
>>>>>>>>>>> UINTN RootBridgeCount;
>>>>>>>>>>> + UINTN TranslationCount;
>>>>>>>>>>> UINTN Index;
>>>>>>>>>>> PCI_ROOT_BRIDGE_APERTURE *MemApertures[4];
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Wouldn't it be much better to add a 'translation' member to
>>>>>>>>>> PCI_ROOT_BRIDGE_APERTURE? That way, existing code just default
>>>>>>>>>> to a translation of 0, and all the handling of the separate
>>>>>>>>>> array can be dropped.
>>>>>>>>>>
>>>>>>>>> Actually my first idea was the same, but when I looked at the
>>>>>>>>> implementation of PciHostBridgeLib of Ovmf, I found it a little
>>>>>>>>> tedious to change the existing code in this way. We'll need to
>>>>>>>>> check everywhere PCI_ROOT_BRIDGE_APERTURE or
>>>
>>> PCI_ROOT_BRIDGE is
>>>>>>>>>
>>>>>>>>> used, to make sure the translation field is initialized to be
>>>>>>>>> zero, e.g. line 235~245.
>>>>>>>>>
>>>>>>>>> What I did in this RFC is not so straightforward indeed. So I
>>>>>>>>> can change the code if we all agree to add Translation field
>>>>>>>>> into PCI_ROOT_BRIDGE_APERTURE directly.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Gary (Heyi Guo)
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I also agree to put the translation fields into the
>>>>>>>> PCI_ROOT_BRIDGE_APERTURE.
>>>>>>>>
>>>>>>>>
>>>>>>>> But I think we may have different understandings to the address
>>>>>>>> translation.
>>>>>>>> My understanding to the whole translation:
>>>>>>>> 1. PciHostBridsamesamege.GetProposedResources () returns resource
>>>>>
>>>>> information
>>>>>>>>
>>>>>>>> for a single root bridge. It only carries the address range in
>>>>>>>> PCI
>>>>>>>> view.
>>>>>>>> The address range/resource is reported/submitted by
>>>>>>>> PciHostBridgeLib.
>>>>>>>> Before the change, CPU view equals to PCI view. So
>>>>>>>> PciHostBridgeDxe
>>>>>>>> driver directly adds the resource to GCD.
>>>>>>>> In your change, PciHostBridgeDxe assumes the source is in PCI
>>>>>>>> view
>>>>>>>> and it adds offset to convert to CPU view.
>>>>>>>> CPU-address = PCI-address + offset. <-- Equation #A
>>>>>>>>
>>>>>>>>
>>>>>>>> 2. PciRootBridgeIo.Configuration() returns the resource information
>>>>>>>> for a single root bridge. It carries the address range in CPU
>>>>>>>> view,
>>>>>>>> and the translation offset.
>>>>>>>> However, per UEFI spec, "Address Translation Offset. Offset to
>>>>>>>> apply
>>>>>>>> to the Starting address of a BAR to convert it to a PCI
>>>>>>>> address. "
>>>>>>>> PCI-address = CPU-address + offset. <-- Equation #B
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> If we get Equation #B from the statement in UEFI spec, does it
>>>>>>> also imply Starting address is "Address Range Minimum" and so it
>>>>>>> is CPU view
>>>>>
>>>>> address?
>>>>>>>
>>>>>>>
>>>>>>> If we use Equation #B, can offset be a negative value? If it is
>>>>>>> not, then it will make translation useless, since we cannot change
>>>>>>> CPU address above 4G into PCI address under 4G for legacy
>>>>>>> compatibility.
>>>>>>>
>>>>>>> Equation #B is also not consistent with how OS treats address
>>>>>>> translation; please see the ACPI ASL code which works well for OS:
>>>>>>>
>>>>>>> https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Hi
>>>>>>> sil
>>>>>>> icon/Hi1616/D05AcpiTables/Dsdt/D05Pci.asl#L201
>>>>>>
>>>>>>
>>>>>>
>>>>>> I agree with your statement about the ASL code.
>>>>>> I copied the following wordings from ACPI spec:
>>>>>>
>>>>>> Byte 14 Address range minimum,
>>>>>> _MIN bits[7:0]
>>>>>> For bridges that translate addresses, this is the address space on
>>>>>> the secondary side of the bridge.
>>>>>>
>>>>>> Byte 30 Address Translation
>>>>>> offset, _TRA bits[7:0]
>>>>>> For bridges that translate addresses across the bridge, this is the
>>>>>> offset that must be added to the address on the secondary side to
>>>>>> obtain the address on the primary side. Non-bridge devices must
>>>>>> list 0 for all Address Translation offset bits.
>>>>>>
>>>>>> It seems UEFI Spec conflicts with ACPI Spec.
>>>>>> UEFI Spec: AddressRangeMin = CPU-view address ACPI Spec:
>>>>>> AddressRangeMin = PCI-view address
>>>>>>
>>>>>> I remembered that this topic was discussed long time ago and
>>>>>> re-discussed in year 2017.
>>>>>> There is no conclusion.
>>>>>>
>>>>>> I tend to agree to align to ACPI spec.
>>>>>> But I am not sure what impact it may cause.
>>>>>>
>>>>>> Actually this CPU/PCI address topic was discussed long time ago and
>>>>>> re-discussed in patch mail:
>>>>>> https://lists.01.org/pipermail/edk2-devel/2017-September/014425.htm
>>>>>> l No conclusion so the patch was also not checked in.
>>>>>>
>>>>>> With your proposed patch (maybe refine or address translation logic
>>>>>> updates are needed), I think it's a good opportunity for us to
>>>>>> review the whole PCI resource allocation logic in
>>>>>> PciHostBridge/PciBus /GCD, and propose a consistent/clear solution.
>>>>>>
>>>>>>
>>>>>
>>>>> This has indeed been discussed many times, but the UEFI spec is quite
>>>>> clear that its definition of the QWORD address space descriptor
>>>>> supersedes the one in the ACPI spec.
>>>>>
>>>>> Given that an offset can be both positive and negative, it is
>>>>> reasonable to assume that this field may be interpreted as signed
>>>>> and/or may be truncated on 64-bit overflow (which come down to the
>>>>> same thing)
>>>>>
>>>>> I would rather not park this discussion again. The UEFI spec may be
>>>>> quirky in this regard, but it is perfectly clear.
>>>>
>>>>
>>>> Ard,
>>>> In my opinion, to align UEFI Spec to ACPI spec on this sounds like
>>>> reasonable.
>>>> Do you see any real problem if we change the meeting of AddressRangeMin?
>>>>
>>>
>>> I agree it would be best to fix the spec, but only if it applies
>>> retroactively, i.e., if
>>> it is incorporated as an erratum into the current version.
>>
>>
>> Copy Mike for opinions.
>
>
> Ard, Heyi,
> If there is no feedback from Mike. I prefer to not violate the UEFI
> Spec, though UEFI spec and ACPI spec have differences on this.
>
> What do you think?
>
Yes, I agree. So PciIo::GetBarAttributes() should follow the UEFI
definition for translation offset. But how we represent internally in
PciHostBridgeLib does not necessarily have to be the same. I think we
all agree that having an offset that is added to the PCI address to
obtain the CPU address makes much more sense, and so we can keep that
definition in PciHostBridgeLib (and hopefully, we will fix
GetBarAttributes() at some point)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
2018-02-05 9:11 ` Ard Biesheuvel
@ 2018-02-06 1:12 ` Guo Heyi
0 siblings, 0 replies; 12+ messages in thread
From: Guo Heyi @ 2018-02-06 1:12 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Ni, Ruiyu,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,Guo Heyi,
Rothman, Michael A, Dong, Eric, Dong Wei, linaro-uefi,
Kinney, Michael D, edk2-devel@lists.01.org, Zeng, Star
On Mon, Feb 05, 2018 at 10:11:30AM +0100, Ard Biesheuvel wrote:
> On 5 February 2018 at 09:06, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> > On 2/3/2018 2:00 PM, Ni, Ruiyu wrote:
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >>> Sent: Friday, February 2, 2018 4:22 PM
> >>> To: Ni, Ruiyu <ruiyu.ni@intel.com>
> >>> Cc: Guo Heyi <heyi.guo@linaro.org>,Dong Wei <Dong.Wei@arm.com>; Dong,
> >>> Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; linaro-uefi <linaro-
> >>> uefi@lists.linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>;
> >>> Zeng,
> >>> Star <star.zeng@intel.com>
> >>> Subject: Re: [edk2] [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for
> >>> address translation
> >>>
> >>> On 2 February 2018 at 00:34, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >>>>> Sent: Friday, February 2, 2018 1:23 AM
> >>>>> To: Ni, Ruiyu <ruiyu.ni@intel.com>
> >>>>> Cc: Guo Heyi <heyi.guo@linaro.org>,Dong Wei <Dong.Wei@arm.com>;
> >>>
> >>> Dong,
> >>>>>
> >>>>> Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; linaro-uefi
> >>>>> <linaro- uefi@lists.linaro.org>; Kinney, Michael D
> >>>>> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
> >>>>> Subject: Re: [edk2] [RFC] MdeModulePkg/PciHostBridgeDxe: Add support
> >>>>> for address translation
> >>>>>
> >>>>> On 1 February 2018 at 05:03, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> >>>>>>
> >>>>>> On 1/29/2018 4:50 PM, Guo Heyi wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> Sorry for the late; I caught cold and didn't work for several days
> >>>>>>> last week :( Please see my comments below:
> >>>>>>>
> >>>>>>>
> >>>>>>> On Mon, Jan 22, 2018 at 11:36:14AM +0800, Ni, Ruiyu wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 1/18/2018 9:26 AM, Guo Heyi wrote:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On Wed, Jan 17, 2018 at 02:08:06PM +0000, Ard Biesheuvel wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> On 15 January 2018 at 14:46, Heyi Guo <heyi.guo@linaro.org> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> This is the draft patch for the discussion posted in
> >>>>>>>>>>> edk2-devel mailing list:
> >>>>>>>>>>> https://lists.01.org/pipermail/edk2-devel/2017-December/019289
> >>>>>>>>>>> .ht
> >>>>>>>>>>> ml
> >>>>>>>>>>>
> >>>>>>>>>>> As discussed in the mailing list, we'd like to add support for
> >>>>>>>>>>> PCI address translation which is necessary for some non-x86
> >>>>>>>>>>> platforms. I also want to minimize the changes to the generic
> >>>>>>>>>>> host bridge driver and platform PciHostBridgeLib
> >>>>>>>>>>> implemetations, so additional two interfaces are added to
> >>>>>>>>>>> expose translation information of the platform. To be generic,
> >>>>>>>>>>> I add translation for each type of IO or memory resources.
> >>>>>>>>>>>
> >>>>>>>>>>> The patch is still a RFC, so I only passed the build for
> >>>>>>>>>>> qemu64 and the function has not been tested yet.
> >>>>>>>>>>>
> >>>>>>>>>>> Please let me know your comments about it.
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks.
> >>>>>>>>>>>
> >>>>>>>>>>> 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>
> >>>>>>>>>>> ---
> >>>>>>>>>>> .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 19 ++++
> >>>>>>>>>>> .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 53
> >>>>>>>>>>> ++++++++---
> >>>>>>>>>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridge.h | 8 +-
> >>>>>>>>>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 101
> >>>>>>>>>>> ++++++++++++++++++---
> >>>>>>>>>>> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 36
> >>>
> >>> ++++++++
> >>>>>>>>>>>
> >>>>>>>>>>> 5 files changed, 192 insertions(+), 25 deletions(-)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git
> >>>>>>>>>>> a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> >>>>>>>>>>> b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> >>>>>>>>>>> index 5b9c887..0c8371a 100644
> >>>>>>>>>>> ---
> >>>>>>>>>>> a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> >>>>>>>>>>> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.
> >>>>>>>>>>> +++ c
> >>>>>>>>>>> @@ -360,6 +360,16 @@ PciHostBridgeGetRootBridges (
> >>>>>>>>>>> return &mRootBridge;
> >>>>>>>>>>> }
> >>>>>>>>>>>
> >>>>>>>>>>> +PCI_ROOT_BRIDGE_TRANSLATION * EFIAPI
> >>>>>>>>>>> +PciHostBridgeGetTranslations (
> >>>>>>>>>>> + UINTN *Count
> >>>>>>>>>>> + )
> >>>>>>>>>>> +{
> >>>>>>>>>>> + *Count = 0;
> >>>>>>>>>>> + return NULL;
> >>>>>>>>>>> +}
> >>>>>>>>>>> +
> >>>>>>>>>>> /**
> >>>>>>>>>>> Free the root bridge instances array returned from
> >>>>>>>>>>> PciHostBridgeGetRootBridges().
> >>>>>>>>>>> @@ -377,6 +387,15 @@ PciHostBridgeFreeRootBridges (
> >>>>>>>>>>> ASSERT (Count == 1);
> >>>>>>>>>>> }
> >>>>>>>>>>>
> >>>>>>>>>>> +VOID
> >>>>>>>>>>> +EFIAPI
> >>>>>>>>>>> +PciHostBridgeFreeTranslations (
> >>>>>>>>>>> + PCI_ROOT_BRIDGE_TRANSLATION *Translations,
> >>>>>>>>>>> + UINTN Count
> >>>>>>>>>>> + )
> >>>>>>>>>>> +{
> >>>>>>>>>>> +}
> >>>>>>>>>>> +
> >>>>>>>>>>> /**
> >>>>>>>>>>> Inform the platform that the resource conflict happens.
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git
> >>>>>>>>>>> asame/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>>>>>>>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>>>>>>>>> index 1494848..835e411 100644
> >>>>>>>>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>>>>>>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>>>>>>>>> @@ -360,18 +360,38 @@ InitializePciHostBridge (
> >>>>>>>>>>> PCI_HOST_BRIDGE_INSTANCE *HostBridge;
> >>>>>>>>>>> PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> >>>>>>>>>>> PCI_ROOT_BRIDGE *RootBridges;
> >>>>>>>>>>> + PCI_ROOT_BRIDGE_TRANSLATION *Translations;
> >>>>>>>>>>> UINTN RootBridgeCount;
> >>>>>>>>>>> + UINTN TranslationCount;
> >>>>>>>>>>> UINTN Index;
> >>>>>>>>>>> PCI_ROOT_BRIDGE_APERTURE *MemApertures[4];
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Wouldn't it be much better to add a 'translation' member to
> >>>>>>>>>> PCI_ROOT_BRIDGE_APERTURE? That way, existing code just default
> >>>>>>>>>> to a translation of 0, and all the handling of the separate
> >>>>>>>>>> array can be dropped.
> >>>>>>>>>>
> >>>>>>>>> Actually my first idea was the same, but when I looked at the
> >>>>>>>>> implementation of PciHostBridgeLib of Ovmf, I found it a little
> >>>>>>>>> tedious to change the existing code in this way. We'll need to
> >>>>>>>>> check everywhere PCI_ROOT_BRIDGE_APERTURE or
> >>>
> >>> PCI_ROOT_BRIDGE is
> >>>>>>>>>
> >>>>>>>>> used, to make sure the translation field is initialized to be
> >>>>>>>>> zero, e.g. line 235~245.
> >>>>>>>>>
> >>>>>>>>> What I did in this RFC is not so straightforward indeed. So I
> >>>>>>>>> can change the code if we all agree to add Translation field
> >>>>>>>>> into PCI_ROOT_BRIDGE_APERTURE directly.
> >>>>>>>>>
> >>>>>>>>> Thanks,
> >>>>>>>>>
> >>>>>>>>> Gary (Heyi Guo)
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> I also agree to put the translation fields into the
> >>>>>>>> PCI_ROOT_BRIDGE_APERTURE.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> But I think we may have different understandings to the address
> >>>>>>>> translation.
> >>>>>>>> My understanding to the whole translation:
> >>>>>>>> 1. PciHostBridsamesamege.GetProposedResources () returns resource
> >>>>>
> >>>>> information
> >>>>>>>>
> >>>>>>>> for a single root bridge. It only carries the address range in
> >>>>>>>> PCI
> >>>>>>>> view.
> >>>>>>>> The address range/resource is reported/submitted by
> >>>>>>>> PciHostBridgeLib.
> >>>>>>>> Before the change, CPU view equals to PCI view. So
> >>>>>>>> PciHostBridgeDxe
> >>>>>>>> driver directly adds the resource to GCD.
> >>>>>>>> In your change, PciHostBridgeDxe assumes the source is in PCI
> >>>>>>>> view
> >>>>>>>> and it adds offset to convert to CPU view.
> >>>>>>>> CPU-address = PCI-address + offset. <-- Equation #A
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> 2. PciRootBridgeIo.Configuration() returns the resource information
> >>>>>>>> for a single root bridge. It carries the address range in CPU
> >>>>>>>> view,
> >>>>>>>> and the translation offset.
> >>>>>>>> However, per UEFI spec, "Address Translation Offset. Offset to
> >>>>>>>> apply
> >>>>>>>> to the Starting address of a BAR to convert it to a PCI
> >>>>>>>> address. "
> >>>>>>>> PCI-address = CPU-address + offset. <-- Equation #B
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> If we get Equation #B from the statement in UEFI spec, does it
> >>>>>>> also imply Starting address is "Address Range Minimum" and so it
> >>>>>>> is CPU view
> >>>>>
> >>>>> address?
> >>>>>>>
> >>>>>>>
> >>>>>>> If we use Equation #B, can offset be a negative value? If it is
> >>>>>>> not, then it will make translation useless, since we cannot change
> >>>>>>> CPU address above 4G into PCI address under 4G for legacy
> >>>>>>> compatibility.
> >>>>>>>
> >>>>>>> Equation #B is also not consistent with how OS treats address
> >>>>>>> translation; please see the ACPI ASL code which works well for OS:
> >>>>>>>
> >>>>>>> https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Hi
> >>>>>>> sil
> >>>>>>> icon/Hi1616/D05AcpiTables/Dsdt/D05Pci.asl#L201
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> I agree with your statement about the ASL code.
> >>>>>> I copied the following wordings from ACPI spec:
> >>>>>>
> >>>>>> Byte 14 Address range minimum,
> >>>>>> _MIN bits[7:0]
> >>>>>> For bridges that translate addresses, this is the address space on
> >>>>>> the secondary side of the bridge.
> >>>>>>
> >>>>>> Byte 30 Address Translation
> >>>>>> offset, _TRA bits[7:0]
> >>>>>> For bridges that translate addresses across the bridge, this is the
> >>>>>> offset that must be added to the address on the secondary side to
> >>>>>> obtain the address on the primary side. Non-bridge devices must
> >>>>>> list 0 for all Address Translation offset bits.
> >>>>>>
> >>>>>> It seems UEFI Spec conflicts with ACPI Spec.
> >>>>>> UEFI Spec: AddressRangeMin = CPU-view address ACPI Spec:
> >>>>>> AddressRangeMin = PCI-view address
> >>>>>>
> >>>>>> I remembered that this topic was discussed long time ago and
> >>>>>> re-discussed in year 2017.
> >>>>>> There is no conclusion.
> >>>>>>
> >>>>>> I tend to agree to align to ACPI spec.
> >>>>>> But I am not sure what impact it may cause.
> >>>>>>
> >>>>>> Actually this CPU/PCI address topic was discussed long time ago and
> >>>>>> re-discussed in patch mail:
> >>>>>> https://lists.01.org/pipermail/edk2-devel/2017-September/014425.htm
> >>>>>> l No conclusion so the patch was also not checked in.
> >>>>>>
> >>>>>> With your proposed patch (maybe refine or address translation logic
> >>>>>> updates are needed), I think it's a good opportunity for us to
> >>>>>> review the whole PCI resource allocation logic in
> >>>>>> PciHostBridge/PciBus /GCD, and propose a consistent/clear solution.
> >>>>>>
> >>>>>>
> >>>>>
> >>>>> This has indeed been discussed many times, but the UEFI spec is quite
> >>>>> clear that its definition of the QWORD address space descriptor
> >>>>> supersedes the one in the ACPI spec.
> >>>>>
> >>>>> Given that an offset can be both positive and negative, it is
> >>>>> reasonable to assume that this field may be interpreted as signed
> >>>>> and/or may be truncated on 64-bit overflow (which come down to the
> >>>>> same thing)
> >>>>>
> >>>>> I would rather not park this discussion again. The UEFI spec may be
> >>>>> quirky in this regard, but it is perfectly clear.
> >>>>
> >>>>
> >>>> Ard,
> >>>> In my opinion, to align UEFI Spec to ACPI spec on this sounds like
> >>>> reasonable.
> >>>> Do you see any real problem if we change the meeting of AddressRangeMin?
> >>>>
> >>>
> >>> I agree it would be best to fix the spec, but only if it applies
> >>> retroactively, i.e., if
> >>> it is incorporated as an erratum into the current version.
> >>
> >>
> >> Copy Mike for opinions.
> >
> >
> > Ard, Heyi,
> > If there is no feedback from Mike. I prefer to not violate the UEFI
> > Spec, though UEFI spec and ACPI spec have differences on this.
> >
> > What do you think?
> >
>
> Yes, I agree. So PciIo::GetBarAttributes() should follow the UEFI
> definition for translation offset. But how we represent internally in
> PciHostBridgeLib does not necessarily have to be the same. I think we
> all agree that having an offset that is added to the PCI address to
> obtain the CPU address makes much more sense, and so we can keep that
> definition in PciHostBridgeLib (and hopefully, we will fix
> GetBarAttributes() at some point)
Happy to see we finally reach the conclusion. I will send out v2 RFC.
Thanks and regards,
Gary (Heyi Guo)
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-02-06 1:06 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-15 14:46 [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for address translation Heyi Guo
2018-01-17 14:08 ` Ard Biesheuvel
2018-01-18 1:26 ` Guo Heyi
2018-01-22 3:36 ` Ni, Ruiyu
2018-01-29 8:50 ` Guo Heyi
[not found] ` <685b27a9-e620-e7e7-e0fb-8cebe16e7b1a@Intel.com>
2018-02-01 17:22 ` Ard Biesheuvel
2018-02-02 0:34 ` Ni, Ruiyu
2018-02-02 8:22 ` Ard Biesheuvel
2018-02-03 6:00 ` Ni, Ruiyu
2018-02-05 8:06 ` Ni, Ruiyu
2018-02-05 9:11 ` Ard Biesheuvel
2018-02-06 1:12 ` Guo Heyi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox