Re: [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for address translation

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

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