From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 24 Jun 2019 16:50:51 -0700 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B740D308626C; Mon, 24 Jun 2019 23:50:48 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-226.ams2.redhat.com [10.36.116.226]) by smtp.corp.redhat.com (Postfix) with ESMTP id ACCE31001B00; Mon, 24 Jun 2019 23:50:47 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 7/7] OvmfPkg: don't assign PCI BARs above 4GiB when CSM enabled To: devel@edk2.groups.io, dwmw2@infradead.org Cc: Ray Ni References: <20190621223156.701502-1-dwmw2@infradead.org> <20190621223156.701502-7-dwmw2@infradead.org> From: "Laszlo Ersek" Message-ID: Date: Tue, 25 Jun 2019 01:50:46 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190621223156.701502-7-dwmw2@infradead.org> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Mon, 24 Jun 2019 23:50:50 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/22/19 00:31, David Woodhouse wrote: > Mostly, this is only necessary for devices that the CSM might have > native support for, such as VirtIO and NVMe; PciBusDxe will already > degrade devices to 32-bit if they have an OpROM. > > However, there doesn't seem to be a generic way of requesting PciBusDxe > to downgrade specific devices. > > There's IncompatiblePciDeviceSupportProtocol but that doesn't provide > the PCI class information or a handle to the device itself, so there's > no simple way to just match on all NVMe devices, for example. > > Just leave gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size set to zero for > CSM builds, until/unless that can be fixed. > > Signed-off-by: David Woodhouse > --- > OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++ > OvmfPkg/OvmfPkgX64.dsc | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index 639e33cb28..8fbe601386 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -542,8 +542,10 @@ > gUefiOvmfPkgTokenSpaceGuid.PcdPciIoSize|0x0 > gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Base|0x0 > gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size|0x0 > +!ifndef $(CSM_ENABLE) > gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base|0x0 > gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000 > +!endif > > gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0 > > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index 69a3497c2c..6f15f11220 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -541,8 +541,10 @@ > gUefiOvmfPkgTokenSpaceGuid.PcdPciIoSize|0x0 > gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Base|0x0 > gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size|0x0 > +!ifndef $(CSM_ENABLE) > gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base|0x0 > gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000 > +!endif > > gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0 > > (1) Side request -- can you please set the "xfuncname" knob so that the diff hunk headers (@@) reflect the section name being modified? It's explained at , and it can be automated by "BaseTools/Scripts/SetupGit.py". (2) For the patch. I'd prefer - setting PcdPciMmio64Base to zero unconditionally, and - setting PcdPciMmio64Size unconditionally as well, just to different values, dependent on CSM_ENABLE. The reason is that there is a difference between setting a default for a dynamic PCD in a platform DSC, and inheriting the PCD as-is (with the same value) from a package DEC file. The difference is that in the latter case, the platform doesn't directly choose an access method (here "dynamic") for the PCD, and then the access method is "deduced". This deduction is not trivial: https://edk2-docs.gitbooks.io/edk-ii-build-specification/content/v/release/1.28/8_pre-build_autogen_stage/84_auto-generated_pcd_database_file.html so I can't guarantee, without checking the build report, that the above patch would keep the access method "dynamic", for the PCDs in question. And, in OvmfPkg/PlatformPei, we have PcdSet64S() calls for PcdPciMmio64Size that are actually reachable in the IA32X64 and X64 builds, dependent on the QEMU command line (they are not reachable in the IA32 build). If possible, I wouldn't like to introduce an avenue to trip an ASSERT_RETURN_ERROR(). If we continue specifying the access method in the DSC files, just change the default value to 0, then PcdSet64S() is guaranteed to succeed, *plus* that fact will not be difficult to see. (It's a different question whether it makes sense for a user to fiddle with "opt/ovmf/X-PciMmio64Mb" in a CSM-enabled build... But a failed assertion is usually one of the less graceful behaviors.) Thanks Laszlo