Re: [PATCH v2] vfio/pci: Refine Intel IGD OpRegion support

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 30 Sep 2020 00:10:38 +0800
Fred Gao <fred.gao@intel.com> wrote:

> Bypass the IGD initialization for Intel's dgfx devices with own expansion
> ROM and the host/LPC bridge config space are no longer accessed.
> 
> v2: simply test if discrete or integrated gfx device
>     with root bus. (Alex Williamson)
> 
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: Xiong Zhang <xiong.y.zhang@intel.com>
> Cc: Hang Yuan <hang.yuan@linux.intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Fred Gao <fred.gao@intel.com>
> ---
>  drivers/vfio/pci/vfio_pci.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index f634c81998bb..9258ccfadb79 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -336,10 +336,11 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
>  	if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev))
>  		vdev->has_vga = true;
>  
> -
> +	/* Intel's dgfx should not appear on root bus */
>  	if (vfio_pci_is_vga(pdev) &&
>  	    pdev->vendor == PCI_VENDOR_ID_INTEL &&
> -	    IS_ENABLED(CONFIG_VFIO_PCI_IGD)) {
> +	    IS_ENABLED(CONFIG_VFIO_PCI_IGD) &&
> +	    pci_is_root_bus(pdev->bus)) {
>  		ret = vfio_pci_igd_init(vdev);
>  		if (ret) {
>  			pci_warn(pdev, "Failed to setup Intel IGD regions\n");


The comment seems rather misplaced here, it only refers to one switch,
several lines down within the set of conditions, but looks like a
header for the entire branch.  I think it would be better to either
expand the comment to describe the entire branch, including the
exclusion, or try to fit the exclusion comment alongside the test, ie.

	/*
	 * Intel IGD requires quirks to support guest drivers.  IGD is
	 * identified as an Intel VGA device on the root bus.
	 */

Or
	    pci_is_root_bus(pdev->bus)) { /* Skip discrete gfx */

The commit title should really include something about excluding
discrete graphics from IGD quirks as well.  It might help downstreams
backport it for support.

It also occurs to me that relying on the physical topology only works
at the bare metal level.  We could for example assign a dgfx device at
address 00:02.0 in the guest.  Nested assignment of that device would
trigger calling vfio_pci_igd_init() and fail.  I see igd has a PCIe
capability type of PCI_EXP_TYPE_RC_END and I'd expect dgfx to have a
type of PCI_EXP_TYPE_LEG_END, but unfortunately QEMU does too good of a
job emulating the PCIe capability and will mangle these to suit the
guest topology.  I wonder then if our best course is to make the above
branch more lenient, for example pruning the failure paths such that we
could use -ENODEV as a non-terminal error like is done for the NVLink
quirks below this code block.  Failure to find an OpRegion might be our
differentiation, where on bare metal we might have both igd and dgfx,
so we'd need the root bus test, but assigning dgfx to a VM and placing
it on the VM root bus wouldn't generate an OpRegion, so both levels
would take the dgfx path.  IGD placed on a non-root bus in the guest
could probably just be considered a misconfiguration by the user...
Thanks,

Alex