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

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 22 Apr 2025 00:31:03 +0800
Tomita Moeko <tomitamoeko@gmail.com> wrote:

> 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.
> -     */
>      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;
> +        }
>      }
>  
>      /* Setup LPC bridge / Host bridge PCI IDs */

I think this still needs some refactoring work.  The setup function
currently does:
 a) test hotplugged
 b) get opregion
 c) call init

We implemented b) above and therefore do a) and c) here, duplicating
them from the setup function.  It would be valid to test a) as we're
getting the opregion, so wouldn't it make sense to turn setup into a
function that does a) and b), returning an opregion, called by both
this path and GVT-g path, and each would call the init function
themselves?  The latter is already implemented in the next patch, but
a) and b) are still duplicated in GVT-g specific code.  Thanks,

Alex