Re: [RFC] MdeModulePkg/PciHostBridge: Add address translation support

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 20 December 2017 at 15:17, gary guo <heyi.guo@linaro.org> wrote:
> On Wed, Dec 20, 2017 at 09:13:58AM +0000, Ard Biesheuvel wrote:
>> Hi Heyi,
>>
>> On 20 December 2017 at 08:21, Heyi Guo <heyi.guo@linaro.org> wrote:
>> > PCIe on some ARM platforms requires address translation, not only for
>> > legacy IO access, but also for 32bit memory BAR access as well. There
>> > will be "Address Translation Unit" or something similar in PCI host
>> > bridges to translation CPU address to PCI address and vice versa. So
>> > we think it may be useful to add address translation support to the
>> > generic PCI host bridge driver.
>> >
>>
>> I agree. While unusual on a PC, it is quite common on other
>> architectures to have more complex non 1:1 topologies, which currently
>> require a forked PciHostBridgeDxe driver with local changes applied.
>>
>> > This RFC only contains one minor change to the definition of
>> > PciHostBridgeLib, and there certainly will be a lot of other changes
>> > to make it work, including:
>> >
>> > 1. Use CPU address for GCD space add and allocate operations, instead
>> > of PCI address; also IO space will be changed to memory space if
>> > translation exists.
>> >
>>
>> For I/O space, the translation should simply be applied to the I/O
>> range. I don't think it makes sense to use memory space here, given
>> that it is specific to architectures that lack native port I/O.
>>
>
> I made an assumption here that platforms supporting real port IO space
> do not need address translation, like IA32 and X64, and port IO
> translation implies the platform does not support real port IO space.
>

This may be a reasonable assumption. But I still think it is better
not to encode any assumptions in the first place.

> Indeed the assumption is not so "generic", so I'll agree if you
> recommend to support IO to IO translation as well. But I still hope to
> have IO to memory translation support in PCI host bridge driver,
> rather than in CPU IO protocol, since the faked IO space might only be
> used for PCI host bridge and we may have overlapping IO ranges for
> each host bridge, which is compatible with PCIe specification and PCIe
> ACPI description.
>

That is fine. Under UEFI, these will translate to non-overlapping I/O
spaces in the CPU's view. Under the OS, this could be totally
different.


For example,

RC0 IO 0x0000 .. 0xffff -> CPU 0x00000 .. 0x0ffff
RC1 IO 0x0000 .. 0xffff -> CPU 0x10000 .. 0x1ffff

This is very similar to how MMIO translation works, and makes I/O
devices behind the host bridges uniquely addressable for drivers.

For our understanding, could you share the host bridge configuration
that you are targetting?

Thanks,
Ard.


> How about adding a flag to indicate whether port IO is translated to
> real port IO space or system memory space?
>
> Thanks and regards,
>
> Heyi
>
>> > 2. RootBridgeIoMemRead/Write, RootBridgeIoRead/Write need to get
>> > translation of the corresponding aperture, add the translation to the
>> > input address, and then call CpuIo2 protocol; IO access will also be
>> > converted to memory access if IO translation exists.
>> >
>>
>> Again, why is this necessary? A host bridge that implements a non 1:1
>> translation for port I/O ranges may be part of a system that has
>> native port I/O, and so the translation should be based on that.
>>
>> > 3. RootBridgeIoConfiguration needs to fill AddrTranslationOffset in
>> > the discriptor.
>> >
>>
>> Indeed. Note that this has been a source of much confusion lately,
>> should we should discuss this carefully before spending time on an
>> implementation.
>>
>> > If it makes sense, then I'll continue to prepare the formal patch.
>> >
>> > Any comments?
>> >
>> > Thanks,
>> >
>> > Gary (Heyi Guo)
>> >
>> > Cc: Star Zeng <star.zeng@intel.com>
>> > Cc: Eric Dong <eric.dong@intel.com>
>> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > Cc: Jason Zhang <jason.zhang@linaro.org>
>> >
>> > ---
>> >  MdeModulePkg/Include/Library/PciHostBridgeLib.h | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
>> > index d42e9ec..b9e8c0f 100644
>> > --- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
>> > +++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
>> > @@ -22,6 +22,7 @@
>> >  typedef struct {
>> >    UINT64 Base;
>> >    UINT64 Limit;
>> > +  UINT64 Translation;
>> >  } PCI_ROOT_BRIDGE_APERTURE;
>> >
>> >  typedef struct {
>> > --
>> > 2.7.4
>> >