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

[edmund.raile]

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.ra...@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