[Qemu-devel] [PATCH v3 0/7] virtio: aio handler API

[Qemu-devel] [PATCH v3 0/7] virtio: aio handler API

[Paolo Bonzini]

This version removes patches 1 and 9, fixes some commit messages, and
fixes some small in the formatting issues.

Michael S. Tsirkin (2):
  virtio: add aio handler
  virtio-blk: use aio handler for data plane

Paolo Bonzini (5):
  virtio: make virtio_queue_notify_vq static
  virtio-blk: fix disabled mode
  virtio-scsi: fix disabled mode
  virtio-scsi: use aio handler for data plane
  virtio: merge virtio_queue_aio_set_host_notifier_handler with
    virtio_queue_set_aio

 hw/block/dataplane/virtio-blk.c | 23 ++++++++++----
 hw/block/virtio-blk.c           | 29 ++++++++++-------
 hw/scsi/virtio-scsi-dataplane.c | 47 +++++++++++++++++++++++-----
 hw/scsi/virtio-scsi.c           | 69 +++++++++++++++++++++++++++--------------
 hw/virtio/virtio.c              | 39 +++++++++++++++++------
 include/hw/virtio/virtio-blk.h  |  3 ++
 include/hw/virtio/virtio-scsi.h |  7 ++---
 include/hw/virtio/virtio.h      |  4 +--
 8 files changed, 158 insertions(+), 63 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/7] virtio: make virtio_queue_notify_vq static

[Paolo Bonzini]

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio/virtio.c         | 2 +-
 include/hw/virtio/virtio.h | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 14d5d91..264d4f6 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1088,7 +1088,7 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align)
     virtio_queue_update_rings(vdev, n);
 }
 
-void virtio_queue_notify_vq(VirtQueue *vq)
+static void virtio_queue_notify_vq(VirtQueue *vq)
 {
     if (vq->vring.desc && vq->handle_output) {
         VirtIODevice *vdev = vq->vdev;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 2b5b248..5afb51c 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -252,7 +252,6 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                bool set_handler);
 void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
                                                 bool assign, bool set_handler);
-void virtio_queue_notify_vq(VirtQueue *vq);
 void virtio_irq(VirtQueue *vq);
 VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector);
 VirtQueue *virtio_vector_next_queue(VirtQueue *vq);
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/7] virtio-blk: fix disabled mode

[Paolo Bonzini]

We must not call virtio_blk_data_plane_notify if dataplane is
disabled: we would hit a segmentation fault in notify_guest_bh as
s->guest_notifier has not been setup and is NULL.

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 7 +++----
 hw/block/virtio-blk.c           | 2 +-
 include/hw/virtio/virtio-blk.h  | 1 +
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index e666dd4..2870d21 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -29,7 +29,6 @@
 struct VirtIOBlockDataPlane {
     bool starting;
     bool stopping;
-    bool disabled;
 
     VirtIOBlkConf *conf;
 
@@ -234,7 +233,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
   fail_host_notifier:
     k->set_guest_notifiers(qbus->parent, 1, false);
   fail_guest_notifiers:
-    s->disabled = true;
+    vblk->dataplane_disabled = true;
     s->starting = false;
     vblk->dataplane_started = true;
 }
@@ -251,8 +250,8 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
     }
 
     /* Better luck next time. */
-    if (s->disabled) {
-        s->disabled = false;
+    if (vblk->dataplane_disabled) {
+        vblk->dataplane_disabled = false;
         vblk->dataplane_started = false;
         return;
     }
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 870d345..151fe78 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -54,7 +54,7 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
 
     stb_p(&req->in->status, status);
     virtqueue_push(s->vq, &req->elem, req->in_len);
-    if (s->dataplane) {
+    if (s->dataplane_started && !s->dataplane_disabled) {
         virtio_blk_data_plane_notify(s->dataplane);
     } else {
         virtio_notify(vdev, s->vq);
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index ae84d92..59ae1e4 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -53,6 +53,7 @@ typedef struct VirtIOBlock {
     unsigned short sector_mask;
     bool original_wce;
     VMChangeStateEntry *change;
+    bool dataplane_disabled;
     bool dataplane_started;
     struct VirtIOBlockDataPlane *dataplane;
 } VirtIOBlock;
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/7] virtio-scsi: fix disabled mode

[Paolo Bonzini]

Add two missing checks for s->dataplane_fenced.  In one case, QEMU
would skip injecting an IRQ due to a write to an uninitialized
EventNotifier's file descriptor.

In the second case, the dataplane_disabled field was used by mistake;
in fact after fixing this occurrence it is completely unused.

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/virtio-scsi.c           | 4 ++--
 include/hw/virtio/virtio-scsi.h | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index ade4972..38f1e2c 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -68,7 +68,7 @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
 
     qemu_iovec_from_buf(&req->resp_iov, 0, &req->resp, req->resp_size);
     virtqueue_push(vq, &req->elem, req->qsgl.size + req->resp_iov.size);
-    if (s->dataplane_started) {
+    if (s->dataplane_started && !s->dataplane_fenced) {
         virtio_scsi_dataplane_notify(vdev, req);
     } else {
         virtio_notify(vdev, vq);
@@ -773,7 +773,7 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
     SCSIDevice *sd = SCSI_DEVICE(dev);
 
-    if (s->ctx && !s->dataplane_disabled) {
+    if (s->ctx && !s->dataplane_fenced) {
         VirtIOSCSIBlkChangeNotifier *insert_notifier, *remove_notifier;
 
         if (blk_op_is_blocked(sd->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 209eaa4..eef4e95 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -91,7 +91,6 @@ typedef struct VirtIOSCSI {
     bool dataplane_started;
     bool dataplane_starting;
     bool dataplane_stopping;
-    bool dataplane_disabled;
     bool dataplane_fenced;
     Error *blocker;
     uint32_t host_features;
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/7] virtio: add aio handler

[Paolo Bonzini]

From: "Michael S. Tsirkin" <mst@redhat.com>

In addition to handling IO in vcpu thread and in io thread, blk dataplane
introduces yet another mode: handling it by AioContext.

Currently, this reuses the same handler as previous modes,
which triggers races as these were not designed to be reentrant.
Add instead a separate handler just for aio; this will make
it possible to disable regular handlers when dataplane is active.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio/virtio.c         | 36 ++++++++++++++++++++++++++++++++----
 include/hw/virtio/virtio.h |  3 +++
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 264d4f6..eb04ac0 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -96,6 +96,7 @@ struct VirtQueue
 
     uint16_t vector;
     void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq);
+    void (*handle_aio_output)(VirtIODevice *vdev, VirtQueue *vq);
     VirtIODevice *vdev;
     EventNotifier guest_notifier;
     EventNotifier host_notifier;
@@ -1088,6 +1089,16 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align)
     virtio_queue_update_rings(vdev, n);
 }
 
+static void virtio_queue_notify_aio_vq(VirtQueue *vq)
+{
+    if (vq->vring.desc && vq->handle_aio_output) {
+        VirtIODevice *vdev = vq->vdev;
+
+        trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
+        vq->handle_aio_output(vdev, vq);
+    }
+}
+
 static void virtio_queue_notify_vq(VirtQueue *vq)
 {
     if (vq->vring.desc && vq->handle_output) {
@@ -1143,10 +1154,19 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
     vdev->vq[i].vring.num_default = queue_size;
     vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN;
     vdev->vq[i].handle_output = handle_output;
+    vdev->vq[i].handle_aio_output = NULL;
 
     return &vdev->vq[i];
 }
 
+void virtio_set_queue_aio(VirtQueue *vq,
+                          void (*handle_output)(VirtIODevice *, VirtQueue *))
+{
+    assert(vq->handle_output);
+
+    vq->handle_aio_output = handle_output;
+}
+
 void virtio_del_queue(VirtIODevice *vdev, int n)
 {
     if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
@@ -1780,11 +1800,11 @@ EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq)
     return &vq->guest_notifier;
 }
 
-static void virtio_queue_host_notifier_read(EventNotifier *n)
+static void virtio_queue_host_notifier_aio_read(EventNotifier *n)
 {
     VirtQueue *vq = container_of(n, VirtQueue, host_notifier);
     if (event_notifier_test_and_clear(n)) {
-        virtio_queue_notify_vq(vq);
+        virtio_queue_notify_aio_vq(vq);
     }
 }
 
@@ -1793,14 +1813,22 @@ void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
 {
     if (assign && set_handler) {
         aio_set_event_notifier(ctx, &vq->host_notifier, true,
-                               virtio_queue_host_notifier_read);
+                               virtio_queue_host_notifier_aio_read);
     } else {
         aio_set_event_notifier(ctx, &vq->host_notifier, true, NULL);
     }
     if (!assign) {
         /* Test and clear notifier before after disabling event,
          * in case poll callback didn't have time to run. */
-        virtio_queue_host_notifier_read(&vq->host_notifier);
+        virtio_queue_host_notifier_aio_read(&vq->host_notifier);
+    }
+}
+
+static void virtio_queue_host_notifier_read(EventNotifier *n)
+{
+    VirtQueue *vq = container_of(n, VirtQueue, host_notifier);
+    if (event_notifier_test_and_clear(n)) {
+        virtio_queue_notify_vq(vq);
     }
 }
 
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 5afb51c..fa3f93b 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -142,6 +142,9 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
                             void (*handle_output)(VirtIODevice *,
                                                   VirtQueue *));
 
+void virtio_set_queue_aio(VirtQueue *vq,
+                          void (*handle_output)(VirtIODevice *, VirtQueue *));
+
 void virtio_del_queue(VirtIODevice *vdev, int n);
 
 void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_num);
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/7] virtio-blk: use aio handler for data plane

[Paolo Bonzini]

From: "Michael S. Tsirkin" <mst@redhat.com>

In addition to handling IO in vcpu thread and in io thread, dataplane
introduces yet another mode: handling it by AioContext.

This reuses the same handler as previous modes, which triggers races as
these were not designed to be reentrant.  Use a separate handler just
for aio, and disable regular handlers when dataplane is active.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 13 +++++++++++++
 hw/block/virtio-blk.c           | 27 +++++++++++++++++----------
 include/hw/virtio/virtio-blk.h  |  2 ++
 3 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 2870d21..65c7f70 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -184,6 +184,17 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
     g_free(s);
 }
 
+static void virtio_blk_data_plane_handle_output(VirtIODevice *vdev,
+                                                VirtQueue *vq)
+{
+    VirtIOBlock *s = (VirtIOBlock *)vdev;
+
+    assert(s->dataplane);
+    assert(s->dataplane_started);
+
+    virtio_blk_handle_vq(s, vq);
+}
+
 /* Context: QEMU global mutex held */
 void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
 {
@@ -226,6 +237,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
 
     /* Get this show started by hooking up our callbacks */
     aio_context_acquire(s->ctx);
+    virtio_set_queue_aio(s->vq, virtio_blk_data_plane_handle_output);
     virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
     aio_context_release(s->ctx);
     return;
@@ -262,6 +274,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
 
     /* Stop notifications for new requests from guest */
     virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, false, false);
+    virtio_set_queue_aio(s->vq, NULL);
 
     /* Drain and switch bs back to the QEMU main loop */
     blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context());
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 151fe78..3f88f8c 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -578,20 +578,11 @@ void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
     }
 }
 
-static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
+void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq)
 {
-    VirtIOBlock *s = VIRTIO_BLK(vdev);
     VirtIOBlockReq *req;
     MultiReqBuffer mrb = {};
 
-    /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
-     * dataplane here instead of waiting for .set_status().
-     */
-    if (s->dataplane && !s->dataplane_started) {
-        virtio_blk_data_plane_start(s->dataplane);
-        return;
-    }
-
     blk_io_plug(s->blk);
 
     while ((req = virtio_blk_get_request(s))) {
@@ -605,6 +596,22 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
     blk_io_unplug(s->blk);
 }
 
+static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtIOBlock *s = (VirtIOBlock *)vdev;
+
+    if (s->dataplane) {
+        /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
+         * dataplane here instead of waiting for .set_status().
+         */
+        virtio_blk_data_plane_start(s->dataplane);
+        if (!s->dataplane_disabled) {
+            return;
+        }
+    }
+    virtio_blk_handle_vq(s, vq);
+}
+
 static void virtio_blk_dma_restart_bh(void *opaque)
 {
     VirtIOBlock *s = opaque;
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 59ae1e4..8f2b056 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -86,4 +86,6 @@ void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb);
 
 void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb);
 
+void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq);
+
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/7] virtio-scsi: use aio handler for data plane

[Paolo Bonzini]

In addition to handling IO in vcpu thread and in io thread, dataplane
introduces yet another mode: handling it by AioContext.

This reuses the same handler as previous modes, which triggers races as
these were not designed to be reentrant.  Use a separate handler just
for aio, and disable regular handlers when dataplane is active.

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/virtio-scsi-dataplane.c | 43 ++++++++++++++++++++++++---
 hw/scsi/virtio-scsi.c           | 65 ++++++++++++++++++++++++++++-------------
 include/hw/virtio/virtio-scsi.h |  6 ++--
 3 files changed, 86 insertions(+), 28 deletions(-)

diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index b44ac5d..39ad086 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -38,7 +38,35 @@ void virtio_scsi_set_iothread(VirtIOSCSI *s, IOThread *iothread)
     }
 }
 
-static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n)
+static void virtio_scsi_data_plane_handle_cmd(VirtIODevice *vdev,
+                                              VirtQueue *vq)
+{
+    VirtIOSCSI *s = (VirtIOSCSI *)vdev;
+
+    assert(s->ctx && s->dataplane_started);
+    virtio_scsi_handle_cmd_vq(s, vq);
+}
+
+static void virtio_scsi_data_plane_handle_ctrl(VirtIODevice *vdev,
+                                               VirtQueue *vq)
+{
+    VirtIOSCSI *s = VIRTIO_SCSI(vdev);
+
+    assert(s->ctx && s->dataplane_started);
+    virtio_scsi_handle_ctrl_vq(s, vq);
+}
+
+static void virtio_scsi_data_plane_handle_event(VirtIODevice *vdev,
+                                                VirtQueue *vq)
+{
+    VirtIOSCSI *s = VIRTIO_SCSI(vdev);
+
+    assert(s->ctx && s->dataplane_started);
+    virtio_scsi_handle_event_vq(s, vq);
+}
+
+static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n,
+                                  void (*fn)(VirtIODevice *vdev, VirtQueue *vq))
 {
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
@@ -54,6 +82,7 @@ static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n)
     }
 
     virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, true, true);
+    virtio_set_queue_aio(vq, fn);
     return 0;
 }
 
@@ -71,9 +100,12 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s)
     int i;
 
     virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, false, false);
+    virtio_set_queue_aio(vs->ctrl_vq, NULL);
     virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, false, false);
+    virtio_set_queue_aio(vs->event_vq, NULL);
     for (i = 0; i < vs->conf.num_queues; i++) {
         virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, false, false);
+        virtio_set_queue_aio(vs->cmd_vqs[i], NULL);
     }
 }
 
@@ -104,16 +136,19 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s)
     }
 
     aio_context_acquire(s->ctx);
-    rc = virtio_scsi_vring_init(s, vs->ctrl_vq, 0);
+    rc = virtio_scsi_vring_init(s, vs->ctrl_vq, 0,
+                                virtio_scsi_data_plane_handle_ctrl);
     if (rc) {
         goto fail_vrings;
     }
-    rc = virtio_scsi_vring_init(s, vs->event_vq, 1);
+    rc = virtio_scsi_vring_init(s, vs->event_vq, 1,
+                                virtio_scsi_data_plane_handle_event);
     if (rc) {
         goto fail_vrings;
     }
     for (i = 0; i < vs->conf.num_queues; i++) {
-        rc = virtio_scsi_vring_init(s, vs->cmd_vqs[i], i + 2);
+        rc = virtio_scsi_vring_init(s, vs->cmd_vqs[i], i + 2,
+                                    virtio_scsi_data_plane_handle_cmd);
         if (rc) {
             goto fail_vrings;
         }
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 38f1e2c..30415c6 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -374,7 +374,7 @@ fail:
     return ret;
 }
 
-void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
+static void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
 {
     VirtIODevice *vdev = (VirtIODevice *)s;
     uint32_t type;
@@ -412,20 +412,28 @@ void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
     }
 }
 
-static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
+void virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq)
 {
-    VirtIOSCSI *s = (VirtIOSCSI *)vdev;
     VirtIOSCSIReq *req;
 
-    if (s->ctx && !s->dataplane_started) {
-        virtio_scsi_dataplane_start(s);
-        return;
-    }
     while ((req = virtio_scsi_pop_req(s, vq))) {
         virtio_scsi_handle_ctrl_req(s, req);
     }
 }
 
+static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtIOSCSI *s = (VirtIOSCSI *)vdev;
+
+    if (s->ctx) {
+        virtio_scsi_dataplane_start(s);
+        if (!s->dataplane_fenced) {
+            return;
+        }
+    }
+    virtio_scsi_handle_ctrl_vq(s, vq);
+}
+
 static void virtio_scsi_complete_cmd_req(VirtIOSCSIReq *req)
 {
     /* Sense data is not in req->resp and is copied separately
@@ -508,7 +516,7 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
     virtio_scsi_complete_cmd_req(req);
 }
 
-bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
+static bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
 {
     VirtIOSCSICommon *vs = &s->parent_obj;
     SCSIDevice *d;
@@ -550,7 +558,7 @@ bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
     return true;
 }
 
-void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req)
+static void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req)
 {
     SCSIRequest *sreq = req->sreq;
     if (scsi_req_enqueue(sreq)) {
@@ -560,17 +568,11 @@ void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req)
     scsi_req_unref(sreq);
 }
 
-static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
+void virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq)
 {
-    /* use non-QOM casts in the data path */
-    VirtIOSCSI *s = (VirtIOSCSI *)vdev;
     VirtIOSCSIReq *req, *next;
     QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs);
 
-    if (s->ctx && !s->dataplane_started) {
-        virtio_scsi_dataplane_start(s);
-        return;
-    }
     while ((req = virtio_scsi_pop_req(s, vq))) {
         if (virtio_scsi_handle_cmd_req_prepare(s, req)) {
             QTAILQ_INSERT_TAIL(&reqs, req, next);
@@ -582,6 +584,20 @@ static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
     }
 }
 
+static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
+{
+    /* use non-QOM casts in the data path */
+    VirtIOSCSI *s = (VirtIOSCSI *)vdev;
+
+    if (s->ctx) {
+        virtio_scsi_dataplane_start(s);
+        if (!s->dataplane_fenced) {
+            return;
+        }
+    }
+    virtio_scsi_handle_cmd_vq(s, vq);
+}
+
 static void virtio_scsi_get_config(VirtIODevice *vdev,
                                    uint8_t *config)
 {
@@ -725,17 +741,24 @@ out:
     }
 }
 
+void virtio_scsi_handle_event_vq(VirtIOSCSI *s, VirtQueue *vq)
+{
+    if (s->events_dropped) {
+        virtio_scsi_push_event(s, NULL, VIRTIO_SCSI_T_NO_EVENT, 0);
+    }
+}
+
 static void virtio_scsi_handle_event(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
-    if (s->ctx && !s->dataplane_started) {
+    if (s->ctx) {
         virtio_scsi_dataplane_start(s);
-        return;
-    }
-    if (s->events_dropped) {
-        virtio_scsi_push_event(s, NULL, VIRTIO_SCSI_T_NO_EVENT, 0);
+        if (!s->dataplane_fenced) {
+            return;
+        }
     }
+    virtio_scsi_handle_event_vq(s, vq);
 }
 
 static void virtio_scsi_change(SCSIBus *bus, SCSIDevice *dev, SCSISense sense)
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index eef4e95..ba2f5ce 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -139,9 +139,9 @@ void virtio_scsi_common_realize(DeviceState *dev, Error **errp,
                                 HandleOutput cmd);
 
 void virtio_scsi_common_unrealize(DeviceState *dev, Error **errp);
-void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req);
-bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req);
-void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req);
+void virtio_scsi_handle_event_vq(VirtIOSCSI *s, VirtQueue *vq);
+void virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq);
+void virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq);
 void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req);
 void virtio_scsi_free_req(VirtIOSCSIReq *req);
 void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
-- 
1.8.3.1

[Qemu-devel] [PATCH 7/7] virtio: merge virtio_queue_aio_set_host_notifier_handler with virtio_queue_set_aio

[Paolo Bonzini]

Eliminating the reentrancy is actually a nice thing that we can do
with the API that Michael proposed, so let's make it first class.
This also hides the complex assign/set_handler conventions from
callers of virtio_queue_aio_set_host_notifier_handler, which in
fact was always called with assign=true.

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/block/dataplane/virtio-blk.c |  7 +++----
 hw/scsi/virtio-scsi-dataplane.c | 12 ++++--------
 hw/virtio/virtio.c              | 17 +++++------------
 include/hw/virtio/virtio.h      |  6 ++----
 4 files changed, 14 insertions(+), 28 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 65c7f70..3cb97c9 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -237,8 +237,8 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
 
     /* Get this show started by hooking up our callbacks */
     aio_context_acquire(s->ctx);
-    virtio_set_queue_aio(s->vq, virtio_blk_data_plane_handle_output);
-    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
+    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx,
+                                               virtio_blk_data_plane_handle_output);
     aio_context_release(s->ctx);
     return;
 
@@ -273,8 +273,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
     aio_context_acquire(s->ctx);
 
     /* Stop notifications for new requests from guest */
-    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, false, false);
-    virtio_set_queue_aio(s->vq, NULL);
+    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, NULL);
 
     /* Drain and switch bs back to the QEMU main loop */
     blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context());
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 39ad086..1a49f1e 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -81,8 +81,7 @@ static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n,
         return rc;
     }
 
-    virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, true, true);
-    virtio_set_queue_aio(vq, fn);
+    virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, fn);
     return 0;
 }
 
@@ -99,13 +98,10 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s)
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
     int i;
 
-    virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, false, false);
-    virtio_set_queue_aio(vs->ctrl_vq, NULL);
-    virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, false, false);
-    virtio_set_queue_aio(vs->event_vq, NULL);
+    virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, NULL);
+    virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, NULL);
     for (i = 0; i < vs->conf.num_queues; i++) {
-        virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, false, false);
-        virtio_set_queue_aio(vs->cmd_vqs[i], NULL);
+        virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, NULL);
     }
 }
 
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index eb04ac0..f745c4a 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1159,14 +1159,6 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
     return &vdev->vq[i];
 }
 
-void virtio_set_queue_aio(VirtQueue *vq,
-                          void (*handle_output)(VirtIODevice *, VirtQueue *))
-{
-    assert(vq->handle_output);
-
-    vq->handle_aio_output = handle_output;
-}
-
 void virtio_del_queue(VirtIODevice *vdev, int n)
 {
     if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
@@ -1809,18 +1801,19 @@ static void virtio_queue_host_notifier_aio_read(EventNotifier *n)
 }
 
 void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
-                                                bool assign, bool set_handler)
+                                                void (*handle_output)(VirtIODevice *,
+                                                                      VirtQueue *))
 {
-    if (assign && set_handler) {
+    if (handle_output) {
+        vq->handle_aio_output = handle_output;
         aio_set_event_notifier(ctx, &vq->host_notifier, true,
                                virtio_queue_host_notifier_aio_read);
     } else {
         aio_set_event_notifier(ctx, &vq->host_notifier, true, NULL);
-    }
-    if (!assign) {
         /* Test and clear notifier before after disabling event,
          * in case poll callback didn't have time to run. */
         virtio_queue_host_notifier_aio_read(&vq->host_notifier);
+        vq->handle_aio_output = NULL;
     }
 }
 
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index fa3f93b..6a37065 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -142,9 +142,6 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
                             void (*handle_output)(VirtIODevice *,
                                                   VirtQueue *));
 
-void virtio_set_queue_aio(VirtQueue *vq,
-                          void (*handle_output)(VirtIODevice *, VirtQueue *));
-
 void virtio_del_queue(VirtIODevice *vdev, int n);
 
 void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_num);
@@ -254,7 +251,8 @@ EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
 void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                bool set_handler);
 void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
-                                                bool assign, bool set_handler);
+                                                void (*fn)(VirtIODevice *,
+                                                           VirtQueue *));
 void virtio_irq(VirtQueue *vq);
 VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector);
 VirtQueue *virtio_vector_next_queue(VirtQueue *vq);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 4/7] virtio: add aio handler

[Cornelia Huck]

On Wed,  6 Apr 2016 12:16:25 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> From: "Michael S. Tsirkin" <mst@redhat.com>
> 
> In addition to handling IO in vcpu thread and in io thread, blk dataplane
> introduces yet another mode: handling it by AioContext.
> 
> Currently, this reuses the same handler as previous modes,
> which triggers races as these were not designed to be reentrant.
> Add instead a separate handler just for aio; this will make
> it possible to disable regular handlers when dataplane is active.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/virtio/virtio.c         | 36 ++++++++++++++++++++++++++++++++----
>  include/hw/virtio/virtio.h |  3 +++
>  2 files changed, 35 insertions(+), 4 deletions(-)
> 

> +static void virtio_queue_notify_aio_vq(VirtQueue *vq)
> +{
> +    if (vq->vring.desc && vq->handle_aio_output) {
> +        VirtIODevice *vdev = vq->vdev;
> +
> +        trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
> +        vq->handle_aio_output(vdev, vq);
> +    }
> +}
> +

So this avoids reentrancy, but might we miss one notify if
->handle_aio_output has already been unset? What am I missing?

Re: [Qemu-devel] [PATCH 4/7] virtio: add aio handler

[Paolo Bonzini]

On 06/04/2016 13:11, Cornelia Huck wrote:
>> > +static void virtio_queue_notify_aio_vq(VirtQueue *vq)
>> > +{
>> > +    if (vq->vring.desc && vq->handle_aio_output) {
>> > +        VirtIODevice *vdev = vq->vdev;
>> > +
>> > +        trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
>> > +        vq->handle_aio_output(vdev, vq);
>> > +    }
>> > +}
>> > +
> So this avoids reentrancy, but might we miss one notify if
> ->handle_aio_output has already been unset? What am I missing?

Calling the notifier just before unset is handled by using "false,
false" when unsetting the notifier, and only setting
vq->handle_aio_output after the notifier has been unset.

Patch 7 makes things clearer.

Paolo

Re: [Qemu-devel] [PATCH 4/7] virtio: add aio handler

[Cornelia Huck]

On Wed, 6 Apr 2016 14:09:12 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 06/04/2016 13:11, Cornelia Huck wrote:
> >> > +static void virtio_queue_notify_aio_vq(VirtQueue *vq)
> >> > +{
> >> > +    if (vq->vring.desc && vq->handle_aio_output) {
> >> > +        VirtIODevice *vdev = vq->vdev;
> >> > +
> >> > +        trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
> >> > +        vq->handle_aio_output(vdev, vq);
> >> > +    }
> >> > +}
> >> > +
> > So this avoids reentrancy, but might we miss one notify if
> > ->handle_aio_output has already been unset? What am I missing?
> 
> Calling the notifier just before unset is handled by using "false,
> false" when unsetting the notifier, and only setting
> vq->handle_aio_output after the notifier has been unset.

Actually, the code is not quite right until the next two patches have
been applied, but I think we can live with that.

> Patch 7 makes things clearer.

That as well.

Re: [Qemu-devel] [PATCH v3 0/7] virtio: aio handler API

[Cornelia Huck]

On Wed,  6 Apr 2016 12:16:21 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This version removes patches 1 and 9, fixes some commit messages, and
> fixes some small in the formatting issues.
> 
> Michael S. Tsirkin (2):
>   virtio: add aio handler
>   virtio-blk: use aio handler for data plane
> 
> Paolo Bonzini (5):
>   virtio: make virtio_queue_notify_vq static
>   virtio-blk: fix disabled mode
>   virtio-scsi: fix disabled mode
>   virtio-scsi: use aio handler for data plane
>   virtio: merge virtio_queue_aio_set_host_notifier_handler with
>     virtio_queue_set_aio
> 
>  hw/block/dataplane/virtio-blk.c | 23 ++++++++++----
>  hw/block/virtio-blk.c           | 29 ++++++++++-------
>  hw/scsi/virtio-scsi-dataplane.c | 47 +++++++++++++++++++++++-----
>  hw/scsi/virtio-scsi.c           | 69 +++++++++++++++++++++++++++--------------
>  hw/virtio/virtio.c              | 39 +++++++++++++++++------
>  include/hw/virtio/virtio-blk.h  |  3 ++
>  include/hw/virtio/virtio-scsi.h |  7 ++---
>  include/hw/virtio/virtio.h      |  4 +--
>  8 files changed, 158 insertions(+), 63 deletions(-)

Seems to work fine so far. We'll try to run some more extensive tests
overnight.

Re: [Qemu-devel] [PATCH v3 0/7] virtio: aio handler API

[Christian Borntraeger]

On 04/06/2016 12:16 PM, Paolo Bonzini wrote:
> This version removes patches 1 and 9, fixes some commit messages, and
> fixes some small in the formatting issues.
> 
> Michael S. Tsirkin (2):
>   virtio: add aio handler
>   virtio-blk: use aio handler for data plane
> 
> Paolo Bonzini (5):
>   virtio: make virtio_queue_notify_vq static
>   virtio-blk: fix disabled mode
>   virtio-scsi: fix disabled mode
>   virtio-scsi: use aio handler for data plane
>   virtio: merge virtio_queue_aio_set_host_notifier_handler with
>     virtio_queue_set_aio
> 
>  hw/block/dataplane/virtio-blk.c | 23 ++++++++++----
>  hw/block/virtio-blk.c           | 29 ++++++++++-------
>  hw/scsi/virtio-scsi-dataplane.c | 47 +++++++++++++++++++++++-----
>  hw/scsi/virtio-scsi.c           | 69 +++++++++++++++++++++++++++--------------
>  hw/virtio/virtio.c              | 39 +++++++++++++++++------
>  include/hw/virtio/virtio-blk.h  |  3 ++
>  include/hw/virtio/virtio-scsi.h |  7 ++---
>  include/hw/virtio/virtio.h      |  4 +--
>  8 files changed, 158 insertions(+), 63 deletions(-)
> 

Seems to fix the crashes on my system. 

Re: [Qemu-devel] [PATCH v3 0/7] virtio: aio handler API

[Christian Borntraeger]

On 04/06/2016 12:16 PM, Paolo Bonzini wrote:
> This version removes patches 1 and 9, fixes some commit messages, and
> fixes some small in the formatting issues.
> 
> Michael S. Tsirkin (2):
>   virtio: add aio handler
>   virtio-blk: use aio handler for data plane
> 
> Paolo Bonzini (5):
>   virtio: make virtio_queue_notify_vq static
>   virtio-blk: fix disabled mode
>   virtio-scsi: fix disabled mode
>   virtio-scsi: use aio handler for data plane
>   virtio: merge virtio_queue_aio_set_host_notifier_handler with
>     virtio_queue_set_aio
> 
>  hw/block/dataplane/virtio-blk.c | 23 ++++++++++----
>  hw/block/virtio-blk.c           | 29 ++++++++++-------
>  hw/scsi/virtio-scsi-dataplane.c | 47 +++++++++++++++++++++++-----
>  hw/scsi/virtio-scsi.c           | 69 +++++++++++++++++++++++++++--------------
>  hw/virtio/virtio.c              | 39 +++++++++++++++++------
>  include/hw/virtio/virtio-blk.h  |  3 ++
>  include/hw/virtio/virtio-scsi.h |  7 ++---
>  include/hw/virtio/virtio.h      |  4 +--
>  8 files changed, 158 insertions(+), 63 deletions(-)
> 

The test team also reports that the original problem no longer reproduces with this patch set.
Would be great to have it merged for 2.6 soon.