From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:400e:c01::236; helo=mail-pl0-x236.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pl0-x236.google.com (mail-pl0-x236.google.com [IPv6:2607:f8b0:400e:c01::236]) (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 214C221F0DA49 for ; Mon, 5 Feb 2018 17:06:56 -0800 (PST) Received: by mail-pl0-x236.google.com with SMTP id j19so211727pll.2 for ; Mon, 05 Feb 2018 17:12:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:date:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=9OJPp7rdsw216Ylwel2xIHkk4ERjcnmBr08qWNLpTD0=; b=WAYTfxwYagFjZb8O3IraogovxEQkoaLztWgP5VIsTrUchyqXH1QLHOVQD5BiaPsYSY ZAmGZzWFgEdtjXL9Ksfjf4HDM3VdC7AGzj8P4RtQEgUhxzgUBZS1/6CO8dCyPwFxCZFn H0QYWWbFoKs9n2CBm0e8ZLa0SvT16vgxS1GB8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=9OJPp7rdsw216Ylwel2xIHkk4ERjcnmBr08qWNLpTD0=; b=GgPSuhhYsx9y85BqQUwPbs/+4hI+iK90ezxGHIu/dFic5QQ1/k3ISwS1qQC34odY5A S+egToEQC+NwCXrdWGi2EDsHeYIAYFQIXF/gV/2bCq+OIU4CwHUkMBp8wRzDnq5/sQGO eaXu4e+IpyD5UeIYYGpmczGnO0IkSah2OFDAu+GmbWcR8End7BbR5zfsx6D/YWXLIqpo LRw4wU3ajiFSnHa99ygl+FEss0iC/OGhXIsg8MVuY7nyYHLJoM6cKFctYluenMh+aNBY 7/EXqWEY4GHCQoynhh67x8WKDJ5NPtUfDlEioQsezfU4IriaO9OMesC1rFucLzzC1bym eoUw== X-Gm-Message-State: APf1xPA1YN8eH+8EeO8mFaHdFWCZAp+/OITyGHQXODhwR7JpJNS2+4IX C8T5XR4gV4uC/YF9nI77Hyt7RA== X-Google-Smtp-Source: AH8x2246QKDTu9TSsMVN+r6mY+OAj5S1kNlagzRWuVqPSKe1OTQwkhXIehYzVZuG/eTRr+G3MJDnQA== X-Received: by 2002:a17:902:780f:: with SMTP id p15-v6mr668958pll.161.1517879558659; Mon, 05 Feb 2018 17:12:38 -0800 (PST) Received: from SZX1000114654 ([104.237.91.79]) by smtp.gmail.com with ESMTPSA id 12sm20524196pfr.147.2018.02.05.17.12.35 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 05 Feb 2018 17:12:38 -0800 (PST) From: Guo Heyi X-Google-Original-From: Guo Heyi Date: Tue, 6 Feb 2018 09:12:33 +0800 To: Ard Biesheuvel Cc: "Ni, Ruiyu" , "Ard Biesheuvel ,Guo Heyi" , "Rothman, Michael A" , "Dong, Eric" , Dong Wei , linaro-uefi , "Kinney, Michael D" , "edk2-devel@lists.01.org" , "Zeng, Star" Message-ID: <20180206011233.GA5795@SZX1000114654> References: <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> <7fab5279-9f46-93c2-6a90-3b6d957b2f27@Intel.com> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) 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: Tue, 06 Feb 2018 01:06:57 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Feb 05, 2018 at 10:11:30AM +0100, Ard Biesheuvel wrote: > On 5 February 2018 at 09:06, Ni, Ruiyu 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 > >>> 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? > > > > 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)