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:c05::242; helo=mail-pg0-x242.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pg0-x242.google.com (mail-pg0-x242.google.com [IPv6:2607:f8b0:400e:c05::242]) (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 505B6222630DD for ; Thu, 22 Feb 2018 17:04:10 -0800 (PST) Received: by mail-pg0-x242.google.com with SMTP id q27so1122871pgn.8 for ; Thu, 22 Feb 2018 17:10:12 -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=X2dkBzEBrZ1yrCWCAFXMJF4oZkO+OClRgWQMSPXE3Uw=; b=PEcqu64W0ozIkXXVvIlcx/N6GpxiYh6/6oAXsDUCIjEnkmFQmJz23OFo8ETRWOut7+ 68f9TI4EeJ3UCHselr98hxAQ9DBnfduzyNXZMofWLCWKC5YqzNKo6nSOCUycfvBVKLXU Po0YP+I9bpxmimKfisNfJCFwyUxh8vnApJA9M= 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=X2dkBzEBrZ1yrCWCAFXMJF4oZkO+OClRgWQMSPXE3Uw=; b=BUd9xl12vkF2peQEBU6PXEsLrFYz7kqVjJ/zcl8NDvx0aH/KWAjgW4I6hWMafKmdzM 2oZybxNJ2u5ggSWJu7PUoXrtqAQNUpeobrpn5FdnVx4YO6ePWiSh/ZGn2VbvPdSZP3GR XM/jJOYQY17IbvZhkHy34Z48x19a3zXCseiptIgDhX7D1yOz6SMlAsruaiaisS9ZRyhT qL4J/6STt384oL8O3y+pS2rIZ/KsPMFkLh821urHBAVBaaN0KVwAGN8exVNYQPJVqhXS ndFVzhiVJ15BaAyNREUVVIbWGpXlaEMJqDTJ5XXFeUAj3ubyBxWk1OnXOf5UW3oXorrH +q0g== X-Gm-Message-State: APf1xPCqfBt7zaQPJPILJxLf1WaTzK6ORmXoXu4Qd25zOa4YQPaxjeCg ZgUOlxqYULSI4CFEyzmRUKAaaA== X-Google-Smtp-Source: AH8x225xgYyzCnfj6QMV63lcoYwHz8Z6dPMZlOM4nbzkd6iMJue/in/AVgKdwERY3g5a6I0UzObHuw== X-Received: by 10.98.234.22 with SMTP id t22mr189688pfh.56.1519348211967; Thu, 22 Feb 2018 17:10:11 -0800 (PST) Received: from SZX1000114654 ([45.56.152.187]) by smtp.gmail.com with ESMTPSA id v65sm1805031pfv.61.2018.02.22.17.10.09 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 22 Feb 2018 17:10:11 -0800 (PST) From: Guo Heyi X-Google-Original-From: Guo Heyi Date: Fri, 23 Feb 2018 09:10:07 +0800 To: Laszlo Ersek Cc: Heyi Guo , edk2-devel@lists.01.org, Ruiyu Ni , Ard Biesheuvel , Star Zeng , Eric Dong , Michael D Kinney Message-ID: <20180223011007.GD95440@SZX1000114654> References: <1519282474-94811-1-git-send-email-heyi.guo@linaro.org> <1519282474-94811-3-git-send-email-heyi.guo@linaro.org> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) 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: Fri, 23 Feb 2018 01:04:11 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Feb 22, 2018 at 11:41:50AM +0100, Laszlo Ersek wrote: > 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? Will double check what it really is. > > ... 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.) I made the patch according to the conclusion here: https://lists.01.org/pipermail/edk2-devel/2018-February/020960.html if I understood that correctly :) However, if it turns out so confusing in the code, especially to someone who didn't participate in the discussions, I agree it may worth keeping all the definitions consistent in EDK2 code, while being different from what in ACPI ASL code. Thanks, Gary (Heyi Guo) > > Thanks > Laszlo