Re: [PATCH 03/11] vfio/igd: Detect IGD device by OpRegion

["Corvin Köhne" <C.Koehne@beckhoff.com>]

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

On Tue, 2025-04-22 at 00:31 +0800, Tomita Moeko wrote:
> CAUTION: External Email!!
> There is currently no straightforward way to distinguish if a Intel
> graphics device is IGD or discrete GPU. However, only IGD devices expose
> OpRegion. Use the presence of VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION
> to identify IGD devices.
> 
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index 36316e50ea..7a7c7735c1 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -479,6 +479,7 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int
> nr)
>  
>  static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
>  {
> +    g_autofree struct vfio_region_info *opregion = NULL;
>      int ret, gen;
>      uint64_t gms_size;
>      uint64_t *bdsm_size;
> @@ -486,16 +487,20 @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice
> *vdev, Error **errp)
>      bool legacy_mode_enabled = false;
>      Error *err = NULL;
>  
> -    /*
> -     * This must be an Intel VGA device at address 00:02.0 for us to even
> -     * consider enabling legacy mode.  The vBIOS has dependencies on the
> -     * PCI bus address.
> -     */

Why do you remove this comment? Yes, the comment is not correct. Some OS driver
and the UEFI GOP depend on address 00:02.0 too. Wouldn't it be better to improve
the comment instead of removing it? This restriction looks a bit odd and IMO a
comment would help future reader to understand it easier. 

>      if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
>          !vfio_is_vga(vdev)) {
>          return true;
>      }
>  
> +    /* IGD device always comes with OpRegion */
> +    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) {
> +        return true;
> +    }
> +    info_report("OpRegion detected on Intel display %x.", vdev->device_id);
> +
>      /*
>       * IGD is not a standard, they like to change their specs often.  We
>       * only attempt to support back to SandBridge and we hope that newer
> @@ -570,9 +575,14 @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice
> *vdev, Error **errp)
>      }
>  
>      /* Setup OpRegion access */
> -    if ((vdev->features & VFIO_FEATURE_ENABLE_IGD_OPREGION) &&
> -        !vfio_pci_igd_setup_opregion(vdev, errp)) {
> -        goto error;
> +    if ((vdev->features & VFIO_FEATURE_ENABLE_IGD_OPREGION)) {
> +        if (vdev->pdev.qdev.hotplugged) {
> +            error_setg(errp, "OpRegion is not supported on hotplugged
> device");
> +            goto error;
> +        }
> +        if (!vfio_pci_igd_opregion_init(vdev, opregion, errp)) {
> +            goto error;
> +        }

How is this part related to "Detect IGD device by OpRegion"?

>      }
>  
>      /* Setup LPC bridge / Host bridge PCI IDs */

-- 
Kind regards,
Corvin

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