Alex,
On 02/18/15 22:08, Laszlo Ersek wrote:
> Unlike the one in PcAtChipsetPkg, our PciHostBridgeDxe module must handle
> address space translation. IO and MMIO addresses expressed in the
> respective apertures may be mapped to different bases in CPU address
> space, and in case of IO, they actually are.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <[email protected]>
> ---
> ArmPlatformPkg/ArmVirtualizationPkg/PciHostBridgeDxe/PciHostBridge.h | 2
> ++
> ArmPlatformPkg/ArmVirtualizationPkg/PciHostBridgeDxe/PciRootBridgeIo.c | 17
> +++++++++++++----
> 2 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git
> a/ArmPlatformPkg/ArmVirtualizationPkg/PciHostBridgeDxe/PciHostBridge.h
> b/ArmPlatformPkg/ArmVirtualizationPkg/PciHostBridgeDxe/PciHostBridge.h
> index 19952a2..2b1ac58 100644
> --- a/ArmPlatformPkg/ArmVirtualizationPkg/PciHostBridgeDxe/PciHostBridge.h
> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/PciHostBridgeDxe/PciHostBridge.h
> @@ -458,6 +458,8 @@ typedef struct {
> UINT64 BusLimit;
> UINT64 MemLimit;
> UINT64 IoLimit;
> + UINT64 MemTranslation;
> + UINT64 IoTranslation;
>
> EFI_DEVICE_PATH_PROTOCOL *DevicePath;
> EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL Io;
> diff --git
> a/ArmPlatformPkg/ArmVirtualizationPkg/PciHostBridgeDxe/PciRootBridgeIo.c
> b/ArmPlatformPkg/ArmVirtualizationPkg/PciHostBridgeDxe/PciRootBridgeIo.c
> index 7c35e85..4c5decf 100644
> --- a/ArmPlatformPkg/ArmVirtualizationPkg/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -640,11 +640,12 @@ RootBridgeConstructor (
> PrivateData = DRIVER_INSTANCE_FROM_PCI_ROOT_BRIDGE_IO_THIS (Protocol);
>
> //
> - // The host to pci bridge, the host memory and io addresses are
> - // direct mapped to pci addresses, so no need translate, set bases to 0.
> + // The host to pci bridge.
> //
> - PrivateData->MemBase = ResAperture->MemBase;
> - PrivateData->IoBase = ResAperture->IoBase;
> + PrivateData->MemBase = ResAperture->MemBase;
> + PrivateData->MemTranslation = ResAperture->MemTranslation;
> + PrivateData->IoBase = ResAperture->IoBase;
> + PrivateData->IoTranslation = ResAperture->IoTranslation;
>
> //
> // The host bridge only supports 32bit addressing for memory
> @@ -886,6 +887,7 @@ RootBridgeIoMemRW (
> )
> {
> EFI_STATUS Status;
> + PCI_ROOT_BRIDGE_INSTANCE *PrivateData;
> UINT8 InStride;
> UINT8 OutStride;
> EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_WIDTH OperationWidth;
> @@ -896,6 +898,9 @@ RootBridgeIoMemRW (
> return Status;
> }
>
> + PrivateData = DRIVER_INSTANCE_FROM_PCI_ROOT_BRIDGE_IO_THIS (This);
> + Address += PrivateData->MemTranslation;
> +
> InStride = mInStride[Width];
> OutStride = mOutStride[Width];
> OperationWidth = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_WIDTH) (Width & 0x03);
> @@ -978,6 +983,7 @@ RootBridgeIoIoRW (
> )
> {
> EFI_STATUS Status;
> + PCI_ROOT_BRIDGE_INSTANCE *PrivateData;
> UINT8 InStride;
> UINT8 OutStride;
> EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_WIDTH OperationWidth;
> @@ -988,6 +994,9 @@ RootBridgeIoIoRW (
> return Status;
> }
>
> + PrivateData = DRIVER_INSTANCE_FROM_PCI_ROOT_BRIDGE_IO_THIS (This);
> + Address += PrivateData->IoTranslation;
> +
> InStride = mInStride[Width];
> OutStride = mOutStride[Width];
> OperationWidth = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_WIDTH) (Width & 0x03);
>
I'm having second thoughts if the translation in this patch covers
everything. I'm wondering if the translation should also be done for the
gDS->XxxxMemorySpace() calls as well -- IOW if the GCD memory operations
should also get translated (== cpu absolute) addresses. This is
especially important for the KVM hack in patch v2 16/28, see
gDS->SetMemorySpaceAttributes().
Given that the MMIO translation offset that QEMU currently exports is
zero, there's no way to tell *by way of testing* if this patch covers
everything related. Forgetting to add zero is identical to adding zero.
So, before I try to reason more about code that's potentially not
covered by this patch, let me ask this -- is it conceivable that in the
following:
//
// child base cpu base
// type address address size
// --------- -------------- -------------- --------------
ranges = <0x1000000 0x0 0x0 0x0 0x3eff0000 0x0 0x10000
0x2000000 0x0 0x10000000 0x0 0x10000000 0x0 0x2eff0000>;
the "child base address" would *ever* be different from the "cpu base
address" for the PCI MMIO32 type range (0x2000000)?
Because, if that's inconceivable in practice, then I'd much prefer
killing the MMIO translation in this patchset -- including the PCD
(platform config database) entry that carries the translation offset --
over investigating (without a nonzero test case!) if everything has been
covered. I'd check (childbase == cpubase) nonetheless, but I wouldn't
try to support the nonzero offset case, just reject bringing up the host
bridge.
If the nonzero offset case is valid and should be handled by the
firmware, then can you please give me a small qemu patch that sets up
stuff like that? I have some ideas as to what additions would be
necessary for this patchset, but without a test, it's all just
speculation.
Thanks,
Laszlo
------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=190641631&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
