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.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]?
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
