[Qemu-devel] [PATCH 0/2] vfio-pci: Introduce vfio_register_event_notifier()

[Qemu-devel] [PATCH 0/2] vfio-pci: Introduce vfio_register_event_notifier()

[Eric Auger]

This small series introduces the vfio_register_event_notifier()
helper which allows to set up/tear down the VFIO signalling of eventfd
for ERR, REQ and INTX irq indices.

On top of that, a new irq index is planned to signal DMA faults
in nested mode use case. This would use exactly the same mechanics.

Eric Auger (2):
  vfio-pci: Introduce vfio_register_event_notifier helper
  vfio-pci: Use vfio_register_event_notifier in vfio_intx_enable_kvm

 hw/vfio/pci.c | 329 ++++++++++++++++++++------------------------------
 1 file changed, 134 insertions(+), 195 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH 1/2] vfio-pci: Introduce vfio_register_event_notifier helper

[Eric Auger]

The code used to attach the eventfd handler for the ERR and
REQ irq indices can be factorized into a helper. In subsequent
patches we will extend this helper to support other irq indices.

We test the notification is allowed outside of the helper:
respectively check vdev->pci_aer and VFIO_FEATURE_ENABLE_REQ.
Depending on the returned value we set vdev->pci_aer and
vdev->req_enabled. An error handle is introduced for future usage
although not strictly useful here.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/vfio/pci.c | 291 ++++++++++++++++++++++----------------------------
 1 file changed, 127 insertions(+), 164 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index c0cb1ec289..c589a4e666 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
     vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
 }
 
+/*
+ * vfio_register_event_notifier - setup/tear down eventfd
+ * notification and handling for IRQ indices that span over
+ * a single IRQ
+ *
+ * @vdev: VFIO device handle
+ * @index: IRQ index the eventfd/handler is associated to
+ * @target_state: true means notifier needs to be set up
+ * @handler to attach if @target_state is true
+ * @errp error handle
+ */
+static int vfio_register_event_notifier(VFIOPCIDevice *vdev,
+                                        int index,
+                                        bool target_state,
+                                        void (*handler)(void *opaque),
+                                        Error **errp)
+{
+    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
+                                      .index = index };
+    struct vfio_irq_set *irq_set;
+    EventNotifier *notifier;
+    int argsz, ret = 0;
+    int32_t *pfd, fd;
+
+    switch (index) {
+    case VFIO_PCI_REQ_IRQ_INDEX:
+        notifier = &vdev->req_notifier;
+        break;
+    case VFIO_PCI_ERR_IRQ_INDEX:
+        notifier = &vdev->err_notifier;
+        break;
+    default:
+        return -EINVAL;
+    }
+
+    if (ioctl(vdev->vbasedev.fd,
+              VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < 1) {
+        error_setg_errno(errp, errno,
+                         "no irq index %d available", index);
+        return -EINVAL;
+    }
+
+    if (target_state) {
+        ret = event_notifier_init(notifier, 0);
+        if (ret) {
+            error_setg_errno(errp, -ret,
+                             "Unable to init event notifier for irq index %d",
+                             index);
+            return ret;
+        }
+    }
+
+    argsz = sizeof(*irq_set) + sizeof(*pfd);
+
+    irq_set = g_malloc0(argsz);
+    irq_set->argsz = argsz;
+    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
+                     VFIO_IRQ_SET_ACTION_TRIGGER;
+    irq_set->index = index;
+    irq_set->start = 0;
+    irq_set->count = 1;
+    pfd = (int32_t *)&irq_set->data;
+
+    if (!notifier) {
+        return -EINVAL;
+    }
+
+    fd = event_notifier_get_fd(notifier);
+
+    if (target_state) {
+        qemu_set_fd_handler(fd, handler, NULL, vdev);
+        *pfd = fd;
+    } else {
+        qemu_set_fd_handler(fd, NULL, NULL, vdev);
+        event_notifier_cleanup(notifier);
+        *pfd = -1;
+    }
+
+    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
+    g_free(irq_set);
+
+    if (ret) {
+        error_setg_errno(errp, -ret,
+                         "vfio: Failed to %s eventfd signalling for index %d",
+                         *pfd < 0 ? "set up" : "tear down", index);
+    }
+    return ret;
+}
+
 static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
 {
 #ifdef CONFIG_KVM
@@ -2621,86 +2710,6 @@ static void vfio_err_notifier_handler(void *opaque)
     vm_stop(RUN_STATE_INTERNAL_ERROR);
 }
 
-/*
- * Registers error notifier for devices supporting error recovery.
- * If we encounter a failure in this function, we report an error
- * and continue after disabling error recovery support for the
- * device.
- */
-static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
-{
-    int ret;
-    int argsz;
-    struct vfio_irq_set *irq_set;
-    int32_t *pfd;
-
-    if (!vdev->pci_aer) {
-        return;
-    }
-
-    if (event_notifier_init(&vdev->err_notifier, 0)) {
-        error_report("vfio: Unable to init event notifier for error detection");
-        vdev->pci_aer = false;
-        return;
-    }
-
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
-
-    irq_set = g_malloc0(argsz);
-    irq_set->argsz = argsz;
-    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
-                     VFIO_IRQ_SET_ACTION_TRIGGER;
-    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-
-    *pfd = event_notifier_get_fd(&vdev->err_notifier);
-    qemu_set_fd_handler(*pfd, vfio_err_notifier_handler, NULL, vdev);
-
-    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
-    if (ret) {
-        error_report("vfio: Failed to set up error notification");
-        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
-        event_notifier_cleanup(&vdev->err_notifier);
-        vdev->pci_aer = false;
-    }
-    g_free(irq_set);
-}
-
-static void vfio_unregister_err_notifier(VFIOPCIDevice *vdev)
-{
-    int argsz;
-    struct vfio_irq_set *irq_set;
-    int32_t *pfd;
-    int ret;
-
-    if (!vdev->pci_aer) {
-        return;
-    }
-
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
-
-    irq_set = g_malloc0(argsz);
-    irq_set->argsz = argsz;
-    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
-                     VFIO_IRQ_SET_ACTION_TRIGGER;
-    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-    *pfd = -1;
-
-    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
-    if (ret) {
-        error_report("vfio: Failed to de-assign error fd: %m");
-    }
-    g_free(irq_set);
-    qemu_set_fd_handler(event_notifier_get_fd(&vdev->err_notifier),
-                        NULL, NULL, vdev);
-    event_notifier_cleanup(&vdev->err_notifier);
-}
-
 static void vfio_req_notifier_handler(void *opaque)
 {
     VFIOPCIDevice *vdev = opaque;
@@ -2716,86 +2725,6 @@ static void vfio_req_notifier_handler(void *opaque)
     }
 }
 
-static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
-{
-    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
-                                      .index = VFIO_PCI_REQ_IRQ_INDEX };
-    int argsz;
-    struct vfio_irq_set *irq_set;
-    int32_t *pfd;
-
-    if (!(vdev->features & VFIO_FEATURE_ENABLE_REQ)) {
-        return;
-    }
-
-    if (ioctl(vdev->vbasedev.fd,
-              VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < 1) {
-        return;
-    }
-
-    if (event_notifier_init(&vdev->req_notifier, 0)) {
-        error_report("vfio: Unable to init event notifier for device request");
-        return;
-    }
-
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
-
-    irq_set = g_malloc0(argsz);
-    irq_set->argsz = argsz;
-    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
-                     VFIO_IRQ_SET_ACTION_TRIGGER;
-    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-
-    *pfd = event_notifier_get_fd(&vdev->req_notifier);
-    qemu_set_fd_handler(*pfd, vfio_req_notifier_handler, NULL, vdev);
-
-    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
-        error_report("vfio: Failed to set up device request notification");
-        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
-        event_notifier_cleanup(&vdev->req_notifier);
-    } else {
-        vdev->req_enabled = true;
-    }
-
-    g_free(irq_set);
-}
-
-static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
-{
-    int argsz;
-    struct vfio_irq_set *irq_set;
-    int32_t *pfd;
-
-    if (!vdev->req_enabled) {
-        return;
-    }
-
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
-
-    irq_set = g_malloc0(argsz);
-    irq_set->argsz = argsz;
-    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
-                     VFIO_IRQ_SET_ACTION_TRIGGER;
-    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-    *pfd = -1;
-
-    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
-        error_report("vfio: Failed to de-assign device request fd: %m");
-    }
-    g_free(irq_set);
-    qemu_set_fd_handler(event_notifier_get_fd(&vdev->req_notifier),
-                        NULL, NULL, vdev);
-    event_notifier_cleanup(&vdev->req_notifier);
-
-    vdev->req_enabled = false;
-}
-
 static void vfio_realize(PCIDevice *pdev, Error **errp)
 {
     VFIOPCIDevice *vdev = PCI_VFIO(pdev);
@@ -3069,8 +2998,29 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         goto out_teardown;
     }
 
-    vfio_register_err_notifier(vdev);
-    vfio_register_req_notifier(vdev);
+    if (vdev->pci_aer) {
+        /*
+         * Registers error notifier for devices supporting error recovery.
+         * If we encounter a failure in this function, we report an error
+         * and continue after disabling error recovery support for the
+         * device.
+         */
+        vdev->pci_aer =
+            !vfio_register_event_notifier(vdev, VFIO_PCI_ERR_IRQ_INDEX, true,
+                                          vfio_err_notifier_handler, &err);
+        if (err) {
+            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+        }
+    }
+
+    if (vdev->features & VFIO_FEATURE_ENABLE_REQ) {
+        vdev->req_enabled =
+            !vfio_register_event_notifier(vdev, VFIO_PCI_REQ_IRQ_INDEX, true,
+                                          vfio_req_notifier_handler, &err);
+        if (err) {
+            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+        }
+    }
     vfio_setup_resetfn_quirk(vdev);
 
     return;
@@ -3106,9 +3056,22 @@ static void vfio_instance_finalize(Object *obj)
 static void vfio_exitfn(PCIDevice *pdev)
 {
     VFIOPCIDevice *vdev = PCI_VFIO(pdev);
+    Error *err = NULL;
 
-    vfio_unregister_req_notifier(vdev);
-    vfio_unregister_err_notifier(vdev);
+    if (vdev->req_enabled) {
+        vfio_register_event_notifier(vdev, VFIO_PCI_REQ_IRQ_INDEX,
+                                     false, NULL, &err);
+        if (err) {
+            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+        }
+    }
+    if (vdev->pci_aer) {
+        vfio_register_event_notifier(vdev, VFIO_PCI_ERR_IRQ_INDEX,
+                                     false, NULL, &err);
+        if (err) {
+            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+        }
+    }
     pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
     vfio_disable_interrupts(vdev);
     if (vdev->intx.mmap_timer) {
-- 
2.17.2

[Qemu-devel] [PATCH 2/2] vfio-pci: Use vfio_register_event_notifier in vfio_intx_enable_kvm

[Eric Auger]

We can also use vfio_register_event_notifier() helper in
vfio_intx_enable_kvm to set the signalling associated to
VFIO_PCI_INTX_IRQ_INDEX.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/vfio/pci.c | 38 +++++++-------------------------------
 1 file changed, 7 insertions(+), 31 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index c589a4e666..db0504ca10 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -136,6 +136,9 @@ static int vfio_register_event_notifier(VFIOPCIDevice *vdev,
     case VFIO_PCI_ERR_IRQ_INDEX:
         notifier = &vdev->err_notifier;
         break;
+    case VFIO_PCI_INTX_IRQ_INDEX:
+        notifier = &vdev->intx.interrupt;
+        break;
     default:
         return -EINVAL;
     }
@@ -351,10 +354,8 @@ static void vfio_intx_update(PCIDevice *pdev)
 static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
 {
     uint8_t pin = vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1);
-    int ret, argsz, retval = 0;
-    struct vfio_irq_set *irq_set;
-    int32_t *pfd;
     Error *err = NULL;
+    int ret;
 
     if (!pin) {
         return 0;
@@ -376,34 +377,12 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
     }
 #endif
 
-    ret = event_notifier_init(&vdev->intx.interrupt, 0);
+    ret = vfio_register_event_notifier(vdev, VFIO_PCI_INTX_IRQ_INDEX, true,
+                                       vfio_intx_interrupt, errp);
     if (ret) {
-        error_setg_errno(errp, -ret, "event_notifier_init failed");
         return ret;
     }
 
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
-
-    irq_set = g_malloc0(argsz);
-    irq_set->argsz = argsz;
-    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER;
-    irq_set->index = VFIO_PCI_INTX_IRQ_INDEX;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-
-    *pfd = event_notifier_get_fd(&vdev->intx.interrupt);
-    qemu_set_fd_handler(*pfd, vfio_intx_interrupt, NULL, vdev);
-
-    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
-    if (ret) {
-        error_setg_errno(errp, -ret, "failed to setup INTx fd");
-        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
-        event_notifier_cleanup(&vdev->intx.interrupt);
-        retval = -errno;
-        goto cleanup;
-    }
-
     vfio_intx_enable_kvm(vdev, &err);
     if (err) {
         warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
@@ -413,10 +392,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
 
     trace_vfio_intx_enable(vdev->vbasedev.name);
 
-cleanup:
-    g_free(irq_set);
-
-    return retval;
+    return 0;
 }
 
 static void vfio_intx_disable(VFIOPCIDevice *vdev)
-- 
2.17.2

Re: [Qemu-devel] [PATCH 1/2] vfio-pci: Introduce vfio_register_event_notifier helper

[Cornelia Huck]

On Fri, 11 Jan 2019 17:58:00 +0100
Eric Auger <eric.auger@redhat.com> wrote:

> The code used to attach the eventfd handler for the ERR and
> REQ irq indices can be factorized into a helper. In subsequent
> patches we will extend this helper to support other irq indices.

Looks like a nice refactoring to me.

> 
> We test the notification is allowed outside of the helper:

s/We test/We test whether/

> respectively check vdev->pci_aer and VFIO_FEATURE_ENABLE_REQ.
> Depending on the returned value we set vdev->pci_aer and
> vdev->req_enabled. An error handle is introduced for future usage
> although not strictly useful here.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/vfio/pci.c | 291 ++++++++++++++++++++++----------------------------
>  1 file changed, 127 insertions(+), 164 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index c0cb1ec289..c589a4e666 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>  }
>  
> +/*
> + * vfio_register_event_notifier - setup/tear down eventfd
> + * notification and handling for IRQ indices that span over
> + * a single IRQ
> + *
> + * @vdev: VFIO device handle
> + * @index: IRQ index the eventfd/handler is associated to

s/to/with/ ?

> + * @target_state: true means notifier needs to be set up
> + * @handler to attach if @target_state is true
> + * @errp error handle
> + */
> +static int vfio_register_event_notifier(VFIOPCIDevice *vdev,
> +                                        int index,
> +                                        bool target_state,
> +                                        void (*handler)(void *opaque),
> +                                        Error **errp)

(...)

> @@ -3069,8 +2998,29 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>          goto out_teardown;
>      }
>  
> -    vfio_register_err_notifier(vdev);
> -    vfio_register_req_notifier(vdev);
> +    if (vdev->pci_aer) {
> +        /*
> +         * Registers error notifier for devices supporting error recovery.
> +         * If we encounter a failure in this function, we report an error

s/in this function/while registering it/ ?

> +         * and continue after disabling error recovery support for the
> +         * device.
> +         */
> +        vdev->pci_aer =
> +            !vfio_register_event_notifier(vdev, VFIO_PCI_ERR_IRQ_INDEX, true,
> +                                          vfio_err_notifier_handler, &err);
> +        if (err) {
> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        }

I think you need to reset err to NULL if you want to reuse the variable.

Alternatively, you could keep the wrappers and define a local error
variable there.

> +    }
> +
> +    if (vdev->features & VFIO_FEATURE_ENABLE_REQ) {
> +        vdev->req_enabled =
> +            !vfio_register_event_notifier(vdev, VFIO_PCI_REQ_IRQ_INDEX, true,
> +                                          vfio_req_notifier_handler, &err);
> +        if (err) {
> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        }
> +    }
>      vfio_setup_resetfn_quirk(vdev);
>  
>      return;
> @@ -3106,9 +3056,22 @@ static void vfio_instance_finalize(Object *obj)
>  static void vfio_exitfn(PCIDevice *pdev)
>  {
>      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
> +    Error *err = NULL;
>  
> -    vfio_unregister_req_notifier(vdev);
> -    vfio_unregister_err_notifier(vdev);
> +    if (vdev->req_enabled) {
> +        vfio_register_event_notifier(vdev, VFIO_PCI_REQ_IRQ_INDEX,
> +                                     false, NULL, &err);
> +        if (err) {
> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        }

Likewise.

> +    }
> +    if (vdev->pci_aer) {
> +        vfio_register_event_notifier(vdev, VFIO_PCI_ERR_IRQ_INDEX,
> +                                     false, NULL, &err);
> +        if (err) {
> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        }
> +    }
>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>      vfio_disable_interrupts(vdev);
>      if (vdev->intx.mmap_timer) {

Re: [Qemu-devel] [PATCH 2/2] vfio-pci: Use vfio_register_event_notifier in vfio_intx_enable_kvm

[Cornelia Huck]

On Fri, 11 Jan 2019 17:58:01 +0100
Eric Auger <eric.auger@redhat.com> wrote:

> We can also use vfio_register_event_notifier() helper in
> vfio_intx_enable_kvm to set the signalling associated to
> VFIO_PCI_INTX_IRQ_INDEX.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/vfio/pci.c | 38 +++++++-------------------------------
>  1 file changed, 7 insertions(+), 31 deletions(-)
> 

>  static void vfio_intx_disable(VFIOPCIDevice *vdev)

I'm wondering why the _disable path can't use the new helper. Ordering
issues?

Re: [Qemu-devel] [PATCH 1/2] vfio-pci: Introduce vfio_register_event_notifier helper

[Auger Eric]

Hi Cornelia,

On 1/15/19 1:03 PM, Cornelia Huck wrote:
> On Fri, 11 Jan 2019 17:58:00 +0100
> Eric Auger <eric.auger@redhat.com> wrote:
> 
>> The code used to attach the eventfd handler for the ERR and
>> REQ irq indices can be factorized into a helper. In subsequent
>> patches we will extend this helper to support other irq indices.
> 
> Looks like a nice refactoring to me.
thank you
> 
>>
>> We test the notification is allowed outside of the helper:
> 
> s/We test/We test whether/
> 
>> respectively check vdev->pci_aer and VFIO_FEATURE_ENABLE_REQ.
>> Depending on the returned value we set vdev->pci_aer and
>> vdev->req_enabled. An error handle is introduced for future usage
>> although not strictly useful here.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/vfio/pci.c | 291 ++++++++++++++++++++++----------------------------
>>  1 file changed, 127 insertions(+), 164 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index c0cb1ec289..c589a4e666 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
>>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>>  }
>>  
>> +/*
>> + * vfio_register_event_notifier - setup/tear down eventfd
>> + * notification and handling for IRQ indices that span over
>> + * a single IRQ
>> + *
>> + * @vdev: VFIO device handle
>> + * @index: IRQ index the eventfd/handler is associated to
> 
> s/to/with/ ?
> 
>> + * @target_state: true means notifier needs to be set up
>> + * @handler to attach if @target_state is true
>> + * @errp error handle
>> + */
>> +static int vfio_register_event_notifier(VFIOPCIDevice *vdev,
>> +                                        int index,
>> +                                        bool target_state,
>> +                                        void (*handler)(void *opaque),
>> +                                        Error **errp)
> 
> (...)
> 
>> @@ -3069,8 +2998,29 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>          goto out_teardown;
>>      }
>>  
>> -    vfio_register_err_notifier(vdev);
>> -    vfio_register_req_notifier(vdev);
>> +    if (vdev->pci_aer) {
>> +        /*
>> +         * Registers error notifier for devices supporting error recovery.
>> +         * If we encounter a failure in this function, we report an error
> 
> s/in this function/while registering it/ ?
> 
>> +         * and continue after disabling error recovery support for the
>> +         * device.
>> +         */
>> +        vdev->pci_aer =
>> +            !vfio_register_event_notifier(vdev, VFIO_PCI_ERR_IRQ_INDEX, true,
>> +                                          vfio_err_notifier_handler, &err);
>> +        if (err) {
>> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> +        }
> 
> I think you need to reset err to NULL if you want to reuse the variable.
Yes you're right. Thanks for spotting this.

> 
> Alternatively, you could keep the wrappers and define a local error
> variable there.
> 
>> +    }
>> +
>> +    if (vdev->features & VFIO_FEATURE_ENABLE_REQ) {
>> +        vdev->req_enabled =
>> +            !vfio_register_event_notifier(vdev, VFIO_PCI_REQ_IRQ_INDEX, true,
>> +                                          vfio_req_notifier_handler, &err);
>> +        if (err) {
>> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> +        }
>> +    }
>>      vfio_setup_resetfn_quirk(vdev);
>>  
>>      return;
>> @@ -3106,9 +3056,22 @@ static void vfio_instance_finalize(Object *obj)
>>  static void vfio_exitfn(PCIDevice *pdev)
>>  {
>>      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
>> +    Error *err = NULL;
>>  
>> -    vfio_unregister_req_notifier(vdev);
>> -    vfio_unregister_err_notifier(vdev);
>> +    if (vdev->req_enabled) {
>> +        vfio_register_event_notifier(vdev, VFIO_PCI_REQ_IRQ_INDEX,
>> +                                     false, NULL, &err);
>> +        if (err) {
>> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> +        }
> 
> Likewise.

Thank you for the review!

Eric

> 
>> +    }
>> +    if (vdev->pci_aer) {
>> +        vfio_register_event_notifier(vdev, VFIO_PCI_ERR_IRQ_INDEX,
>> +                                     false, NULL, &err);
>> +        if (err) {
>> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> +        }
>> +    }
>>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>>      vfio_disable_interrupts(vdev);
>>      if (vdev->intx.mmap_timer) {
> 
> 

Re: [Qemu-devel] [PATCH 2/2] vfio-pci: Use vfio_register_event_notifier in vfio_intx_enable_kvm

[Auger Eric]

Hi Cornelia,

On 1/15/19 1:12 PM, Cornelia Huck wrote:
> On Fri, 11 Jan 2019 17:58:01 +0100
> Eric Auger <eric.auger@redhat.com> wrote:
> 
>> We can also use vfio_register_event_notifier() helper in
>> vfio_intx_enable_kvm to set the signalling associated to
>> VFIO_PCI_INTX_IRQ_INDEX.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/vfio/pci.c | 38 +++++++-------------------------------
>>  1 file changed, 7 insertions(+), 31 deletions(-)
>>
> 
>>  static void vfio_intx_disable(VFIOPCIDevice *vdev)
> 
> I'm wondering why the _disable path can't use the new helper. Ordering
> issues?
> 
Yes the interleaving of actions scared me a little bit. I am going to
study this a little bit further ...

Thanks

Eric

Re: [Qemu-devel] [PATCH 1/2] vfio-pci: Introduce vfio_register_event_notifier helper

[Alexey Kardashevskiy]

On 12/01/2019 03:58, Eric Auger wrote:
> The code used to attach the eventfd handler for the ERR and
> REQ irq indices can be factorized into a helper. In subsequent
> patches we will extend this helper to support other irq indices.
> 
> We test the notification is allowed outside of the helper:
> respectively check vdev->pci_aer and VFIO_FEATURE_ENABLE_REQ.
> Depending on the returned value we set vdev->pci_aer and
> vdev->req_enabled. An error handle is introduced for future usage
> although not strictly useful here.>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/vfio/pci.c | 291 ++++++++++++++++++++++----------------------------
>  1 file changed, 127 insertions(+), 164 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index c0cb1ec289..c589a4e666 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>  }
>  
> +/*
> + * vfio_register_event_notifier - setup/tear down eventfd
> + * notification and handling for IRQ indices that span over
> + * a single IRQ
> + *
> + * @vdev: VFIO device handle
> + * @index: IRQ index the eventfd/handler is associated to
> + * @target_state: true means notifier needs to be set up
> + * @handler to attach if @target_state is true
> + * @errp error handle
> + */
> +static int vfio_register_event_notifier(VFIOPCIDevice *vdev,
> +                                        int index,
> +                                        bool target_state,
> +                                        void (*handler)(void *opaque),
> +                                        Error **errp)
> +{
> +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
> +                                      .index = index };
> +    struct vfio_irq_set *irq_set;
> +    EventNotifier *notifier;
> +    int argsz, ret = 0;
> +    int32_t *pfd, fd;
> +
> +    switch (index) {

I'd pass the notifier as a parameter as well so index/handler/notifier
would walk together.


> +    case VFIO_PCI_REQ_IRQ_INDEX:
> +        notifier = &vdev->req_notifier;
> +        break;
> +    case VFIO_PCI_ERR_IRQ_INDEX:
> +        notifier = &vdev->err_notifier;
> +        break;
> +    default:
> +        return -EINVAL;
> +    }
> +
> +    if (ioctl(vdev->vbasedev.fd,
> +              VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < 1) {
> +        error_setg_errno(errp, errno,
> +                         "no irq index %d available", index);
> +        return -EINVAL;
> +    }
> +
> +    if (target_state) {
> +        ret = event_notifier_init(notifier, 0);
> +        if (ret) {
> +            error_setg_errno(errp, -ret,
> +                             "Unable to init event notifier for irq index %d",
> +                             index);
> +            return ret;
> +        }
> +    }
> +
> +    argsz = sizeof(*irq_set) + sizeof(*pfd);
> +
> +    irq_set = g_malloc0(argsz);
> +    irq_set->argsz = argsz;
> +    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> +                     VFIO_IRQ_SET_ACTION_TRIGGER;
> +    irq_set->index = index;
> +    irq_set->start = 0;
> +    irq_set->count = 1;
> +    pfd = (int32_t *)&irq_set->data;
> +
> +    if (!notifier) {
> +        return -EINVAL;
> +    }
> +
> +    fd = event_notifier_get_fd(notifier);
> +
> +    if (target_state) {
> +        qemu_set_fd_handler(fd, handler, NULL, vdev);
> +        *pfd = fd;
> +    } else {
> +        qemu_set_fd_handler(fd, NULL, NULL, vdev);
> +        event_notifier_cleanup(notifier);
> +        *pfd = -1;
> +    }
> +
> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> +    g_free(irq_set);
> +
> +    if (ret) {
> +        error_setg_errno(errp, -ret,
> +                         "vfio: Failed to %s eventfd signalling for index %d",
> +                         *pfd < 0 ? "set up" : "tear down", index);
> +    }
> +    return ret;
> +}
> +
>  static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
>  {
>  #ifdef CONFIG_KVM
> @@ -2621,86 +2710,6 @@ static void vfio_err_notifier_handler(void *opaque)
>      vm_stop(RUN_STATE_INTERNAL_ERROR);
>  }
>  
> -/*
> - * Registers error notifier for devices supporting error recovery.
> - * If we encounter a failure in this function, we report an error
> - * and continue after disabling error recovery support for the
> - * device.
> - */
> -static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
> -{
> -    int ret;
> -    int argsz;
> -    struct vfio_irq_set *irq_set;
> -    int32_t *pfd;
> -
> -    if (!vdev->pci_aer) {
> -        return;
> -    }
> -
> -    if (event_notifier_init(&vdev->err_notifier, 0)) {
> -        error_report("vfio: Unable to init event notifier for error detection");
> -        vdev->pci_aer = false;
> -        return;
> -    }
> -
> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
> -
> -    irq_set = g_malloc0(argsz);
> -    irq_set->argsz = argsz;
> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
> -    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
> -    irq_set->start = 0;
> -    irq_set->count = 1;
> -    pfd = (int32_t *)&irq_set->data;
> -
> -    *pfd = event_notifier_get_fd(&vdev->err_notifier);
> -    qemu_set_fd_handler(*pfd, vfio_err_notifier_handler, NULL, vdev);
> -
> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> -    if (ret) {
> -        error_report("vfio: Failed to set up error notification");
> -        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
> -        event_notifier_cleanup(&vdev->err_notifier);


qemu_set_fd_handler() and event_notifier_cleanup() have not made it to
the new vfio_register_event_notifier() for the case when
ioctl(VFIO_DEVICE_SET_IRQS) failed.


> -        vdev->pci_aer = false;
> -    }
> -    g_free(irq_set);
> -}
> -
> -static void vfio_unregister_err_notifier(VFIOPCIDevice *vdev)
> -{
> -    int argsz;
> -    struct vfio_irq_set *irq_set;
> -    int32_t *pfd;
> -    int ret;
> -
> -    if (!vdev->pci_aer) {
> -        return;
> -    }
> -
> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
> -
> -    irq_set = g_malloc0(argsz);
> -    irq_set->argsz = argsz;
> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
> -    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
> -    irq_set->start = 0;
> -    irq_set->count = 1;
> -    pfd = (int32_t *)&irq_set->data;
> -    *pfd = -1;
> -
> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> -    if (ret) {
> -        error_report("vfio: Failed to de-assign error fd: %m");
> -    }
> -    g_free(irq_set);
> -    qemu_set_fd_handler(event_notifier_get_fd(&vdev->err_notifier),
> -                        NULL, NULL, vdev);
> -    event_notifier_cleanup(&vdev->err_notifier);
> -}
> -
>  static void vfio_req_notifier_handler(void *opaque)
>  {
>      VFIOPCIDevice *vdev = opaque;
> @@ -2716,86 +2725,6 @@ static void vfio_req_notifier_handler(void *opaque)
>      }
>  }
>  
> -static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
> -{
> -    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
> -                                      .index = VFIO_PCI_REQ_IRQ_INDEX };
> -    int argsz;
> -    struct vfio_irq_set *irq_set;
> -    int32_t *pfd;
> -
> -    if (!(vdev->features & VFIO_FEATURE_ENABLE_REQ)) {
> -        return;
> -    }
> -
> -    if (ioctl(vdev->vbasedev.fd,
> -              VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < 1) {
> -        return;
> -    }
> -
> -    if (event_notifier_init(&vdev->req_notifier, 0)) {
> -        error_report("vfio: Unable to init event notifier for device request");
> -        return;
> -    }
> -
> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
> -
> -    irq_set = g_malloc0(argsz);
> -    irq_set->argsz = argsz;
> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
> -    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
> -    irq_set->start = 0;
> -    irq_set->count = 1;
> -    pfd = (int32_t *)&irq_set->data;
> -
> -    *pfd = event_notifier_get_fd(&vdev->req_notifier);
> -    qemu_set_fd_handler(*pfd, vfio_req_notifier_handler, NULL, vdev);
> -
> -    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
> -        error_report("vfio: Failed to set up device request notification");
> -        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);

Same here.


> -        event_notifier_cleanup(&vdev->req_notifier);
> -    } else {
> -        vdev->req_enabled = true;
> -    }
> -
> -    g_free(irq_set);
> -}
> -
> -static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
> -{
> -    int argsz;
> -    struct vfio_irq_set *irq_set;
> -    int32_t *pfd;
> -
> -    if (!vdev->req_enabled) {
> -        return;
> -    }
> -
> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
> -
> -    irq_set = g_malloc0(argsz);
> -    irq_set->argsz = argsz;
> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
> -    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
> -    irq_set->start = 0;
> -    irq_set->count = 1;
> -    pfd = (int32_t *)&irq_set->data;
> -    *pfd = -1;
> -
> -    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
> -        error_report("vfio: Failed to de-assign device request fd: %m");
> -    }
> -    g_free(irq_set);
> -    qemu_set_fd_handler(event_notifier_get_fd(&vdev->req_notifier),
> -                        NULL, NULL, vdev);
> -    event_notifier_cleanup(&vdev->req_notifier);
> -
> -    vdev->req_enabled = false;
> -}
> -
>  static void vfio_realize(PCIDevice *pdev, Error **errp)
>  {
>      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
> @@ -3069,8 +2998,29 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>          goto out_teardown;
>      }
>  
> -    vfio_register_err_notifier(vdev);
> -    vfio_register_req_notifier(vdev);
> +    if (vdev->pci_aer) {
> +        /*
> +         * Registers error notifier for devices supporting error recovery.
> +         * If we encounter a failure in this function, we report an error
> +         * and continue after disabling error recovery support for the
> +         * device.
> +         */
> +        vdev->pci_aer =
> +            !vfio_register_event_notifier(vdev, VFIO_PCI_ERR_IRQ_INDEX, true,
> +                                          vfio_err_notifier_handler, &err);
> +        if (err) {
> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        }


As the commit log said, these returned @err are not particularly useful
as they are handled exactly the same way and they are not forwarded further.

Also, the new helper returns int which is not tested anywhere and it can
also return with an error and err==NULL so these errors will be silently
skipped. Some consistency here would not hurt :)


> +    }
> +
> +    if (vdev->features & VFIO_FEATURE_ENABLE_REQ) {
> +        vdev->req_enabled =
> +            !vfio_register_event_notifier(vdev, VFIO_PCI_REQ_IRQ_INDEX, true,
> +                                          vfio_req_notifier_handler, &err);
> +        if (err) {
> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        }
> +    }
>      vfio_setup_resetfn_quirk(vdev);
>  
>      return;
> @@ -3106,9 +3056,22 @@ static void vfio_instance_finalize(Object *obj)
>  static void vfio_exitfn(PCIDevice *pdev)
>  {
>      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
> +    Error *err = NULL;
>  
> -    vfio_unregister_req_notifier(vdev);
> -    vfio_unregister_err_notifier(vdev);
> +    if (vdev->req_enabled) {
> +        vfio_register_event_notifier(vdev, VFIO_PCI_REQ_IRQ_INDEX,
> +                                     false, NULL, &err);
> +        if (err) {
> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        }
> +    }
> +    if (vdev->pci_aer) {
> +        vfio_register_event_notifier(vdev, VFIO_PCI_ERR_IRQ_INDEX,
> +                                     false, NULL, &err);
> +        if (err) {
> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        }
> +    }
>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>      vfio_disable_interrupts(vdev);
>      if (vdev->intx.mmap_timer) {
> 

-- 
Alexey

Re: [Qemu-devel] [PATCH 2/2] vfio-pci: Use vfio_register_event_notifier in vfio_intx_enable_kvm

[Alexey Kardashevskiy]

On 12/01/2019 03:58, Eric Auger wrote:
> We can also use vfio_register_event_notifier() helper in
> vfio_intx_enable_kvm to set the signalling associated to
> VFIO_PCI_INTX_IRQ_INDEX.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/vfio/pci.c | 38 +++++++-------------------------------
>  1 file changed, 7 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index c589a4e666..db0504ca10 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -136,6 +136,9 @@ static int vfio_register_event_notifier(VFIOPCIDevice *vdev,
>      case VFIO_PCI_ERR_IRQ_INDEX:
>          notifier = &vdev->err_notifier;
>          break;
> +    case VFIO_PCI_INTX_IRQ_INDEX:
> +        notifier = &vdev->intx.interrupt;
> +        break;
>      default:
>          return -EINVAL;
>      }
> @@ -351,10 +354,8 @@ static void vfio_intx_update(PCIDevice *pdev)
>  static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
>  {
>      uint8_t pin = vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1);
> -    int ret, argsz, retval = 0;
> -    struct vfio_irq_set *irq_set;
> -    int32_t *pfd;
>      Error *err = NULL;
> +    int ret;
>  
>      if (!pin) {
>          return 0;
> @@ -376,34 +377,12 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
>      }
>  #endif
>  
> -    ret = event_notifier_init(&vdev->intx.interrupt, 0);
> +    ret = vfio_register_event_notifier(vdev, VFIO_PCI_INTX_IRQ_INDEX, true,
> +                                       vfio_intx_interrupt, errp);
>      if (ret) {
> -        error_setg_errno(errp, -ret, "event_notifier_init failed");
>          return ret;
>      }
>  
> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
> -
> -    irq_set = g_malloc0(argsz);
> -    irq_set->argsz = argsz;
> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER;
> -    irq_set->index = VFIO_PCI_INTX_IRQ_INDEX;
> -    irq_set->start = 0;
> -    irq_set->count = 1;
> -    pfd = (int32_t *)&irq_set->data;
> -
> -    *pfd = event_notifier_get_fd(&vdev->intx.interrupt);
> -    qemu_set_fd_handler(*pfd, vfio_intx_interrupt, NULL, vdev);
> -
> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> -    if (ret) {
> -        error_setg_errno(errp, -ret, "failed to setup INTx fd");
> -        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
> -        event_notifier_cleanup(&vdev->intx.interrupt);


After this change if ioctl(VFIO_DEVICE_SET_IRQS) failed, there will be
no event_notifier_cleanup() called as this new
vfio_register_event_notifier() does not do this either.


> -        retval = -errno;
> -        goto cleanup;
> -    }
> -
>      vfio_intx_enable_kvm(vdev, &err);
>      if (err) {
>          warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> @@ -413,10 +392,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
>  
>      trace_vfio_intx_enable(vdev->vbasedev.name);
>  
> -cleanup:
> -    g_free(irq_set);
> -
> -    return retval;
> +    return 0;
>  }
>  
>  static void vfio_intx_disable(VFIOPCIDevice *vdev)
> 

-- 
Alexey

Re: [Qemu-devel] [PATCH 1/2] vfio-pci: Introduce vfio_register_event_notifier helper

[Cornelia Huck]

On Thu, 17 Jan 2019 14:46:42 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 12/01/2019 03:58, Eric Auger wrote:
> > The code used to attach the eventfd handler for the ERR and
> > REQ irq indices can be factorized into a helper. In subsequent
> > patches we will extend this helper to support other irq indices.
> > 
> > We test the notification is allowed outside of the helper:
> > respectively check vdev->pci_aer and VFIO_FEATURE_ENABLE_REQ.
> > Depending on the returned value we set vdev->pci_aer and
> > vdev->req_enabled. An error handle is introduced for future usage
> > although not strictly useful here.>
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> > ---
> >  hw/vfio/pci.c | 291 ++++++++++++++++++++++----------------------------
> >  1 file changed, 127 insertions(+), 164 deletions(-)
> > 
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index c0cb1ec289..c589a4e666 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
> >      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
> >  }
> >  
> > +/*
> > + * vfio_register_event_notifier - setup/tear down eventfd
> > + * notification and handling for IRQ indices that span over
> > + * a single IRQ
> > + *
> > + * @vdev: VFIO device handle
> > + * @index: IRQ index the eventfd/handler is associated to
> > + * @target_state: true means notifier needs to be set up
> > + * @handler to attach if @target_state is true
> > + * @errp error handle
> > + */
> > +static int vfio_register_event_notifier(VFIOPCIDevice *vdev,
> > +                                        int index,
> > +                                        bool target_state,
> > +                                        void (*handler)(void *opaque),
> > +                                        Error **errp)
> > +{
> > +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
> > +                                      .index = index };
> > +    struct vfio_irq_set *irq_set;
> > +    EventNotifier *notifier;
> > +    int argsz, ret = 0;
> > +    int32_t *pfd, fd;
> > +
> > +    switch (index) {  
> 
> I'd pass the notifier as a parameter as well so index/handler/notifier
> would walk together.

I first had the same thought, but I actually like that everything is
together in this switch statement.

Alternatively, pack it together in an indexed array? But that gets a
bit awkward with the intx stuff.

> 
> 
> > +    case VFIO_PCI_REQ_IRQ_INDEX:
> > +        notifier = &vdev->req_notifier;
> > +        break;
> > +    case VFIO_PCI_ERR_IRQ_INDEX:
> > +        notifier = &vdev->err_notifier;
> > +        break;
> > +    default:
> > +        return -EINVAL;
> > +    }

Re: [Qemu-devel] [PATCH 1/2] vfio-pci: Introduce vfio_register_event_notifier helper

[Auger Eric]

Hi Alexey, Cornelia,

On 1/17/19 4:46 AM, Alexey Kardashevskiy wrote:
> 
> 
> On 12/01/2019 03:58, Eric Auger wrote:
>> The code used to attach the eventfd handler for the ERR and
>> REQ irq indices can be factorized into a helper. In subsequent
>> patches we will extend this helper to support other irq indices.
>>
>> We test the notification is allowed outside of the helper:
>> respectively check vdev->pci_aer and VFIO_FEATURE_ENABLE_REQ.
>> Depending on the returned value we set vdev->pci_aer and
>> vdev->req_enabled. An error handle is introduced for future usage
>> although not strictly useful here.>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/vfio/pci.c | 291 ++++++++++++++++++++++----------------------------
>>  1 file changed, 127 insertions(+), 164 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index c0cb1ec289..c589a4e666 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
>>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>>  }
>>  
>> +/*
>> + * vfio_register_event_notifier - setup/tear down eventfd
>> + * notification and handling for IRQ indices that span over
>> + * a single IRQ
>> + *
>> + * @vdev: VFIO device handle
>> + * @index: IRQ index the eventfd/handler is associated to
>> + * @target_state: true means notifier needs to be set up
>> + * @handler to attach if @target_state is true
>> + * @errp error handle
>> + */
>> +static int vfio_register_event_notifier(VFIOPCIDevice *vdev,
>> +                                        int index,
>> +                                        bool target_state,
>> +                                        void (*handler)(void *opaque),
>> +                                        Error **errp)
>> +{
>> +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
>> +                                      .index = index };
>> +    struct vfio_irq_set *irq_set;
>> +    EventNotifier *notifier;
>> +    int argsz, ret = 0;
>> +    int32_t *pfd, fd;
>> +
>> +    switch (index) {
> 
> I'd pass the notifier as a parameter as well so index/handler/notifier
> would walk together.

I tend to agree with Cornelia. moving the notifier out of this helper
would remove some factorization and this way, the caller does not have
to care about it.
> 
> 
>> +    case VFIO_PCI_REQ_IRQ_INDEX:
>> +        notifier = &vdev->req_notifier;
>> +        break;
>> +    case VFIO_PCI_ERR_IRQ_INDEX:
>> +        notifier = &vdev->err_notifier;
>> +        break;
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (ioctl(vdev->vbasedev.fd,
>> +              VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < 1) {
>> +        error_setg_errno(errp, errno,
>> +                         "no irq index %d available", index);
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (target_state) {
>> +        ret = event_notifier_init(notifier, 0);
>> +        if (ret) {
>> +            error_setg_errno(errp, -ret,
>> +                             "Unable to init event notifier for irq index %d",
>> +                             index);
>> +            return ret;
>> +        }
>> +    }
>> +
>> +    argsz = sizeof(*irq_set) + sizeof(*pfd);
>> +
>> +    irq_set = g_malloc0(argsz);
>> +    irq_set->argsz = argsz;
>> +    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>> +                     VFIO_IRQ_SET_ACTION_TRIGGER;
>> +    irq_set->index = index;
>> +    irq_set->start = 0;
>> +    irq_set->count = 1;
>> +    pfd = (int32_t *)&irq_set->data;
>> +
>> +    if (!notifier) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    fd = event_notifier_get_fd(notifier);
>> +
>> +    if (target_state) {
>> +        qemu_set_fd_handler(fd, handler, NULL, vdev);
>> +        *pfd = fd;
>> +    } else {
>> +        qemu_set_fd_handler(fd, NULL, NULL, vdev);
>> +        event_notifier_cleanup(notifier);
>> +        *pfd = -1;
>> +    }
>> +
>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> +    g_free(irq_set);
>> +
>> +    if (ret) {
>> +        error_setg_errno(errp, -ret,
>> +                         "vfio: Failed to %s eventfd signalling for index %d",
>> +                         *pfd < 0 ? "set up" : "tear down", index);
>> +    }
>> +    return ret;
>> +}
>> +
>>  static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
>>  {
>>  #ifdef CONFIG_KVM
>> @@ -2621,86 +2710,6 @@ static void vfio_err_notifier_handler(void *opaque)
>>      vm_stop(RUN_STATE_INTERNAL_ERROR);
>>  }
>>  
>> -/*
>> - * Registers error notifier for devices supporting error recovery.
>> - * If we encounter a failure in this function, we report an error
>> - * and continue after disabling error recovery support for the
>> - * device.
>> - */
>> -static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
>> -{
>> -    int ret;
>> -    int argsz;
>> -    struct vfio_irq_set *irq_set;
>> -    int32_t *pfd;
>> -
>> -    if (!vdev->pci_aer) {
>> -        return;
>> -    }
>> -
>> -    if (event_notifier_init(&vdev->err_notifier, 0)) {
>> -        error_report("vfio: Unable to init event notifier for error detection");
>> -        vdev->pci_aer = false;
>> -        return;
>> -    }
>> -
>> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
>> -
>> -    irq_set = g_malloc0(argsz);
>> -    irq_set->argsz = argsz;
>> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
>> -    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
>> -    irq_set->start = 0;
>> -    irq_set->count = 1;
>> -    pfd = (int32_t *)&irq_set->data;
>> -
>> -    *pfd = event_notifier_get_fd(&vdev->err_notifier);
>> -    qemu_set_fd_handler(*pfd, vfio_err_notifier_handler, NULL, vdev);
>> -
>> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> -    if (ret) {
>> -        error_report("vfio: Failed to set up error notification");
>> -        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
>> -        event_notifier_cleanup(&vdev->err_notifier);
> 
> 
> qemu_set_fd_handler() and event_notifier_cleanup() have not made it to
> the new vfio_register_event_notifier() for the case when
> ioctl(VFIO_DEVICE_SET_IRQS) failed.

definitively missing. Thanks!
> 
> 
>> -        vdev->pci_aer = false;
>> -    }
>> -    g_free(irq_set);
>> -}
>> -
>> -static void vfio_unregister_err_notifier(VFIOPCIDevice *vdev)
>> -{
>> -    int argsz;
>> -    struct vfio_irq_set *irq_set;
>> -    int32_t *pfd;
>> -    int ret;
>> -
>> -    if (!vdev->pci_aer) {
>> -        return;
>> -    }
>> -
>> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
>> -
>> -    irq_set = g_malloc0(argsz);
>> -    irq_set->argsz = argsz;
>> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
>> -    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
>> -    irq_set->start = 0;
>> -    irq_set->count = 1;
>> -    pfd = (int32_t *)&irq_set->data;
>> -    *pfd = -1;
>> -
>> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> -    if (ret) {
>> -        error_report("vfio: Failed to de-assign error fd: %m");
>> -    }
>> -    g_free(irq_set);
>> -    qemu_set_fd_handler(event_notifier_get_fd(&vdev->err_notifier),
>> -                        NULL, NULL, vdev);
>> -    event_notifier_cleanup(&vdev->err_notifier);
>> -}
>> -
>>  static void vfio_req_notifier_handler(void *opaque)
>>  {
>>      VFIOPCIDevice *vdev = opaque;
>> @@ -2716,86 +2725,6 @@ static void vfio_req_notifier_handler(void *opaque)
>>      }
>>  }
>>  
>> -static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
>> -{
>> -    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
>> -                                      .index = VFIO_PCI_REQ_IRQ_INDEX };
>> -    int argsz;
>> -    struct vfio_irq_set *irq_set;
>> -    int32_t *pfd;
>> -
>> -    if (!(vdev->features & VFIO_FEATURE_ENABLE_REQ)) {
>> -        return;
>> -    }
>> -
>> -    if (ioctl(vdev->vbasedev.fd,
>> -              VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < 1) {
>> -        return;
>> -    }
>> -
>> -    if (event_notifier_init(&vdev->req_notifier, 0)) {
>> -        error_report("vfio: Unable to init event notifier for device request");
>> -        return;
>> -    }
>> -
>> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
>> -
>> -    irq_set = g_malloc0(argsz);
>> -    irq_set->argsz = argsz;
>> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
>> -    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
>> -    irq_set->start = 0;
>> -    irq_set->count = 1;
>> -    pfd = (int32_t *)&irq_set->data;
>> -
>> -    *pfd = event_notifier_get_fd(&vdev->req_notifier);
>> -    qemu_set_fd_handler(*pfd, vfio_req_notifier_handler, NULL, vdev);
>> -
>> -    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
>> -        error_report("vfio: Failed to set up device request notification");
>> -        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
> 
> Same here.
> 
> 
>> -        event_notifier_cleanup(&vdev->req_notifier);
>> -    } else {
>> -        vdev->req_enabled = true;
>> -    }
>> -
>> -    g_free(irq_set);
>> -}
>> -
>> -static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>> -{
>> -    int argsz;
>> -    struct vfio_irq_set *irq_set;
>> -    int32_t *pfd;
>> -
>> -    if (!vdev->req_enabled) {
>> -        return;
>> -    }
>> -
>> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
>> -
>> -    irq_set = g_malloc0(argsz);
>> -    irq_set->argsz = argsz;
>> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
>> -    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
>> -    irq_set->start = 0;
>> -    irq_set->count = 1;
>> -    pfd = (int32_t *)&irq_set->data;
>> -    *pfd = -1;
>> -
>> -    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
>> -        error_report("vfio: Failed to de-assign device request fd: %m");
>> -    }
>> -    g_free(irq_set);
>> -    qemu_set_fd_handler(event_notifier_get_fd(&vdev->req_notifier),
>> -                        NULL, NULL, vdev);
>> -    event_notifier_cleanup(&vdev->req_notifier);
>> -
>> -    vdev->req_enabled = false;
>> -}
>> -
>>  static void vfio_realize(PCIDevice *pdev, Error **errp)
>>  {
>>      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
>> @@ -3069,8 +2998,29 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>          goto out_teardown;
>>      }
>>  
>> -    vfio_register_err_notifier(vdev);
>> -    vfio_register_req_notifier(vdev);
>> +    if (vdev->pci_aer) {
>> +        /*
>> +         * Registers error notifier for devices supporting error recovery.
>> +         * If we encounter a failure in this function, we report an error
>> +         * and continue after disabling error recovery support for the
>> +         * device.
>> +         */
>> +        vdev->pci_aer =
>> +            !vfio_register_event_notifier(vdev, VFIO_PCI_ERR_IRQ_INDEX, true,
>> +                                          vfio_err_notifier_handler, &err);
>> +        if (err) {
>> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> +        }
> 
> 
> As the commit log said, these returned @err are not particularly useful
> as they are handled exactly the same way and they are not forwarded further.
Yes that's awkward but the issue is vfio_intx_enable() propagates the error.
> 
> Also, the new helper returns int which is not tested anywhere and it can
> also return with an error and err==NULL so these errors will be silently
> skipped. Some consistency here would not hurt :)
Yes I think I will make it a void function and the caller will only test
err.

Thank you for the review!

Eric
> 
> 
>> +    }
>> +
>> +    if (vdev->features & VFIO_FEATURE_ENABLE_REQ) {
>> +        vdev->req_enabled =
>> +            !vfio_register_event_notifier(vdev, VFIO_PCI_REQ_IRQ_INDEX, true,
>> +                                          vfio_req_notifier_handler, &err);
>> +        if (err) {
>> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> +        }
>> +    }
>>      vfio_setup_resetfn_quirk(vdev);
>>  
>>      return;
>> @@ -3106,9 +3056,22 @@ static void vfio_instance_finalize(Object *obj)
>>  static void vfio_exitfn(PCIDevice *pdev)
>>  {
>>      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
>> +    Error *err = NULL;
>>  
>> -    vfio_unregister_req_notifier(vdev);
>> -    vfio_unregister_err_notifier(vdev);
>> +    if (vdev->req_enabled) {
>> +        vfio_register_event_notifier(vdev, VFIO_PCI_REQ_IRQ_INDEX,
>> +                                     false, NULL, &err);
>> +        if (err) {
>> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> +        }
>> +    }
>> +    if (vdev->pci_aer) {
>> +        vfio_register_event_notifier(vdev, VFIO_PCI_ERR_IRQ_INDEX,
>> +                                     false, NULL, &err);
>> +        if (err) {
>> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> +        }
>> +    }
>>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>>      vfio_disable_interrupts(vdev);
>>      if (vdev->intx.mmap_timer) {
>>
> 

Re: [Qemu-devel] [PATCH 1/2] vfio-pci: Introduce vfio_register_event_notifier helper

[Alexey Kardashevskiy]

On 17/01/2019 20:16, Auger Eric wrote:
> Hi Alexey, Cornelia,
> 
> On 1/17/19 4:46 AM, Alexey Kardashevskiy wrote:
>>
>>
>> On 12/01/2019 03:58, Eric Auger wrote:
>>> The code used to attach the eventfd handler for the ERR and
>>> REQ irq indices can be factorized into a helper. In subsequent
>>> patches we will extend this helper to support other irq indices.
>>>
>>> We test the notification is allowed outside of the helper:
>>> respectively check vdev->pci_aer and VFIO_FEATURE_ENABLE_REQ.
>>> Depending on the returned value we set vdev->pci_aer and
>>> vdev->req_enabled. An error handle is introduced for future usage
>>> although not strictly useful here.>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>>  hw/vfio/pci.c | 291 ++++++++++++++++++++++----------------------------
>>>  1 file changed, 127 insertions(+), 164 deletions(-)
>>>
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index c0cb1ec289..c589a4e666 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
>>>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>>>  }
>>>  
>>> +/*
>>> + * vfio_register_event_notifier - setup/tear down eventfd
>>> + * notification and handling for IRQ indices that span over
>>> + * a single IRQ
>>> + *
>>> + * @vdev: VFIO device handle
>>> + * @index: IRQ index the eventfd/handler is associated to
>>> + * @target_state: true means notifier needs to be set up
>>> + * @handler to attach if @target_state is true
>>> + * @errp error handle
>>> + */
>>> +static int vfio_register_event_notifier(VFIOPCIDevice *vdev,
>>> +                                        int index,
>>> +                                        bool target_state,
>>> +                                        void (*handler)(void *opaque),
>>> +                                        Error **errp)
>>> +{
>>> +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
>>> +                                      .index = index };
>>> +    struct vfio_irq_set *irq_set;
>>> +    EventNotifier *notifier;
>>> +    int argsz, ret = 0;
>>> +    int32_t *pfd, fd;
>>> +
>>> +    switch (index) {
>>
>> I'd pass the notifier as a parameter as well so index/handler/notifier
>> would walk together.
> 
> I tend to agree with Cornelia. moving the notifier out of this helper
> would remove some factorization and this way, the caller does not have
> to care about it.


Then why pass the handler? It also could go into this switch,
vfio_register_event_notifier()/vfio_set_event_handler() is never called
with more than one handler per index (or NULL but then target_state==false).


-- 
Alexey

Re: [Qemu-devel] [PATCH 1/2] vfio-pci: Introduce vfio_register_event_notifier helper

[Auger Eric]

Hi Alexey,

On 1/18/19 5:14 AM, Alexey Kardashevskiy wrote:
> 
> 
> On 17/01/2019 20:16, Auger Eric wrote:
>> Hi Alexey, Cornelia,
>>
>> On 1/17/19 4:46 AM, Alexey Kardashevskiy wrote:
>>>
>>>
>>> On 12/01/2019 03:58, Eric Auger wrote:
>>>> The code used to attach the eventfd handler for the ERR and
>>>> REQ irq indices can be factorized into a helper. In subsequent
>>>> patches we will extend this helper to support other irq indices.
>>>>
>>>> We test the notification is allowed outside of the helper:
>>>> respectively check vdev->pci_aer and VFIO_FEATURE_ENABLE_REQ.
>>>> Depending on the returned value we set vdev->pci_aer and
>>>> vdev->req_enabled. An error handle is introduced for future usage
>>>> although not strictly useful here.>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>>  hw/vfio/pci.c | 291 ++++++++++++++++++++++----------------------------
>>>>  1 file changed, 127 insertions(+), 164 deletions(-)
>>>>
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index c0cb1ec289..c589a4e666 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
>>>>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>>>>  }
>>>>  
>>>> +/*
>>>> + * vfio_register_event_notifier - setup/tear down eventfd
>>>> + * notification and handling for IRQ indices that span over
>>>> + * a single IRQ
>>>> + *
>>>> + * @vdev: VFIO device handle
>>>> + * @index: IRQ index the eventfd/handler is associated to
>>>> + * @target_state: true means notifier needs to be set up
>>>> + * @handler to attach if @target_state is true
>>>> + * @errp error handle
>>>> + */
>>>> +static int vfio_register_event_notifier(VFIOPCIDevice *vdev,
>>>> +                                        int index,
>>>> +                                        bool target_state,
>>>> +                                        void (*handler)(void *opaque),
>>>> +                                        Error **errp)
>>>> +{
>>>> +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
>>>> +                                      .index = index };
>>>> +    struct vfio_irq_set *irq_set;
>>>> +    EventNotifier *notifier;
>>>> +    int argsz, ret = 0;
>>>> +    int32_t *pfd, fd;
>>>> +
>>>> +    switch (index) {
>>>
>>> I'd pass the notifier as a parameter as well so index/handler/notifier
>>> would walk together.
>>
>> I tend to agree with Cornelia. moving the notifier out of this helper
>> would remove some factorization and this way, the caller does not have
>> to care about it.
> 
> 
> Then why pass the handler? It also could go into this switch,
> vfio_register_event_notifier()/vfio_set_event_handler() is never called
> with more than one handler per index (or NULL but then target_state==false).
I don't have any strong opinion here. I will align with the majority's
opinion.

Thanks

Eric
> 
> 

Re: [Qemu-devel] [PATCH 1/2] vfio-pci: Introduce vfio_register_event_notifier helper

[Cornelia Huck]

On Fri, 18 Jan 2019 10:08:12 +0100
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Alexey,
> 
> On 1/18/19 5:14 AM, Alexey Kardashevskiy wrote:
> > 
> > 
> > On 17/01/2019 20:16, Auger Eric wrote:  
> >> Hi Alexey, Cornelia,
> >>
> >> On 1/17/19 4:46 AM, Alexey Kardashevskiy wrote:  
> >>>
> >>>
> >>> On 12/01/2019 03:58, Eric Auger wrote:  
> >>>> The code used to attach the eventfd handler for the ERR and
> >>>> REQ irq indices can be factorized into a helper. In subsequent
> >>>> patches we will extend this helper to support other irq indices.
> >>>>
> >>>> We test the notification is allowed outside of the helper:
> >>>> respectively check vdev->pci_aer and VFIO_FEATURE_ENABLE_REQ.
> >>>> Depending on the returned value we set vdev->pci_aer and
> >>>> vdev->req_enabled. An error handle is introduced for future usage
> >>>> although not strictly useful here.>
> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>>> ---
> >>>>  hw/vfio/pci.c | 291 ++++++++++++++++++++++----------------------------
> >>>>  1 file changed, 127 insertions(+), 164 deletions(-)
> >>>>
> >>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >>>> index c0cb1ec289..c589a4e666 100644
> >>>> --- a/hw/vfio/pci.c
> >>>> +++ b/hw/vfio/pci.c
> >>>> @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
> >>>>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
> >>>>  }
> >>>>  
> >>>> +/*
> >>>> + * vfio_register_event_notifier - setup/tear down eventfd
> >>>> + * notification and handling for IRQ indices that span over
> >>>> + * a single IRQ
> >>>> + *
> >>>> + * @vdev: VFIO device handle
> >>>> + * @index: IRQ index the eventfd/handler is associated to
> >>>> + * @target_state: true means notifier needs to be set up
> >>>> + * @handler to attach if @target_state is true
> >>>> + * @errp error handle
> >>>> + */
> >>>> +static int vfio_register_event_notifier(VFIOPCIDevice *vdev,
> >>>> +                                        int index,
> >>>> +                                        bool target_state,
> >>>> +                                        void (*handler)(void *opaque),
> >>>> +                                        Error **errp)
> >>>> +{
> >>>> +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
> >>>> +                                      .index = index };
> >>>> +    struct vfio_irq_set *irq_set;
> >>>> +    EventNotifier *notifier;
> >>>> +    int argsz, ret = 0;
> >>>> +    int32_t *pfd, fd;
> >>>> +
> >>>> +    switch (index) {  
> >>>
> >>> I'd pass the notifier as a parameter as well so index/handler/notifier
> >>> would walk together.  
> >>
> >> I tend to agree with Cornelia. moving the notifier out of this helper
> >> would remove some factorization and this way, the caller does not have
> >> to care about it.  
> > 
> > 
> > Then why pass the handler? It also could go into this switch,
> > vfio_register_event_notifier()/vfio_set_event_handler() is never called
> > with more than one handler per index (or NULL but then target_state==false).  
> I don't have any strong opinion here. I will align with the majority's
> opinion.

If we are sure that the same index/notifier will always use the same
handler when setting (i.e., not a different handler if some feature is
available), we could also move getting it to the switch statement.
OTOH, passing in a handler is a common pattern. Don't really have a
strong opinion here.