fix IRQ disable path (#386)

Properly fix IRQ disabling:

Allow count == 0 to mean "disable all IRQS of the given type". On our side,
disabling an IRQ means forgetting about the eventfd that was previously passed
over the socket.

Allow individual IRQs to be disabled, by means of a VFIO_IRQ_SET_DATA_EVENTFD
message with no file descriptors passed. In vfio, this is done via setting "-1"
in the fd slots; which isn't possible via auxiliary data. Thus, only one IRQ can
be disabled a a time in vfio-user.

Clean up "->type": this is never set, so wasn't having any effect. Follow up
changes will likely re-introduce this in some form.

Signed-off-by: John Levon <john.levon@nutanix.com>
Reviewed-by: Thanos Makatos <thanos.makatos@nutanix.com>

3 files changed, 138 insertions, 178 deletions

diff --git a/lib/irq.c b/lib/irq.c
index 050453e..7ea0349 100644
--- a/lib/irq.c
+++ b/lib/irq.c
@@ -54,52 +54,87 @@ vfio_irq_idx_to_str(int index)
 }
 
 static long
-irqs_disable(vfu_ctx_t *vfu_ctx, uint32_t index)
+dev_get_irqinfo(vfu_ctx_t *vfu_ctx, struct vfio_irq_info *irq_info_in,
+                struct vfio_irq_info *irq_info_out)
+{
+    assert(vfu_ctx != NULL);
+    assert(irq_info_in != NULL);
+    assert(irq_info_out != NULL);
+
+    // Ensure provided argsz is sufficiently big and index is within bounds.
+    if ((irq_info_in->argsz < sizeof(struct vfio_irq_info)) ||
+        (irq_info_in->index >= VFU_DEV_NUM_IRQS)) {
+        vfu_log(vfu_ctx, LOG_DEBUG, "bad irq_info (size=%d index=%d)\n",
+                irq_info_in->argsz, irq_info_in->index);
+        return -EINVAL;
+    }
+
+    irq_info_out->count = vfu_ctx->irq_count[irq_info_in->index];
+    irq_info_out->flags = VFIO_IRQ_INFO_EVENTFD;
+
+    return 0;
+}
+
+int
+handle_device_get_irq_info(vfu_ctx_t *vfu_ctx, uint32_t size,
+                           struct vfio_irq_info *irq_info_in,
+                           struct vfio_irq_info *irq_info_out)
+{
+    assert(vfu_ctx != NULL);
+    assert(irq_info_in != NULL);
+    assert(irq_info_out != NULL);
+
+    if (size != sizeof(*irq_info_in) || size != irq_info_in->argsz) {
+        vfu_log(vfu_ctx, LOG_WARNING, "IRQ info size %d", size);
+        return -EINVAL;
+    }
+
+    return dev_get_irqinfo(vfu_ctx, irq_info_in, irq_info_out);
+}
+
+static long
+irqs_disable(vfu_ctx_t *vfu_ctx, uint32_t index, uint32_t start, uint32_t count)
 {
-    int *irq_efd = NULL;
-    uint32_t i;
+    size_t i;
+    int *efds;
 
     assert(vfu_ctx != NULL);
     assert(index < VFU_DEV_NUM_IRQS);
+    assert(start + count <= vfu_ctx->irq_count[index]);
+
+    vfu_log(vfu_ctx, LOG_DEBUG, "disabling IRQ type %s range [%u-%u)",
+            vfio_irq_idx_to_str(index), start, start + count);
+
+    if (count == 0) {
+        count = vfu_ctx->irq_count[index];
+    }
 
     switch (index) {
     case VFIO_PCI_INTX_IRQ_INDEX:
     case VFIO_PCI_MSI_IRQ_INDEX:
     case VFIO_PCI_MSIX_IRQ_INDEX:
-        vfu_log(vfu_ctx, LOG_DEBUG, "disabling IRQ %s",
-                vfio_irq_idx_to_str(index));
-        vfu_ctx->irqs->type = IRQ_NONE;
-        for (i = 0; i < vfu_ctx->irqs->max_ivs; i++) {
-            if (vfu_ctx->irqs->efds[i] >= 0) {
-                if (close(vfu_ctx->irqs->efds[i]) == -1) {
-                    vfu_log(vfu_ctx, LOG_DEBUG, "failed to close IRQ fd %d: %m",
-                           vfu_ctx->irqs->efds[i]);
-                }
-                vfu_ctx->irqs->efds[i] = -1;
-            }
-        }
-        return 0;
+        efds = vfu_ctx->irqs[index].efds;
+        break;
     case VFIO_PCI_ERR_IRQ_INDEX:
-        irq_efd = &vfu_ctx->irqs->err_efd;
+        efds = &vfu_ctx->irqs->err_efd;
         break;
     case VFIO_PCI_REQ_IRQ_INDEX:
-        irq_efd = &vfu_ctx->irqs->req_efd;
+        efds = &vfu_ctx->irqs->req_efd;
         break;
     }
 
-    if (irq_efd != NULL) {
-        if (*irq_efd != -1) {
-            if (close(*irq_efd) == -1) {
+    for (i = start; i < count; i++) {
+        if (efds[i] >= 0) {
+            if (close(efds[i]) == -1) {
                 vfu_log(vfu_ctx, LOG_DEBUG, "failed to close IRQ fd %d: %m",
-                       *irq_efd);
+                        efds[i]);
             }
-            *irq_efd = -1;
+
+            efds[i] = -1;
         }
-        return 0;
     }
 
-    vfu_log(vfu_ctx, LOG_DEBUG, "failed to disable IRQs");
-    return -EINVAL;
+    return 0;
 }
 
 static int
@@ -185,39 +220,11 @@ irqs_set_data_eventfd(vfu_ctx_t *vfu_ctx, struct vfio_irq_set *irq_set,
 }
 
 static long
-irqs_trigger(vfu_ctx_t *vfu_ctx, struct vfio_irq_set *irq_set, void *data)
-{
-    int err = 0;
-
-    assert(vfu_ctx != NULL);
-    assert(irq_set != NULL);
-
-    if (irq_set->count == 0) {
-        return irqs_disable(vfu_ctx, irq_set->index);
-    }
-
-    vfu_log(vfu_ctx, LOG_DEBUG, "setting IRQ %s flags=%#x",
-            vfio_irq_idx_to_str(irq_set->index), irq_set->flags);
-
-    switch (irq_set->flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
-    case VFIO_IRQ_SET_DATA_NONE:
-        err = irqs_set_data_none(vfu_ctx, irq_set);
-        break;
-    case VFIO_IRQ_SET_DATA_BOOL:
-        err = irqs_set_data_bool(vfu_ctx, irq_set, data);
-        break;
-    case VFIO_IRQ_SET_DATA_EVENTFD:
-        err = irqs_set_data_eventfd(vfu_ctx, irq_set, data);
-        break;
-    }
-
-    return err;
-}
-
-static long
-dev_set_irqs_validate(vfu_ctx_t *vfu_ctx, struct vfio_irq_set *irq_set)
+device_set_irqs_validate(vfu_ctx_t *vfu_ctx, struct vfio_irq_set *irq_set,
+                         size_t nr_fds)
 {
     uint32_t a_type, d_type;
+    int line;
 
     assert(vfu_ctx != NULL);
     assert(irq_set != NULL);
@@ -228,162 +235,122 @@ dev_set_irqs_validate(vfu_ctx_t *vfu_ctx, struct vfio_irq_set *irq_set)
 
     // Ensure index is within bounds.
     if (irq_set->index >= VFU_DEV_NUM_IRQS) {
-        vfu_log(vfu_ctx, LOG_DEBUG, "bad IRQ index %d\n", irq_set->index);
-        return -EINVAL;
+        line = __LINE__;
+        goto invalid;
     }
 
-    /* TODO make each condition a function */
-
     // Only one of MASK/UNMASK/TRIGGER is valid.
     if ((a_type != VFIO_IRQ_SET_ACTION_MASK) &&
         (a_type != VFIO_IRQ_SET_ACTION_UNMASK) &&
         (a_type != VFIO_IRQ_SET_ACTION_TRIGGER)) {
-        vfu_log(vfu_ctx, LOG_DEBUG, "bad IRQ action mask %d\n", a_type);
-        return -EINVAL;
+        line = __LINE__;
+        goto invalid;
     }
     // Only one of NONE/BOOL/EVENTFD is valid.
     if ((d_type != VFIO_IRQ_SET_DATA_NONE) &&
         (d_type != VFIO_IRQ_SET_DATA_BOOL) &&
         (d_type != VFIO_IRQ_SET_DATA_EVENTFD)) {
-        vfu_log(vfu_ctx, LOG_DEBUG, "bad IRQ data %d\n", d_type);
-        return -EINVAL;
+        line = __LINE__;
+        goto invalid;
     }
     // Ensure irq_set's start and count are within bounds.
     if ((irq_set->start >= vfu_ctx->irq_count[irq_set->index]) ||
         (irq_set->start + irq_set->count > vfu_ctx->irq_count[irq_set->index])) {
-        vfu_log(vfu_ctx, LOG_DEBUG, "bad IRQ start/count\n");
-        return -EINVAL;
+        line = __LINE__;
+        goto invalid;
     }
     // Only TRIGGER is valid for ERR/REQ.
     if (((irq_set->index == VFIO_PCI_ERR_IRQ_INDEX) ||
          (irq_set->index == VFIO_PCI_REQ_IRQ_INDEX)) &&
         (a_type != VFIO_IRQ_SET_ACTION_TRIGGER)) {
-        vfu_log(vfu_ctx, LOG_DEBUG, "bad IRQ trigger w/o ERR/REQ\n");
-        return -EINVAL;
+        line = __LINE__;
+        goto invalid;
+    }
+    // if count == 0, start must be 0 too
+    if ((irq_set->count == 0) && (irq_set->start != 0)) {
+        line = __LINE__;
+        goto invalid;
     }
     // count == 0 is only valid with ACTION_TRIGGER and DATA_NONE.
     if ((irq_set->count == 0) && ((a_type != VFIO_IRQ_SET_ACTION_TRIGGER) ||
                                   (d_type != VFIO_IRQ_SET_DATA_NONE))) {
-        vfu_log(vfu_ctx, LOG_DEBUG, "bad IRQ count %d\n", irq_set->count);
-        return -EINVAL;
+        line = __LINE__;
+        goto invalid;
     }
-    // If IRQs are set, ensure index matches what's enabled for the device.
-    if ((irq_set->count != 0) && (vfu_ctx->irqs->type != IRQ_NONE) &&
-        (irq_set->index != LM2VFIO_IRQT(vfu_ctx->irqs->type))) {
-        vfu_log(vfu_ctx, LOG_DEBUG, "bad IRQ index\n");
-        return -EINVAL;
+    // If fd's are provided, ensure it's only for VFIO_IRQ_SET_DATA_EVENTFD
+    if (nr_fds != 0 && d_type != VFIO_IRQ_SET_DATA_EVENTFD) {
+        line = __LINE__;
+        goto invalid;
+    }
+    // If fd's are provided, ensure they match ->count
+    if (nr_fds != 0 && nr_fds != irq_set->count) {
+        line = __LINE__;
+        goto invalid;
     }
 
     return 0;
+
+invalid:
+    vfu_log(vfu_ctx, LOG_DEBUG, "invalid SET_IRQS (%d): action=%u data_type=%u "
+            "index=%u start=%u count=%u nr_fds=%u", line, a_type, d_type,
+            irq_set->index, irq_set->start, irq_set->count, nr_fds);
+    return -EINVAL;
 }
 
-static long
-dev_set_irqs(vfu_ctx_t *vfu_ctx, struct vfio_irq_set *irq_set, void *data)
+int
+handle_device_set_irqs(vfu_ctx_t *vfu_ctx, uint32_t size,
+                       int *fds, size_t nr_fds, struct vfio_irq_set *irq_set)
 {
-    long ret;
+    uint32_t data_type;
+    int ret;
 
     assert(vfu_ctx != NULL);
     assert(irq_set != NULL);
 
-    // Ensure irq_set is valid.
-    ret = dev_set_irqs_validate(vfu_ctx, irq_set);
+    if (size < sizeof(*irq_set) || size != irq_set->argsz) {
+        vfu_log(vfu_ctx, LOG_ERR, "%s: bad size %u", __func__, size);
+        return -EINVAL;
+    }
+
+    ret = device_set_irqs_validate(vfu_ctx, irq_set, nr_fds);
     if (ret != 0) {
         return ret;
     }
 
     switch (irq_set->flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
-    case VFIO_IRQ_SET_ACTION_MASK:     // fallthrough
+    case VFIO_IRQ_SET_ACTION_MASK:
     case VFIO_IRQ_SET_ACTION_UNMASK:
         // We're always edge-triggered without un/mask support.
+        // FIXME: return an error? We don't report MASKABLE
         return 0;
+    case VFIO_IRQ_SET_ACTION_TRIGGER:
+        break;
     }
 
-    return irqs_trigger(vfu_ctx, irq_set, data);
-}
-
-static long
-dev_get_irqinfo(vfu_ctx_t *vfu_ctx, struct vfio_irq_info *irq_info_in,
-                struct vfio_irq_info *irq_info_out)
-{
-    assert(vfu_ctx != NULL);
-    assert(irq_info_in != NULL);
-    assert(irq_info_out != NULL);
-
-    // Ensure provided argsz is sufficiently big and index is within bounds.
-    if ((irq_info_in->argsz < sizeof(struct vfio_irq_info)) ||
-        (irq_info_in->index >= VFU_DEV_NUM_IRQS)) {
-        vfu_log(vfu_ctx, LOG_DEBUG, "bad irq_info (size=%d index=%d)\n",
-                irq_info_in->argsz, irq_info_in->index);
-        return -EINVAL;
-    }
-
-    irq_info_out->count = vfu_ctx->irq_count[irq_info_in->index];
-    irq_info_out->flags = VFIO_IRQ_INFO_EVENTFD;
-
-    return 0;
-}
+    data_type = irq_set->flags & VFIO_IRQ_SET_DATA_TYPE_MASK;
 
-int
-handle_device_get_irq_info(vfu_ctx_t *vfu_ctx, uint32_t size,
-                           struct vfio_irq_info *irq_info_in,
-                           struct vfio_irq_info *irq_info_out)
-{
-    assert(vfu_ctx != NULL);
-    assert(irq_info_in != NULL);
-    assert(irq_info_out != NULL);
-
-    if (size != sizeof(*irq_info_in) || size != irq_info_in->argsz) {
-        vfu_log(vfu_ctx, LOG_WARNING, "IRQ info size %d", size);
-        return -EINVAL;
+    if ((data_type == VFIO_IRQ_SET_DATA_NONE && irq_set->count == 0) ||
+        (data_type == VFIO_IRQ_SET_DATA_EVENTFD && nr_fds == 0)) {
+        return irqs_disable(vfu_ctx, irq_set->index,
+                            irq_set->start, irq_set->count);
     }
 
-    return dev_get_irqinfo(vfu_ctx, irq_info_in, irq_info_out);
-}
+    vfu_log(vfu_ctx, LOG_DEBUG, "setting IRQ %s flags=%#x range [%u-%u)",
+            vfio_irq_idx_to_str(irq_set->index), irq_set->flags,
+            irq_set->start, irq_set->start + irq_set->count);
 
-int
-handle_device_set_irqs(vfu_ctx_t *vfu_ctx, uint32_t size,
-                       int *fds, size_t nr_fds, struct vfio_irq_set *irq_set)
-{
-    void *data = NULL;
-
-    assert(vfu_ctx != NULL);
-    assert(irq_set != NULL);
-
-    if (size < sizeof(*irq_set) || size != irq_set->argsz) {
-        vfu_log(vfu_ctx, LOG_ERR, "bad size %d", size);
-        return -EINVAL;
-    }
-
-    switch (irq_set->flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
-        case VFIO_IRQ_SET_DATA_NONE:
-            /*
-             * FIXME The way it's handled in this case is a hack, it should be
-             * properly fixed, see issue #359 for more details.
-             */
-            vfu_log(vfu_ctx, LOG_NOTICE,
-                    "ignore IRQ DATA_NONE, action=%#x, index=%ld, start=%ld, count=%ld",
-                    irq_set->flags & VFIO_IRQ_SET_ACTION_TYPE_MASK,
-                    irq_set->index, irq_set->start, irq_set->count);
-            return 0;
-        case VFIO_IRQ_SET_DATA_EVENTFD:
-            data = fds;
-            if (nr_fds != irq_set->count) {
-                vfu_log(vfu_ctx, LOG_ERR,
-                        "bad number of FDs, expected=%u, actual=%d", nr_fds,
-                        irq_set->count);
-                return -EINVAL;
-            }
-            break;
-        case VFIO_IRQ_SET_DATA_BOOL:
-            data = irq_set + 1;
-            break;
-        default:
-            vfu_log(vfu_ctx, LOG_ERR, "invalid IRQ type %d",
-                    irq_set->flags & VFIO_IRQ_SET_DATA_TYPE_MASK);
-            return -EINVAL;
+    switch (data_type) {
+    case VFIO_IRQ_SET_DATA_NONE:
+        return irqs_set_data_none(vfu_ctx, irq_set);
+    case VFIO_IRQ_SET_DATA_EVENTFD:
+        return irqs_set_data_eventfd(vfu_ctx, irq_set, fds);
+    case VFIO_IRQ_SET_DATA_BOOL:
+        return irqs_set_data_bool(vfu_ctx, irq_set, irq_set + 1);
+        break;
+    default:
+        // we already checked this
+        abort();
     }
-
-    return dev_set_irqs(vfu_ctx, irq_set, data);
 }
 
 static bool
diff --git a/lib/libvfio-user.c b/lib/libvfio-user.c
index 77c9f0c..8df125f 100644
--- a/lib/libvfio-user.c
+++ b/lib/libvfio-user.c
@@ -1037,7 +1037,7 @@ vfu_realize_ctx(vfu_ctx_t *vfu_ctx)
             }
         }
 
-        //FIXME: assert(max_ivs > 0)?
+        // FIXME: assert(max_ivs > 0)?
         size = sizeof(int) * max_ivs;
         vfu_ctx->irqs = calloc(1, sizeof(vfu_irqs_t) + size);
         if (vfu_ctx->irqs == NULL) {
@@ -1051,7 +1051,6 @@ vfu_realize_ctx(vfu_ctx_t *vfu_ctx)
         }
         vfu_ctx->irqs->err_efd = -1;
         vfu_ctx->irqs->req_efd = -1;
-        vfu_ctx->irqs->type = IRQ_NONE;
         vfu_ctx->irqs->max_ivs = max_ivs;
 
         // Reflect on the config space whether INTX is available.
@@ -1423,10 +1422,8 @@ vfu_setup_device_nr_irqs(vfu_ctx_t *vfu_ctx, enum vfu_dev_irq_type type,
 
     assert(vfu_ctx != NULL);
 
-    if (type < VFU_DEV_INTX_IRQ || type > VFU_DEV_REQ_IRQ) {
-        vfu_log(vfu_ctx, LOG_ERR, "Invalid IRQ index %d, should be between "
-               "(%d to %d)", type, VFU_DEV_INTX_IRQ,
-               VFU_DEV_REQ_IRQ);
+    if (type >= VFU_DEV_NUM_IRQS) {
+        vfu_log(vfu_ctx, LOG_ERR, "Invalid IRQ type index %u", type);
         return ERROR_INT(EINVAL);
     }
 
diff --git a/lib/private.h b/lib/private.h
index 8e068c6..60bbba9 100644
--- a/lib/private.h
+++ b/lib/private.h
@@ -77,19 +77,11 @@ struct transport_ops {
     void (*fini)(vfu_ctx_t *vfu_ctx);
 };
 
-typedef enum {
-    IRQ_NONE = 0,
-    IRQ_INTX,
-    IRQ_MSI,
-    IRQ_MSIX,
-} irq_type_t;
-
 typedef struct {
-    irq_type_t  type;       /* irq type this device is using */
     int         err_efd;    /* eventfd for irq err */
     int         req_efd;    /* eventfd for irq req */
     uint32_t    max_ivs;    /* maximum number of ivs supported */
-    int         efds[0];    /* XXX must be last */
+    int         efds[0];    /* must be last */
 } vfu_irqs_t;
 
 struct migration;
@@ -190,6 +182,10 @@ cmd_allowed_when_stopped_and_copying(uint16_t cmd);
 bool
 should_exec_command(vfu_ctx_t *vfu_ctx, uint16_t cmd);
 
+int
+handle_device_set_irqs(vfu_ctx_t *vfu_ctx, uint32_t size,
+                       int *fds, size_t nr_fds, struct vfio_irq_set *irq_set);
+
 #endif /* LIB_VFIO_USER_PRIVATE_H */
 
 /* ex: set tabstop=4 shiftwidth=4 softtabstop=4 expandtab: */