From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::243; helo=mail-it0-x243.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x243.google.com (mail-it0-x243.google.com [IPv6:2607:f8b0:4001:c0b::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 35A0E22393624 for ; Fri, 2 Feb 2018 00:16:35 -0800 (PST) Received: by mail-it0-x243.google.com with SMTP id j21-v6so359913ita.1 for ; Fri, 02 Feb 2018 00:22:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=fXomu0weHnhPHRwWK3gCzeVyeLgqQIqdnvR4sQoHsuw=; b=C4b/ZdobgDAu2KikDV165TrHGeMSGbCBt6LhJG+9MLKe3OVumDjkBZhyMjeT++ripo NnVqSHa6TFJpgOQHScugv5yc8wuuCyw7nj5IEr1QAy57mJhCPbB/VdLd0q9sAFz7dpwS I23iGdFtBmdFhC1gS2fFHYqDXlcJkhX3lS93Y= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=fXomu0weHnhPHRwWK3gCzeVyeLgqQIqdnvR4sQoHsuw=; b=D4ypSTpkoBjFnM7ynDPNrHZ+s+IDzjMLtW2p69FZkPxgY399I08cy575pNRvdzNR52 hMGcEBsoAmaMB8lAYxm1LKBuvUqkGrOWTE1htppk73gykK8aIdEtaol0NDass5m/Qoj/ fCn+SgJvH2JN4qZQem8XXwxrrRANthBaxmpfBd7P7oz8KAbnpny/esP3t6fNV8pf7IK9 2pB9AvAiLTF5QJn9eLR2T0TPNKE6mDm94LLDUWW0l6tpSj7vjbTdWcI0dtfJDROR7CDx zZwlY22I+wEQxj/+acge+eKFqASVaBAy4RpNbFRVOi0XuMqGIBrwxJsI5pAlVKJ5Kx/0 xkWw== X-Gm-Message-State: AKwxyte3nzVNF/VE/P81Cxnd4xeZT6uf97U/zTqlFyiSaJgfI+YyyBHW f2ZdNe/Rasoh6VnPDgcehl8sTQN403pviA/5WZg6IQ== X-Google-Smtp-Source: AH8x226wkzc1vlACZPaG+hMta7kprndN/zWUMm2+XuTYSofMyhvSp6pC1RkjC7aPLqKDBH/z1mqu51HEipYNowQcag4= X-Received: by 10.36.228.200 with SMTP id o191mr25606655ith.143.1517559732856; Fri, 02 Feb 2018 00:22:12 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.112.13 with HTTP; Fri, 2 Feb 2018 00:22:12 -0800 (PST) In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5BB73CBF@SHSMSX104.ccr.corp.intel.com> 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> From: Ard Biesheuvel Date: Fri, 2 Feb 2018 08:22:12 +0000 Message-ID: To: "Ni, Ruiyu" Cc: "Guo Heyi , Dong Wei" , "Dong, Eric" , "edk2-devel@lists.01.org" , linaro-uefi , "Kinney, Michael D" , "Zeng, Star" 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: Fri, 02 Feb 2018 08:16:36 -0000 Content-Type: text/plain; charset="UTF-8" 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 > uefi@lists.linaro.org>; 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/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.