On 2 February 2018 at 00:34, Ni, Ruiyu <ruiyu...@intel.com> wrote: > > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] >> Sent: Friday, February 2, 2018 1:23 AM >> To: Ni, Ruiyu <ruiyu...@intel.com> >> Cc: Guo Heyi <heyi....@linaro.org>,Dong Wei <dong....@arm.com>; Dong, >> Eric <eric.d...@intel.com>; edk2-devel@lists.01.org; linaro-uefi <linaro- >> u...@lists.linaro.org>; Kinney, Michael D <michael.d.kin...@intel.com>; Zeng, >> Star <star.z...@intel.com> >> Subject: Re: [edk2] [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for >> address translation >> >> On 1 February 2018 at 05:03, Ni, Ruiyu <ruiyu...@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....@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....@linaro.org> >> >>>>>> Cc: Ruiyu Ni <ruiyu...@intel.com> >> >>>>>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> >> >>>>>> Cc: Star Zeng <star.z...@intel.com> >> >>>>>> Cc: Eric Dong <eric.d...@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? >
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. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
