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

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

[Eric Auger]

This small series introduces the vfio_set_event_handler()
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_set_event_handler helper
  vfio-pci: Use vfio_set_event_handler in vfio_intx_enable

 hw/vfio/pci.c | 347 +++++++++++++++++++++-----------------------------
 1 file changed, 145 insertions(+), 202 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v2 1/2] vfio-pci: Introduce vfio_set_event_handler 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 whether 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>

---

v1 -> v2:
- s/vfio_register_event_notifier/vfio_set_event_handler
- turned into a void function
- do the qemu_set_fd_handler(*pfd, NULL, NULL, vdev) and
  event_notifier_cleanup on VFIO_DEVICE_SET_IRQS failure
  as reported by Alexey
- reset err in vfio_realize as reported by Cornelia
- Text/comment fixes suggested by Cornelia
---
 hw/vfio/pci.c | 296 ++++++++++++++++++++++----------------------------
 1 file changed, 132 insertions(+), 164 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index c0cb1ec289..3cae4c99ef 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -105,6 +105,96 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
     vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
 }
 
+/*
+ * vfio_set_event_handler - 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 with
+ * @enable: true means notifier chain needs to be set up
+ * @handler: handler to attach if @enable is true
+ * @errp: error handle
+ */
+static void vfio_set_event_handler(VFIOPCIDevice *vdev,
+                                   int index,
+                                   bool enable,
+                                   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:
+        g_assert_not_reached();
+    }
+
+    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;
+    }
+
+    if (enable) {
+        ret = event_notifier_init(notifier, 0);
+        if (ret) {
+            error_setg_errno(errp, -ret,
+                             "Unable to init event notifier for irq index %d",
+                             index);
+            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 = index;
+    irq_set->start = 0;
+    irq_set->count = 1;
+    pfd = (int32_t *)&irq_set->data;
+
+    if (!notifier) {
+        error_setg(errp, "Notifier not initialized for irq index %d", index);
+        return;
+    }
+
+    fd = event_notifier_get_fd(notifier);
+
+    if (enable) {
+        qemu_set_fd_handler(fd, handler, NULL, vdev);
+        *pfd = fd;
+    } else {
+        *pfd = -1;
+    }
+
+    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
+    g_free(irq_set);
+
+    if (ret) {
+        error_setg_errno(errp, -ret,
+                         "Failed to %s eventfd signalling for index %d",
+                         enable ? "set up" : "tear down", index);
+    }
+    if (ret || !enable) {
+        qemu_set_fd_handler(fd, NULL, NULL, vdev);
+        event_notifier_cleanup(notifier);
+    }
+}
+
 static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
 {
 #ifdef CONFIG_KVM
@@ -2621,86 +2711,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 +2726,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 +2999,33 @@ 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) {
+        Error *err = NULL;
+
+        /*
+         * Registers error notifier for devices supporting error recovery.
+         * If we encounter a failure during registration, we report an error
+         * and continue after disabling error recovery support for the
+         * device.
+         */
+        vfio_set_event_handler(vdev, VFIO_PCI_ERR_IRQ_INDEX, true,
+                               vfio_err_notifier_handler, &err);
+        if (err) {
+            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+        }
+        vdev->pci_aer = !err;
+    }
+
+    if (vdev->features & VFIO_FEATURE_ENABLE_REQ) {
+        Error *err = NULL;
+
+        vfio_set_event_handler(vdev, VFIO_PCI_REQ_IRQ_INDEX, true,
+                               vfio_req_notifier_handler, &err);
+        if (err) {
+            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+        }
+        vdev->req_enabled = !err;
+    }
     vfio_setup_resetfn_quirk(vdev);
 
     return;
@@ -3106,9 +3061,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_set_event_handler(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_set_event_handler(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.20.1

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

[Alex Williamson]

On Thu, 17 Jan 2019 22:06:31 +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.
> 
> We test whether 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>
> 
> ---
> 
> v1 -> v2:
> - s/vfio_register_event_notifier/vfio_set_event_handler
> - turned into a void function
> - do the qemu_set_fd_handler(*pfd, NULL, NULL, vdev) and
>   event_notifier_cleanup on VFIO_DEVICE_SET_IRQS failure
>   as reported by Alexey
> - reset err in vfio_realize as reported by Cornelia
> - Text/comment fixes suggested by Cornelia
> ---
>  hw/vfio/pci.c | 296 ++++++++++++++++++++++----------------------------
>  1 file changed, 132 insertions(+), 164 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index c0cb1ec289..3cae4c99ef 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -105,6 +105,96 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>  }
>  
> +/*
> + * vfio_set_event_handler - 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 with
> + * @enable: true means notifier chain needs to be set up
> + * @handler: handler to attach if @enable is true

Therefore @enable is redundant.

> + * @errp: error handle
> + */
> +static void vfio_set_event_handler(VFIOPCIDevice *vdev,
> +                                   int index,
> +                                   bool enable,
> +                                   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;

This blows the abstraction of a helper that this seems to be trying to
create, seems cleaner to pass @notifier.

> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    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;

The implicit count, aka sub-index, is also problematic for the
abstraction.  Can we tackle applying this to MSI/X to validate if this
needs to go another step to allow the caller to specify index and
sub-index?

> +    }
> +
> +    if (enable) {
> +        ret = event_notifier_init(notifier, 0);
> +        if (ret) {
> +            error_setg_errno(errp, -ret,
> +                             "Unable to init event notifier for irq index %d",
> +                             index);
> +            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 = index;
> +    irq_set->start = 0;
> +    irq_set->count = 1;
> +    pfd = (int32_t *)&irq_set->data;
> +
> +    if (!notifier) {
> +        error_setg(errp, "Notifier not initialized for irq index %d", index);
> +        return;
> +    }

What is this supposed to check?  @notifier is not NULL initialized, the
case statement will assert if it doesn't get set, and this doesn't
actually test if it's properly initialized.

> +
> +    fd = event_notifier_get_fd(notifier);
> +
> +    if (enable) {
> +        qemu_set_fd_handler(fd, handler, NULL, vdev);
> +        *pfd = fd;
> +    } else {
> +        *pfd = -1;
> +    }
> +
> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> +    g_free(irq_set);
> +
> +    if (ret) {
> +        error_setg_errno(errp, -ret,
> +                         "Failed to %s eventfd signalling for index %d",
> +                         enable ? "set up" : "tear down", index);
> +    }
> +    if (ret || !enable) {
> +        qemu_set_fd_handler(fd, NULL, NULL, vdev);
> +        event_notifier_cleanup(notifier);
> +    }
> +}

Suggest passing @notifier as a parameter and using @handler in place of
@enable, more generic and more obvious calling convention.

> +
>  static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
>  {
>  #ifdef CONFIG_KVM
> @@ -2621,86 +2711,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 +2726,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 +2999,33 @@ 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) {
> +        Error *err = NULL;
> +
> +        /*
> +         * Registers error notifier for devices supporting error recovery.
> +         * If we encounter a failure during registration, we report an error
> +         * and continue after disabling error recovery support for the
> +         * device.
> +         */
> +        vfio_set_event_handler(vdev, VFIO_PCI_ERR_IRQ_INDEX, true,
> +                               vfio_err_notifier_handler, &err);
> +        if (err) {
> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        }

Why not just return -1 on error and zero on success so we can call as:

    if (vfio_set_event_handler(...)) {
        warn_reportf_err()...
    }

> +        vdev->pci_aer = !err;

We could also avoid this weirdness of this negation to get a bool.

> +    }

It's not obvious how doing away with the register/unregister helpers
and doing everything inline is an improvement.  Simple helpers calling
common helpers seems better than inline sprawl calling common helpers.
Thanks,

Alex

> +
> +    if (vdev->features & VFIO_FEATURE_ENABLE_REQ) {
> +        Error *err = NULL;
> +
> +        vfio_set_event_handler(vdev, VFIO_PCI_REQ_IRQ_INDEX, true,
> +                               vfio_req_notifier_handler, &err);
> +        if (err) {
> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        }
> +        vdev->req_enabled = !err;
> +    }
>      vfio_setup_resetfn_quirk(vdev);
>  
>      return;
> @@ -3106,9 +3061,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_set_event_handler(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_set_event_handler(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 v2 1/2] vfio-pci: Introduce vfio_set_event_handler helper

[Auger Eric]

Hi Alex,

On 1/22/19 8:51 PM, Alex Williamson wrote:
> On Thu, 17 Jan 2019 22:06:31 +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.
>>
>> We test whether 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>
>>
>> ---
>>
>> v1 -> v2:
>> - s/vfio_register_event_notifier/vfio_set_event_handler
>> - turned into a void function
>> - do the qemu_set_fd_handler(*pfd, NULL, NULL, vdev) and
>>   event_notifier_cleanup on VFIO_DEVICE_SET_IRQS failure
>>   as reported by Alexey
>> - reset err in vfio_realize as reported by Cornelia
>> - Text/comment fixes suggested by Cornelia
>> ---
>>  hw/vfio/pci.c | 296 ++++++++++++++++++++++----------------------------
>>  1 file changed, 132 insertions(+), 164 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index c0cb1ec289..3cae4c99ef 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -105,6 +105,96 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
>>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>>  }
>>  
>> +/*
>> + * vfio_set_event_handler - 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 with
>> + * @enable: true means notifier chain needs to be set up
>> + * @handler: handler to attach if @enable is true
> 
> Therefore @enable is redundant.
agreed
> 
>> + * @errp: error handle
>> + */
>> +static void vfio_set_event_handler(VFIOPCIDevice *vdev,
>> +                                   int index,
>> +                                   bool enable,
>> +                                   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;
> 
> This blows the abstraction of a helper that this seems to be trying to
> create, seems cleaner to pass @notifier.


Not sure I really get the point eventually. Don't we have the following
indirection: irq index -> notifier.fd -> handler. When we want to use
this helper don't we simply want to associate an handler to a given IRQ
index without taking care of the notifier mechanics.

As discussed with Alexey, moving the notifier initialization outside of
this helper removes some code factorization.
> 
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +
>> +    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;
> 
> The implicit count, aka sub-index, is also problematic for the
> abstraction.  Can we tackle applying this to MSI/X to validate if this
> needs to go another step to allow the caller to specify index and
> sub-index?
I mentioned the helper stands for IRQ indices with no sub-index. I am
afraid applying this to MSI/X would oblige use to revisit a bulk of code
without knowing whether it is interesting.
> 
>> +    }
>> +
>> +    if (enable) {
>> +        ret = event_notifier_init(notifier, 0);
>> +        if (ret) {
>> +            error_setg_errno(errp, -ret,
>> +                             "Unable to init event notifier for irq index %d",
>> +                             index);
>> +            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 = index;
>> +    irq_set->start = 0;
>> +    irq_set->count = 1;
>> +    pfd = (int32_t *)&irq_set->data;
>> +
>> +    if (!notifier) {
>> +        error_setg(errp, "Notifier not initialized for irq index %d", index);
>> +        return;
>> +    }
> 
> What is this supposed to check?  @notifier is not NULL initialized, the
> case statement will assert if it doesn't get set, and this doesn't
> actually test if it's properly initialized.
The goal was to check the helper was not called on a valid IRQ index
with !enabled while the notifier was not properly initialized. But if we
trust the calling sites I can remove it.
> 
>> +
>> +    fd = event_notifier_get_fd(notifier);
>> +
>> +    if (enable) {
>> +        qemu_set_fd_handler(fd, handler, NULL, vdev);
>> +        *pfd = fd;
>> +    } else {
>> +        *pfd = -1;
>> +    }
>> +
>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> +    g_free(irq_set);
>> +
>> +    if (ret) {
>> +        error_setg_errno(errp, -ret,
>> +                         "Failed to %s eventfd signalling for index %d",
>> +                         enable ? "set up" : "tear down", index);
>> +    }
>> +    if (ret || !enable) {
>> +        qemu_set_fd_handler(fd, NULL, NULL, vdev);
>> +        event_notifier_cleanup(notifier);
>> +    }
>> +}
> 
> Suggest passing @notifier as a parameter and using @handler in place of
> @enable, more generic and more obvious calling convention.
ok
> 
>> +
>>  static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
>>  {
>>  #ifdef CONFIG_KVM
>> @@ -2621,86 +2711,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 +2726,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 +2999,33 @@ 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) {
>> +        Error *err = NULL;
>> +
>> +        /*
>> +         * Registers error notifier for devices supporting error recovery.
>> +         * If we encounter a failure during registration, we report an error
>> +         * and continue after disabling error recovery support for the
>> +         * device.
>> +         */
>> +        vfio_set_event_handler(vdev, VFIO_PCI_ERR_IRQ_INDEX, true,
>> +                               vfio_err_notifier_handler, &err);
>> +        if (err) {
>> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> +        }
> 
> Why not just return -1 on error and zero on success so we can call as:
> 
>     if (vfio_set_event_handler(...)) {
>         warn_reportf_err()...
>     }
The point is that if you have both the err and the returned value , it
is error prone as you expect both to be consistent (reported by Alexey).
You expect err to be set whenever you have a an error returned. The
calling site can call warn_reportf_err() whenever the function returns
an error and if somebody, later on, ignores to set the err on error
case, it will crash.

Reading again Markus' answer
(https://www.mail-archive.com/qemu-devel@nongnu.org/msg402893.html), I
may put the returned value back and make sure my code is consistent ;-)
> 
>> +        vdev->pci_aer = !err;
> 
> We could also avoid this weirdness of this negation to get a bool.
ok
> 
>> +    }
> 
> It's not obvious how doing away with the register/unregister helpers
> and doing everything inline is an improvement.  Simple helpers calling
> common helpers seems better than inline sprawl calling common helpers.
> Thanks,

On my side I noticed vfio_(un)register_err|req_notifier are mostly
identical at the exception of the irq index/notifier and enable logic.
As I am about to propose another single index IRQ, I am going to add 2
similar functions and I felt it was a pitty. Now it is not a big deal
and if you prefer to keep the code as it is I will simply add those.

Thanks

Eric

> 
> Alex
> 
>> +
>> +    if (vdev->features & VFIO_FEATURE_ENABLE_REQ) {
>> +        Error *err = NULL;
>> +
>> +        vfio_set_event_handler(vdev, VFIO_PCI_REQ_IRQ_INDEX, true,
>> +                               vfio_req_notifier_handler, &err);
>> +        if (err) {
>> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> +        }
>> +        vdev->req_enabled = !err;
>> +    }
>>      vfio_setup_resetfn_quirk(vdev);
>>  
>>      return;
>> @@ -3106,9 +3061,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_set_event_handler(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_set_event_handler(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 v2 1/2] vfio-pci: Introduce vfio_set_event_handler helper

[Alex Williamson]

On Wed, 23 Jan 2019 16:28:50 +0100
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Alex,
> 
> On 1/22/19 8:51 PM, Alex Williamson wrote:
> > On Thu, 17 Jan 2019 22:06:31 +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.
> >>
> >> We test whether 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>
> >>
> >> ---
> >>
> >> v1 -> v2:
> >> - s/vfio_register_event_notifier/vfio_set_event_handler
> >> - turned into a void function
> >> - do the qemu_set_fd_handler(*pfd, NULL, NULL, vdev) and
> >>   event_notifier_cleanup on VFIO_DEVICE_SET_IRQS failure
> >>   as reported by Alexey
> >> - reset err in vfio_realize as reported by Cornelia
> >> - Text/comment fixes suggested by Cornelia
> >> ---
> >>  hw/vfio/pci.c | 296 ++++++++++++++++++++++----------------------------
> >>  1 file changed, 132 insertions(+), 164 deletions(-)
> >>
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index c0cb1ec289..3cae4c99ef 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -105,6 +105,96 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
> >>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
> >>  }
> >>  
> >> +/*
> >> + * vfio_set_event_handler - 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 with
> >> + * @enable: true means notifier chain needs to be set up
> >> + * @handler: handler to attach if @enable is true  
> > 
> > Therefore @enable is redundant.  
> agreed
> >   
> >> + * @errp: error handle
> >> + */
> >> +static void vfio_set_event_handler(VFIOPCIDevice *vdev,
> >> +                                   int index,
> >> +                                   bool enable,
> >> +                                   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;  
> > 
> > This blows the abstraction of a helper that this seems to be trying to
> > create, seems cleaner to pass @notifier.  
> 
> 
> Not sure I really get the point eventually. Don't we have the following
> indirection: irq index -> notifier.fd -> handler. When we want to use
> this helper don't we simply want to associate an handler to a given IRQ
> index without taking care of the notifier mechanics.

With that logic we could eliminate all the parameters and have the
function infer everything.  I don't think that's how we build a good
helper function though.  To me the function is wanting to set or clear
a handler for an irq index, but it also needs the notifier to trigger
to call that handler, so we simply need to pass that as another arg
rather than inferring it from the index.
 
> As discussed with Alexey, moving the notifier initialization outside of
> this helper removes some code factorization.

I don't see why passing the notifier implies this function cannot
perform the init and cleanup of that notifier.

> >> +    default:
> >> +        g_assert_not_reached();
> >> +    }
> >> +
> >> +    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;  
> > 
> > The implicit count, aka sub-index, is also problematic for the
> > abstraction.  Can we tackle applying this to MSI/X to validate if this
> > needs to go another step to allow the caller to specify index and
> > sub-index?  
> I mentioned the helper stands for IRQ indices with no sub-index. I am
> afraid applying this to MSI/X would oblige use to revisit a bulk of code
> without knowing whether it is interesting.

I'm afraid that the helper shows some holes with INTx integration and
I'm wondering if MSI/X integration would show us how to improve the
helper.

> >> +    }
> >> +
> >> +    if (enable) {
> >> +        ret = event_notifier_init(notifier, 0);
> >> +        if (ret) {
> >> +            error_setg_errno(errp, -ret,
> >> +                             "Unable to init event notifier for irq index %d",
> >> +                             index);
> >> +            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 = index;
> >> +    irq_set->start = 0;
> >> +    irq_set->count = 1;
> >> +    pfd = (int32_t *)&irq_set->data;
> >> +
> >> +    if (!notifier) {
> >> +        error_setg(errp, "Notifier not initialized for irq index %d", index);
> >> +        return;
> >> +    }  
> > 
> > What is this supposed to check?  @notifier is not NULL initialized, the
> > case statement will assert if it doesn't get set, and this doesn't
> > actually test if it's properly initialized.  
> The goal was to check the helper was not called on a valid IRQ index
> with !enabled while the notifier was not properly initialized. But if we
> trust the calling sites I can remove it.

But this doesn't test if the notifier is initialized.  Seems you'd need
to check if fd of the notifier is -1.

> >   
> >> +
> >> +    fd = event_notifier_get_fd(notifier);
> >> +
> >> +    if (enable) {
> >> +        qemu_set_fd_handler(fd, handler, NULL, vdev);
> >> +        *pfd = fd;
> >> +    } else {
> >> +        *pfd = -1;
> >> +    }
> >> +
> >> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> >> +    g_free(irq_set);
> >> +
> >> +    if (ret) {
> >> +        error_setg_errno(errp, -ret,
> >> +                         "Failed to %s eventfd signalling for index %d",
> >> +                         enable ? "set up" : "tear down", index);
> >> +    }
> >> +    if (ret || !enable) {
> >> +        qemu_set_fd_handler(fd, NULL, NULL, vdev);
> >> +        event_notifier_cleanup(notifier);
> >> +    }
> >> +}  
> > 
> > Suggest passing @notifier as a parameter and using @handler in place of
> > @enable, more generic and more obvious calling convention.  
> ok
> >   
> >> +
> >>  static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
> >>  {
> >>  #ifdef CONFIG_KVM
> >> @@ -2621,86 +2711,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 +2726,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 +2999,33 @@ 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) {
> >> +        Error *err = NULL;
> >> +
> >> +        /*
> >> +         * Registers error notifier for devices supporting error recovery.
> >> +         * If we encounter a failure during registration, we report an error
> >> +         * and continue after disabling error recovery support for the
> >> +         * device.
> >> +         */
> >> +        vfio_set_event_handler(vdev, VFIO_PCI_ERR_IRQ_INDEX, true,
> >> +                               vfio_err_notifier_handler, &err);
> >> +        if (err) {
> >> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> >> +        }  
> > 
> > Why not just return -1 on error and zero on success so we can call as:
> > 
> >     if (vfio_set_event_handler(...)) {
> >         warn_reportf_err()...
> >     }  
> The point is that if you have both the err and the returned value , it
> is error prone as you expect both to be consistent (reported by Alexey).
> You expect err to be set whenever you have a an error returned. The
> calling site can call warn_reportf_err() whenever the function returns
> an error and if somebody, later on, ignores to set the err on error
> case, it will crash.
> 
> Reading again Markus' answer
> (https://www.mail-archive.com/qemu-devel@nongnu.org/msg402893.html), I
> may put the returned value back and make sure my code is consistent ;-)

It's not like C programmers aren't used to checking return values.  Not
exactly analogous, but we check the return value of an ioctl() then
look at errno.  So it feels like standard practice to return an error
code on failure and try to only use void returns for functions that
cannot fail.

> >   
> >> +        vdev->pci_aer = !err;  
> > 
> > We could also avoid this weirdness of this negation to get a bool.  
> ok
> >   
> >> +    }  
> > 
> > It's not obvious how doing away with the register/unregister helpers
> > and doing everything inline is an improvement.  Simple helpers calling
> > common helpers seems better than inline sprawl calling common helpers.
> > Thanks,  
> 
> On my side I noticed vfio_(un)register_err|req_notifier are mostly
> identical at the exception of the irq index/notifier and enable logic.
> As I am about to propose another single index IRQ, I am going to add 2
> similar functions and I felt it was a pitty. Now it is not a big deal
> and if you prefer to keep the code as it is I will simply add those.

Simple helper functions are a good thing IMO, especially with the
prospects of open coding two more setup and teardown sections further
bloating the base function.  Thanks,

Alex

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

[Auger Eric]

Hi Alex,

On 1/23/19 5:00 PM, Alex Williamson wrote:
> On Wed, 23 Jan 2019 16:28:50 +0100
> Auger Eric <eric.auger@redhat.com> wrote:
> 
>> Hi Alex,
>>
>> On 1/22/19 8:51 PM, Alex Williamson wrote:
>>> On Thu, 17 Jan 2019 22:06:31 +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.
>>>>
>>>> We test whether 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>
>>>>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>> - s/vfio_register_event_notifier/vfio_set_event_handler
>>>> - turned into a void function
>>>> - do the qemu_set_fd_handler(*pfd, NULL, NULL, vdev) and
>>>>   event_notifier_cleanup on VFIO_DEVICE_SET_IRQS failure
>>>>   as reported by Alexey
>>>> - reset err in vfio_realize as reported by Cornelia
>>>> - Text/comment fixes suggested by Cornelia
>>>> ---
>>>>  hw/vfio/pci.c | 296 ++++++++++++++++++++++----------------------------
>>>>  1 file changed, 132 insertions(+), 164 deletions(-)
>>>>
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index c0cb1ec289..3cae4c99ef 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -105,6 +105,96 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
>>>>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>>>>  }
>>>>  
>>>> +/*
>>>> + * vfio_set_event_handler - 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 with
>>>> + * @enable: true means notifier chain needs to be set up
>>>> + * @handler: handler to attach if @enable is true  
>>>
>>> Therefore @enable is redundant.  
>> agreed
>>>   
>>>> + * @errp: error handle
>>>> + */
>>>> +static void vfio_set_event_handler(VFIOPCIDevice *vdev,
>>>> +                                   int index,
>>>> +                                   bool enable,
>>>> +                                   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;  
>>>
>>> This blows the abstraction of a helper that this seems to be trying to
>>> create, seems cleaner to pass @notifier.  
>>
>>
>> Not sure I really get the point eventually. Don't we have the following
>> indirection: irq index -> notifier.fd -> handler. When we want to use
>> this helper don't we simply want to associate an handler to a given IRQ
>> index without taking care of the notifier mechanics.
> 
> With that logic we could eliminate all the parameters and have the
> function infer everything.  I don't think that's how we build a good
> helper function though.  To me the function is wanting to set or clear
> a handler for an irq index, but it also needs the notifier to trigger
> to call that handler, so we simply need to pass that as another arg
> rather than inferring it from the index.
OK
>  
>> As discussed with Alexey, moving the notifier initialization outside of
>> this helper removes some code factorization.
> 
> I don't see why passing the notifier implies this function cannot
> perform the init and cleanup of that notifier.
Got it now.
> 
>>>> +    default:
>>>> +        g_assert_not_reached();
>>>> +    }
>>>> +
>>>> +    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;  
>>>
>>> The implicit count, aka sub-index, is also problematic for the
>>> abstraction.  Can we tackle applying this to MSI/X to validate if this
>>> needs to go another step to allow the caller to specify index and
>>> sub-index?  
>> I mentioned the helper stands for IRQ indices with no sub-index. I am
>> afraid applying this to MSI/X would oblige use to revisit a bulk of code
>> without knowing whether it is interesting.
> 
> I'm afraid that the helper shows some holes with INTx integration and
> I'm wondering if MSI/X integration would show us how to improve the
> helper.
OK I will do the exercise
> 
>>>> +    }
>>>> +
>>>> +    if (enable) {
>>>> +        ret = event_notifier_init(notifier, 0);
>>>> +        if (ret) {
>>>> +            error_setg_errno(errp, -ret,
>>>> +                             "Unable to init event notifier for irq index %d",
>>>> +                             index);
>>>> +            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 = index;
>>>> +    irq_set->start = 0;
>>>> +    irq_set->count = 1;
>>>> +    pfd = (int32_t *)&irq_set->data;
>>>> +
>>>> +    if (!notifier) {
>>>> +        error_setg(errp, "Notifier not initialized for irq index %d", index);
>>>> +        return;
>>>> +    }  
>>>
>>> What is this supposed to check?  @notifier is not NULL initialized, the
>>> case statement will assert if it doesn't get set, and this doesn't
>>> actually test if it's properly initialized.  
>> The goal was to check the helper was not called on a valid IRQ index
>> with !enabled while the notifier was not properly initialized. But if we
>> trust the calling sites I can remove it.
> 
> But this doesn't test if the notifier is initialized.  Seems you'd need
> to check if fd of the notifier is -1.
I understand the point now
> 
>>>   
>>>> +
>>>> +    fd = event_notifier_get_fd(notifier);
>>>> +
>>>> +    if (enable) {
>>>> +        qemu_set_fd_handler(fd, handler, NULL, vdev);
>>>> +        *pfd = fd;
>>>> +    } else {
>>>> +        *pfd = -1;
>>>> +    }
>>>> +
>>>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>>>> +    g_free(irq_set);
>>>> +
>>>> +    if (ret) {
>>>> +        error_setg_errno(errp, -ret,
>>>> +                         "Failed to %s eventfd signalling for index %d",
>>>> +                         enable ? "set up" : "tear down", index);
>>>> +    }
>>>> +    if (ret || !enable) {
>>>> +        qemu_set_fd_handler(fd, NULL, NULL, vdev);
>>>> +        event_notifier_cleanup(notifier);
>>>> +    }
>>>> +}  
>>>
>>> Suggest passing @notifier as a parameter and using @handler in place of
>>> @enable, more generic and more obvious calling convention.  
>> ok
>>>   
>>>> +
>>>>  static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
>>>>  {
>>>>  #ifdef CONFIG_KVM
>>>> @@ -2621,86 +2711,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 +2726,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 +2999,33 @@ 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) {
>>>> +        Error *err = NULL;
>>>> +
>>>> +        /*
>>>> +         * Registers error notifier for devices supporting error recovery.
>>>> +         * If we encounter a failure during registration, we report an error
>>>> +         * and continue after disabling error recovery support for the
>>>> +         * device.
>>>> +         */
>>>> +        vfio_set_event_handler(vdev, VFIO_PCI_ERR_IRQ_INDEX, true,
>>>> +                               vfio_err_notifier_handler, &err);
>>>> +        if (err) {
>>>> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>>>> +        }  
>>>
>>> Why not just return -1 on error and zero on success so we can call as:
>>>
>>>     if (vfio_set_event_handler(...)) {
>>>         warn_reportf_err()...
>>>     }  
>> The point is that if you have both the err and the returned value , it
>> is error prone as you expect both to be consistent (reported by Alexey).
>> You expect err to be set whenever you have a an error returned. The
>> calling site can call warn_reportf_err() whenever the function returns
>> an error and if somebody, later on, ignores to set the err on error
>> case, it will crash.
>>
>> Reading again Markus' answer
>> (https://www.mail-archive.com/qemu-devel@nongnu.org/msg402893.html), I
>> may put the returned value back and make sure my code is consistent ;-)
> 
> It's not like C programmers aren't used to checking return values.  Not
> exactly analogous, but we check the return value of an ioctl() then
> look at errno.  So it feels like standard practice to return an error
> code on failure and try to only use void returns for functions that
> cannot fail.
ok
> 
>>>   
>>>> +        vdev->pci_aer = !err;  
>>>
>>> We could also avoid this weirdness of this negation to get a bool.  
>> ok
>>>   
>>>> +    }  
>>>
>>> It's not obvious how doing away with the register/unregister helpers
>>> and doing everything inline is an improvement.  Simple helpers calling
>>> common helpers seems better than inline sprawl calling common helpers.
>>> Thanks,  
>>
>> On my side I noticed vfio_(un)register_err|req_notifier are mostly
>> identical at the exception of the irq index/notifier and enable logic.
>> As I am about to propose another single index IRQ, I am going to add 2
>> similar functions and I felt it was a pitty. Now it is not a big deal
>> and if you prefer to keep the code as it is I will simply add those.
> 
> Simple helper functions are a good thing IMO, especially with the
> prospects of open coding two more setup and teardown sections further
> bloating the base function.  Thanks,
Thanks

Eric
> 
> Alex
> 

[Qemu-devel] [PATCH v2 2/2] vfio-pci: Use vfio_set_event_handler in vfio_intx_enable

[Eric Auger]

vfio_set_event_handler() can be used  in vfio_intx_enable()
to set the signalling associated with VFIO_PCI_INTX_IRQ_INDEX.

We also turn vfio_intx_enable() into a void function.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

vfio_set_event_handler cannot be used in vfio_intx_disable. The reason
is not a call sequence issue but the VFIO_DEVICE_SET_IRQS invoction.
vfio_disable_irqindex uses ACTION_TRIGGER/DATA_NONE/count==0 whereas
the new helper uses ACTION_TRIGGER/DATA_EVENTFD/fd==-1. Surprisingly
to me, both are not resulting into the same kernel actions. The first
one does a complete tear down of the INTx (vfio_intx_disable), while
the second - just taking care of a single index, the only one by the
way for INTx -, does an incomplete tear down (vfio_intx_set_signal).
If the last one is used, a subsequent setup of MSIx vectors fails.

v1 -> v2:
- turn vfio_intx_enable into a void function
- s/vfio_intx_enable_kvm/vfio_intx_enable in title and commit msg
---
 hw/vfio/pci.c | 51 +++++++++++++--------------------------------------
 1 file changed, 13 insertions(+), 38 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 3cae4c99ef..dcba7a1539 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -136,6 +136,9 @@ static void vfio_set_event_handler(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:
         g_assert_not_reached();
     }
@@ -349,16 +352,13 @@ static void vfio_intx_update(PCIDevice *pdev)
     vfio_intx_eoi(&vdev->vbasedev);
 }
 
-static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
+static void 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;
 
     if (!pin) {
-        return 0;
+        return;
     }
 
     vfio_disable_interrupts(vdev);
@@ -377,32 +377,11 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
     }
 #endif
 
-    ret = event_notifier_init(&vdev->intx.interrupt, 0);
-    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_set_event_handler(vdev, VFIO_PCI_INTX_IRQ_INDEX, true,
+                                 vfio_intx_interrupt, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
     }
 
     vfio_intx_enable_kvm(vdev, &err);
@@ -413,11 +392,6 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
     vdev->interrupt = VFIO_INT_INTx;
 
     trace_vfio_intx_enable(vdev->vbasedev.name);
-
-cleanup:
-    g_free(irq_set);
-
-    return retval;
 }
 
 static void vfio_intx_disable(VFIOPCIDevice *vdev)
@@ -2982,8 +2956,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         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_update);
-        ret = vfio_intx_enable(vdev, errp);
-        if (ret) {
+        vfio_intx_enable(vdev, &err);
+        if (err) {
+            error_propagate(errp, err);
             goto out_teardown;
         }
     }
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 2/2] vfio-pci: Use vfio_set_event_handler in vfio_intx_enable

[Alex Williamson]

On Thu, 17 Jan 2019 22:06:32 +0100
Eric Auger <eric.auger@redhat.com> wrote:

> vfio_set_event_handler() can be used  in vfio_intx_enable()
> to set the signalling associated with VFIO_PCI_INTX_IRQ_INDEX.
> 
> We also turn vfio_intx_enable() into a void function.

Why?

> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> vfio_set_event_handler cannot be used in vfio_intx_disable. The reason
> is not a call sequence issue but the VFIO_DEVICE_SET_IRQS invoction.
> vfio_disable_irqindex uses ACTION_TRIGGER/DATA_NONE/count==0 whereas
> the new helper uses ACTION_TRIGGER/DATA_EVENTFD/fd==-1. Surprisingly
> to me, both are not resulting into the same kernel actions. The first
> one does a complete tear down of the INTx (vfio_intx_disable), while
> the second - just taking care of a single index, the only one by the
> way for INTx -, does an incomplete tear down (vfio_intx_set_signal).
> If the last one is used, a subsequent setup of MSIx vectors fails.

As we discussed offline, this is documented in the uapi and reflects
that the device can be in an interrupt mode without any signaling
vectors.  For instance the MSI or MSI-X capability on a PCI device can
be enabled, but no vectors are configured to signal.  It's a bit of an
after thought how we enter this mode (see vfio_msix_enable), but it has
proven useful.  If INTx is going to use the helper for setup but not
teardown there at least needs to be a comment in the code indicating
the limitation so that an unwitting contributor doesn't fall for the
seemingly obvious simplification.

> 
> v1 -> v2:
> - turn vfio_intx_enable into a void function
> - s/vfio_intx_enable_kvm/vfio_intx_enable in title and commit msg
> ---
>  hw/vfio/pci.c | 51 +++++++++++++--------------------------------------
>  1 file changed, 13 insertions(+), 38 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 3cae4c99ef..dcba7a1539 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -136,6 +136,9 @@ static void vfio_set_event_handler(VFIOPCIDevice *vdev,
>      case VFIO_PCI_ERR_IRQ_INDEX:
>          notifier = &vdev->err_notifier;
>          break;
> +    case VFIO_PCI_INTX_IRQ_INDEX:
> +        notifier = &vdev->intx.interrupt;
> +        break;


I'm still not a fan of a helper than needs to be updated for each new
caller.

>      default:
>          g_assert_not_reached();
>      }
> @@ -349,16 +352,13 @@ static void vfio_intx_update(PCIDevice *pdev)
>      vfio_intx_eoi(&vdev->vbasedev);
>  }
>  
> -static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
> +static void 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;
>  
>      if (!pin) {
> -        return 0;
> +        return;
>      }
>  
>      vfio_disable_interrupts(vdev);
> @@ -377,32 +377,11 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
>      }
>  #endif
>  
> -    ret = event_notifier_init(&vdev->intx.interrupt, 0);
> -    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_set_event_handler(vdev, VFIO_PCI_INTX_IRQ_INDEX, true,
> +                                 vfio_intx_interrupt, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
>      }
>  
>      vfio_intx_enable_kvm(vdev, &err);
> @@ -413,11 +392,6 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
>      vdev->interrupt = VFIO_INT_INTx;
>  
>      trace_vfio_intx_enable(vdev->vbasedev.name);
> -
> -cleanup:
> -    g_free(irq_set);
> -
> -    return retval;
>  }
>  
>  static void vfio_intx_disable(VFIOPCIDevice *vdev)
> @@ -2982,8 +2956,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>          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_update);
> -        ret = vfio_intx_enable(vdev, errp);
> -        if (ret) {
> +        vfio_intx_enable(vdev, &err);
> +        if (err) {
> +            error_propagate(errp, err);

Wouldn't it still be easier to return -1/0 rather than test err?  I'm
not sure what was gained with the conversion to void.  Thanks,

Alex

>              goto out_teardown;
>          }
>      }