[PATCH v8 06/28] vfio: refactor out vfio_interrupt_setup()

John Levon posted 28 patches 1 month, 2 weeks ago
[PATCH v8 06/28] vfio: refactor out vfio_interrupt_setup()
Posted by John Levon 1 month, 2 weeks ago
Refactor the interrupt setup code out of vfio_realize(), as we will
later need this for vfio-user too.

Signed-off-by: John Levon <john.levon@nutanix.com>
---
 hw/vfio/pci.c | 54 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 21 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 89d900e9cf..5fb6c4c4c6 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2957,6 +2957,37 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
     vdev->req_enabled = false;
 }
 
+static bool vfio_interrupt_setup(VFIOPCIDevice *vdev, Error **errp)
+{
+    PCIDevice *pdev = &vdev->pdev;
+
+    /* QEMU emulates all of MSI & MSIX */
+    if (pdev->cap_present & QEMU_PCI_CAP_MSIX) {
+        memset(vdev->emulated_config_bits + pdev->msix_cap, 0xff,
+               MSIX_CAP_LENGTH);
+    }
+
+    if (pdev->cap_present & QEMU_PCI_CAP_MSI) {
+        memset(vdev->emulated_config_bits + pdev->msi_cap, 0xff,
+               vdev->msi_cap_size);
+    }
+
+    if (vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1)) {
+        vdev->intx.mmap_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
+                                                  vfio_intx_mmap_enable, vdev);
+        pci_device_set_intx_routing_notifier(&vdev->pdev,
+                                             vfio_intx_routing_notifier);
+        vdev->irqchip_change_notifier.notify = vfio_irqchip_change;
+        kvm_irqchip_add_change_notifier(&vdev->irqchip_change_notifier);
+        if (!vfio_intx_enable(vdev, errp)) {
+            pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
+            kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
+            return false;
+        }
+    }
+    return true;
+}
+
 static void vfio_realize(PCIDevice *pdev, Error **errp)
 {
     ERRP_GUARD();
@@ -3157,27 +3188,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         }
     }
 
-    /* QEMU emulates all of MSI & MSIX */
-    if (pdev->cap_present & QEMU_PCI_CAP_MSIX) {
-        memset(vdev->emulated_config_bits + pdev->msix_cap, 0xff,
-               MSIX_CAP_LENGTH);
-    }
-
-    if (pdev->cap_present & QEMU_PCI_CAP_MSI) {
-        memset(vdev->emulated_config_bits + pdev->msi_cap, 0xff,
-               vdev->msi_cap_size);
-    }
-
-    if (vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1)) {
-        vdev->intx.mmap_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
-                                                  vfio_intx_mmap_enable, vdev);
-        pci_device_set_intx_routing_notifier(&vdev->pdev,
-                                             vfio_intx_routing_notifier);
-        vdev->irqchip_change_notifier.notify = vfio_irqchip_change;
-        kvm_irqchip_add_change_notifier(&vdev->irqchip_change_notifier);
-        if (!vfio_intx_enable(vdev, errp)) {
-            goto out_deregister;
-        }
+    if (!vfio_interrupt_setup(vdev, errp)) {
+        goto out_teardown;
     }
 
     if (vdev->display != ON_OFF_AUTO_OFF) {
-- 
2.34.1
Re: [PATCH v8 06/28] vfio: refactor out vfio_interrupt_setup()
Posted by Cédric Le Goater 1 week ago
On 2/19/25 15:48, John Levon wrote:
> Refactor the interrupt setup code out of vfio_realize(), as we will
> later need this for vfio-user too.
> 
> Signed-off-by: John Levon <john.levon@nutanix.com>
> ---
>   hw/vfio/pci.c | 54 +++++++++++++++++++++++++++++++--------------------
>   1 file changed, 33 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 89d900e9cf..5fb6c4c4c6 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2957,6 +2957,37 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>       vdev->req_enabled = false;
>   }
>   
> +static bool vfio_interrupt_setup(VFIOPCIDevice *vdev, Error **errp)
> +{
> +    PCIDevice *pdev = &vdev->pdev;
> +
> +    /* QEMU emulates all of MSI & MSIX */
> +    if (pdev->cap_present & QEMU_PCI_CAP_MSIX) {
> +        memset(vdev->emulated_config_bits + pdev->msix_cap, 0xff,
> +               MSIX_CAP_LENGTH);
> +    }
> +
> +    if (pdev->cap_present & QEMU_PCI_CAP_MSI) {
> +        memset(vdev->emulated_config_bits + pdev->msi_cap, 0xff,
> +               vdev->msi_cap_size);
> +    }
> +
> +    if (vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1)) {
> +        vdev->intx.mmap_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
> +                                                  vfio_intx_mmap_enable, vdev);
> +        pci_device_set_intx_routing_notifier(&vdev->pdev,
> +                                             vfio_intx_routing_notifier);
> +        vdev->irqchip_change_notifier.notify = vfio_irqchip_change;
> +        kvm_irqchip_add_change_notifier(&vdev->irqchip_change_notifier);
> +        if (!vfio_intx_enable(vdev, errp)) {
> +            pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
> +            kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
> +            return false;
> +        }
> +    }
> +    return true;
> +}
> +
>   static void vfio_realize(PCIDevice *pdev, Error **errp)
>   {
>       ERRP_GUARD();
> @@ -3157,27 +3188,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>           }
>       }
>   
> -    /* QEMU emulates all of MSI & MSIX */
> -    if (pdev->cap_present & QEMU_PCI_CAP_MSIX) {
> -        memset(vdev->emulated_config_bits + pdev->msix_cap, 0xff,
> -               MSIX_CAP_LENGTH);
> -    }
> -
> -    if (pdev->cap_present & QEMU_PCI_CAP_MSI) {
> -        memset(vdev->emulated_config_bits + pdev->msi_cap, 0xff,
> -               vdev->msi_cap_size);
> -    }
> -
> -    if (vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1)) {
> -        vdev->intx.mmap_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
> -                                                  vfio_intx_mmap_enable, vdev);
> -        pci_device_set_intx_routing_notifier(&vdev->pdev,
> -                                             vfio_intx_routing_notifier);
> -        vdev->irqchip_change_notifier.notify = vfio_irqchip_change;
> -        kvm_irqchip_add_change_notifier(&vdev->irqchip_change_notifier);
> -        if (!vfio_intx_enable(vdev, errp)) {
> -            goto out_deregister;
> -        }
> +    if (!vfio_interrupt_setup(vdev, errp)) {
> +        goto out_teardown;

is that the correct exit label ?


Thanks,

C.


>       }
>   
>       if (vdev->display != ON_OFF_AUTO_OFF) {
Re: [PATCH v8 06/28] vfio: refactor out vfio_interrupt_setup()
Posted by John Levon 1 week ago
On Thu, Apr 03, 2025 at 11:23:09AM +0200, Cédric Le Goater wrote:

> On 2/19/25 15:48, John Levon wrote:
> > Refactor the interrupt setup code out of vfio_realize(), as we will
> > later need this for vfio-user too.
> > 
> > Signed-off-by: John Levon <john.levon@nutanix.com>
> >   static void vfio_realize(PCIDevice *pdev, Error **errp)
> >   {
> >       ERRP_GUARD();
> > @@ -3157,27 +3188,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> >           }
> >       }
> > -    /* QEMU emulates all of MSI & MSIX */
> > -    if (pdev->cap_present & QEMU_PCI_CAP_MSIX) {
> > -        memset(vdev->emulated_config_bits + pdev->msix_cap, 0xff,
> > -               MSIX_CAP_LENGTH);
> > -    }
> > -
> > -    if (pdev->cap_present & QEMU_PCI_CAP_MSI) {
> > -        memset(vdev->emulated_config_bits + pdev->msi_cap, 0xff,
> > -               vdev->msi_cap_size);
> > -    }
> > -
> > -    if (vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1)) {
> > -        vdev->intx.mmap_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
> > -                                                  vfio_intx_mmap_enable, vdev);
> > -        pci_device_set_intx_routing_notifier(&vdev->pdev,
> > -                                             vfio_intx_routing_notifier);
> > -        vdev->irqchip_change_notifier.notify = vfio_irqchip_change;
> > -        kvm_irqchip_add_change_notifier(&vdev->irqchip_change_notifier);
> > -        if (!vfio_intx_enable(vdev, errp)) {
> > -            goto out_deregister;
> > -        }
> > +    if (!vfio_interrupt_setup(vdev, errp)) {
> > +        goto out_teardown;
> 
> is that the correct exit label ?

Thanks, missed during rebase, it should be out_unset_idev

There is also a bug in the self-cleanup of vfio_interrupt_setup(): it's not
doing timer_free(vdev->intx.mmap_timer);

regards
john