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

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

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?

> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 


-- 
Thanks,
Ray