[PATCH] vfio/igd: Fix incorrect error propagation in vfio_pci_igd_opregion_detect()

[PATCH] vfio/igd: Fix incorrect error propagation in vfio_pci_igd_opregion_detect()

[Tomita Moeko]

In vfio_pci_igd_opregion_detect(), errp will be set when device does
not have OpRegion or is hotplugged. This errp will be propergated to
pci_qdev_realize(), which interprets it as failure, causing unexpected
termination on devices without OpRegion like SR-IOV VFs or discrete
GPUs. Fix it by not setting errp in vfio_pci_igd_opregion_detect().

This patch also checks if the device has OpRegion before hotplug status
to prvent unwanted warning messages on non-IGD devices.

Fixes: c0273e77f2d7 ("vfio/igd: Detect IGD device by OpRegion")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2968
Reported-by: Edmund Raile <edmund.raile@protonmail.com>
Link: https://lore.kernel.org/qemu-devel/30044d14-17ec-46e3-b9c3-63d27a5bde27@gmail.com
Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index e7952d15a0..e7a9d1ffc1 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -187,23 +187,21 @@ static bool vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
 }
 
 static bool vfio_pci_igd_opregion_detect(VFIOPCIDevice *vdev,
-                                         struct vfio_region_info **opregion,
-                                         Error **errp)
+                                         struct vfio_region_info **opregion)
 {
     int ret;
 
-    /* Hotplugging is not supported for opregion access */
-    if (vdev->pdev.qdev.hotplugged) {
-        error_setg(errp, "IGD OpRegion is not supported on hotplugged device");
-        return false;
-    }
-
     ret = vfio_device_get_region_info_type(&vdev->vbasedev,
                     VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
                     VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, opregion);
     if (ret) {
-        error_setg_errno(errp, -ret,
-                         "Device does not supports IGD OpRegion feature");
+        return false;
+    }
+
+    /* Hotplugging is not supported for opregion access */
+    if (vdev->pdev.qdev.hotplugged) {
+        warn_report("IGD device detected, but OpRegion is not supported "
+                    "on hotplugged device.");
         return false;
     }
 
@@ -524,7 +522,7 @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
     }
 
     /* IGD device always comes with OpRegion */
-    if (!vfio_pci_igd_opregion_detect(vdev, &opregion, errp)) {
+    if (!vfio_pci_igd_opregion_detect(vdev, &opregion)) {
         return true;
     }
     info_report("OpRegion detected on Intel display %x.", vdev->device_id);
@@ -695,7 +693,7 @@ static bool vfio_pci_kvmgt_config_quirk(VFIOPCIDevice *vdev, Error **errp)
         return true;
     }
 
-    if (!vfio_pci_igd_opregion_detect(vdev, &opregion, errp)) {
+    if (!vfio_pci_igd_opregion_detect(vdev, &opregion)) {
         /* Should never reach here, KVMGT always emulates OpRegion */
         return false;
     }
-- 
2.47.2

Re: [PATCH] vfio/igd: Fix incorrect error propagation in vfio_pci_igd_opregion_detect()

[Cédric Le Goater]

Moeko,

On 5/21/25 17:40, Tomita Moeko wrote:
> In vfio_pci_igd_opregion_detect(), errp will be set when device does
> not have OpRegion or is hotplugged. This errp will be propergated to
> pci_qdev_realize(), which interprets it as failure, causing unexpected
> termination on devices without OpRegion like SR-IOV VFs or discrete
> GPUs. Fix it by not setting errp in vfio_pci_igd_opregion_detect().
> 
> This patch also checks if the device has OpRegion before hotplug status
> to prvent unwanted warning messages on non-IGD devices.
> 
> Fixes: c0273e77f2d7 ("vfio/igd: Detect IGD device by OpRegion")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2968
> Reported-by: Edmund Raile <edmund.raile@protonmail.com>
> Link: https://lore.kernel.org/qemu-devel/30044d14-17ec-46e3-b9c3-63d27a5bde27@gmail.com
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>

Could you please resend a patch that applies on vfio-next ?

   https://github.com/legoater/qemu/commits/vfio-next


Thanks,

C.


> ---
>   hw/vfio/igd.c | 22 ++++++++++------------
>   1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index e7952d15a0..e7a9d1ffc1 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -187,23 +187,21 @@ static bool vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
>   }
>   
>   static bool vfio_pci_igd_opregion_detect(VFIOPCIDevice *vdev,
> -                                         struct vfio_region_info **opregion,
> -                                         Error **errp)
> +                                         struct vfio_region_info **opregion)
>   {
>       int ret;
>   
> -    /* Hotplugging is not supported for opregion access */
> -    if (vdev->pdev.qdev.hotplugged) {
> -        error_setg(errp, "IGD OpRegion is not supported on hotplugged device");
> -        return false;
> -    }
> -
>       ret = vfio_device_get_region_info_type(&vdev->vbasedev,
>                       VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
>                       VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, opregion);
>       if (ret) {
> -        error_setg_errno(errp, -ret,
> -                         "Device does not supports IGD OpRegion feature");
> +        return false;
> +    }
> +
> +    /* Hotplugging is not supported for opregion access */
> +    if (vdev->pdev.qdev.hotplugged) {
> +        warn_report("IGD device detected, but OpRegion is not supported "
> +                    "on hotplugged device.");
>           return false;
>       }
>   
> @@ -524,7 +522,7 @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
>       }
>   
>       /* IGD device always comes with OpRegion */
> -    if (!vfio_pci_igd_opregion_detect(vdev, &opregion, errp)) {
> +    if (!vfio_pci_igd_opregion_detect(vdev, &opregion)) {
>           return true;
>       }
>       info_report("OpRegion detected on Intel display %x.", vdev->device_id);
> @@ -695,7 +693,7 @@ static bool vfio_pci_kvmgt_config_quirk(VFIOPCIDevice *vdev, Error **errp)
>           return true;
>       }
>   
> -    if (!vfio_pci_igd_opregion_detect(vdev, &opregion, errp)) {
> +    if (!vfio_pci_igd_opregion_detect(vdev, &opregion)) {
>           /* Should never reach here, KVMGT always emulates OpRegion */
>           return false;
>       }

Re: [PATCH] vfio/igd: Fix incorrect error propagation in vfio_pci_igd_opregion_detect()

[Alex Williamson]

On Wed, 21 May 2025 23:40:36 +0800
Tomita Moeko <tomitamoeko@gmail.com> wrote:

> In vfio_pci_igd_opregion_detect(), errp will be set when device does
> not have OpRegion or is hotplugged. This errp will be propergated to

propagated

> pci_qdev_realize(), which interprets it as failure, causing unexpected
> termination on devices without OpRegion like SR-IOV VFs or discrete
> GPUs. Fix it by not setting errp in vfio_pci_igd_opregion_detect().
> 
> This patch also checks if the device has OpRegion before hotplug status
> to prvent unwanted warning messages on non-IGD devices.
> 
> Fixes: c0273e77f2d7 ("vfio/igd: Detect IGD device by OpRegion")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2968
> Reported-by: Edmund Raile <edmund.raile@protonmail.com>
> Link: https://lore.kernel.org/qemu-devel/30044d14-17ec-46e3-b9c3-63d27a5bde27@gmail.com
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index e7952d15a0..e7a9d1ffc1 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -187,23 +187,21 @@ static bool vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
>  }
>  
>  static bool vfio_pci_igd_opregion_detect(VFIOPCIDevice *vdev,
> -                                         struct vfio_region_info **opregion,
> -                                         Error **errp)
> +                                         struct vfio_region_info **opregion)
>  {
>      int ret;
>  
> -    /* Hotplugging is not supported for opregion access */
> -    if (vdev->pdev.qdev.hotplugged) {
> -        error_setg(errp, "IGD OpRegion is not supported on hotplugged device");
> -        return false;
> -    }
> -
>      ret = vfio_device_get_region_info_type(&vdev->vbasedev,
>                      VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
>                      VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, opregion);
>      if (ret) {
> -        error_setg_errno(errp, -ret,
> -                         "Device does not supports IGD OpRegion feature");
> +        return false;
> +    }
> +
> +    /* Hotplugging is not supported for opregion access */
> +    if (vdev->pdev.qdev.hotplugged) {
> +        warn_report("IGD device detected, but OpRegion is not supported "
> +                    "on hotplugged device.");
>          return false;
>      }
>  
> @@ -524,7 +522,7 @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
>      }
>  
>      /* IGD device always comes with OpRegion */
> -    if (!vfio_pci_igd_opregion_detect(vdev, &opregion, errp)) {
> +    if (!vfio_pci_igd_opregion_detect(vdev, &opregion)) {
>          return true;
>      }
>      info_report("OpRegion detected on Intel display %x.", vdev->device_id);
> @@ -695,7 +693,7 @@ static bool vfio_pci_kvmgt_config_quirk(VFIOPCIDevice *vdev, Error **errp)
>          return true;
>      }
>  
> -    if (!vfio_pci_igd_opregion_detect(vdev, &opregion, errp)) {
> +    if (!vfio_pci_igd_opregion_detect(vdev, &opregion)) {
>          /* Should never reach here, KVMGT always emulates OpRegion */
>          return false;
>      }

Re: [PATCH] vfio/igd: Fix incorrect error propagation in vfio_pci_igd_opregion_detect()

[Corvin Köhne]

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

On Wed, 2025-05-21 at 23:40 +0800, Tomita Moeko wrote:
> CAUTION: External Email!!
> In vfio_pci_igd_opregion_detect(), errp will be set when device does
> not have OpRegion or is hotplugged. This errp will be propergated to
> pci_qdev_realize(), which interprets it as failure, causing unexpected
> termination on devices without OpRegion like SR-IOV VFs or discrete
> GPUs. Fix it by not setting errp in vfio_pci_igd_opregion_detect().
> 
> This patch also checks if the device has OpRegion before hotplug status
> to prvent unwanted warning messages on non-IGD devices.
> 

prevent

> Fixes: c0273e77f2d7 ("vfio/igd: Detect IGD device by OpRegion")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2968
> Reported-by: Edmund Raile <edmund.raile@protonmail.com>
> Link:
> https://nospamproxywebp.beckhoff.com/enQsig/link?id=BAgAAADuIFWfr8z-RqgAAABiHH1XDaojIZQJ51lW0MfdmbQ31Dc6Q9h8spLn3SSpPU_3qX9yZXScfxI-jMpRRWHhwwz3WzjHhTZuw3K8bmq6pYob7gKjPbBhTbeNLyeDio9w7y6aczQOHjPiGEAK1Zd5bXPhuYMJhd0r02BDXxk2NzfU10_-lfkisL6dUaMagg0Urr9aoCFD5may09obVXsyPg-RtsX8nfXOGmiQX-6W6i3Z7jYH4Ys1
>  
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
> 

-- 
Kind regards,
Corvin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] vfio/igd: Fix incorrect error propagation in vfio_pci_igd_opregion_detect()

[Tomita Moeko]

On 5/22/25 15:31, edmund.raile wrote:
> Hi Moeko,
> 
>> I did some further debugging, and found it was caused by a mistake in
>> error handling. In vfio_pci_igd_opregion_detect(), `errp` is set when
>> OpRegion is not found. However, in pci_qdev_realize(), the caller of
>> vfio_realize(), it checks if the errp is set for failure, rather than
>> the return value.
> 
> Am I correct in assuming that errp can just be removed because the
> return status already carries the information?

Hi, Edmund

Please "reply all" to get your reply posted to the mailing list :)

Not really so. Here we checks if kernel exposes OpRegion VFIO region on
that device, if it doesn't, we believe it is not a IGD. So it should not
be a error if OpRegion is not found on a Intel VGA device (It can be
discrete GPU, SR-IOV VF, or something else). Here I choose to only print
the "OpRegion detected..." message if OpRegion found rather than the
misleading "OpRegion not supported" message on all Intel VGA.

>> I will submit a patch later to fix error handling.
> 
> I've had to manually apply the patch by hand.
> For me, your patch became:
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index e7952d15a0..e7a9d1ffc1 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -187,23 +187,21 @@ static bool vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
>  }
>  
>  static bool vfio_pci_igd_opregion_detect(VFIOPCIDevice *vdev,
> -                                         struct vfio_region_info **opregion,
> -                                         Error **errp)
> +                                         struct vfio_region_info **opregion)
>  {
>      int ret;
>  
> -    /* Hotplugging is not supported for opregion access */
> -    if (vdev->pdev.qdev.hotplugged) {
> -        error_setg(errp, "IGD OpRegion is not supported on hotplugged device");
> -        return false;
> -    }
> -
>      ret = vfio_device_get_region_info_type(&vdev->vbasedev,
>                      VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
>                      VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, opregion);
>      if (ret) {
> -        error_setg_errno(errp, -ret,
> -                         "Device does not supports IGD OpRegion feature");
> +        return false;
> +    }
> +
> +    /* Hotplugging is not supported for opregion access */
> +    if (vdev->pdev.qdev.hotplugged) {
> +        warn_report("IGD device detected, but OpRegion is not supported "
> +                    "on hotplugged device.");
>          return false;
>      }
>  
> @@ -524,7 +522,7 @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
>      }
>  
>      /* IGD device always comes with OpRegion */
> -    if (!vfio_pci_igd_opregion_detect(vdev, &opregion, errp)) {
> +    if (!vfio_pci_igd_opregion_detect(vdev, &opregion)) {
>          return true;
>      }
>      info_report("OpRegion detected on Intel display %x.", vdev->device_id);
> @@ -695,7 +693,7 @@ static bool vfio_pci_kvmgt_config_quirk(VFIOPCIDevice *vdev, Error **errp)
>          return true;
>      }
>  
> -    if (!vfio_pci_igd_opregion_detect(vdev, &opregion, errp)) {
> +    if (!vfio_pci_igd_opregion_detect(vdev, &opregion)) {
>          /* Should never reach here, KVMGT always emulates OpRegion */
>          return false;
>      }
> 
> Testing it, it works (for me)!
> Though I can only confirm the negative (for a SR-IOV iGPU
> virtual function).
> I can't unbind my primary GPU (the root iGPU device) since
> I'm using it.
> The VM boots fine regardless whether
> `-device vfio-pci,...,x-igd-opregion=` is set to off or on now.
> Tested-by: Edmund Raile <edmund.raile@protonmail.com>
> 
> Your patch provides successful automatic OpRegion detection.
> 
> But for the case that this automatic detection should some day break
> or become ineffective due to new hardware: should I still send in my
> [PATCH v3]?

I'm not sure if it is really needed. We only apply quirks on device that
exposes OpRegion, ignoring others. And x-igd-opregion=off disables
passing it to guest. So far "A Intel VGA device with OpRegion is an IGD"
is true, but there may be other possibilities in future... It also
breaks the behavior as other quirks (like ASLS register) will not be
applied when x-igd-opregion=off.

If we decide to move the flag check before vfio_pci_igd_opregion_detect,
then probably the check before calling vfio_pci_igd_opregion_init can be
removed.

Any ideas will be appreciated.

Thanks,
Moeko
 
> [PATCH v3] vfio/igd: Respect x-igd-opregion=off in IGD quirk handling
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index e7a9d1ffc1..ac9f504e8f 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -521,6 +521,11 @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
>          return true;
>      }
>  
> +    /* Respect x-igd-opregion=off by skipping OpRegion handling */
> +    if (!(vdev->features & VFIO_FEATURE_ENABLE_IGD_OPREGION)) {
> +        return true;
> +    }
> +
>      /* IGD device always comes with OpRegion */
>      if (!vfio_pci_igd_opregion_detect(vdev, &opregion)) {
>          return true;
> 
> That way, we'd have a certain way to force it off if need be
> A redundancy, in case.
> Can't always expect everything to always work perfectly.
> 
> Thanks,
> Edmund

Re: [PATCH] vfio/igd: Fix incorrect error propagation in vfio_pci_igd_opregion_detect()

[edmund.raile via]

Hi Moeko,

>I did some further debugging, and found it was caused by a mistake in
>error handling. In vfio_pci_igd_opregion_detect(), `errp` is set when
>OpRegion is not found. However, in pci_qdev_realize(), the caller of
>vfio_realize(), it checks if the errp is set for failure, rather than
>the return value.

Am I correct in assuming that errp can just be removed because the
return status already carries the information?

>I will submit a patch later to fix error handling.

I've had to manually apply the patch by hand.
For me, your patch became:

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index e7952d15a0..e7a9d1ffc1 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -187,23 +187,21 @@ static bool vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
 }
 
 static bool vfio_pci_igd_opregion_detect(VFIOPCIDevice *vdev,
-                                         struct vfio_region_info **opregion,
-                                         Error **errp)
+                                         struct vfio_region_info **opregion)
 {
     int ret;
 
-    /* Hotplugging is not supported for opregion access */
-    if (vdev->pdev.qdev.hotplugged) {
-        error_setg(errp, "IGD OpRegion is not supported on hotplugged device");
-        return false;
-    }
-
     ret = vfio_device_get_region_info_type(&vdev->vbasedev,
                     VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
                     VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, opregion);
     if (ret) {
-        error_setg_errno(errp, -ret,
-                         "Device does not supports IGD OpRegion feature");
+        return false;
+    }
+
+    /* Hotplugging is not supported for opregion access */
+    if (vdev->pdev.qdev.hotplugged) {
+        warn_report("IGD device detected, but OpRegion is not supported "
+                    "on hotplugged device.");
         return false;
     }
 
@@ -524,7 +522,7 @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
     }
 
     /* IGD device always comes with OpRegion */
-    if (!vfio_pci_igd_opregion_detect(vdev, &opregion, errp)) {
+    if (!vfio_pci_igd_opregion_detect(vdev, &opregion)) {
         return true;
     }
     info_report("OpRegion detected on Intel display %x.", vdev->device_id);
@@ -695,7 +693,7 @@ static bool vfio_pci_kvmgt_config_quirk(VFIOPCIDevice *vdev, Error **errp)
         return true;
     }
 
-    if (!vfio_pci_igd_opregion_detect(vdev, &opregion, errp)) {
+    if (!vfio_pci_igd_opregion_detect(vdev, &opregion)) {
         /* Should never reach here, KVMGT always emulates OpRegion */
         return false;
     }

Testing it, it works (for me)!
Though I can only confirm the negative (for a SR-IOV iGPU
virtual function).
I can't unbind my primary GPU (the root iGPU device) since
I'm using it.
The VM boots fine regardless whether
`-device vfio-pci,...,x-igd-opregion=` is set to off or on now.
Tested-by: Edmund Raile <edmund.raile@protonmail.com>

Your patch provides successful automatic OpRegion detection.

But for the case that this automatic detection should some day break
or become ineffective due to new hardware: should I still send in my
[PATCH v3]?

[PATCH v3] vfio/igd: Respect x-igd-opregion=off in IGD quirk handling

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index e7a9d1ffc1..ac9f504e8f 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -521,6 +521,11 @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
         return true;
     }
 
+    /* Respect x-igd-opregion=off by skipping OpRegion handling */
+    if (!(vdev->features & VFIO_FEATURE_ENABLE_IGD_OPREGION)) {
+        return true;
+    }
+
     /* IGD device always comes with OpRegion */
     if (!vfio_pci_igd_opregion_detect(vdev, &opregion)) {
         return true;

That way, we'd have a certain way to force it off if need be
A redundancy, in case.
Can't always expect everything to always work perfectly.

Thanks,
Edmund