[PATCH 0/5] virtio-blk: iothread-vq-mapping cleanups

[PATCH 0/5] virtio-blk: iothread-vq-mapping cleanups

[Stefan Hajnoczi]

Hanna reviewed the iothread-vq-mapping patches after they were applied to
qemu.git. This series consists of code cleanups that Hanna identified.

There are no functional changes or bug fixes that need to be backported to the
stable tree here, but it may make sense to backport them in the future to avoid
conflicts.

Stefan Hajnoczi (5):
  virtio-blk: enforce iothread-vq-mapping validation
  virtio-blk: clarify that there is at least 1 virtqueue
  virtio-blk: add vq_rq[] bounds check in virtio_blk_dma_restart_cb()
  virtio-blk: declare VirtIOBlock::rq with a type
  monitor: use aio_co_reschedule_self()

 include/hw/virtio/virtio-blk.h |   2 +-
 hw/block/virtio-blk.c          | 194 ++++++++++++++++++---------------
 qapi/qmp-dispatch.c            |   7 +-
 3 files changed, 112 insertions(+), 91 deletions(-)

-- 
2.43.0

[PATCH 1/5] virtio-blk: enforce iothread-vq-mapping validation

[Stefan Hajnoczi]

Hanna Czenczek <hreitz@redhat.com> noticed that the safety of
`vq_aio_context[vq->value] = ctx;` with user-defined vq->value inputs is
not obvious.

The code is structured in validate() + apply() steps so input validation
is there, but it happens way earlier and there is nothing that
guarantees apply() can only be called with validated inputs.

This patch moves the validate() call inside the apply() function so
validation is guaranteed. I also added the bounds checking assertion
that Hanna suggested.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 192 +++++++++++++++++++++++-------------------
 1 file changed, 107 insertions(+), 85 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 227d83569f..e8b37fd5f4 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1485,6 +1485,72 @@ static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
     return 0;
 }
 
+static void virtio_resize_cb(void *opaque)
+{
+    VirtIODevice *vdev = opaque;
+
+    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
+    virtio_notify_config(vdev);
+}
+
+static void virtio_blk_resize(void *opaque)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
+
+    /*
+     * virtio_notify_config() needs to acquire the BQL,
+     * so it can't be called from an iothread. Instead, schedule
+     * it to be run in the main context BH.
+     */
+    aio_bh_schedule_oneshot(qemu_get_aio_context(), virtio_resize_cb, vdev);
+}
+
+static void virtio_blk_ioeventfd_detach(VirtIOBlock *s)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+    for (uint16_t i = 0; i < s->conf.num_queues; i++) {
+        VirtQueue *vq = virtio_get_queue(vdev, i);
+        virtio_queue_aio_detach_host_notifier(vq, s->vq_aio_context[i]);
+    }
+}
+
+static void virtio_blk_ioeventfd_attach(VirtIOBlock *s)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+    for (uint16_t i = 0; i < s->conf.num_queues; i++) {
+        VirtQueue *vq = virtio_get_queue(vdev, i);
+        virtio_queue_aio_attach_host_notifier(vq, s->vq_aio_context[i]);
+    }
+}
+
+/* Suspend virtqueue ioeventfd processing during drain */
+static void virtio_blk_drained_begin(void *opaque)
+{
+    VirtIOBlock *s = opaque;
+
+    if (s->ioeventfd_started) {
+        virtio_blk_ioeventfd_detach(s);
+    }
+}
+
+/* Resume virtqueue ioeventfd processing after drain */
+static void virtio_blk_drained_end(void *opaque)
+{
+    VirtIOBlock *s = opaque;
+
+    if (s->ioeventfd_started) {
+        virtio_blk_ioeventfd_attach(s);
+    }
+}
+
+static const BlockDevOps virtio_block_ops = {
+    .resize_cb     = virtio_blk_resize,
+    .drained_begin = virtio_blk_drained_begin,
+    .drained_end   = virtio_blk_drained_end,
+};
+
 static bool
 validate_iothread_vq_mapping_list(IOThreadVirtQueueMappingList *list,
         uint16_t num_queues, Error **errp)
@@ -1547,81 +1613,33 @@ validate_iothread_vq_mapping_list(IOThreadVirtQueueMappingList *list,
     return true;
 }
 
-static void virtio_resize_cb(void *opaque)
-{
-    VirtIODevice *vdev = opaque;
-
-    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
-    virtio_notify_config(vdev);
-}
-
-static void virtio_blk_resize(void *opaque)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
-
-    /*
-     * virtio_notify_config() needs to acquire the BQL,
-     * so it can't be called from an iothread. Instead, schedule
-     * it to be run in the main context BH.
-     */
-    aio_bh_schedule_oneshot(qemu_get_aio_context(), virtio_resize_cb, vdev);
-}
-
-static void virtio_blk_ioeventfd_detach(VirtIOBlock *s)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
-
-    for (uint16_t i = 0; i < s->conf.num_queues; i++) {
-        VirtQueue *vq = virtio_get_queue(vdev, i);
-        virtio_queue_aio_detach_host_notifier(vq, s->vq_aio_context[i]);
-    }
-}
-
-static void virtio_blk_ioeventfd_attach(VirtIOBlock *s)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
-
-    for (uint16_t i = 0; i < s->conf.num_queues; i++) {
-        VirtQueue *vq = virtio_get_queue(vdev, i);
-        virtio_queue_aio_attach_host_notifier(vq, s->vq_aio_context[i]);
-    }
-}
-
-/* Suspend virtqueue ioeventfd processing during drain */
-static void virtio_blk_drained_begin(void *opaque)
-{
-    VirtIOBlock *s = opaque;
-
-    if (s->ioeventfd_started) {
-        virtio_blk_ioeventfd_detach(s);
-    }
-}
-
-/* Resume virtqueue ioeventfd processing after drain */
-static void virtio_blk_drained_end(void *opaque)
-{
-    VirtIOBlock *s = opaque;
-
-    if (s->ioeventfd_started) {
-        virtio_blk_ioeventfd_attach(s);
-    }
-}
-
-static const BlockDevOps virtio_block_ops = {
-    .resize_cb     = virtio_blk_resize,
-    .drained_begin = virtio_blk_drained_begin,
-    .drained_end   = virtio_blk_drained_end,
-};
-
-/* Generate vq:AioContext mappings from a validated iothread-vq-mapping list */
-static void
-apply_vq_mapping(IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
-                 AioContext **vq_aio_context, uint16_t num_queues)
+/**
+ * apply_iothread_vq_mapping:
+ * @iothread_vq_mapping_list: The mapping of virtqueues to IOThreads.
+ * @vq_aio_context: The array of AioContext pointers to fill in.
+ * @num_queues: The length of @vq_aio_context.
+ * @errp: If an error occurs, a pointer to the area to store the error.
+ *
+ * Fill in the AioContext for each virtqueue in the @vq_aio_context array given
+ * the iothread-vq-mapping parameter in @iothread_vq_mapping_list.
+ *
+ * Returns: %true on success, %false on failure.
+ **/
+static bool apply_iothread_vq_mapping(
+        IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
+        AioContext **vq_aio_context,
+        uint16_t num_queues,
+        Error **errp)
 {
     IOThreadVirtQueueMappingList *node;
     size_t num_iothreads = 0;
     size_t cur_iothread = 0;
 
+    if (!validate_iothread_vq_mapping_list(iothread_vq_mapping_list,
+                                           num_queues, errp)) {
+        return false;
+    }
+
     for (node = iothread_vq_mapping_list; node; node = node->next) {
         num_iothreads++;
     }
@@ -1638,6 +1656,7 @@ apply_vq_mapping(IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
 
             /* Explicit vq:IOThread assignment */
             for (vq = node->value->vqs; vq; vq = vq->next) {
+                assert(vq->value < num_queues);
                 vq_aio_context[vq->value] = ctx;
             }
         } else {
@@ -1650,6 +1669,8 @@ apply_vq_mapping(IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
 
         cur_iothread++;
     }
+
+    return true;
 }
 
 /* Context: BQL held */
@@ -1660,6 +1681,14 @@ static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp)
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
 
+    if (conf->iothread && conf->iothread_vq_mapping_list) {
+        if (conf->iothread) {
+            error_setg(errp, "iothread and iothread-vq-mapping properties "
+                             "cannot be set at the same time");
+            return false;
+        }
+    }
+
     if (conf->iothread || conf->iothread_vq_mapping_list) {
         if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
             error_setg(errp,
@@ -1685,8 +1714,14 @@ static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp)
     s->vq_aio_context = g_new(AioContext *, conf->num_queues);
 
     if (conf->iothread_vq_mapping_list) {
-        apply_vq_mapping(conf->iothread_vq_mapping_list, s->vq_aio_context,
-                         conf->num_queues);
+        if (!apply_iothread_vq_mapping(conf->iothread_vq_mapping_list,
+                                       s->vq_aio_context,
+                                       conf->num_queues,
+                                       errp)) {
+            g_free(s->vq_aio_context);
+            s->vq_aio_context = NULL;
+            return false;
+        }
     } else if (conf->iothread) {
         AioContext *ctx = iothread_get_aio_context(conf->iothread);
         for (unsigned i = 0; i < conf->num_queues; i++) {
@@ -1996,19 +2031,6 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    if (conf->iothread_vq_mapping_list) {
-        if (conf->iothread) {
-            error_setg(errp, "iothread and iothread-vq-mapping properties "
-                             "cannot be set at the same time");
-            return;
-        }
-
-        if (!validate_iothread_vq_mapping_list(conf->iothread_vq_mapping_list,
-                                               conf->num_queues, errp)) {
-            return;
-        }
-    }
-
     s->config_size = virtio_get_config_size(&virtio_blk_cfg_size_params,
                                             s->host_features);
     virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
-- 
2.43.0

Re: [PATCH 1/5] virtio-blk: enforce iothread-vq-mapping validation

[Manos Pitsidianakis]

On Mon, 05 Feb 2024 19:26, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>Hanna Czenczek <hreitz@redhat.com> noticed that the safety of
>`vq_aio_context[vq->value] = ctx;` with user-defined vq->value inputs is
>not obvious.
>
>The code is structured in validate() + apply() steps so input validation
>is there, but it happens way earlier and there is nothing that
>guarantees apply() can only be called with validated inputs.
>
>This patch moves the validate() call inside the apply() function so
>validation is guaranteed. I also added the bounds checking assertion
>that Hanna suggested.
>
>Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>---
> hw/block/virtio-blk.c | 192 +++++++++++++++++++++++-------------------
> 1 file changed, 107 insertions(+), 85 deletions(-)
>
>diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>index 227d83569f..e8b37fd5f4 100644
>--- a/hw/block/virtio-blk.c
>+++ b/hw/block/virtio-blk.c
>@@ -1485,6 +1485,72 @@ static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
>     return 0;
> }
> 
>+static void virtio_resize_cb(void *opaque)
>+{
>+    VirtIODevice *vdev = opaque;
>+
>+    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
>+    virtio_notify_config(vdev);
>+}
>+
>+static void virtio_blk_resize(void *opaque)
>+{
>+    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
>+
>+    /*
>+     * virtio_notify_config() needs to acquire the BQL,
>+     * so it can't be called from an iothread. Instead, schedule
>+     * it to be run in the main context BH.
>+     */
>+    aio_bh_schedule_oneshot(qemu_get_aio_context(), virtio_resize_cb, vdev);
>+}
>+
>+static void virtio_blk_ioeventfd_detach(VirtIOBlock *s)
>+{
>+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
>+
>+    for (uint16_t i = 0; i < s->conf.num_queues; i++) {
>+        VirtQueue *vq = virtio_get_queue(vdev, i);
>+        virtio_queue_aio_detach_host_notifier(vq, s->vq_aio_context[i]);
>+    }
>+}
>+
>+static void virtio_blk_ioeventfd_attach(VirtIOBlock *s)
>+{
>+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
>+
>+    for (uint16_t i = 0; i < s->conf.num_queues; i++) {
>+        VirtQueue *vq = virtio_get_queue(vdev, i);
>+        virtio_queue_aio_attach_host_notifier(vq, s->vq_aio_context[i]);
>+    }
>+}
>+
>+/* Suspend virtqueue ioeventfd processing during drain */
>+static void virtio_blk_drained_begin(void *opaque)
>+{
>+    VirtIOBlock *s = opaque;
>+
>+    if (s->ioeventfd_started) {
>+        virtio_blk_ioeventfd_detach(s);
>+    }
>+}
>+
>+/* Resume virtqueue ioeventfd processing after drain */
>+static void virtio_blk_drained_end(void *opaque)
>+{
>+    VirtIOBlock *s = opaque;
>+
>+    if (s->ioeventfd_started) {
>+        virtio_blk_ioeventfd_attach(s);
>+    }
>+}
>+
>+static const BlockDevOps virtio_block_ops = {
>+    .resize_cb     = virtio_blk_resize,
>+    .drained_begin = virtio_blk_drained_begin,
>+    .drained_end   = virtio_blk_drained_end,
>+};
>+
> static bool
> validate_iothread_vq_mapping_list(IOThreadVirtQueueMappingList *list,
>         uint16_t num_queues, Error **errp)
>@@ -1547,81 +1613,33 @@ validate_iothread_vq_mapping_list(IOThreadVirtQueueMappingList *list,
>     return true;
> }
> 
>-static void virtio_resize_cb(void *opaque)
>-{
>-    VirtIODevice *vdev = opaque;
>-
>-    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
>-    virtio_notify_config(vdev);
>-}
>-
>-static void virtio_blk_resize(void *opaque)
>-{
>-    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
>-
>-    /*
>-     * virtio_notify_config() needs to acquire the BQL,
>-     * so it can't be called from an iothread. Instead, schedule
>-     * it to be run in the main context BH.
>-     */
>-    aio_bh_schedule_oneshot(qemu_get_aio_context(), virtio_resize_cb, vdev);
>-}
>-
>-static void virtio_blk_ioeventfd_detach(VirtIOBlock *s)
>-{
>-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
>-
>-    for (uint16_t i = 0; i < s->conf.num_queues; i++) {
>-        VirtQueue *vq = virtio_get_queue(vdev, i);
>-        virtio_queue_aio_detach_host_notifier(vq, s->vq_aio_context[i]);
>-    }
>-}
>-
>-static void virtio_blk_ioeventfd_attach(VirtIOBlock *s)
>-{
>-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
>-
>-    for (uint16_t i = 0; i < s->conf.num_queues; i++) {
>-        VirtQueue *vq = virtio_get_queue(vdev, i);
>-        virtio_queue_aio_attach_host_notifier(vq, s->vq_aio_context[i]);
>-    }
>-}
>-
>-/* Suspend virtqueue ioeventfd processing during drain */
>-static void virtio_blk_drained_begin(void *opaque)
>-{
>-    VirtIOBlock *s = opaque;
>-
>-    if (s->ioeventfd_started) {
>-        virtio_blk_ioeventfd_detach(s);
>-    }
>-}
>-
>-/* Resume virtqueue ioeventfd processing after drain */
>-static void virtio_blk_drained_end(void *opaque)
>-{
>-    VirtIOBlock *s = opaque;
>-
>-    if (s->ioeventfd_started) {
>-        virtio_blk_ioeventfd_attach(s);
>-    }
>-}
>-
>-static const BlockDevOps virtio_block_ops = {
>-    .resize_cb     = virtio_blk_resize,
>-    .drained_begin = virtio_blk_drained_begin,
>-    .drained_end   = virtio_blk_drained_end,
>-};
>-
>-/* Generate vq:AioContext mappings from a validated iothread-vq-mapping list */
>-static void
>-apply_vq_mapping(IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
>-                 AioContext **vq_aio_context, uint16_t num_queues)
>+/**
>+ * apply_iothread_vq_mapping:
>+ * @iothread_vq_mapping_list: The mapping of virtqueues to IOThreads.
>+ * @vq_aio_context: The array of AioContext pointers to fill in.
>+ * @num_queues: The length of @vq_aio_context.
>+ * @errp: If an error occurs, a pointer to the area to store the error.
>+ *
>+ * Fill in the AioContext for each virtqueue in the @vq_aio_context array given
>+ * the iothread-vq-mapping parameter in @iothread_vq_mapping_list.
>+ *
>+ * Returns: %true on success, %false on failure.
>+ **/
>+static bool apply_iothread_vq_mapping(
>+        IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
>+        AioContext **vq_aio_context,
>+        uint16_t num_queues,
>+        Error **errp)
> {
>     IOThreadVirtQueueMappingList *node;
>     size_t num_iothreads = 0;
>     size_t cur_iothread = 0;
> 
>+    if (!validate_iothread_vq_mapping_list(iothread_vq_mapping_list,
>+                                           num_queues, errp)) {
>+        return false;
>+    }
>+
>     for (node = iothread_vq_mapping_list; node; node = node->next) {
>         num_iothreads++;
>     }
>@@ -1638,6 +1656,7 @@ apply_vq_mapping(IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
> 
>             /* Explicit vq:IOThread assignment */
>             for (vq = node->value->vqs; vq; vq = vq->next) {
>+                assert(vq->value < num_queues);
>                 vq_aio_context[vq->value] = ctx;
>             }
>         } else {
>@@ -1650,6 +1669,8 @@ apply_vq_mapping(IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
> 
>         cur_iothread++;
>     }
>+
>+    return true;
> }
> 
> /* Context: BQL held */
>@@ -1660,6 +1681,14 @@ static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp)
>     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
>     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> 
>+    if (conf->iothread && conf->iothread_vq_mapping_list) {
>+        if (conf->iothread) {
>+            error_setg(errp, "iothread and iothread-vq-mapping properties "
>+                             "cannot be set at the same time");
>+            return false;
>+        }
>+    }
>+
>     if (conf->iothread || conf->iothread_vq_mapping_list) {
>         if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
>             error_setg(errp,
>@@ -1685,8 +1714,14 @@ static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp)
>     s->vq_aio_context = g_new(AioContext *, conf->num_queues);
> 
>     if (conf->iothread_vq_mapping_list) {
>-        apply_vq_mapping(conf->iothread_vq_mapping_list, s->vq_aio_context,
>-                         conf->num_queues);
>+        if (!apply_iothread_vq_mapping(conf->iothread_vq_mapping_list,
>+                                       s->vq_aio_context,
>+                                       conf->num_queues,
>+                                       errp)) {
>+            g_free(s->vq_aio_context);
>+            s->vq_aio_context = NULL;
>+            return false;
>+        }
>     } else if (conf->iothread) {
>         AioContext *ctx = iothread_get_aio_context(conf->iothread);
>         for (unsigned i = 0; i < conf->num_queues; i++) {
>@@ -1996,19 +2031,6 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>         return;
>     }
> 
>-    if (conf->iothread_vq_mapping_list) {
>-        if (conf->iothread) {
>-            error_setg(errp, "iothread and iothread-vq-mapping properties "
>-                             "cannot be set at the same time");
>-            return;
>-        }
>-
>-        if (!validate_iothread_vq_mapping_list(conf->iothread_vq_mapping_list,
>-                                               conf->num_queues, errp)) {
>-            return;
>-        }
>-    }
>-
>     s->config_size = virtio_get_config_size(&virtio_blk_cfg_size_params,
>                                             s->host_features);
>     virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
>-- 
>2.43.0
>
>


virtio_block_ops and methods are moved around without changes in the 
diff, is that on purpose? If no the patch and history would be less 
noisy.


Regardless:

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH 1/5] virtio-blk: enforce iothread-vq-mapping validation

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 10847 bytes --]

On Tue, Feb 06, 2024 at 09:29:29AM +0200, Manos Pitsidianakis wrote:
> On Mon, 05 Feb 2024 19:26, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > Hanna Czenczek <hreitz@redhat.com> noticed that the safety of
> > `vq_aio_context[vq->value] = ctx;` with user-defined vq->value inputs is
> > not obvious.
> > 
> > The code is structured in validate() + apply() steps so input validation
> > is there, but it happens way earlier and there is nothing that
> > guarantees apply() can only be called with validated inputs.
> > 
> > This patch moves the validate() call inside the apply() function so
> > validation is guaranteed. I also added the bounds checking assertion
> > that Hanna suggested.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > hw/block/virtio-blk.c | 192 +++++++++++++++++++++++-------------------
> > 1 file changed, 107 insertions(+), 85 deletions(-)
> > 
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 227d83569f..e8b37fd5f4 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -1485,6 +1485,72 @@ static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
> >     return 0;
> > }
> > 
> > +static void virtio_resize_cb(void *opaque)
> > +{
> > +    VirtIODevice *vdev = opaque;
> > +
> > +    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> > +    virtio_notify_config(vdev);
> > +}
> > +
> > +static void virtio_blk_resize(void *opaque)
> > +{
> > +    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
> > +
> > +    /*
> > +     * virtio_notify_config() needs to acquire the BQL,
> > +     * so it can't be called from an iothread. Instead, schedule
> > +     * it to be run in the main context BH.
> > +     */
> > +    aio_bh_schedule_oneshot(qemu_get_aio_context(), virtio_resize_cb, vdev);
> > +}
> > +
> > +static void virtio_blk_ioeventfd_detach(VirtIOBlock *s)
> > +{
> > +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> > +
> > +    for (uint16_t i = 0; i < s->conf.num_queues; i++) {
> > +        VirtQueue *vq = virtio_get_queue(vdev, i);
> > +        virtio_queue_aio_detach_host_notifier(vq, s->vq_aio_context[i]);
> > +    }
> > +}
> > +
> > +static void virtio_blk_ioeventfd_attach(VirtIOBlock *s)
> > +{
> > +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> > +
> > +    for (uint16_t i = 0; i < s->conf.num_queues; i++) {
> > +        VirtQueue *vq = virtio_get_queue(vdev, i);
> > +        virtio_queue_aio_attach_host_notifier(vq, s->vq_aio_context[i]);
> > +    }
> > +}
> > +
> > +/* Suspend virtqueue ioeventfd processing during drain */
> > +static void virtio_blk_drained_begin(void *opaque)
> > +{
> > +    VirtIOBlock *s = opaque;
> > +
> > +    if (s->ioeventfd_started) {
> > +        virtio_blk_ioeventfd_detach(s);
> > +    }
> > +}
> > +
> > +/* Resume virtqueue ioeventfd processing after drain */
> > +static void virtio_blk_drained_end(void *opaque)
> > +{
> > +    VirtIOBlock *s = opaque;
> > +
> > +    if (s->ioeventfd_started) {
> > +        virtio_blk_ioeventfd_attach(s);
> > +    }
> > +}
> > +
> > +static const BlockDevOps virtio_block_ops = {
> > +    .resize_cb     = virtio_blk_resize,
> > +    .drained_begin = virtio_blk_drained_begin,
> > +    .drained_end   = virtio_blk_drained_end,
> > +};
> > +
> > static bool
> > validate_iothread_vq_mapping_list(IOThreadVirtQueueMappingList *list,
> >         uint16_t num_queues, Error **errp)
> > @@ -1547,81 +1613,33 @@ validate_iothread_vq_mapping_list(IOThreadVirtQueueMappingList *list,
> >     return true;
> > }
> > 
> > -static void virtio_resize_cb(void *opaque)
> > -{
> > -    VirtIODevice *vdev = opaque;
> > -
> > -    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> > -    virtio_notify_config(vdev);
> > -}
> > -
> > -static void virtio_blk_resize(void *opaque)
> > -{
> > -    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
> > -
> > -    /*
> > -     * virtio_notify_config() needs to acquire the BQL,
> > -     * so it can't be called from an iothread. Instead, schedule
> > -     * it to be run in the main context BH.
> > -     */
> > -    aio_bh_schedule_oneshot(qemu_get_aio_context(), virtio_resize_cb, vdev);
> > -}
> > -
> > -static void virtio_blk_ioeventfd_detach(VirtIOBlock *s)
> > -{
> > -    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> > -
> > -    for (uint16_t i = 0; i < s->conf.num_queues; i++) {
> > -        VirtQueue *vq = virtio_get_queue(vdev, i);
> > -        virtio_queue_aio_detach_host_notifier(vq, s->vq_aio_context[i]);
> > -    }
> > -}
> > -
> > -static void virtio_blk_ioeventfd_attach(VirtIOBlock *s)
> > -{
> > -    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> > -
> > -    for (uint16_t i = 0; i < s->conf.num_queues; i++) {
> > -        VirtQueue *vq = virtio_get_queue(vdev, i);
> > -        virtio_queue_aio_attach_host_notifier(vq, s->vq_aio_context[i]);
> > -    }
> > -}
> > -
> > -/* Suspend virtqueue ioeventfd processing during drain */
> > -static void virtio_blk_drained_begin(void *opaque)
> > -{
> > -    VirtIOBlock *s = opaque;
> > -
> > -    if (s->ioeventfd_started) {
> > -        virtio_blk_ioeventfd_detach(s);
> > -    }
> > -}
> > -
> > -/* Resume virtqueue ioeventfd processing after drain */
> > -static void virtio_blk_drained_end(void *opaque)
> > -{
> > -    VirtIOBlock *s = opaque;
> > -
> > -    if (s->ioeventfd_started) {
> > -        virtio_blk_ioeventfd_attach(s);
> > -    }
> > -}
> > -
> > -static const BlockDevOps virtio_block_ops = {
> > -    .resize_cb     = virtio_blk_resize,
> > -    .drained_begin = virtio_blk_drained_begin,
> > -    .drained_end   = virtio_blk_drained_end,
> > -};
> > -
> > -/* Generate vq:AioContext mappings from a validated iothread-vq-mapping list */
> > -static void
> > -apply_vq_mapping(IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
> > -                 AioContext **vq_aio_context, uint16_t num_queues)
> > +/**
> > + * apply_iothread_vq_mapping:
> > + * @iothread_vq_mapping_list: The mapping of virtqueues to IOThreads.
> > + * @vq_aio_context: The array of AioContext pointers to fill in.
> > + * @num_queues: The length of @vq_aio_context.
> > + * @errp: If an error occurs, a pointer to the area to store the error.
> > + *
> > + * Fill in the AioContext for each virtqueue in the @vq_aio_context array given
> > + * the iothread-vq-mapping parameter in @iothread_vq_mapping_list.
> > + *
> > + * Returns: %true on success, %false on failure.
> > + **/
> > +static bool apply_iothread_vq_mapping(
> > +        IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
> > +        AioContext **vq_aio_context,
> > +        uint16_t num_queues,
> > +        Error **errp)
> > {
> >     IOThreadVirtQueueMappingList *node;
> >     size_t num_iothreads = 0;
> >     size_t cur_iothread = 0;
> > 
> > +    if (!validate_iothread_vq_mapping_list(iothread_vq_mapping_list,
> > +                                           num_queues, errp)) {
> > +        return false;
> > +    }
> > +
> >     for (node = iothread_vq_mapping_list; node; node = node->next) {
> >         num_iothreads++;
> >     }
> > @@ -1638,6 +1656,7 @@ apply_vq_mapping(IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
> > 
> >             /* Explicit vq:IOThread assignment */
> >             for (vq = node->value->vqs; vq; vq = vq->next) {
> > +                assert(vq->value < num_queues);
> >                 vq_aio_context[vq->value] = ctx;
> >             }
> >         } else {
> > @@ -1650,6 +1669,8 @@ apply_vq_mapping(IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
> > 
> >         cur_iothread++;
> >     }
> > +
> > +    return true;
> > }
> > 
> > /* Context: BQL held */
> > @@ -1660,6 +1681,14 @@ static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp)
> >     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> >     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> > 
> > +    if (conf->iothread && conf->iothread_vq_mapping_list) {
> > +        if (conf->iothread) {
> > +            error_setg(errp, "iothread and iothread-vq-mapping properties "
> > +                             "cannot be set at the same time");
> > +            return false;
> > +        }
> > +    }
> > +
> >     if (conf->iothread || conf->iothread_vq_mapping_list) {
> >         if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
> >             error_setg(errp,
> > @@ -1685,8 +1714,14 @@ static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp)
> >     s->vq_aio_context = g_new(AioContext *, conf->num_queues);
> > 
> >     if (conf->iothread_vq_mapping_list) {
> > -        apply_vq_mapping(conf->iothread_vq_mapping_list, s->vq_aio_context,
> > -                         conf->num_queues);
> > +        if (!apply_iothread_vq_mapping(conf->iothread_vq_mapping_list,
> > +                                       s->vq_aio_context,
> > +                                       conf->num_queues,
> > +                                       errp)) {
> > +            g_free(s->vq_aio_context);
> > +            s->vq_aio_context = NULL;
> > +            return false;
> > +        }
> >     } else if (conf->iothread) {
> >         AioContext *ctx = iothread_get_aio_context(conf->iothread);
> >         for (unsigned i = 0; i < conf->num_queues; i++) {
> > @@ -1996,19 +2031,6 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
> >         return;
> >     }
> > 
> > -    if (conf->iothread_vq_mapping_list) {
> > -        if (conf->iothread) {
> > -            error_setg(errp, "iothread and iothread-vq-mapping properties "
> > -                             "cannot be set at the same time");
> > -            return;
> > -        }
> > -
> > -        if (!validate_iothread_vq_mapping_list(conf->iothread_vq_mapping_list,
> > -                                               conf->num_queues, errp)) {
> > -            return;
> > -        }
> > -    }
> > -
> >     s->config_size = virtio_get_config_size(&virtio_blk_cfg_size_params,
> >                                             s->host_features);
> >     virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
> > -- 
> > 2.43.0
> > 
> > 
> 
> 
> virtio_block_ops and methods are moved around without changes in the diff,
> is that on purpose? If no the patch and history would be less noisy.

Yes, it's because I moved the validate() function immediately before the
apply() function. Previously they were far apart. I guess git's diff
algorithm optimized for the shortest patch rather than the most readable
patch.

> Regardless:
> 
> Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/5] virtio-blk: enforce iothread-vq-mapping validation

[Hanna Czenczek]

On 05.02.24 18:26, Stefan Hajnoczi wrote:
> Hanna Czenczek <hreitz@redhat.com> noticed that the safety of
> `vq_aio_context[vq->value] = ctx;` with user-defined vq->value inputs is
> not obvious.
>
> The code is structured in validate() + apply() steps so input validation
> is there, but it happens way earlier and there is nothing that
> guarantees apply() can only be called with validated inputs.
>
> This patch moves the validate() call inside the apply() function so
> validation is guaranteed. I also added the bounds checking assertion
> that Hanna suggested.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   hw/block/virtio-blk.c | 192 +++++++++++++++++++++++-------------------
>   1 file changed, 107 insertions(+), 85 deletions(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 227d83569f..e8b37fd5f4 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c

[...]

> @@ -1660,6 +1681,14 @@ static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp)
>       BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
>       VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>   
> +    if (conf->iothread && conf->iothread_vq_mapping_list) {
> +        if (conf->iothread) {

This inner condition should probably be dropped.  With that done:

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

> +            error_setg(errp, "iothread and iothread-vq-mapping properties "
> +                             "cannot be set at the same time");
> +            return false;
> +        }
> +    }
> +
>       if (conf->iothread || conf->iothread_vq_mapping_list) {
>           if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
>               error_setg(errp,

[PATCH 2/5] virtio-blk: clarify that there is at least 1 virtqueue

[Stefan Hajnoczi]

It is not possible to instantiate a virtio-blk device with 0 virtqueues.
The following check is located in ->realize():

  if (!conf->num_queues) {
      error_setg(errp, "num-queues property must be larger than 0");
      return;
  }

Later on we access s->vq_aio_context[0] under the assumption that there
is as least one virtqueue. Hanna Czenczek <hreitz@redhat.com> noted that
it would help to show that the array index is already valid.

Add an assertion to document that s->vq_aio_context[0] is always
safe...and catch future code changes that break this assumption.

Suggested-by: Hanna Czenczek <hreitz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e8b37fd5f4..a0735a9bca 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1825,6 +1825,7 @@ static int virtio_blk_start_ioeventfd(VirtIODevice *vdev)
      * Try to change the AioContext so that block jobs and other operations can
      * co-locate their activity in the same AioContext. If it fails, nevermind.
      */
+    assert(nvqs > 0); /* enforced during ->realize() */
     r = blk_set_aio_context(s->conf.conf.blk, s->vq_aio_context[0],
                             &local_err);
     if (r < 0) {
-- 
2.43.0

Re: [PATCH 2/5] virtio-blk: clarify that there is at least 1 virtqueue

[Manos Pitsidianakis]

On Mon, 05 Feb 2024 19:26, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>It is not possible to instantiate a virtio-blk device with 0 virtqueues.
>The following check is located in ->realize():
>
>  if (!conf->num_queues) {
>      error_setg(errp, "num-queues property must be larger than 0");
>      return;
>  }
>
>Later on we access s->vq_aio_context[0] under the assumption that there
>is as least one virtqueue. Hanna Czenczek <hreitz@redhat.com> noted that
>it would help to show that the array index is already valid.
>
>Add an assertion to document that s->vq_aio_context[0] is always
>safe...and catch future code changes that break this assumption.
>
>Suggested-by: Hanna Czenczek <hreitz@redhat.com>
>Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>---
> hw/block/virtio-blk.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>index e8b37fd5f4..a0735a9bca 100644
>--- a/hw/block/virtio-blk.c
>+++ b/hw/block/virtio-blk.c
>@@ -1825,6 +1825,7 @@ static int virtio_blk_start_ioeventfd(VirtIODevice *vdev)
>      * Try to change the AioContext so that block jobs and other operations can
>      * co-locate their activity in the same AioContext. If it fails, nevermind.
>      */
>+    assert(nvqs > 0); /* enforced during ->realize() */
>     r = blk_set_aio_context(s->conf.conf.blk, s->vq_aio_context[0],
>                             &local_err);
>     if (r < 0) {
>-- 
>2.43.0
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH 2/5] virtio-blk: clarify that there is at least 1 virtqueue

[Hanna Czenczek]

[-- Attachment #1: Type: text/plain, Size: 871 bytes --]

On 05.02.24 18:26, Stefan Hajnoczi wrote:
> It is not possible to instantiate a virtio-blk device with 0 virtqueues.
> The following check is located in ->realize():
>
>    if (!conf->num_queues) {
>        error_setg(errp, "num-queues property must be larger than 0");
>        return;
>    }
>
> Later on we access s->vq_aio_context[0] under the assumption that there
> is as least one virtqueue. Hanna Czenczek<hreitz@redhat.com>  noted that
> it would help to show that the array index is already valid.
>
> Add an assertion to document that s->vq_aio_context[0] is always
> safe...and catch future code changes that break this assumption.
>
> Suggested-by: Hanna Czenczek<hreitz@redhat.com>
> Signed-off-by: Stefan Hajnoczi<stefanha@redhat.com>
> ---
>   hw/block/virtio-blk.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

[-- Attachment #2: Type: text/html, Size: 1513 bytes --]

[PATCH 3/5] virtio-blk: add vq_rq[] bounds check in virtio_blk_dma_restart_cb()

[Stefan Hajnoczi]

Hanna Czenczek <hreitz@redhat.com> noted that the array index in
virtio_blk_dma_restart_cb() is not bounds-checked:

  g_autofree VirtIOBlockReq **vq_rq = g_new0(VirtIOBlockReq *, num_queues);
  ...
  while (rq) {
      VirtIOBlockReq *next = rq->next;
      uint16_t idx = virtio_get_queue_index(rq->vq);

      rq->next = vq_rq[idx];
                 ^^^^^^^^^^

The code is correct because both rq->vq and vq_rq[] depend on
num_queues, but this is indirect and not 100% obvious. Add an assertion.

Suggested-by: Hanna Czenczek <hreitz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index a0735a9bca..f3193f4b75 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1209,6 +1209,7 @@ static void virtio_blk_dma_restart_cb(void *opaque, bool running,
         VirtIOBlockReq *next = rq->next;
         uint16_t idx = virtio_get_queue_index(rq->vq);
 
+        assert(idx < num_queues);
         rq->next = vq_rq[idx];
         vq_rq[idx] = rq;
         rq = next;
-- 
2.43.0

Re: [PATCH 3/5] virtio-blk: add vq_rq[] bounds check in virtio_blk_dma_restart_cb()

[Manos Pitsidianakis]

On Mon, 05 Feb 2024 19:26, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>Hanna Czenczek <hreitz@redhat.com> noted that the array index in
>virtio_blk_dma_restart_cb() is not bounds-checked:
>
>  g_autofree VirtIOBlockReq **vq_rq = g_new0(VirtIOBlockReq *, num_queues);
>  ...
>  while (rq) {
>      VirtIOBlockReq *next = rq->next;
>      uint16_t idx = virtio_get_queue_index(rq->vq);
>
>      rq->next = vq_rq[idx];
>                 ^^^^^^^^^^
>
>The code is correct because both rq->vq and vq_rq[] depend on
>num_queues, but this is indirect and not 100% obvious. Add an assertion.

This sentence could be useful as an inline comment too.

>
>Suggested-by: Hanna Czenczek <hreitz@redhat.com>
>Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>---
> hw/block/virtio-blk.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>index a0735a9bca..f3193f4b75 100644
>--- a/hw/block/virtio-blk.c
>+++ b/hw/block/virtio-blk.c
>@@ -1209,6 +1209,7 @@ static void virtio_blk_dma_restart_cb(void *opaque, bool running,
>         VirtIOBlockReq *next = rq->next;
>         uint16_t idx = virtio_get_queue_index(rq->vq);
> 
>+        assert(idx < num_queues);
>         rq->next = vq_rq[idx];
>         vq_rq[idx] = rq;
>         rq = next;
>-- 
>2.43.0
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH 3/5] virtio-blk: add vq_rq[] bounds check in virtio_blk_dma_restart_cb()

[Hanna Czenczek]

On 05.02.24 18:26, Stefan Hajnoczi wrote:
> Hanna Czenczek <hreitz@redhat.com> noted that the array index in
> virtio_blk_dma_restart_cb() is not bounds-checked:
>
>    g_autofree VirtIOBlockReq **vq_rq = g_new0(VirtIOBlockReq *, num_queues);
>    ...
>    while (rq) {
>        VirtIOBlockReq *next = rq->next;
>        uint16_t idx = virtio_get_queue_index(rq->vq);
>
>        rq->next = vq_rq[idx];
>                   ^^^^^^^^^^
>
> The code is correct because both rq->vq and vq_rq[] depend on
> num_queues, but this is indirect and not 100% obvious. Add an assertion.
>
> Suggested-by: Hanna Czenczek <hreitz@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   hw/block/virtio-blk.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

[PATCH 4/5] virtio-blk: declare VirtIOBlock::rq with a type

[Stefan Hajnoczi]

The VirtIOBlock::rq field has had the type void * since its introduction
in commit 869a5c6df19a ("Stop VM on error in virtio-blk. (Gleb
Natapov)").

Perhaps this was done to avoid the forward declaration of
VirtIOBlockReq.

Hanna Czenczek <hreitz@redhat.com> pointed out the missing type. Specify
the actual type because there is no need to use void * here.

Suggested-by: Hanna Czenczek <hreitz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/virtio/virtio-blk.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 833a9a344f..5c14110c4b 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -55,7 +55,7 @@ struct VirtIOBlock {
     VirtIODevice parent_obj;
     BlockBackend *blk;
     QemuMutex rq_lock;
-    void *rq; /* protected by rq_lock */
+    struct VirtIOBlockReq *rq; /* protected by rq_lock */
     VirtIOBlkConf conf;
     unsigned short sector_mask;
     bool original_wce;
-- 
2.43.0

Re: [PATCH 4/5] virtio-blk: declare VirtIOBlock::rq with a type

[Manos Pitsidianakis]

On Mon, 05 Feb 2024 19:26, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>The VirtIOBlock::rq field has had the type void * since its introduction
>in commit 869a5c6df19a ("Stop VM on error in virtio-blk. (Gleb
>Natapov)").
>
>Perhaps this was done to avoid the forward declaration of
>VirtIOBlockReq.
>
>Hanna Czenczek <hreitz@redhat.com> pointed out the missing type. Specify
>the actual type because there is no need to use void * here.
>
>Suggested-by: Hanna Czenczek <hreitz@redhat.com>
>Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>---
> include/hw/virtio/virtio-blk.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
>index 833a9a344f..5c14110c4b 100644
>--- a/include/hw/virtio/virtio-blk.h
>+++ b/include/hw/virtio/virtio-blk.h
>@@ -55,7 +55,7 @@ struct VirtIOBlock {
>     VirtIODevice parent_obj;
>     BlockBackend *blk;
>     QemuMutex rq_lock;
>-    void *rq; /* protected by rq_lock */
>+    struct VirtIOBlockReq *rq; /* protected by rq_lock */
>     VirtIOBlkConf conf;
>     unsigned short sector_mask;
>     bool original_wce;
>-- 
>2.43.0
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH 4/5] virtio-blk: declare VirtIOBlock::rq with a type

[Hanna Czenczek]

On 05.02.24 18:26, Stefan Hajnoczi wrote:
> The VirtIOBlock::rq field has had the type void * since its introduction
> in commit 869a5c6df19a ("Stop VM on error in virtio-blk. (Gleb
> Natapov)").
>
> Perhaps this was done to avoid the forward declaration of
> VirtIOBlockReq.
>
> Hanna Czenczek <hreitz@redhat.com> pointed out the missing type. Specify
> the actual type because there is no need to use void * here.
>
> Suggested-by: Hanna Czenczek <hreitz@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   include/hw/virtio/virtio-blk.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

[PATCH 5/5] monitor: use aio_co_reschedule_self()

[Stefan Hajnoczi]

The aio_co_reschedule_self() API is designed to avoid the race
condition between scheduling the coroutine in another AioContext and
yielding.

The QMP dispatch code uses the open-coded version that appears
susceptible to the race condition at first glance:

  aio_co_schedule(qemu_get_aio_context(), qemu_coroutine_self());
  qemu_coroutine_yield();

The code is actually safe because the iohandler and qemu_aio_context
AioContext run under the Big QEMU Lock. Nevertheless, set a good example
and use aio_co_reschedule_self() so it's obvious that there is no race.

Suggested-by: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qapi/qmp-dispatch.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 176b549473..f3488afeef 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -212,8 +212,7 @@ QDict *coroutine_mixed_fn qmp_dispatch(const QmpCommandList *cmds, QObject *requ
              * executing the command handler so that it can make progress if it
              * involves an AIO_WAIT_WHILE().
              */
-            aio_co_schedule(qemu_get_aio_context(), qemu_coroutine_self());
-            qemu_coroutine_yield();
+            aio_co_reschedule_self(qemu_get_aio_context());
         }
 
         monitor_set_cur(qemu_coroutine_self(), cur_mon);
@@ -227,9 +226,7 @@ QDict *coroutine_mixed_fn qmp_dispatch(const QmpCommandList *cmds, QObject *requ
              * Move back to iohandler_ctx so that nested event loops for
              * qemu_aio_context don't start new monitor commands.
              */
-            aio_co_schedule(iohandler_get_aio_context(),
-                            qemu_coroutine_self());
-            qemu_coroutine_yield();
+            aio_co_reschedule_self(iohandler_get_aio_context());
         }
     } else {
        /*
-- 
2.43.0

Re: [PATCH 5/5] monitor: use aio_co_reschedule_self()

[Manos Pitsidianakis]

On Mon, 05 Feb 2024 19:26, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>The aio_co_reschedule_self() API is designed to avoid the race
>condition between scheduling the coroutine in another AioContext and
>yielding.
>
>The QMP dispatch code uses the open-coded version that appears
>susceptible to the race condition at first glance:
>
>  aio_co_schedule(qemu_get_aio_context(), qemu_coroutine_self());
>  qemu_coroutine_yield();
>
>The code is actually safe because the iohandler and qemu_aio_context
>AioContext run under the Big QEMU Lock. Nevertheless, set a good example
>and use aio_co_reschedule_self() so it's obvious that there is no race.
>
>Suggested-by: Hanna Reitz <hreitz@redhat.com>
>Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>---
> qapi/qmp-dispatch.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
>diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
>index 176b549473..f3488afeef 100644
>--- a/qapi/qmp-dispatch.c
>+++ b/qapi/qmp-dispatch.c
>@@ -212,8 +212,7 @@ QDict *coroutine_mixed_fn qmp_dispatch(const QmpCommandList *cmds, QObject *requ
>              * executing the command handler so that it can make progress if it
>              * involves an AIO_WAIT_WHILE().
>              */
>-            aio_co_schedule(qemu_get_aio_context(), qemu_coroutine_self());
>-            qemu_coroutine_yield();
>+            aio_co_reschedule_self(qemu_get_aio_context());
>         }
> 
>         monitor_set_cur(qemu_coroutine_self(), cur_mon);
>@@ -227,9 +226,7 @@ QDict *coroutine_mixed_fn qmp_dispatch(const QmpCommandList *cmds, QObject *requ
>              * Move back to iohandler_ctx so that nested event loops for
>              * qemu_aio_context don't start new monitor commands.
>              */
>-            aio_co_schedule(iohandler_get_aio_context(),
>-                            qemu_coroutine_self());
>-            qemu_coroutine_yield();
>+            aio_co_reschedule_self(iohandler_get_aio_context());
>         }
>     } else {
>        /*
>-- 
>2.43.0
>
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH 5/5] monitor: use aio_co_reschedule_self()

[Hanna Czenczek]

On 05.02.24 18:26, Stefan Hajnoczi wrote:
> The aio_co_reschedule_self() API is designed to avoid the race
> condition between scheduling the coroutine in another AioContext and
> yielding.
>
> The QMP dispatch code uses the open-coded version that appears
> susceptible to the race condition at first glance:
>
>    aio_co_schedule(qemu_get_aio_context(), qemu_coroutine_self());
>    qemu_coroutine_yield();
>
> The code is actually safe because the iohandler and qemu_aio_context
> AioContext run under the Big QEMU Lock. Nevertheless, set a good example
> and use aio_co_reschedule_self() so it's obvious that there is no race.
>
> Suggested-by: Hanna Reitz <hreitz@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   qapi/qmp-dispatch.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Re: [PATCH 5/5] monitor: use aio_co_reschedule_self()

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> The aio_co_reschedule_self() API is designed to avoid the race
> condition between scheduling the coroutine in another AioContext and
> yielding.
>
> The QMP dispatch code uses the open-coded version that appears
> susceptible to the race condition at first glance:
>
>   aio_co_schedule(qemu_get_aio_context(), qemu_coroutine_self());
>   qemu_coroutine_yield();
>
> The code is actually safe because the iohandler and qemu_aio_context
> AioContext run under the Big QEMU Lock. Nevertheless, set a good example
> and use aio_co_reschedule_self() so it's obvious that there is no race.
>
> Suggested-by: Hanna Reitz <hreitz@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Acked-by: Markus Armbruster <armbru@redhat.com>

Feel free to merge this together with the remainder of the series.