Re: [PATCH v3] vfio/pci: match IGD devices in display controller class

[Alex Williamson <alex.williamson@xxxxxxxxxx>]

On Fri, 24 Jan 2025 00:34:15 +0800
Tomita Moeko <tomitamoeko@xxxxxxxxx> wrote:

> IGD device can either expose as a VGA controller or display controller
> depending on whether it is configured as the primary display device in
> BIOS. In both cases, the OpRegion may be present. A new helper function
> vfio_pci_is_intel_display() is introduced to check if the device might
> be an IGD device.
> 
> Signed-off-by: Tomita Moeko <tomitamoeko@xxxxxxxxx>
> ---
> Changelog:
> v3:
> * Removed BDF condition as Intel discrete GPUs does not have OpRegion
> * Added a helper function to match all possible devices with base class
> * Renamed from "vfio/pci: update igd matching conditions"
> Link: https://lore.kernel.org/lkml/20241230161054.3674-2-tomitamoeko@xxxxxxxxx/
> 
> v2:
> Fix misuse of pci_get_domain_bus_and_slot(), now only compares bdf
> without touching device reference count.
> Link: https://lore.kernel.org/all/20241229155140.7434-1-tomitamoeko@xxxxxxxxx/
> 
>  drivers/vfio/pci/vfio_pci.c      | 4 +---
>  drivers/vfio/pci/vfio_pci_igd.c  | 6 ++++++
>  drivers/vfio/pci/vfio_pci_priv.h | 6 ++++++
>  3 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index e727941f589d..5f169496376a 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -111,9 +111,7 @@ static int vfio_pci_open_device(struct vfio_device *core_vdev)
>  	if (ret)
>  		return ret;
>  
> -	if (vfio_pci_is_vga(pdev) &&
> -	    pdev->vendor == PCI_VENDOR_ID_INTEL &&
> -	    IS_ENABLED(CONFIG_VFIO_PCI_IGD)) {
> +	if (vfio_pci_is_intel_display(pdev)) {

I'd slightly prefer to keep:

    if (vfio_pci_is_intel_display(pdev) &&
        IS_ENABLED(CONFIG_VFIO_PCI_IGD)) {

as otherwise the helper function isn't entirely honest in the implied
test.  Not a huge deal though, so

Reviewed-by: Alex Williamson <alex.williamson@xxxxxxxxxx>

>  		ret = vfio_pci_igd_init(vdev);
>  		if (ret && ret != -ENODEV) {
>  			pci_warn(pdev, "Failed to setup Intel IGD regions\n");
> diff --git a/drivers/vfio/pci/vfio_pci_igd.c b/drivers/vfio/pci/vfio_pci_igd.c
> index dd70e2431bd7..ef490a4545f4 100644
> --- a/drivers/vfio/pci/vfio_pci_igd.c
> +++ b/drivers/vfio/pci/vfio_pci_igd.c
> @@ -435,6 +435,12 @@ static int vfio_pci_igd_cfg_init(struct vfio_pci_core_device *vdev)
>  	return 0;
>  }
>  
> +bool vfio_pci_is_intel_display(struct pci_dev *pdev)
> +{
> +	return (pdev->vendor == PCI_VENDOR_ID_INTEL) &&
> +	       ((pdev->class >> 16) == PCI_BASE_CLASS_DISPLAY);
> +}
> +
>  int vfio_pci_igd_init(struct vfio_pci_core_device *vdev)
>  {
>  	int ret;
> diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h
> index 5e4fa69aee16..a9972eacb293 100644
> --- a/drivers/vfio/pci/vfio_pci_priv.h
> +++ b/drivers/vfio/pci/vfio_pci_priv.h
> @@ -67,8 +67,14 @@ void vfio_pci_memory_unlock_and_restore(struct vfio_pci_core_device *vdev,
>  					u16 cmd);
>  
>  #ifdef CONFIG_VFIO_PCI_IGD
> +bool vfio_pci_is_intel_display(struct pci_dev *pdev);
>  int vfio_pci_igd_init(struct vfio_pci_core_device *vdev);
>  #else
> +static inline bool vfio_pci_is_intel_display(struct pci_dev *pdev)
> +{
> +	return false;
> +}
> +
>  static inline int vfio_pci_igd_init(struct vfio_pci_core_device *vdev)
>  {
>  	return -ENODEV;