From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.115; helo=mga14.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4627C2215BDA5 for ; Mon, 5 Feb 2018 00:01:21 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Feb 2018 00:07:02 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,464,1511856000"; d="scan'208";a="28824440" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.19]) ([10.239.9.19]) by orsmga001.jf.intel.com with ESMTP; 05 Feb 2018 00:06:59 -0800 To: "Ard Biesheuvel ;Guo Heyi" , "Rothman, Michael A" Cc: "Dong, Eric" , Guo Heyi , Dong Wei , linaro-uefi , "Kinney, Michael D" , "edk2-devel@lists.01.org" , "Zeng, Star" References: <1516027600-32172-1-git-send-email-heyi.guo@linaro.org> <20180118012639.GA113567@SZX1000114654> <20180129085020.GA127987@SZX1000114654> <685b27a9-e620-e7e7-e0fb-8cebe16e7b1a@Intel.com> <734D49CCEBEEF84792F5B80ED585239D5BB73CBF@SHSMSX104.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5BB845A2@SHSMSX104.ccr.corp.intel.com> From: "Ni, Ruiyu" Message-ID: <7fab5279-9f46-93c2-6a90-3b6d957b2f27@Intel.com> Date: Mon, 5 Feb 2018 16:06:58 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5BB845A2@SHSMSX104.ccr.corp.intel.com> Subject: Re: [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for address translation X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 05 Feb 2018 08:01:21 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> Cc: Guo Heyi ,Dong Wei ; Dong, >> Eric ; edk2-devel@lists.01.org; linaro-uefi > uefi@lists.linaro.org>; Kinney, Michael D ; Zeng, >> Star >> Subject: Re: [edk2] [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for >> address translation >> >> On 2 February 2018 at 00:34, Ni, Ruiyu wrote: >>> >>> >>>> -----Original Message----- >>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >>>> Sent: Friday, February 2, 2018 1:23 AM >>>> To: Ni, Ruiyu >>>> Cc: Guo Heyi ,Dong Wei ; >> Dong, >>>> Eric ; edk2-devel@lists.01.org; linaro-uefi >>>> ; Kinney, Michael D >>>> ; Zeng, Star >>>> Subject: Re: [edk2] [RFC] MdeModulePkg/PciHostBridgeDxe: Add support >>>> for address translation >>>> >>>> On 1 February 2018 at 05:03, Ni, Ruiyu 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 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 >>>>>>>>>> Cc: Ruiyu Ni >>>>>>>>>> Cc: Ard Biesheuvel >>>>>>>>>> Cc: Star Zeng >>>>>>>>>> Cc: Eric Dong >>>>>>>>>> --- >>>>>>>>>> .../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