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

[Guo Heyi <heyi.guo@linaro.org>]

On Thu, Dec 21, 2017 at 08:32:37AM +0000, Ard Biesheuvel wrote:
> On 21 December 2017 at 08:27, Guo Heyi <heyi.guo@linaro.org> wrote:
> > On Wed, Dec 20, 2017 at 03:26:45PM +0000, Ard Biesheuvel wrote:
> >> 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?
> >
> > IO translation on one of our platforms is like below:
> >
> > PCI IO space        CPU memory space
> > 0x0000 .. 0xffff -> 0xafff0000 .. 0xafffffff
> > (The sizes are always 0x10000 so I will omit the limit for others)
> > 0x0000 .. 0xffff -> 0x8abff0000
> > 0x0000 .. 0xffff -> 0x8b7ff0000
> > ......
> >
> > The translated addresses may be beyond 32bit address, will this
> > violate IO space limitation? From EDK2 code, I didn't see such
> > limitation for IO space.
> >
> 
> The MMIO address will not be used for I/O port addressing by the CPU.
> The MMIO to IO translation is an implementation detail of your CpuIo2
> protocol implementation.
> 
> So there will be two stacked translations, one for PCI I/O to CPU I/O,
> and one for CPU I/O to CPU MMIO. The latter is transparent to the PCI
> host bridge driver.

Yes this should work.

Hi Star, Eric and Ruiyu,

Any comments on this RFC?

Thanks,

Gary

> 
> > In my opinion, if we still treat the translated address as IO space in
> > PCI host bridge driver while IO space is really translated to memory
> > space, then we couldn't utilize the existing GCD manipulation code in
> > PCI host bridge, including allocating memory space to avoid space
> > colliding (we can't avoid colliding with other drivers which may also
> > use the same memory address), setting MMU page tables. Anyway, this is
> > not a blocking issue.
> >
> > Thanks and regards,
> >
> > Gary (Heyi Guo)
> >
> >>
> >> 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
> >> >> >