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

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

On Mon, Feb 05, 2018 at 10:11:30AM +0100, Ard Biesheuvel wrote:
> On 5 February 2018 at 09:06, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> > On 2/3/2018 2:00 PM, Ni, Ruiyu wrote:
> >>
> >>
> >>
> >>> -----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.
> >
> >
> > Ard, Heyi,
> > If there is no feedback from Mike. I prefer to not violate the UEFI
> > Spec, though UEFI spec and ACPI spec have differences on this.
> >
> > What do you think?
> >
> 
> Yes, I agree. So PciIo::GetBarAttributes() should follow the UEFI
> definition for translation offset. But how we represent internally in
> PciHostBridgeLib does not necessarily have to be the same. I think we
> all agree that having an offset that is added to the PCI address to
> obtain the CPU address makes much more sense, and so we can keep that
> definition in PciHostBridgeLib (and hopefully, we will fix
> GetBarAttributes() at some point)

Happy to see we finally reach the conclusion. I will send out v2 RFC.

Thanks and regards,

Gary (Heyi Guo)