Re: [edk2-devel] [PATCH 7/7] OvmfPkg: don't assign PCI BARs above 4GiB when CSM enabled

["Laszlo Ersek" <lersek@redhat.com>]

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 <dwmw2@infradead.org>
> ---
>  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
<https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers>,
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