1. Patchew
  2. QEMU
  3. [PATCH 0/4] vfio/igd: remove incorrect IO BAR4 quirk
  4. View patch

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

Tomita Moeko posted 4 patches 1 year ago
There is a newer version of this series
[PATCH 3/4] vfio/igd: refactor vfio_probe_igd_bar4_quirk() into pci config quirk
Posted by Tomita Moeko 1 year ago 
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.

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;
 
-- 
2.45.2
Re: [PATCH 3/4] vfio/igd: refactor vfio_probe_igd_bar4_quirk() into pci config quirk
Posted by Alex Williamson 1 year ago 
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;
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.