[Qemu-devel] [PATCH v4] vfio/common: Introduce vfio_set_irq_signaling helper

Eric Auger posted 1 patch 1 week ago
Test s390x passed
Test checkpatch passed
Test asan passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190612080004.24275-1-eric.auger@redhat.com
Maintainers: Alex Williamson <alex.williamson@redhat.com>
hw/vfio/common.c              |  76 ++++++++++++
hw/vfio/pci.c                 | 217 ++++++++--------------------------
hw/vfio/platform.c            |  62 ++++------
include/hw/vfio/vfio-common.h |   2 +
4 files changed, 149 insertions(+), 208 deletions(-)

[Qemu-devel] [PATCH v4] vfio/common: Introduce vfio_set_irq_signaling helper

Posted by Eric Auger 1 week ago
The code used to assign an interrupt index/subindex to an
eventfd is duplicated many times. Let's introduce an helper that
allows to set/unset the signaling for an ACTION_TRIGGER,
ACTION_MASK or ACTION_UNMASK action.

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

---

v3 -> v4:
- renamed irq_to_str into index_to_str
- avoid usage of g_strdup_printf
- print both the index and subindex in generic case

v2 -> v3:
- irq_to_str() simply outputs the IRQ index if the VFIO device is
  not of PCI type
- removed "vfio: failed to start eventfd signaling ../.." in vfio_platform

v1 -> v2:
- don't call GET_IRQ_INFO in vfio_set_irq_signaling()
  and restore quiet check in vfio_register_req_notifier.
  Nicer display of the IRQ name.

This is a follow-up to
[PATCH v2 0/2] vfio-pci: Introduce vfio_set_event_handler().
It looks to me that introducing vfio_set_irq_signaling() has more
benefits in term of code reduction and the helper abstraction
looks cleaner.
---
 hw/vfio/common.c              |  76 ++++++++++++
 hw/vfio/pci.c                 | 217 ++++++++--------------------------
 hw/vfio/platform.c            |  62 ++++------
 include/hw/vfio/vfio-common.h |   2 +
 4 files changed, 149 insertions(+), 208 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 4374cc6176..4b18eefe99 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -95,6 +95,82 @@ void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index)
     ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, &irq_set);
 }
 
+static inline const char *action_to_str(int action)
+{
+    switch (action) {
+    case VFIO_IRQ_SET_ACTION_MASK:
+        return "MASK";
+    case VFIO_IRQ_SET_ACTION_UNMASK:
+        return "UNMASK";
+    case VFIO_IRQ_SET_ACTION_TRIGGER:
+        return "TRIGGER";
+    default:
+        return "UNKNOWN ACTION";
+    }
+}
+
+static const char *index_to_str(VFIODevice *vbasedev, int index)
+{
+    if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
+        return NULL;
+    }
+
+    switch (index) {
+    case VFIO_PCI_INTX_IRQ_INDEX:
+        return "INTX";
+    case VFIO_PCI_MSI_IRQ_INDEX:
+        return "MSI";
+    case VFIO_PCI_MSIX_IRQ_INDEX:
+        return "MSIX";
+    case VFIO_PCI_ERR_IRQ_INDEX:
+        return "ERR";
+    case VFIO_PCI_REQ_IRQ_INDEX:
+        return "REQ";
+    default:
+        return NULL;
+    }
+}
+
+int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
+                           int action, int fd, Error **errp)
+{
+    struct vfio_irq_set *irq_set;
+    int argsz, ret = 0;
+    const char *name;
+    int32_t *pfd;
+
+    argsz = sizeof(*irq_set) + sizeof(*pfd);
+
+    irq_set = g_malloc0(argsz);
+    irq_set->argsz = argsz;
+    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | action;
+    irq_set->index = index;
+    irq_set->start = subindex;
+    irq_set->count = 1;
+    pfd = (int32_t *)&irq_set->data;
+    *pfd = fd;
+
+    ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+    g_free(irq_set);
+
+    if (!ret) {
+        return 0;
+    }
+
+    error_setg_errno(errp, -ret, "VFIO_DEVICE_SET_IRQS failure");
+    name = index_to_str(vbasedev, index);
+    if (name) {
+        error_prepend(errp, "%s-%d: ", name, subindex);
+    } else {
+        error_prepend(errp, "index %d-%d: ", index, subindex);
+    }
+    error_prepend(errp,
+                  "Failed to %s %s eventfd signaling for interrupt ",
+                  fd < 0 ? "tear down" : "set up", action_to_str(action));
+    return ret;
+}
+
 /*
  * IO Port/MMIO - Beware of the endians, VFIO is always little endian
  */
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 2a4091d216..65c2e17028 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -113,9 +113,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
         .gsi = vdev->intx.route.irq,
         .flags = KVM_IRQFD_FLAG_RESAMPLE,
     };
-    struct vfio_irq_set *irq_set;
-    int ret, argsz;
-    int32_t *pfd;
+    Error *err = NULL;
 
     if (vdev->no_kvm_intx || !kvm_irqfds_enabled() ||
         vdev->intx.route.mode != PCI_INTX_ENABLED ||
@@ -143,22 +141,10 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
         goto fail_irqfd;
     }
 
-    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_UNMASK;
-    irq_set->index = VFIO_PCI_INTX_IRQ_INDEX;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-
-    *pfd = irqfd.resamplefd;
-
-    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
-    g_free(irq_set);
-    if (ret) {
-        error_setg_errno(errp, -ret, "failed to setup INTx unmask fd");
+    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0,
+                               VFIO_IRQ_SET_ACTION_UNMASK,
+                               irqfd.resamplefd, &err)) {
+        error_propagate(errp, err);
         goto fail_vfio;
     }
 
@@ -262,10 +248,10 @@ 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;
+    int32_t fd;
+    int ret;
+
 
     if (!pin) {
         return 0;
@@ -292,27 +278,15 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
         error_setg_errno(errp, -ret, "event_notifier_init failed");
         return ret;
     }
+    fd = event_notifier_get_fd(&vdev->intx.interrupt);
+    qemu_set_fd_handler(fd, vfio_intx_interrupt, NULL, vdev);
 
-    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);
+    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0,
+                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
+        error_propagate(errp, err);
+        qemu_set_fd_handler(fd, NULL, NULL, vdev);
         event_notifier_cleanup(&vdev->intx.interrupt);
-        retval = -errno;
-        goto cleanup;
+        return -errno;
     }
 
     vfio_intx_enable_kvm(vdev, &err);
@@ -323,11 +297,7 @@ 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;
+    return 0;
 }
 
 static void vfio_intx_disable(VFIOPCIDevice *vdev)
@@ -530,31 +500,19 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
             error_report("vfio: failed to enable vectors, %d", ret);
         }
     } else {
-        int argsz;
-        struct vfio_irq_set *irq_set;
-        int32_t *pfd;
-
-        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_MSIX_IRQ_INDEX;
-        irq_set->start = nr;
-        irq_set->count = 1;
-        pfd = (int32_t *)&irq_set->data;
+        Error *err = NULL;
+        int32_t fd;
 
         if (vector->virq >= 0) {
-            *pfd = event_notifier_get_fd(&vector->kvm_interrupt);
+            fd = event_notifier_get_fd(&vector->kvm_interrupt);
         } else {
-            *pfd = event_notifier_get_fd(&vector->interrupt);
+            fd = event_notifier_get_fd(&vector->interrupt);
         }
 
-        ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
-        g_free(irq_set);
-        if (ret) {
-            error_report("vfio: failed to modify vector, %d", ret);
+        if (vfio_set_irq_signaling(&vdev->vbasedev,
+                                     VFIO_PCI_MSIX_IRQ_INDEX, nr,
+                                     VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
+            error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
         }
     }
 
@@ -591,26 +549,10 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
      * be re-asserted on unmask.  Nothing to do if already using QEMU mode.
      */
     if (vector->virq >= 0) {
-        int argsz;
-        struct vfio_irq_set *irq_set;
-        int32_t *pfd;
+        int32_t fd = event_notifier_get_fd(&vector->interrupt);
 
-        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_MSIX_IRQ_INDEX;
-        irq_set->start = nr;
-        irq_set->count = 1;
-        pfd = (int32_t *)&irq_set->data;
-
-        *pfd = event_notifier_get_fd(&vector->interrupt);
-
-        ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
-
-        g_free(irq_set);
+        vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX, nr,
+                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, NULL);
     }
 }
 
@@ -2636,10 +2578,8 @@ static void vfio_err_notifier_handler(void *opaque)
  */
 static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
 {
-    int ret;
-    int argsz;
-    struct vfio_irq_set *irq_set;
-    int32_t *pfd;
+    Error *err = NULL;
+    int32_t fd;
 
     if (!vdev->pci_aer) {
         return;
@@ -2651,58 +2591,30 @@ static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
         return;
     }
 
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
+    fd = event_notifier_get_fd(&vdev->err_notifier);
+    qemu_set_fd_handler(fd, vfio_err_notifier_handler, NULL, vdev);
 
-    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);
+    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0,
+                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
+        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+        qemu_set_fd_handler(fd, 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;
+    Error *err = NULL;
 
     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");
+    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0,
+                               VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
+        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
     }
-    g_free(irq_set);
     qemu_set_fd_handler(event_notifier_get_fd(&vdev->err_notifier),
                         NULL, NULL, vdev);
     event_notifier_cleanup(&vdev->err_notifier);
@@ -2727,9 +2639,8 @@ 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;
+    Error *err = NULL;
+    int32_t fd;
 
     if (!(vdev->features & VFIO_FEATURE_ENABLE_REQ)) {
         return;
@@ -2745,57 +2656,31 @@ static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
         return;
     }
 
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
+    fd = event_notifier_get_fd(&vdev->req_notifier);
+    qemu_set_fd_handler(fd, vfio_req_notifier_handler, NULL, vdev);
 
-    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);
+    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0,
+                           VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
+        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+        qemu_set_fd_handler(fd, 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;
+    Error *err = NULL;
 
     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");
+    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0,
+                               VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
+        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
     }
-    g_free(irq_set);
     qemu_set_fd_handler(event_notifier_get_fd(&vdev->req_notifier),
                         NULL, NULL, vdev);
     event_notifier_cleanup(&vdev->req_notifier);
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index d52d6552e0..8d6012857e 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -106,26 +106,19 @@ static int vfio_set_trigger_eventfd(VFIOINTp *intp,
                                     eventfd_user_side_handler_t handler)
 {
     VFIODevice *vbasedev = &intp->vdev->vbasedev;
-    struct vfio_irq_set *irq_set;
-    int argsz, ret;
-    int32_t *pfd;
+    int32_t fd = event_notifier_get_fd(intp->interrupt);
+    Error *err = NULL;
+    int 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 = intp->pin;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-    *pfd = event_notifier_get_fd(intp->interrupt);
-    qemu_set_fd_handler(*pfd, (IOHandler *)handler, NULL, intp);
-    ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
-    if (ret < 0) {
-        error_report("vfio: Failed to set trigger eventfd: %m");
-        qemu_set_fd_handler(*pfd, NULL, NULL, NULL);
+    qemu_set_fd_handler(fd, (IOHandler *)handler, NULL, intp);
+
+    ret = vfio_set_irq_signaling(vbasedev, intp->pin, 0,
+                                 VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err);
+    if (ret) {
+        error_reportf_err(err, VFIO_MSG_PREFIX, vbasedev->name);
+        qemu_set_fd_handler(fd, NULL, NULL, NULL);
     }
-    g_free(irq_set);
+
     return ret;
 }
 
@@ -330,7 +323,6 @@ static void vfio_platform_eoi(VFIODevice *vbasedev)
 
 static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq irq)
 {
-    int ret;
     VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
     VFIOINTp *intp;
 
@@ -341,10 +333,7 @@ static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq irq)
     }
     assert(intp);
 
-    ret = vfio_set_trigger_eventfd(intp, vfio_intp_interrupt);
-    if (ret) {
-        error_report("vfio: failed to start eventfd signaling for IRQ %d: %m",
-                     intp->pin);
+    if (vfio_set_trigger_eventfd(intp, vfio_intp_interrupt)) {
         abort();
     }
 }
@@ -361,25 +350,16 @@ static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq irq)
  */
 static int vfio_set_resample_eventfd(VFIOINTp *intp)
 {
+    int32_t fd = event_notifier_get_fd(intp->unmask);
     VFIODevice *vbasedev = &intp->vdev->vbasedev;
-    struct vfio_irq_set *irq_set;
-    int argsz, ret;
-    int32_t *pfd;
+    Error *err = NULL;
+    int 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_UNMASK;
-    irq_set->index = intp->pin;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-    *pfd = event_notifier_get_fd(intp->unmask);
-    qemu_set_fd_handler(*pfd, NULL, NULL, NULL);
-    ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
-    g_free(irq_set);
-    if (ret < 0) {
-        error_report("vfio: Failed to set resample eventfd: %m");
+    qemu_set_fd_handler(fd, NULL, NULL, NULL);
+    ret = vfio_set_irq_signaling(vbasedev, intp->pin, 0,
+                                 VFIO_IRQ_SET_ACTION_UNMASK, fd, &err);
+    if (ret) {
+        error_reportf_err(err, VFIO_MSG_PREFIX, vbasedev->name);
     }
     return ret;
 }
@@ -435,8 +415,6 @@ static void vfio_start_irqfd_injection(SysBusDevice *sbdev, qemu_irq irq)
     return;
 fail_vfio:
     kvm_irqchip_remove_irqfd_notifier(kvm_state, intp->interrupt, irq);
-    error_report("vfio: failed to start eventfd signaling for IRQ %d: %m",
-                 intp->pin);
     abort();
 fail_irqfd:
     vfio_start_eventfd_injection(sbdev, irq);
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 1155b79678..686d99ff8c 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -167,6 +167,8 @@ void vfio_put_base_device(VFIODevice *vbasedev);
 void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
 void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
 void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index);
+int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
+                           int action, int fd, Error **errp);
 void vfio_region_write(void *opaque, hwaddr addr,
                            uint64_t data, unsigned size);
 uint64_t vfio_region_read(void *opaque,
-- 
2.20.1


Re: [Qemu-devel] [PATCH v4] vfio/common: Introduce vfio_set_irq_signaling helper

Posted by Li Qiang 1 week ago
Eric Auger <eric.auger@redhat.com> 于2019年6月12日周三 下午4:14写道:

> The code used to assign an interrupt index/subindex to an
> eventfd is duplicated many times. Let's introduce an helper that
> allows to set/unset the signaling for an ACTION_TRIGGER,
> ACTION_MASK or ACTION_UNMASK action.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> v3 -> v4:
> - renamed irq_to_str into index_to_str
> - avoid usage of g_strdup_printf
> - print both the index and subindex in generic case
>
> v2 -> v3:
> - irq_to_str() simply outputs the IRQ index if the VFIO device is
>   not of PCI type
> - removed "vfio: failed to start eventfd signaling ../.." in vfio_platform
>
> v1 -> v2:
> - don't call GET_IRQ_INFO in vfio_set_irq_signaling()
>   and restore quiet check in vfio_register_req_notifier.
>   Nicer display of the IRQ name.
>
> This is a follow-up to
> [PATCH v2 0/2] vfio-pci: Introduce vfio_set_event_handler().
> It looks to me that introducing vfio_set_irq_signaling() has more
> benefits in term of code reduction and the helper abstraction
> looks cleaner.
> ---
>  hw/vfio/common.c              |  76 ++++++++++++
>  hw/vfio/pci.c                 | 217 ++++++++--------------------------
>  hw/vfio/platform.c            |  62 ++++------
>  include/hw/vfio/vfio-common.h |   2 +
>  4 files changed, 149 insertions(+), 208 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 4374cc6176..4b18eefe99 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -95,6 +95,82 @@ void vfio_mask_single_irqindex(VFIODevice *vbasedev,
> int index)
>      ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, &irq_set);
>  }
>
> +static inline const char *action_to_str(int action)
> +{
> +    switch (action) {
> +    case VFIO_IRQ_SET_ACTION_MASK:
> +        return "MASK";
> +    case VFIO_IRQ_SET_ACTION_UNMASK:
> +        return "UNMASK";
> +    case VFIO_IRQ_SET_ACTION_TRIGGER:
> +        return "TRIGGER";
> +    default:
> +        return "UNKNOWN ACTION";
> +    }
> +}
> +
> +static const char *index_to_str(VFIODevice *vbasedev, int index)
> +{
> +    if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
> +        return NULL;
> +    }
> +
> +    switch (index) {
> +    case VFIO_PCI_INTX_IRQ_INDEX:
> +        return "INTX";
> +    case VFIO_PCI_MSI_IRQ_INDEX:
> +        return "MSI";
> +    case VFIO_PCI_MSIX_IRQ_INDEX:
> +        return "MSIX";
> +    case VFIO_PCI_ERR_IRQ_INDEX:
> +        return "ERR";
> +    case VFIO_PCI_REQ_IRQ_INDEX:
> +        return "REQ";
> +    default:
> +        return NULL;
> +    }
> +}
> +
> +int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
> +                           int action, int fd, Error **errp)
> +{
> +    struct vfio_irq_set *irq_set;
> +    int argsz, ret = 0;
> +    const char *name;
> +    int32_t *pfd;
> +
> +    argsz = sizeof(*irq_set) + sizeof(*pfd);
> +
> +    irq_set = g_malloc0(argsz);
> +    irq_set->argsz = argsz;
> +    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | action;
> +    irq_set->index = index;
> +    irq_set->start = subindex;
> +    irq_set->count = 1;
> +    pfd = (int32_t *)&irq_set->data;
> +    *pfd = fd;
> +
> +    ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
> +
> +    g_free(irq_set);
> +
> +    if (!ret) {
> +        return 0;
> +    }
> +
> +    error_setg_errno(errp, -ret, "VFIO_DEVICE_SET_IRQS failure");
>

Hello Eric,

I noticed an issue several days ago not related with this patch(maybe split
in another patch).
IIUC, here the 'ret' is -1 if the ioctl fails.
It seems we should use 'errno', however, can we make sure the 'g_free'
will not touch 'errno'? If it doesn't touch, we can just use errno here, if
it touch,
we may save errno right after ioctl call.

Thanks,
Li Qiang




> +    name = index_to_str(vbasedev, index);
> +    if (name) {
> +        error_prepend(errp, "%s-%d: ", name, subindex);
> +    } else {
> +        error_prepend(errp, "index %d-%d: ", index, subindex);
> +    }
> +    error_prepend(errp,
> +                  "Failed to %s %s eventfd signaling for interrupt ",
> +                  fd < 0 ? "tear down" : "set up", action_to_str(action));
> +    return ret;
> +}
> +
>  /*
>   * IO Port/MMIO - Beware of the endians, VFIO is always little endian
>   */
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 2a4091d216..65c2e17028 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -113,9 +113,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev,
> Error **errp)
>          .gsi = vdev->intx.route.irq,
>          .flags = KVM_IRQFD_FLAG_RESAMPLE,
>      };
> -    struct vfio_irq_set *irq_set;
> -    int ret, argsz;
> -    int32_t *pfd;
> +    Error *err = NULL;
>
>      if (vdev->no_kvm_intx || !kvm_irqfds_enabled() ||
>          vdev->intx.route.mode != PCI_INTX_ENABLED ||
> @@ -143,22 +141,10 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice
> *vdev, Error **errp)
>          goto fail_irqfd;
>      }
>
> -    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_UNMASK;
> -    irq_set->index = VFIO_PCI_INTX_IRQ_INDEX;
> -    irq_set->start = 0;
> -    irq_set->count = 1;
> -    pfd = (int32_t *)&irq_set->data;
> -
> -    *pfd = irqfd.resamplefd;
> -
> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> -    g_free(irq_set);
> -    if (ret) {
> -        error_setg_errno(errp, -ret, "failed to setup INTx unmask fd");
> +    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX,
> 0,
> +                               VFIO_IRQ_SET_ACTION_UNMASK,
> +                               irqfd.resamplefd, &err)) {
> +        error_propagate(errp, err);
>          goto fail_vfio;
>      }
>
> @@ -262,10 +248,10 @@ 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;
> +    int32_t fd;
> +    int ret;
> +
>
>      if (!pin) {
>          return 0;
> @@ -292,27 +278,15 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev,
> Error **errp)
>          error_setg_errno(errp, -ret, "event_notifier_init failed");
>          return ret;
>      }
> +    fd = event_notifier_get_fd(&vdev->intx.interrupt);
> +    qemu_set_fd_handler(fd, vfio_intx_interrupt, NULL, vdev);
>
> -    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);
> +    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX,
> 0,
> +                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
> +        error_propagate(errp, err);
> +        qemu_set_fd_handler(fd, NULL, NULL, vdev);
>          event_notifier_cleanup(&vdev->intx.interrupt);
> -        retval = -errno;
> -        goto cleanup;
> +        return -errno;
>      }
>
>      vfio_intx_enable_kvm(vdev, &err);
> @@ -323,11 +297,7 @@ 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;
> +    return 0;
>  }
>
>  static void vfio_intx_disable(VFIOPCIDevice *vdev)
> @@ -530,31 +500,19 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev,
> unsigned int nr,
>              error_report("vfio: failed to enable vectors, %d", ret);
>          }
>      } else {
> -        int argsz;
> -        struct vfio_irq_set *irq_set;
> -        int32_t *pfd;
> -
> -        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_MSIX_IRQ_INDEX;
> -        irq_set->start = nr;
> -        irq_set->count = 1;
> -        pfd = (int32_t *)&irq_set->data;
> +        Error *err = NULL;
> +        int32_t fd;
>
>          if (vector->virq >= 0) {
> -            *pfd = event_notifier_get_fd(&vector->kvm_interrupt);
> +            fd = event_notifier_get_fd(&vector->kvm_interrupt);
>          } else {
> -            *pfd = event_notifier_get_fd(&vector->interrupt);
> +            fd = event_notifier_get_fd(&vector->interrupt);
>          }
>
> -        ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> -        g_free(irq_set);
> -        if (ret) {
> -            error_report("vfio: failed to modify vector, %d", ret);
> +        if (vfio_set_irq_signaling(&vdev->vbasedev,
> +                                     VFIO_PCI_MSIX_IRQ_INDEX, nr,
> +                                     VFIO_IRQ_SET_ACTION_TRIGGER, fd,
> &err)) {
> +            error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>          }
>      }
>
> @@ -591,26 +549,10 @@ static void vfio_msix_vector_release(PCIDevice
> *pdev, unsigned int nr)
>       * be re-asserted on unmask.  Nothing to do if already using QEMU
> mode.
>       */
>      if (vector->virq >= 0) {
> -        int argsz;
> -        struct vfio_irq_set *irq_set;
> -        int32_t *pfd;
> +        int32_t fd = event_notifier_get_fd(&vector->interrupt);
>
> -        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_MSIX_IRQ_INDEX;
> -        irq_set->start = nr;
> -        irq_set->count = 1;
> -        pfd = (int32_t *)&irq_set->data;
> -
> -        *pfd = event_notifier_get_fd(&vector->interrupt);
> -
> -        ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> -
> -        g_free(irq_set);
> +        vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX,
> nr,
> +                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, NULL);
>      }
>  }
>
> @@ -2636,10 +2578,8 @@ static void vfio_err_notifier_handler(void *opaque)
>   */
>  static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
>  {
> -    int ret;
> -    int argsz;
> -    struct vfio_irq_set *irq_set;
> -    int32_t *pfd;
> +    Error *err = NULL;
> +    int32_t fd;
>
>      if (!vdev->pci_aer) {
>          return;
> @@ -2651,58 +2591,30 @@ static void
> vfio_register_err_notifier(VFIOPCIDevice *vdev)
>          return;
>      }
>
> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
> +    fd = event_notifier_get_fd(&vdev->err_notifier);
> +    qemu_set_fd_handler(fd, vfio_err_notifier_handler, NULL, vdev);
>
> -    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);
> +    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0,
> +                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
> +        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        qemu_set_fd_handler(fd, 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;
> +    Error *err = NULL;
>
>      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");
> +    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0,
> +                               VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
> +        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>      }
> -    g_free(irq_set);
>      qemu_set_fd_handler(event_notifier_get_fd(&vdev->err_notifier),
>                          NULL, NULL, vdev);
>      event_notifier_cleanup(&vdev->err_notifier);
> @@ -2727,9 +2639,8 @@ 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;
> +    Error *err = NULL;
> +    int32_t fd;
>
>      if (!(vdev->features & VFIO_FEATURE_ENABLE_REQ)) {
>          return;
> @@ -2745,57 +2656,31 @@ static void
> vfio_register_req_notifier(VFIOPCIDevice *vdev)
>          return;
>      }
>
> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
> +    fd = event_notifier_get_fd(&vdev->req_notifier);
> +    qemu_set_fd_handler(fd, vfio_req_notifier_handler, NULL, vdev);
>
> -    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);
> +    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0,
> +                           VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
> +        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        qemu_set_fd_handler(fd, 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;
> +    Error *err = NULL;
>
>      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");
> +    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0,
> +                               VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
> +        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>      }
> -    g_free(irq_set);
>      qemu_set_fd_handler(event_notifier_get_fd(&vdev->req_notifier),
>                          NULL, NULL, vdev);
>      event_notifier_cleanup(&vdev->req_notifier);
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index d52d6552e0..8d6012857e 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -106,26 +106,19 @@ static int vfio_set_trigger_eventfd(VFIOINTp *intp,
>                                      eventfd_user_side_handler_t handler)
>  {
>      VFIODevice *vbasedev = &intp->vdev->vbasedev;
> -    struct vfio_irq_set *irq_set;
> -    int argsz, ret;
> -    int32_t *pfd;
> +    int32_t fd = event_notifier_get_fd(intp->interrupt);
> +    Error *err = NULL;
> +    int 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 = intp->pin;
> -    irq_set->start = 0;
> -    irq_set->count = 1;
> -    pfd = (int32_t *)&irq_set->data;
> -    *pfd = event_notifier_get_fd(intp->interrupt);
> -    qemu_set_fd_handler(*pfd, (IOHandler *)handler, NULL, intp);
> -    ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
> -    if (ret < 0) {
> -        error_report("vfio: Failed to set trigger eventfd: %m");
> -        qemu_set_fd_handler(*pfd, NULL, NULL, NULL);
> +    qemu_set_fd_handler(fd, (IOHandler *)handler, NULL, intp);
> +
> +    ret = vfio_set_irq_signaling(vbasedev, intp->pin, 0,
> +                                 VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err);
> +    if (ret) {
> +        error_reportf_err(err, VFIO_MSG_PREFIX, vbasedev->name);
> +        qemu_set_fd_handler(fd, NULL, NULL, NULL);
>      }
> -    g_free(irq_set);
> +
>      return ret;
>  }
>
> @@ -330,7 +323,6 @@ static void vfio_platform_eoi(VFIODevice *vbasedev)
>
>  static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq
> irq)
>  {
> -    int ret;
>      VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
>      VFIOINTp *intp;
>
> @@ -341,10 +333,7 @@ static void vfio_start_eventfd_injection(SysBusDevice
> *sbdev, qemu_irq irq)
>      }
>      assert(intp);
>
> -    ret = vfio_set_trigger_eventfd(intp, vfio_intp_interrupt);
> -    if (ret) {
> -        error_report("vfio: failed to start eventfd signaling for IRQ %d:
> %m",
> -                     intp->pin);
> +    if (vfio_set_trigger_eventfd(intp, vfio_intp_interrupt)) {
>          abort();
>      }
>  }
> @@ -361,25 +350,16 @@ static void
> vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq irq)
>   */
>  static int vfio_set_resample_eventfd(VFIOINTp *intp)
>  {
> +    int32_t fd = event_notifier_get_fd(intp->unmask);
>      VFIODevice *vbasedev = &intp->vdev->vbasedev;
> -    struct vfio_irq_set *irq_set;
> -    int argsz, ret;
> -    int32_t *pfd;
> +    Error *err = NULL;
> +    int 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_UNMASK;
> -    irq_set->index = intp->pin;
> -    irq_set->start = 0;
> -    irq_set->count = 1;
> -    pfd = (int32_t *)&irq_set->data;
> -    *pfd = event_notifier_get_fd(intp->unmask);
> -    qemu_set_fd_handler(*pfd, NULL, NULL, NULL);
> -    ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
> -    g_free(irq_set);
> -    if (ret < 0) {
> -        error_report("vfio: Failed to set resample eventfd: %m");
> +    qemu_set_fd_handler(fd, NULL, NULL, NULL);
> +    ret = vfio_set_irq_signaling(vbasedev, intp->pin, 0,
> +                                 VFIO_IRQ_SET_ACTION_UNMASK, fd, &err);
> +    if (ret) {
> +        error_reportf_err(err, VFIO_MSG_PREFIX, vbasedev->name);
>      }
>      return ret;
>  }
> @@ -435,8 +415,6 @@ static void vfio_start_irqfd_injection(SysBusDevice
> *sbdev, qemu_irq irq)
>      return;
>  fail_vfio:
>      kvm_irqchip_remove_irqfd_notifier(kvm_state, intp->interrupt, irq);
> -    error_report("vfio: failed to start eventfd signaling for IRQ %d: %m",
> -                 intp->pin);
>      abort();
>  fail_irqfd:
>      vfio_start_eventfd_injection(sbdev, irq);
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 1155b79678..686d99ff8c 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -167,6 +167,8 @@ void vfio_put_base_device(VFIODevice *vbasedev);
>  void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
>  void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
>  void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index);
> +int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
> +                           int action, int fd, Error **errp);
>  void vfio_region_write(void *opaque, hwaddr addr,
>                             uint64_t data, unsigned size);
>  uint64_t vfio_region_read(void *opaque,
> --
> 2.20.1
>
>
>

Re: [Qemu-devel] [PATCH v4] vfio/common: Introduce vfio_set_irq_signaling helper

Posted by Auger Eric 1 week ago
Hi Li,

On 6/12/19 10:24 AM, Li Qiang wrote:
> Eric Auger <eric.auger@redhat.com> 于2019年6月12日周三 下午4:14写道:
> 
>> The code used to assign an interrupt index/subindex to an
>> eventfd is duplicated many times. Let's introduce an helper that
>> allows to set/unset the signaling for an ACTION_TRIGGER,
>> ACTION_MASK or ACTION_UNMASK action.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v3 -> v4:
>> - renamed irq_to_str into index_to_str
>> - avoid usage of g_strdup_printf
>> - print both the index and subindex in generic case
>>
>> v2 -> v3:
>> - irq_to_str() simply outputs the IRQ index if the VFIO device is
>>   not of PCI type
>> - removed "vfio: failed to start eventfd signaling ../.." in vfio_platform
>>
>> v1 -> v2:
>> - don't call GET_IRQ_INFO in vfio_set_irq_signaling()
>>   and restore quiet check in vfio_register_req_notifier.
>>   Nicer display of the IRQ name.
>>
>> This is a follow-up to
>> [PATCH v2 0/2] vfio-pci: Introduce vfio_set_event_handler().
>> It looks to me that introducing vfio_set_irq_signaling() has more
>> benefits in term of code reduction and the helper abstraction
>> looks cleaner.
>> ---
>>  hw/vfio/common.c              |  76 ++++++++++++
>>  hw/vfio/pci.c                 | 217 ++++++++--------------------------
>>  hw/vfio/platform.c            |  62 ++++------
>>  include/hw/vfio/vfio-common.h |   2 +
>>  4 files changed, 149 insertions(+), 208 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 4374cc6176..4b18eefe99 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -95,6 +95,82 @@ void vfio_mask_single_irqindex(VFIODevice *vbasedev,
>> int index)
>>      ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, &irq_set);
>>  }
>>
>> +static inline const char *action_to_str(int action)
>> +{
>> +    switch (action) {
>> +    case VFIO_IRQ_SET_ACTION_MASK:
>> +        return "MASK";
>> +    case VFIO_IRQ_SET_ACTION_UNMASK:
>> +        return "UNMASK";
>> +    case VFIO_IRQ_SET_ACTION_TRIGGER:
>> +        return "TRIGGER";
>> +    default:
>> +        return "UNKNOWN ACTION";
>> +    }
>> +}
>> +
>> +static const char *index_to_str(VFIODevice *vbasedev, int index)
>> +{
>> +    if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
>> +        return NULL;
>> +    }
>> +
>> +    switch (index) {
>> +    case VFIO_PCI_INTX_IRQ_INDEX:
>> +        return "INTX";
>> +    case VFIO_PCI_MSI_IRQ_INDEX:
>> +        return "MSI";
>> +    case VFIO_PCI_MSIX_IRQ_INDEX:
>> +        return "MSIX";
>> +    case VFIO_PCI_ERR_IRQ_INDEX:
>> +        return "ERR";
>> +    case VFIO_PCI_REQ_IRQ_INDEX:
>> +        return "REQ";
>> +    default:
>> +        return NULL;
>> +    }
>> +}
>> +
>> +int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
>> +                           int action, int fd, Error **errp)
>> +{
>> +    struct vfio_irq_set *irq_set;
>> +    int argsz, ret = 0;
>> +    const char *name;
>> +    int32_t *pfd;
>> +
>> +    argsz = sizeof(*irq_set) + sizeof(*pfd);
>> +
>> +    irq_set = g_malloc0(argsz);
>> +    irq_set->argsz = argsz;
>> +    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | action;
>> +    irq_set->index = index;
>> +    irq_set->start = subindex;
>> +    irq_set->count = 1;
>> +    pfd = (int32_t *)&irq_set->data;
>> +    *pfd = fd;
>> +
>> +    ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> +
>> +    g_free(irq_set);
>> +
>> +    if (!ret) {
>> +        return 0;
>> +    }
>> +
>> +    error_setg_errno(errp, -ret, "VFIO_DEVICE_SET_IRQS failure");
>>
> 
> Hello Eric,
> 
> I noticed an issue several days ago not related with this patch(maybe split
> in another patch).
> IIUC, here the 'ret' is -1 if the ioctl fails.
> It seems we should use 'errno', however, can we make sure the 'g_free'
> will not touch 'errno'? If it doesn't touch, we can just use errno here, if
> it touch,
> we may save errno right after ioctl call.

Hum you're right. I will fix that.

Thanks!

Eric
> 
> Thanks,
> Li Qiang
> 
> 
> 
> 
>> +    name = index_to_str(vbasedev, index);
>> +    if (name) {
>> +        error_prepend(errp, "%s-%d: ", name, subindex);
>> +    } else {
>> +        error_prepend(errp, "index %d-%d: ", index, subindex);
>> +    }
>> +    error_prepend(errp,
>> +                  "Failed to %s %s eventfd signaling for interrupt ",
>> +                  fd < 0 ? "tear down" : "set up", action_to_str(action));
>> +    return ret;
>> +}
>> +
>>  /*
>>   * IO Port/MMIO - Beware of the endians, VFIO is always little endian
>>   */
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 2a4091d216..65c2e17028 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -113,9 +113,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev,
>> Error **errp)
>>          .gsi = vdev->intx.route.irq,
>>          .flags = KVM_IRQFD_FLAG_RESAMPLE,
>>      };
>> -    struct vfio_irq_set *irq_set;
>> -    int ret, argsz;
>> -    int32_t *pfd;
>> +    Error *err = NULL;
>>
>>      if (vdev->no_kvm_intx || !kvm_irqfds_enabled() ||
>>          vdev->intx.route.mode != PCI_INTX_ENABLED ||
>> @@ -143,22 +141,10 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice
>> *vdev, Error **errp)
>>          goto fail_irqfd;
>>      }
>>
>> -    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_UNMASK;
>> -    irq_set->index = VFIO_PCI_INTX_IRQ_INDEX;
>> -    irq_set->start = 0;
>> -    irq_set->count = 1;
>> -    pfd = (int32_t *)&irq_set->data;
>> -
>> -    *pfd = irqfd.resamplefd;
>> -
>> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> -    g_free(irq_set);
>> -    if (ret) {
>> -        error_setg_errno(errp, -ret, "failed to setup INTx unmask fd");
>> +    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX,
>> 0,
>> +                               VFIO_IRQ_SET_ACTION_UNMASK,
>> +                               irqfd.resamplefd, &err)) {
>> +        error_propagate(errp, err);
>>          goto fail_vfio;
>>      }
>>
>> @@ -262,10 +248,10 @@ 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;
>> +    int32_t fd;
>> +    int ret;
>> +
>>
>>      if (!pin) {
>>          return 0;
>> @@ -292,27 +278,15 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev,
>> Error **errp)
>>          error_setg_errno(errp, -ret, "event_notifier_init failed");
>>          return ret;
>>      }
>> +    fd = event_notifier_get_fd(&vdev->intx.interrupt);
>> +    qemu_set_fd_handler(fd, vfio_intx_interrupt, NULL, vdev);
>>
>> -    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);
>> +    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX,
>> 0,
>> +                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
>> +        error_propagate(errp, err);
>> +        qemu_set_fd_handler(fd, NULL, NULL, vdev);
>>          event_notifier_cleanup(&vdev->intx.interrupt);
>> -        retval = -errno;
>> -        goto cleanup;
>> +        return -errno;
>>      }
>>
>>      vfio_intx_enable_kvm(vdev, &err);
>> @@ -323,11 +297,7 @@ 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;
>> +    return 0;
>>  }
>>
>>  static void vfio_intx_disable(VFIOPCIDevice *vdev)
>> @@ -530,31 +500,19 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev,
>> unsigned int nr,
>>              error_report("vfio: failed to enable vectors, %d", ret);
>>          }
>>      } else {
>> -        int argsz;
>> -        struct vfio_irq_set *irq_set;
>> -        int32_t *pfd;
>> -
>> -        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_MSIX_IRQ_INDEX;
>> -        irq_set->start = nr;
>> -        irq_set->count = 1;
>> -        pfd = (int32_t *)&irq_set->data;
>> +        Error *err = NULL;
>> +        int32_t fd;
>>
>>          if (vector->virq >= 0) {
>> -            *pfd = event_notifier_get_fd(&vector->kvm_interrupt);
>> +            fd = event_notifier_get_fd(&vector->kvm_interrupt);
>>          } else {
>> -            *pfd = event_notifier_get_fd(&vector->interrupt);
>> +            fd = event_notifier_get_fd(&vector->interrupt);
>>          }
>>
>> -        ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> -        g_free(irq_set);
>> -        if (ret) {
>> -            error_report("vfio: failed to modify vector, %d", ret);
>> +        if (vfio_set_irq_signaling(&vdev->vbasedev,
>> +                                     VFIO_PCI_MSIX_IRQ_INDEX, nr,
>> +                                     VFIO_IRQ_SET_ACTION_TRIGGER, fd,
>> &err)) {
>> +            error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>>          }
>>      }
>>
>> @@ -591,26 +549,10 @@ static void vfio_msix_vector_release(PCIDevice
>> *pdev, unsigned int nr)
>>       * be re-asserted on unmask.  Nothing to do if already using QEMU
>> mode.
>>       */
>>      if (vector->virq >= 0) {
>> -        int argsz;
>> -        struct vfio_irq_set *irq_set;
>> -        int32_t *pfd;
>> +        int32_t fd = event_notifier_get_fd(&vector->interrupt);
>>
>> -        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_MSIX_IRQ_INDEX;
>> -        irq_set->start = nr;
>> -        irq_set->count = 1;
>> -        pfd = (int32_t *)&irq_set->data;
>> -
>> -        *pfd = event_notifier_get_fd(&vector->interrupt);
>> -
>> -        ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> -
>> -        g_free(irq_set);
>> +        vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX,
>> nr,
>> +                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, NULL);
>>      }
>>  }
>>
>> @@ -2636,10 +2578,8 @@ static void vfio_err_notifier_handler(void *opaque)
>>   */
>>  static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
>>  {
>> -    int ret;
>> -    int argsz;
>> -    struct vfio_irq_set *irq_set;
>> -    int32_t *pfd;
>> +    Error *err = NULL;
>> +    int32_t fd;
>>
>>      if (!vdev->pci_aer) {
>>          return;
>> @@ -2651,58 +2591,30 @@ static void
>> vfio_register_err_notifier(VFIOPCIDevice *vdev)
>>          return;
>>      }
>>
>> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
>> +    fd = event_notifier_get_fd(&vdev->err_notifier);
>> +    qemu_set_fd_handler(fd, vfio_err_notifier_handler, NULL, vdev);
>>
>> -    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);
>> +    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0,
>> +                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
>> +        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> +        qemu_set_fd_handler(fd, 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;
>> +    Error *err = NULL;
>>
>>      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");
>> +    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0,
>> +                               VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
>> +        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>>      }
>> -    g_free(irq_set);
>>      qemu_set_fd_handler(event_notifier_get_fd(&vdev->err_notifier),
>>                          NULL, NULL, vdev);
>>      event_notifier_cleanup(&vdev->err_notifier);
>> @@ -2727,9 +2639,8 @@ 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;
>> +    Error *err = NULL;
>> +    int32_t fd;
>>
>>      if (!(vdev->features & VFIO_FEATURE_ENABLE_REQ)) {
>>          return;
>> @@ -2745,57 +2656,31 @@ static void
>> vfio_register_req_notifier(VFIOPCIDevice *vdev)
>>          return;
>>      }
>>
>> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
>> +    fd = event_notifier_get_fd(&vdev->req_notifier);
>> +    qemu_set_fd_handler(fd, vfio_req_notifier_handler, NULL, vdev);
>>
>> -    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);
>> +    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0,
>> +                           VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
>> +        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> +        qemu_set_fd_handler(fd, 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;
>> +    Error *err = NULL;
>>
>>      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");
>> +    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0,
>> +                               VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
>> +        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>>      }
>> -    g_free(irq_set);
>>      qemu_set_fd_handler(event_notifier_get_fd(&vdev->req_notifier),
>>                          NULL, NULL, vdev);
>>      event_notifier_cleanup(&vdev->req_notifier);
>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
>> index d52d6552e0..8d6012857e 100644
>> --- a/hw/vfio/platform.c
>> +++ b/hw/vfio/platform.c
>> @@ -106,26 +106,19 @@ static int vfio_set_trigger_eventfd(VFIOINTp *intp,
>>                                      eventfd_user_side_handler_t handler)
>>  {
>>      VFIODevice *vbasedev = &intp->vdev->vbasedev;
>> -    struct vfio_irq_set *irq_set;
>> -    int argsz, ret;
>> -    int32_t *pfd;
>> +    int32_t fd = event_notifier_get_fd(intp->interrupt);
>> +    Error *err = NULL;
>> +    int 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 = intp->pin;
>> -    irq_set->start = 0;
>> -    irq_set->count = 1;
>> -    pfd = (int32_t *)&irq_set->data;
>> -    *pfd = event_notifier_get_fd(intp->interrupt);
>> -    qemu_set_fd_handler(*pfd, (IOHandler *)handler, NULL, intp);
>> -    ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> -    if (ret < 0) {
>> -        error_report("vfio: Failed to set trigger eventfd: %m");
>> -        qemu_set_fd_handler(*pfd, NULL, NULL, NULL);
>> +    qemu_set_fd_handler(fd, (IOHandler *)handler, NULL, intp);
>> +
>> +    ret = vfio_set_irq_signaling(vbasedev, intp->pin, 0,
>> +                                 VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err);
>> +    if (ret) {
>> +        error_reportf_err(err, VFIO_MSG_PREFIX, vbasedev->name);
>> +        qemu_set_fd_handler(fd, NULL, NULL, NULL);
>>      }
>> -    g_free(irq_set);
>> +
>>      return ret;
>>  }
>>
>> @@ -330,7 +323,6 @@ static void vfio_platform_eoi(VFIODevice *vbasedev)
>>
>>  static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq
>> irq)
>>  {
>> -    int ret;
>>      VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
>>      VFIOINTp *intp;
>>
>> @@ -341,10 +333,7 @@ static void vfio_start_eventfd_injection(SysBusDevice
>> *sbdev, qemu_irq irq)
>>      }
>>      assert(intp);
>>
>> -    ret = vfio_set_trigger_eventfd(intp, vfio_intp_interrupt);
>> -    if (ret) {
>> -        error_report("vfio: failed to start eventfd signaling for IRQ %d:
>> %m",
>> -                     intp->pin);
>> +    if (vfio_set_trigger_eventfd(intp, vfio_intp_interrupt)) {
>>          abort();
>>      }
>>  }
>> @@ -361,25 +350,16 @@ static void
>> vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq irq)
>>   */
>>  static int vfio_set_resample_eventfd(VFIOINTp *intp)
>>  {
>> +    int32_t fd = event_notifier_get_fd(intp->unmask);
>>      VFIODevice *vbasedev = &intp->vdev->vbasedev;
>> -    struct vfio_irq_set *irq_set;
>> -    int argsz, ret;
>> -    int32_t *pfd;
>> +    Error *err = NULL;
>> +    int 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_UNMASK;
>> -    irq_set->index = intp->pin;
>> -    irq_set->start = 0;
>> -    irq_set->count = 1;
>> -    pfd = (int32_t *)&irq_set->data;
>> -    *pfd = event_notifier_get_fd(intp->unmask);
>> -    qemu_set_fd_handler(*pfd, NULL, NULL, NULL);
>> -    ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> -    g_free(irq_set);
>> -    if (ret < 0) {
>> -        error_report("vfio: Failed to set resample eventfd: %m");
>> +    qemu_set_fd_handler(fd, NULL, NULL, NULL);
>> +    ret = vfio_set_irq_signaling(vbasedev, intp->pin, 0,
>> +                                 VFIO_IRQ_SET_ACTION_UNMASK, fd, &err);
>> +    if (ret) {
>> +        error_reportf_err(err, VFIO_MSG_PREFIX, vbasedev->name);
>>      }
>>      return ret;
>>  }
>> @@ -435,8 +415,6 @@ static void vfio_start_irqfd_injection(SysBusDevice
>> *sbdev, qemu_irq irq)
>>      return;
>>  fail_vfio:
>>      kvm_irqchip_remove_irqfd_notifier(kvm_state, intp->interrupt, irq);
>> -    error_report("vfio: failed to start eventfd signaling for IRQ %d: %m",
>> -                 intp->pin);
>>      abort();
>>  fail_irqfd:
>>      vfio_start_eventfd_injection(sbdev, irq);
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 1155b79678..686d99ff8c 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -167,6 +167,8 @@ void vfio_put_base_device(VFIODevice *vbasedev);
>>  void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
>>  void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
>>  void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index);
>> +int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
>> +                           int action, int fd, Error **errp);
>>  void vfio_region_write(void *opaque, hwaddr addr,
>>                             uint64_t data, unsigned size);
>>  uint64_t vfio_region_read(void *opaque,
>> --
>> 2.20.1
>>
>>
>>