From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 5EE1D2034D8FF for ; Thu, 22 Feb 2018 02:35:52 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0A7EE818B103; Thu, 22 Feb 2018 10:41:52 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-138.rdu2.redhat.com [10.10.120.138]) by smtp.corp.redhat.com (Postfix) with ESMTP id C028D2026E04; Thu, 22 Feb 2018 10:41:50 +0000 (UTC) To: Heyi Guo , edk2-devel@lists.01.org Cc: Ruiyu Ni , Ard Biesheuvel , Star Zeng , Eric Dong , Michael D Kinney References: <1519282474-94811-1-git-send-email-heyi.guo@linaro.org> <1519282474-94811-3-git-send-email-heyi.guo@linaro.org> From: Laszlo Ersek Message-ID: Date: Thu, 22 Feb 2018 11:41:50 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <1519282474-94811-3-git-send-email-heyi.guo@linaro.org> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Thu, 22 Feb 2018 10:41:52 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Thu, 22 Feb 2018 10:41:52 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [RFC v2 2/2] MdeModulePkg/PciBus: return CPU address for GetBarAttributes 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: Thu, 22 Feb 2018 10:35:52 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/22/18 07:54, Heyi Guo wrote: > PciIo::GetBarAttributes should return CPU view address according to > UEFI spec 2.7, so we change the implementation to follow the spec. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Heyi Guo > Cc: Ruiyu Ni > Cc: Ard Biesheuvel > Cc: Star Zeng > Cc: Eric Dong > Cc: Laszlo Ersek > Cc: Michael D Kinney > > --- > MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > index 190f4b0..0aafcba 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > @@ -1814,8 +1814,8 @@ GetMmioAddressTranslationOffset ( > > while (Configuration->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) { > if ((Configuration->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) && > - (Configuration->AddrRangeMin <= AddrRangeMin) && > - (Configuration->AddrRangeMin + Configuration->AddrLen >= AddrRangeMin + AddrLen) > + (Configuration->AddrRangeMin + Configuration->AddrTranslationOffset <= AddrRangeMin) && > + (Configuration->AddrRangeMin + Configuration->AddrLen + Configuration->AddrTranslationOffset >= AddrRangeMin + AddrLen) > ) { > return Configuration->AddrTranslationOffset; > } > @@ -1968,6 +1968,11 @@ PciIoGetBarAttributes ( > return EFI_UNSUPPORTED; > } > } > + > + // According to UEFI spec 2.7, we need return CPU view address for PciIo::GetBarAttributes, > + // and PCI view = CPU view + translation > + Descriptor->AddrRangeMin -= Descriptor->AddrTranslationOffset; > + Descriptor->AddrRangeMax -= Descriptor->AddrTranslationOffset; > } > > return EFI_SUCCESS; > According to your blurb -- which should be instead part of the commit message of patch #1, as discussed before --, we have the following interpretations: * internal: host = device + ATO * external: device = host + ATO The GetMmioAddressTranslationOffset() change looks correct, because (according to your blurb) RootBridgeIo->Configuration() returns a host (CPU) view. Adding the ATO we get the device view, and then we can do the comparison against the BAR values read from the device. OK. In PciIoGetBarAttributes(), Descriptor->AddrRangeMin is first set from PciIoDevice, so it's a device view. I think the subtraction is correct; the caller will re-add the ATO if it wants to return to the device view. In PciIoGetBarAttributes(), I think the AddrRangeMax manipulation is incorrect (possibly due to a preexistent bug in PciBusDxe). Namely, Descriptor->AddrRangeMax is first set to the Alignment of the BAR, from PciIoDevice, not the end address of the BAR. In order to output the value required by the UEFI spec, we have to calculate the end address using AddrLen. Is that right? ... To repeat myself, I find it extremely hard to reason about this feature while using different internal and external ATO interpretations. Can we pick one formula and stick with it everywhere? (I don't insist, but without it, I don't think I can sensibly review future iterations of this set.) Thanks Laszlo