From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (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 4159820945C0D for ; Mon, 11 Sep 2017 21:58:52 -0700 (PDT) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Sep 2017 22:01:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,381,1500966000"; d="scan'208";a="148129910" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga005.jf.intel.com with ESMTP; 11 Sep 2017 22:01:48 -0700 Received: from fmsmsx122.amr.corp.intel.com (10.18.125.37) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 11 Sep 2017 22:01:47 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx122.amr.corp.intel.com (10.18.125.37) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 11 Sep 2017 22:01:47 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.117]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.93]) with mapi id 14.03.0319.002; Tue, 12 Sep 2017 13:01:45 +0800 From: "Ni, Ruiyu" To: Laszlo Ersek , "edk2-devel@lists.01.org" CC: Dong Wei , Benjamin Herrenschmidt , Andrew Fish , Ard Biesheuvel Thread-Topic: [edk2] [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address Thread-Index: AQHTKrsJa1BkM5niF0GIJ87bFEEK+qKut9wAgAH6L/A= Date: Tue, 12 Sep 2017 05:01:44 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5BA2BABB@SHSMSX104.ccr.corp.intel.com> References: <20170911050121.85708-1-ruiyu.ni@intel.com> <7fbd6e6b-0577-f470-3e89-f785ddd5dee1@redhat.com> In-Reply-To: <7fbd6e6b-0577-f470-3e89-f785ddd5dee1@redhat.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Sep 2017 04:58:52 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, Your understanding is: DeviceAddress =3D HostAddress + AddressTranslationOf= fset But my patch assumes: HostAddress =3D DeviceAddress + AddressTranslationOff= set They are totally different. If I follow your understanding, the patch is wr= ong! Since UEFI spec doesn't describe "apply to" in sentence " Offset to apply t= o the Starting address of a BAR to convert it to a PCI address" very clearly, I q= uoted the statement from ACPI spec. Your understanding to "apply to" is "add", my understanding is "minus". Thanks/Ray > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Monday, September 11, 2017 2:47 PM > To: Ni, Ruiyu ; edk2-devel@lists.01.org > Cc: Dong Wei ; Benjamin Herrenschmidt > ; Andrew Fish ; Ard Biesheuvel > > Subject: Re: [edk2] [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() > returns Host address >=20 > On 09/11/17 07:01, Ruiyu Ni wrote: > > Per the UEFI Spec, GetBarAttributes() should return the Host address. > > But current implementation returns the address read from the BAR, > > which is the Device address. > > Per the description of AddressTranslationOffset in ACPI spec: > > "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." >=20 > The ACPI spec also says: >=20 > "Non-bridge devices must list 0 for all Address Translation offset bits." >=20 > However, the UEFI spec (v2.7) says, under > EFI_PCI_IO_PROTOCOL.GetBarAttributes(): >=20 > "The ACPI Specification does not define how to the use the Address > Translation Offset for non-bridge devices. The UEFI Specification is exte= nding > the definition of Address Translation Offset to support systems that have > different mapping for HostAddress and DeviceAddress. > [...] Address Translation Offset. Offset to apply to the Starting address= of a > BAR to convert it to a PCI address. This value is zero unless the HostAdd= ress > and DeviceAddress for the BAR are different." >=20 > So, I think the patch is correct, but the commit message should not refer= to > the ACPI spec. It should refer to / quote the UEFI spec only. >=20 > > HostAddress =3D DeviceAddress + AddressTranslationOffset. >=20 > The sentences from the UEFI spec are "Address Translation Offset. Offset = to > apply to the Starting address of a BAR to convert it to a PCI address", a= nd > "Address Range Minimum. Starting address of BAR." >=20 > To me this seems to imply that AddrRangeMin is already a host address, an= d >=20 > DeviceAddress =3D AddrRangeMin + AddressTranslationOffset >=20 > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Ruiyu Ni > > Cc: Ard Biesheuvel > > Cc: Benjamin Herrenschmidt > > Cc: Andrew Fish > > Cc: Dong Wei > > Cc: Laszlo Ersek > > Cc: Bartosz Szczepanek > > --- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > > index cc7125e4fc..852d35d710 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > > @@ -1955,7 +1955,7 @@ PciIoGetBarAttributes ( > > End->Checksum =3D 0; > > > > // > > - // Get the Address Translation Offset > > + // Get the Address Translation Offset and convert the Device addre= ss to > Host address. > > // > > if (Descriptor->ResType =3D=3D ACPI_ADDRESS_SPACE_TYPE_MEM) { > > Descriptor->AddrTranslationOffset =3D > > GetMmioAddressTranslationOffset ( @@ -1967,6 +1967,7 @@ > PciIoGetBarAttributes ( > > FreePool (Descriptor); > > return EFI_UNSUPPORTED; > > } > > + Descriptor->AddrRangeMin +=3D Descriptor->AddrTranslationOffset; > > } > > } > > > > >=20 > Actually, let me circle back to the initial problem here (apologies if it= 's too late > for that) -- why are we adding the offset inside the > GetBarAttributes() function? Isn't it the caller's responsibility to do t= he > addition after GetBarAttributes() returns? >=20 > I mean if a PCI driver author reads the UEFI 2.7 spec, the spec seems to = give > that impression. >=20 > (I'm sorry if I should have raised these questions last week -- I don't w= ish to > block this patch. Please go ahead if I'm wrong.) >=20 > Thanks > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel