[Qemu-devel] [PATCH 0/3] qemu-kvm/vhost: fix qemu assert triggerable by guest

[Qemu-devel] [PATCH 0/3] qemu-kvm/vhost: fix qemu assert triggerable by guest

[Michael S. Tsirkin]

When guest maps a vector to multiple vqs, qemu kvm triggers an assert.
A simple way to test this is by supplying vectors=2 for
virtio-net-pci in qemu-kvm.

The first 2 patches apply to qemu.git. The 3d does not,
it has to be applied only to qemu-kvm.git.

Avi, marcelo, I think to prevent bisect errors,
the best way to apply this would be to wait
for 1st two ones to get applied on qemu.git, then
apply all three on qemu-kvm.git and then merge.

Michael S. Tsirkin (3):
  virtio: change set guest notifier to per-device
  vhost: error code
  vhost: fix up irqfd support

 hw/msix.c       |   68 +++++++++++++++++++++++++++++++--------------
 hw/msix.h       |    4 +-
 hw/pci.h        |    3 +-
 hw/vhost.c      |   54 ++++++++++++++++++++----------------
 hw/virtio-pci.c |   83 ++++++++++++++++++++++++++++++++++++++++++++++++------
 hw/virtio.h     |    2 +-
 6 files changed, 154 insertions(+), 60 deletions(-)

-- 
1.7.3-rc1

[Qemu-devel] [PATCH 1/3] virtio: change set guest notifier to per-device

[Michael S. Tsirkin]

When using irqfd with vhost-net to inject interrupts,
a single evenfd might inject multiple interrupts.
Implementing this is much easier with a single
per-device callback to set guest notifiers.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/vhost.c      |   52 ++++++++++++++++++++++++++++------------------------
 hw/virtio-pci.c |   29 ++++++++++++++++++++++++++++-
 hw/virtio.h     |    2 +-
 3 files changed, 57 insertions(+), 26 deletions(-)

diff --git a/hw/vhost.c b/hw/vhost.c
index 34c4745..5ac283b 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -455,11 +455,6 @@ static int vhost_virtqueue_init(struct vhost_dev *dev,
     };
     struct VirtQueue *vvq = virtio_get_queue(vdev, idx);
 
-    if (!vdev->binding->set_guest_notifier) {
-        fprintf(stderr, "binding does not support guest notifiers\n");
-        return -ENOSYS;
-    }
-
     if (!vdev->binding->set_host_notifier) {
         fprintf(stderr, "binding does not support host notifiers\n");
         return -ENOSYS;
@@ -512,12 +507,6 @@ static int vhost_virtqueue_init(struct vhost_dev *dev,
         r = -errno;
         goto fail_alloc;
     }
-    r = vdev->binding->set_guest_notifier(vdev->binding_opaque, idx, true);
-    if (r < 0) {
-        fprintf(stderr, "Error binding guest notifier: %d\n", -r);
-        goto fail_guest_notifier;
-    }
-
     r = vdev->binding->set_host_notifier(vdev->binding_opaque, idx, true);
     if (r < 0) {
         fprintf(stderr, "Error binding host notifier: %d\n", -r);
@@ -542,8 +531,6 @@ fail_call:
 fail_kick:
     vdev->binding->set_host_notifier(vdev->binding_opaque, idx, false);
 fail_host_notifier:
-    vdev->binding->set_guest_notifier(vdev->binding_opaque, idx, false);
-fail_guest_notifier:
 fail_alloc:
     cpu_physical_memory_unmap(vq->ring, virtio_queue_get_ring_size(vdev, idx),
                               0, 0);
@@ -569,13 +556,6 @@ static void vhost_virtqueue_cleanup(struct vhost_dev *dev,
         .index = idx,
     };
     int r;
-    r = vdev->binding->set_guest_notifier(vdev->binding_opaque, idx, false);
-    if (r < 0) {
-        fprintf(stderr, "vhost VQ %d guest cleanup failed: %d\n", idx, r);
-        fflush(stderr);
-    }
-    assert (r >= 0);
-
     r = vdev->binding->set_host_notifier(vdev->binding_opaque, idx, false);
     if (r < 0) {
         fprintf(stderr, "vhost VQ %d host cleanup failed: %d\n", idx, r);
@@ -648,15 +628,26 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
 int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
 {
     int i, r;
+    if (!vdev->binding->set_guest_notifiers) {
+        fprintf(stderr, "binding does not support guest notifiers\n");
+        r = -ENOSYS;
+        goto fail;
+    }
+
+    r = vdev->binding->set_guest_notifiers(vdev->binding_opaque, true);
+    if (r < 0) {
+        fprintf(stderr, "Error binding guest notifier: %d\n", -r);
+        goto fail_notifiers;
+    }
 
     r = vhost_dev_set_features(hdev, hdev->log_enabled);
     if (r < 0) {
-        goto fail;
+        goto fail_features;
     }
     r = ioctl(hdev->control, VHOST_SET_MEM_TABLE, hdev->mem);
     if (r < 0) {
         r = -errno;
-        goto fail;
+        goto fail_mem;
     }
     for (i = 0; i < hdev->nvqs; ++i) {
         r = vhost_virtqueue_init(hdev,
@@ -676,13 +667,14 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
                   (uint64_t)(unsigned long)hdev->log);
         if (r < 0) {
             r = -errno;
-            goto fail_vq;
+            goto fail_log;
         }
     }
 
     hdev->started = true;
 
     return 0;
+fail_log:
 fail_vq:
     while (--i >= 0) {
         vhost_virtqueue_cleanup(hdev,
@@ -690,13 +682,18 @@ fail_vq:
                                 hdev->vqs + i,
                                 i);
     }
+fail_mem:
+fail_features:
+    vdev->binding->set_guest_notifiers(vdev->binding_opaque, false);
+fail_notifiers:
 fail:
     return r;
 }
 
 void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
 {
-    int i;
+    int i, r;
+
     for (i = 0; i < hdev->nvqs; ++i) {
         vhost_virtqueue_cleanup(hdev,
                                 vdev,
@@ -705,6 +702,13 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
     }
     vhost_client_sync_dirty_bitmap(&hdev->client, 0,
                                    (target_phys_addr_t)~0x0ull);
+    r = vdev->binding->set_guest_notifiers(vdev->binding_opaque, false);
+    if (r < 0) {
+        fprintf(stderr, "vhost guest notifier cleanup failed: %d\n", r);
+        fflush(stderr);
+    }
+    assert (r >= 0);
+
     hdev->started = false;
     qemu_free(hdev->log);
     hdev->log_size = 0;
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 1357d40..232f943 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -482,6 +482,33 @@ static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign)
     return 0;
 }
 
+static int virtio_pci_set_guest_notifiers(void *opaque, bool assign)
+{
+    VirtIOPCIProxy *proxy = opaque;
+    VirtIODevice *vdev = proxy->vdev;
+    int r, n;
+
+    for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
+        if (!virtio_queue_get_num(vdev, n)) {
+            break;
+        }
+
+        r = virtio_pci_set_guest_notifier(opaque, n, assign);
+        if (r < 0) {
+            goto assign_error;
+        }
+    }
+
+    return 0;
+
+assign_error:
+    /* We get here on assignment failure. Recover by undoing for VQs 0 .. n. */
+    while (--n >= 0) {
+        virtio_pci_set_guest_notifier(opaque, n, !assign);
+    }
+    return r;
+}
+
 static int virtio_pci_set_host_notifier(void *opaque, int n, bool assign)
 {
     VirtIOPCIProxy *proxy = opaque;
@@ -519,7 +546,7 @@ static const VirtIOBindings virtio_pci_bindings = {
     .load_queue = virtio_pci_load_queue,
     .get_features = virtio_pci_get_features,
     .set_host_notifier = virtio_pci_set_host_notifier,
-    .set_guest_notifier = virtio_pci_set_guest_notifier,
+    .set_guest_notifiers = virtio_pci_set_guest_notifiers,
 };
 
 static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
diff --git a/hw/virtio.h b/hw/virtio.h
index 5836ab6..457edb2 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -92,7 +92,7 @@ typedef struct {
     int (*load_config)(void * opaque, QEMUFile *f);
     int (*load_queue)(void * opaque, int n, QEMUFile *f);
     unsigned (*get_features)(void * opaque);
-    int (*set_guest_notifier)(void * opaque, int n, bool assigned);
+    int (*set_guest_notifiers)(void * opaque, bool assigned);
     int (*set_host_notifier)(void * opaque, int n, bool assigned);
 } VirtIOBindings;
 
-- 
1.7.3-rc1

[Qemu-devel] [PATCH 3/3] vhost: fix up irqfd support

[Michael S. Tsirkin]

vhost irqfd support: case where many vqs are
mapped to a single msix vector is currently broken.
Fix it up.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/msix.c       |   68 ++++++++++++++++++++++++++++++++++++++-----------------
 hw/msix.h       |    4 +-
 hw/pci.h        |    3 +-
 hw/virtio-pci.c |   54 ++++++++++++++++++++++++++++++++++++-------
 4 files changed, 95 insertions(+), 34 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 3dd0456..3d4dd61 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -300,10 +300,8 @@ static void msix_mmio_writel(void *opaque, target_phys_addr_t addr,
     if (kvm_enabled() && kvm_irqchip_in_kernel()) {
         kvm_msix_update(dev, vector, was_masked, msix_is_masked(dev, vector));
     }
-    if (was_masked != msix_is_masked(dev, vector) &&
-        dev->msix_mask_notifier && dev->msix_mask_notifier_opaque[vector]) {
+    if (was_masked != msix_is_masked(dev, vector) && dev->msix_mask_notifier) {
         int r = dev->msix_mask_notifier(dev, vector,
-					dev->msix_mask_notifier_opaque[vector],
 					msix_is_masked(dev, vector));
         assert(r >= 0);
     }
@@ -351,9 +349,8 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
         int was_masked = msix_is_masked(dev, vector);
         dev->msix_table_page[offset] |= MSIX_VECTOR_MASK;
         if (was_masked != msix_is_masked(dev, vector) &&
-            dev->msix_mask_notifier && dev->msix_mask_notifier_opaque[vector]) {
+            dev->msix_mask_notifier) {
             r = dev->msix_mask_notifier(dev, vector,
-                                        dev->msix_mask_notifier_opaque[vector],
                                         msix_is_masked(dev, vector));
             assert(r >= 0);
         }
@@ -379,8 +376,6 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
                                             sizeof *dev->msix_irq_entries);
     }
 #endif
-    dev->msix_mask_notifier_opaque =
-        qemu_mallocz(nentries * sizeof *dev->msix_mask_notifier_opaque);
     dev->msix_mask_notifier = NULL;
     dev->msix_entry_used = qemu_mallocz(MSIX_MAX_ENTRIES *
                                         sizeof *dev->msix_entry_used);
@@ -444,8 +439,6 @@ int msix_uninit(PCIDevice *dev)
     dev->msix_entry_used = NULL;
     qemu_free(dev->msix_irq_entries);
     dev->msix_irq_entries = NULL;
-    qemu_free(dev->msix_mask_notifier_opaque);
-    dev->msix_mask_notifier_opaque = NULL;
     dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
     return 0;
 }
@@ -590,46 +583,79 @@ void msix_unuse_all_vectors(PCIDevice *dev)
     msix_free_irq_entries(dev);
 }
 
-int msix_set_mask_notifier(PCIDevice *dev, unsigned vector, void *opaque)
+static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
 {
     int r = 0;
     if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
         return 0;
 
     assert(dev->msix_mask_notifier);
-    assert(opaque);
-    assert(!dev->msix_mask_notifier_opaque[vector]);
 
     /* Unmask the new notifier unless vector is masked. */
     if (!msix_is_masked(dev, vector)) {
-        r = dev->msix_mask_notifier(dev, vector, opaque, false);
+        r = dev->msix_mask_notifier(dev, vector, false);
         if (r < 0) {
             return r;
         }
     }
-    dev->msix_mask_notifier_opaque[vector] = opaque;
     return r;
 }
 
-int msix_unset_mask_notifier(PCIDevice *dev, unsigned vector)
+static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
 {
     int r = 0;
-    void *opaque;
     if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
         return 0;
 
-    opaque = dev->msix_mask_notifier_opaque[vector];
-
     assert(dev->msix_mask_notifier);
-    assert(opaque);
 
     /* Mask the old notifier unless it is already masked. */
     if (!msix_is_masked(dev, vector)) {
-        r = dev->msix_mask_notifier(dev, vector, opaque, true);
+        r = dev->msix_mask_notifier(dev, vector, true);
         if (r < 0) {
             return r;
         }
     }
-    dev->msix_mask_notifier_opaque[vector] = NULL;
+    return r;
+}
+
+int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
+{
+    int r, n;
+    assert(!dev->msix_mask_notifier);
+    dev->msix_mask_notifier = f;
+    for (n = 0; n < dev->msix_entries_nr; ++n) {
+        r = msix_set_mask_notifier_for_vector(dev, n);
+        if (r < 0) {
+            goto undo;
+        }
+    }
+    return 0;
+
+undo:
+    while (--n >= 0) {
+        msix_unset_mask_notifier_for_vector(dev, n);
+    }
+    dev->msix_mask_notifier = NULL;
+    return r;
+}
+
+int msix_unset_mask_notifier(PCIDevice *dev)
+{
+    int r, n;
+    assert(dev->msix_mask_notifier);
+    for (n = 0; n < dev->msix_entries_nr; ++n) {
+        r = msix_unset_mask_notifier_for_vector(dev, n);
+        if (r < 0) {
+            goto undo;
+        }
+    }
+    dev->msix_mask_notifier = NULL;
+    return 0;
+
+undo:
+    while (--n >= 0) {
+        msix_set_mask_notifier_for_vector(dev, n);
+    }
     return r;
 }
diff --git a/hw/msix.h b/hw/msix.h
index 6b21ffb..5a81df5 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -33,6 +33,6 @@ void msix_reset(PCIDevice *dev);
 
 extern int msix_supported;
 
-int msix_set_mask_notifier(PCIDevice *dev, unsigned vector, void *opaque);
-int msix_unset_mask_notifier(PCIDevice *dev, unsigned vector);
+int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func);
+int msix_unset_mask_notifier(PCIDevice *dev);
 #endif
diff --git a/hw/pci.h b/hw/pci.h
index ccb99d0..a40dc14 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -131,7 +131,7 @@ enum {
 #define PCI_CAPABILITY_CONFIG_MSIX_LENGTH 0x10
 
 typedef int (*msix_mask_notifier_func)(PCIDevice *, unsigned vector,
-				       void *opaque, int masked);
+				       int masked);
 
 struct PCIDevice {
     DeviceState qdev;
@@ -198,7 +198,6 @@ struct PCIDevice {
 
     struct kvm_irq_routing_entry *msix_irq_entries;
 
-    void **msix_mask_notifier_opaque;
     msix_mask_notifier_func msix_mask_notifier;
 
     /* Device capability configuration space */
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 232f943..6a2dfea 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -428,11 +428,10 @@ static void virtio_pci_guest_notifier_read(void *opaque)
     }
 }
 
-static int virtio_pci_mask_notifier(PCIDevice *dev, unsigned vector,
-                                    void *opaque, int masked)
+static int virtio_pci_mask_vq(PCIDevice *dev, unsigned vector,
+                              VirtQueue *vq, int masked)
 {
 #ifdef CONFIG_KVM
-    VirtQueue *vq = opaque;
     EventNotifier *notifier = virtio_queue_get_guest_notifier(vq);
     int r = kvm_set_irqfd(dev->msix_irq_entries[vector].gsi,
                           event_notifier_get_fd(notifier),
@@ -453,6 +452,34 @@ static int virtio_pci_mask_notifier(PCIDevice *dev, unsigned vector,
 #endif
 }
 
+static int virtio_pci_mask_notifier(PCIDevice *dev, unsigned vector,
+                                    int masked)
+{
+    VirtIOPCIProxy *proxy = container_of(dev, VirtIOPCIProxy, pci_dev);
+    VirtIODevice *vdev = proxy->vdev;
+    int r, n;
+
+    for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
+        if (!virtio_queue_get_num(vdev, n)) {
+            break;
+        }
+        if (virtio_queue_vector(vdev, n) != vector) {
+            continue;
+        }
+        r = virtio_pci_mask_vq(dev, vector, virtio_get_queue(vdev, n), masked);
+        if (r < 0) {
+            goto undo;
+        }
+    }
+    return 0;
+undo:
+    while (--n >= 0) {
+        virtio_pci_mask_vq(dev, vector, virtio_get_queue(vdev, n), !masked);
+    }
+    return r;
+}
+
+
 static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign)
 {
     VirtIOPCIProxy *proxy = opaque;
@@ -466,11 +493,7 @@ static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign)
         }
         qemu_set_fd_handler(event_notifier_get_fd(notifier),
                             virtio_pci_guest_notifier_read, NULL, vq);
-        msix_set_mask_notifier(&proxy->pci_dev,
-                               virtio_queue_vector(proxy->vdev, n), vq);
     } else {
-        msix_unset_mask_notifier(&proxy->pci_dev,
-				 virtio_queue_vector(proxy->vdev, n));
         qemu_set_fd_handler(event_notifier_get_fd(notifier),
                             NULL, NULL, NULL);
         /* Test and clear notifier before closing it,
@@ -498,11 +521,26 @@ static int virtio_pci_set_guest_notifiers(void *opaque, bool assign)
             goto assign_error;
         }
     }
+    if (assign) {
+	    r = msix_set_mask_notifier(&proxy->pci_dev,
+				       virtio_pci_mask_notifier);
+    } else {
+	    r = msix_unset_mask_notifier(&proxy->pci_dev);
+    }
+    if (r < 0) {
+	    goto assign_error;
+    }
 
     return 0;
 
 assign_error:
     /* We get here on assignment failure. Recover by undoing for VQs 0 .. n. */
+    if (assign) {
+	    msix_unset_mask_notifier(&proxy->pci_dev);
+    } else {
+	    msix_set_mask_notifier(&proxy->pci_dev,
+				   virtio_pci_mask_notifier);
+    }
     while (--n >= 0) {
         virtio_pci_set_guest_notifier(opaque, n, !assign);
     }
@@ -584,8 +622,6 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
 
     proxy->pci_dev.config_write = virtio_write_config;
 
-    proxy->pci_dev.msix_mask_notifier = virtio_pci_mask_notifier;
-
     size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev) + vdev->config_len;
     if (size & (size-1))
         size = 1 << qemu_fls(size);
-- 
1.7.3-rc1

[Qemu-devel] [PATCH 2/3] vhost: error code

[Michael S. Tsirkin]

fix up errors returned to include errno, not just -1

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/vhost.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/vhost.c b/hw/vhost.c
index 5ac283b..2e5d7a6 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -516,12 +516,14 @@ static int vhost_virtqueue_init(struct vhost_dev *dev,
     file.fd = event_notifier_get_fd(virtio_queue_get_host_notifier(vvq));
     r = ioctl(dev->control, VHOST_SET_VRING_KICK, &file);
     if (r) {
+        r = -errno;
         goto fail_kick;
     }
 
     file.fd = event_notifier_get_fd(virtio_queue_get_guest_notifier(vvq));
     r = ioctl(dev->control, VHOST_SET_VRING_CALL, &file);
     if (r) {
+        r = -errno;
         goto fail_call;
     }
 
-- 
1.7.3-rc1