[Qemu-devel] [RFC v2 0/3] scsi: restart dma after vm change state handlers

[Qemu-devel] [RFC v2 0/3] scsi: restart dma after vm change state handlers

[Stefan Hajnoczi]

v2:
 * Do it properly with a clean API instead of deferring to a BH!
   Thanks for encouraging me to do this, Kevin.

These patches solve a deadlock when the 'cont' command is used and there are
failed requests on a virtio-scsi device with iothreads.  The deadlock itself is
actually not the thing we need to fix because we should never reach that case
anyway.  Instead we need to make sure DMA restart is only performed after the
virtio-scsi iothread is re-initialized.

Stefan Hajnoczi (3):
  virtio: add vdc->vmchange_state() callback
  scsi: add scsi_bus_dma_restart()
  virtio-scsi: fix iothread deadlock on 'cont'

 include/hw/scsi/scsi.h     |  5 +++++
 include/hw/virtio/virtio.h |  7 +++++++
 hw/scsi/scsi-bus.c         | 37 ++++++++++++++++++++++++++++++-------
 hw/scsi/virtio-scsi.c      | 17 +++++++++++++++++
 hw/virtio/virtio.c         |  9 +++++++++
 5 files changed, 68 insertions(+), 7 deletions(-)

-- 
2.21.0

[Qemu-devel] [RFC v2 1/3] virtio: add vdc->vmchange_state() callback

[Stefan Hajnoczi]

The core virtio code invokes ->set_status() followed by
->ioeventfd_start() when the guest resumes execution.  Both of these
functions are also called in other cases unrelated to vm change state.

This patch introduces ->vmstate_change() so that devices can act on
guest pause/resume.  The existing qemu_add_vm_change_state_handler() API
isn't usable by virtio devices since the ordering between vm change
state handlers is undefined.  The new ->vmstate_change() callback is
always invoked after ->set_status() and ->ioeventfd_start() when
resuming a guest.

A later patch makes use of this new callback.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/virtio/virtio.h | 7 +++++++
 hw/virtio/virtio.c         | 9 +++++++++
 2 files changed, 16 insertions(+)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 7140381e3a..46e54362dc 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -156,6 +156,13 @@ typedef struct VirtioDeviceClass {
     void (*save)(VirtIODevice *vdev, QEMUFile *f);
     int (*load)(VirtIODevice *vdev, QEMUFile *f, int version_id);
     const VMStateDescription *vmsd;
+
+    /* Called when the device should start/stop running because the guest was
+     * resumed/paused.  Note that this takes VIRTIO_CONFIG_S_DRIVER_OK into
+     * account so running is true iff the guest is resumed and the guest driver
+     * has already indicated it is ready.
+     */
+    void (*vmstate_change)(VirtIODevice *vdev, bool running);
 } VirtioDeviceClass;
 
 void virtio_instance_init_common(Object *proxy_obj, void *data,
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 28056a7ef7..f9f31b5325 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2246,6 +2246,7 @@ static void virtio_vmstate_change(void *opaque, int running, RunState state)
     VirtIODevice *vdev = opaque;
     BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
     bool backend_run = running && (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK);
     vdev->vm_running = running;
 
@@ -2253,10 +2254,18 @@ static void virtio_vmstate_change(void *opaque, int running, RunState state)
         virtio_set_status(vdev, vdev->status);
     }
 
+    if (!backend_run && k->vmstate_change) {
+        vdc->vmstate_change(vdev, backend_run);
+    }
+
     if (k->vmstate_change) {
         k->vmstate_change(qbus->parent, backend_run);
     }
 
+    if (backend_run && k->vmstate_change) {
+        vdc->vmstate_change(vdev, backend_run);
+    }
+
     if (!backend_run) {
         virtio_set_status(vdev, vdev->status);
     }
-- 
2.21.0

[Qemu-devel] [RFC v2 2/3] scsi: add scsi_bus_dma_restart()

[Stefan Hajnoczi]

By default scsi-bus.c registers vm change state handlers for SCSIDevice
instances and restarts DMA when guest execution is resumed.

Unfortunately virtio-scsi with iothreads has a special ordering
requirement that the core virtio code's vm change state handler runs
before scsi-bus.c's vm change state handler.

This patch allows SCSI busses to disable the default vm change state
handler for DMA restart.  The new scsi_bus_dma_restart() API allows them
to manually restart DMA at a time of their choice.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/scsi/scsi.h |  5 +++++
 hw/scsi/scsi-bus.c     | 37 ++++++++++++++++++++++++++++++-------
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index acef25faa4..e9cc563daa 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -120,6 +120,10 @@ struct SCSIReqOps {
 struct SCSIBusInfo {
     int tcq;
     int max_channel, max_target, max_lun;
+
+    /* Will the bus call scsi_bus_dma_restart() itself? */
+    bool custom_dma_restart;
+
     int (*parse_cdb)(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf,
                      void *hba_private);
     void (*transfer_data)(SCSIRequest *req, uint32_t arg);
@@ -160,6 +164,7 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk,
                                       const char *serial, Error **errp);
 void scsi_bus_legacy_handle_cmdline(SCSIBus *bus);
 void scsi_legacy_handle_cmdline(void);
+void scsi_bus_dma_restart(SCSIBus *bus);
 
 SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
                             uint32_t tag, uint32_t lun, void *hba_private);
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index c480553083..d2c5a1652b 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -134,6 +134,27 @@ void scsi_req_retry(SCSIRequest *req)
     req->retry = true;
 }
 
+static void scsi_device_dma_restart(SCSIDevice *dev)
+{
+    if (!dev->bh) {
+        AioContext *ctx = blk_get_aio_context(dev->conf.blk);
+        dev->bh = aio_bh_new(ctx, scsi_dma_restart_bh, dev);
+        qemu_bh_schedule(dev->bh);
+    }
+}
+
+void scsi_bus_dma_restart(SCSIBus *bus)
+{
+    BusChild *kid;
+
+    QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
+        DeviceState *qdev = kid->child;
+        SCSIDevice *dev = SCSI_DEVICE(qdev);
+
+        scsi_device_dma_restart(dev);
+    }
+}
+
 static void scsi_dma_restart_cb(void *opaque, int running, RunState state)
 {
     SCSIDevice *s = opaque;
@@ -141,11 +162,8 @@ static void scsi_dma_restart_cb(void *opaque, int running, RunState state)
     if (!running) {
         return;
     }
-    if (!s->bh) {
-        AioContext *ctx = blk_get_aio_context(s->conf.blk);
-        s->bh = aio_bh_new(ctx, scsi_dma_restart_bh, s);
-        qemu_bh_schedule(s->bh);
-    }
+
+    scsi_device_dma_restart(s);
 }
 
 static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
@@ -206,8 +224,13 @@ static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
         error_propagate(errp, local_err);
         return;
     }
-    dev->vmsentry = qemu_add_vm_change_state_handler(scsi_dma_restart_cb,
-                                                     dev);
+
+    if (bus->info->custom_dma_restart) {
+        dev->vmsentry = NULL;
+    } else {
+        dev->vmsentry = qemu_add_vm_change_state_handler(scsi_dma_restart_cb,
+                                                         dev);
+    }
 }
 
 static void scsi_qdev_unrealize(DeviceState *qdev, Error **errp)
-- 
2.21.0

[Qemu-devel] [RFC v2 3/3] virtio-scsi: fix iothread deadlock on 'cont'

[Stefan Hajnoczi]

When the 'cont' command resumes guest execution the vm change state
handlers are invoked.  Unfortunately there is no explicit ordering
between vm change state handlers.  When two layers of code both use vm
change state handlers, we don't control which handler runs first.

virtio-scsi with iothreads hits a deadlock when a failed SCSI command is
restarted and completes before the iothread is re-initialized.

This patch makes sure that DMA restart happens after the iothread has
been started again.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/scsi/virtio-scsi.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 839f120256..236a0ee873 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -846,12 +846,28 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev,
     qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
 }
 
+static void virtio_scsi_vmstate_change(VirtIODevice *vdev, bool running)
+{
+    VirtIOSCSI *s = VIRTIO_SCSI(vdev);
+
+    if (running) {
+        scsi_bus_dma_restart(&s->bus);
+    }
+}
+
 static struct SCSIBusInfo virtio_scsi_scsi_info = {
     .tcq = true,
     .max_channel = VIRTIO_SCSI_MAX_CHANNEL,
     .max_target = VIRTIO_SCSI_MAX_TARGET,
     .max_lun = VIRTIO_SCSI_MAX_LUN,
 
+    /* We call scsi_bus_dma_restart() ourselves to control the ordering between
+     * ->start_ioeventfd() and DMA restart.  Do it in
+     * virtio_scsi_vmstate_change(), which is called by the core virtio code
+     * after ->start_ioeventfd().
+     */
+    .custom_dma_restart = true,
+
     .complete = virtio_scsi_command_complete,
     .cancel = virtio_scsi_request_cancelled,
     .change = virtio_scsi_change,
@@ -986,6 +1002,7 @@ static void virtio_scsi_class_init(ObjectClass *klass, void *data)
     vdc->reset = virtio_scsi_reset;
     vdc->start_ioeventfd = virtio_scsi_dataplane_start;
     vdc->stop_ioeventfd = virtio_scsi_dataplane_stop;
+    vdc->vmstate_change = virtio_scsi_vmstate_change;
     hc->plug = virtio_scsi_hotplug;
     hc->unplug = virtio_scsi_hotunplug;
 }
-- 
2.21.0

Re: [Qemu-devel] [RFC v2 1/3] virtio: add vdc->vmchange_state() callback

[Stefan Hajnoczi]

On Thu, May 23, 2019 at 2:55 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> @@ -2253,10 +2254,18 @@ static void virtio_vmstate_change(void *opaque, int running, RunState state)
>          virtio_set_status(vdev, vdev->status);
>      }
>
> +    if (!backend_run && k->vmstate_change) {
> +        vdc->vmstate_change(vdev, backend_run);
> +    }
> +
>      if (k->vmstate_change) {
>          k->vmstate_change(qbus->parent, backend_run);
>      }
>
> +    if (backend_run && k->vmstate_change) {
> +        vdc->vmstate_change(vdev, backend_run);
> +    }

Oops, k->vmstate_change should be vdc->vmstate_change.  I'll send a
new revision.

Stefan