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 4:22 PM
> 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 2 February 2018 at 00:34, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> >
> >
> >> -----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/Hi
> >> >> sil
> >> >> 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.htm
> >> > l 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?
> >
> 
> I agree it would be best to fix the spec, but only if it applies retroactively, i.e., if
> it is incorporated as an erratum into the current version.

Copy Mike for opinions.