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

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

[Zhenyu Wang]

[-- Attachment #1: Type: text/plain, Size: 1748 bytes --]


On 2020.09.30 00:10:38 +0800, Fred Gao 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)
>

Patch title is somehow misleading that better change to what this one does
that skip VFIO IGD setup for Intel discrete graphics card.

With that,

Reviewed-by: Zhenyu Wang <zhenyuw@linux.intel.com>

> 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");
> -- 
> 2.24.1.1.gb6d4d82bd5
> 

-- 

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Fred Gao]

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");
-- 
2.24.1.1.gb6d4d82bd5

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

[Alex Williamson]

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

[PATCH v3] vfio/pci: Bypass IGD init in case of -ENODEV

[Fred Gao]

Bypass the IGD initialization when -ENODEV returns,
that should be the case if opregion is not available for IGD
or within discrete graphics device's option ROM,
or host/lpc bridge is not found.

Then use of -ENODEV here means no special device resources found
which needs special care for VFIO, but we still allow other normal
device resource access.

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: Fred Gao <fred.gao@intel.com>
---
 drivers/vfio/pci/vfio_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index f634c81998bb..c88cf9937469 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -341,7 +341,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
 	    pdev->vendor == PCI_VENDOR_ID_INTEL &&
 	    IS_ENABLED(CONFIG_VFIO_PCI_IGD)) {
 		ret = vfio_pci_igd_init(vdev);
-		if (ret) {
+		if (ret && ret != -ENODEV) {
 			pci_warn(pdev, "Failed to setup Intel IGD regions\n");
 			goto disable_exit;
 		}
-- 
2.24.1.1.gb6d4d82bd5

Re: [PATCH v3] vfio/pci: Bypass IGD init in case of -ENODEV

[Alex Williamson]

On Tue,  3 Nov 2020 02:01:20 +0800
Fred Gao <fred.gao@intel.com> wrote:

> Bypass the IGD initialization when -ENODEV returns,
> that should be the case if opregion is not available for IGD
> or within discrete graphics device's option ROM,
> or host/lpc bridge is not found.
> 
> Then use of -ENODEV here means no special device resources found
> which needs special care for VFIO, but we still allow other normal
> device resource access.
> 
> 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: Fred Gao <fred.gao@intel.com>
> ---
>  drivers/vfio/pci/vfio_pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to vfio for-linus branch for v5.10.  Thanks,

Alex

> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index f634c81998bb..c88cf9937469 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -341,7 +341,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
>  	    pdev->vendor == PCI_VENDOR_ID_INTEL &&
>  	    IS_ENABLED(CONFIG_VFIO_PCI_IGD)) {
>  		ret = vfio_pci_igd_init(vdev);
> -		if (ret) {
> +		if (ret && ret != -ENODEV) {
>  			pci_warn(pdev, "Failed to setup Intel IGD regions\n");
>  			goto disable_exit;
>  		}