Re: [PATCH 3/4] vfio/igd: refactor vfio_probe_igd_bar4_quirk() into pci config quirk

[Alex Williamson]

On Thu, 23 Jan 2025 01:17:30 +0800
Tomita Moeko <tomitamoeko@gmail.com> wrote:

> The actual IO BAR4 write quirk in vfio_probe_igd_bar4_quirk() was
> removed in previous change, leaving the function not matching its name,
> so move it into the newly introduced vfio_config_quirk_setup(). There
> is no functional change in this commit. If any failure occurs, the
> function simply returns and proceeds.

I don't understand why vfio_config_quirk_setup() returns bool rather
than void given that it can never fail based on this series.
Otherwise, while I'm surprised that the GTT re-writing is unnecessary
(seems I wouldn't have invented such a need), removing it is really the
only way to fully validate that, and we can always revisit if we start
getting regression reports.  Thanks,

Alex

> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd.c        | 30 ++++++++++++++++--------------
>  hw/vfio/pci-quirks.c |  6 +++++-
>  hw/vfio/pci.h        |  2 +-
>  3 files changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index 4f9a90f36f..33e5202052 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -359,7 +359,8 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int 
> nr)
>      QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, bdsm_quirk, next);
>  }
>  
> -void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> +bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
> +                                 Error **errp G_GNUC_UNUSED)
>  {
>      g_autofree struct vfio_region_info *rom = NULL;
>      g_autofree struct vfio_region_info *opregion = NULL;
> @@ -378,10 +379,9 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int 
> nr)
>       * PCI bus address.
>       */
>      if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
> -        nr != 4 ||
>          &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
>                                         0, PCI_DEVFN(0x2, 0))) {
> -        return;
> +        return true;
>      }
>  
>      /*
> @@ -395,7 +395,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int 
> nr)
>                                             "vfio-pci-igd-lpc-bridge")) {
>          error_report("IGD device %s cannot support legacy mode due to 
> existing "
>                       "devices at address 1f.0", vdev->vbasedev.name);
> -        return;
> +        return true;
>      }
>  
>      /*
> @@ -407,7 +407,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int 
> nr)
>      if (gen == -1) {
>          error_report("IGD device %s is unsupported in legacy mode, "
>                       "try SandyBridge or newer", vdev->vbasedev.name);
> -        return;
> +        return true;
>      }
>  
>      /*
> @@ -420,7 +420,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int 
> nr)
>      if ((ret || !rom->size) && !vdev->pdev.romfile) {
>          error_report("IGD device %s has no ROM, legacy mode disabled",
>                       vdev->vbasedev.name);
> -        return;
> +        return true;
>      }
>  
>      /*
> @@ -431,7 +431,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int 
> nr)
>          error_report("IGD device %s hotplugged, ROM disabled, "
>                       "legacy mode disabled", vdev->vbasedev.name);
>          vdev->rom_read_failed = true;
> -        return;
> +        return true;
>      }
>  
>      /*
> @@ -444,7 +444,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int 
> nr)
>      if (ret) {
>          error_report("IGD device %s does not support OpRegion access,"
>                       "legacy mode disabled", vdev->vbasedev.name);
> -        return;
> +        return true;
>      }
>  
>      ret = vfio_get_dev_region_info(&vdev->vbasedev,
> @@ -453,7 +453,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int 
> nr)
>      if (ret) {
>          error_report("IGD device %s does not support host bridge access,"
>                       "legacy mode disabled", vdev->vbasedev.name);
> -        return;
> +        return true;
>      }
>  
>      ret = vfio_get_dev_region_info(&vdev->vbasedev,
> @@ -462,7 +462,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int 
> nr)
>      if (ret) {
>          error_report("IGD device %s does not support LPC bridge access,"
>                       "legacy mode disabled", vdev->vbasedev.name);
> -        return;
> +        return true;
>      }
>  
>      gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4);
> @@ -476,7 +476,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int 
> nr)
>          error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>          error_report("IGD device %s failed to enable VGA access, "
>                       "legacy mode disabled", vdev->vbasedev.name);
> -        return;
> +        return true;
>      }
>  
>      /* Create our LPC/ISA bridge */
> @@ -484,7 +484,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int 
> nr)
>      if (ret) {
>          error_report("IGD device %s failed to create LPC bridge, "
>                       "legacy mode disabled", vdev->vbasedev.name);
> -        return;
> +        return true;
>      }
>  
>      /* Stuff some host values into the VM PCI host bridge */
> @@ -492,14 +492,14 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int 
> nr)
>      if (ret) {
>          error_report("IGD device %s failed to modify host bridge, "
>                       "legacy mode disabled", vdev->vbasedev.name);
> -        return;
> +        return true;
>      }
>  
>      /* Setup OpRegion access */
>      if (!vfio_pci_igd_opregion_init(vdev, opregion, &err)) {
>          error_append_hint(&err, "IGD legacy mode disabled\n");
>          error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> -        return;
> +        return true;
>      }
>  
>      /*
> @@ -561,4 +561,6 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int 
> nr)
>  
>      trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name,
>                                      (ggms_size + gms_size) / MiB);
> +
> +    return true;
>  }
> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> index c40e3ca88f..b8379cb512 100644
> --- a/hw/vfio/pci-quirks.c
> +++ b/hw/vfio/pci-quirks.c
> @@ -1169,6 +1169,11 @@ bool vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
>   */
>  bool vfio_config_quirk_setup(VFIOPCIDevice *vdev, Error **errp)
>  {
> +#ifdef CONFIG_VFIO_IGD
> +    if (!vfio_probe_igd_config_quirk(vdev, errp)) {
> +        return false;
> +    }
> +#endif
>      return true;
>  }
>  
> @@ -1220,7 +1225,6 @@ void vfio_bar_quirk_setup(VFIOPCIDevice *vdev, int nr)
>      vfio_probe_rtl8168_bar2_quirk(vdev, nr);
>  #ifdef CONFIG_VFIO_IGD
>      vfio_probe_igd_bar0_quirk(vdev, nr);
> -    vfio_probe_igd_bar4_quirk(vdev, nr);
>  #endif
>  }
>  
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 5359e94f18..5c64de0270 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -217,7 +217,7 @@ bool vfio_add_virt_caps(VFIOPCIDevice *vdev, Error 
> **errp);
>  void vfio_quirk_reset(VFIOPCIDevice *vdev);
>  VFIOQuirk *vfio_quirk_alloc(int nr_mem);
>  void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr);
> -void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr);
> +bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp);
>  
>  extern const PropertyInfo qdev_prop_nv_gpudirect_clique;
>