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

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 12/20/2017 11:26 PM, 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.
> 

I fully agree with the PciRootBridgeLib interface change to support
MMIO offset. I guess that you will propose to add a
AddrTranslationOffset to PCI_ROOT_BRIDGE structure.

But I don't quite understand the needs of IO offset support.

And further more, there was a discussion in the mailing list
regarding how to utilize the Offset. There was 2 points
about the meaning of Offset and we cannot agree with each
other. Spec is not quite clear.
1. Offset = Host - Device
2. Offset = Device - Host


> 
> 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
>>>>


-- 
Thanks,
Ray