* [Qemu-devel] [PATCH v2 0/2] virtio-scsi: fix hotplug ->reset() vs event race @ 2018-07-16 8:37 Stefan Hajnoczi 2018-07-16 8:37 ` [Qemu-devel] [PATCH v2 1/2] qdev: add HotplugHandler->post_plug() callback Stefan Hajnoczi ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Stefan Hajnoczi @ 2018-07-16 8:37 UTC (permalink / raw) To: qemu-devel Cc: l00284672, Igor Mammedov, Fam Zheng, Michael S. Tsirkin, Paolo Bonzini, Stefan Hajnoczi v2: * Drop Error **errp argument to post_plug() handler [Paolo] * Move post_plug() call outside if (dev->hotplugged) The virtio-scsi command virtqueues run during hotplug. This creates the possibility of race conditions since the guest can submit commands while the monitor is performing hotplug. See Patch 2 for a fix for the ->reset() vs event race condition that Zhengui Li encountered. Stefan Hajnoczi (2): qdev: add HotplugHandler->post_plug() callback virtio-scsi: fix hotplug ->reset() vs event race include/hw/hotplug.h | 11 +++++++++++ hw/core/hotplug.c | 10 ++++++++++ hw/core/qdev.c | 4 ++++ hw/scsi/virtio-scsi.c | 11 ++++++++++- 4 files changed, 35 insertions(+), 1 deletion(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH v2 1/2] qdev: add HotplugHandler->post_plug() callback 2018-07-16 8:37 [Qemu-devel] [PATCH v2 0/2] virtio-scsi: fix hotplug ->reset() vs event race Stefan Hajnoczi @ 2018-07-16 8:37 ` Stefan Hajnoczi 2018-07-16 8:37 ` [Qemu-devel] [PATCH v2 2/2] virtio-scsi: fix hotplug ->reset() vs event race Stefan Hajnoczi 2018-07-16 9:28 ` [Qemu-devel] [PATCH v2 0/2] " Igor Mammedov 2 siblings, 0 replies; 6+ messages in thread From: Stefan Hajnoczi @ 2018-07-16 8:37 UTC (permalink / raw) To: qemu-devel Cc: l00284672, Igor Mammedov, Fam Zheng, Michael S. Tsirkin, Paolo Bonzini, Stefan Hajnoczi The ->pre_plug() callback is invoked before the device is realized. The ->plug() callback is invoked when the device is being realized but before it is reset. This patch adds a ->post_plug() callback which is invoked after the device has been reset. This callback is needed by HotplugHandlers that need to wait until after ->reset(). Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- include/hw/hotplug.h | 11 +++++++++++ hw/core/hotplug.c | 10 ++++++++++ hw/core/qdev.c | 4 ++++ 3 files changed, 25 insertions(+) diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h index 1a0516a479..51541d63e1 100644 --- a/include/hw/hotplug.h +++ b/include/hw/hotplug.h @@ -47,6 +47,8 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler, * @parent: Opaque parent interface. * @pre_plug: pre plug callback called at start of device.realize(true) * @plug: plug callback called at end of device.realize(true). + * @post_plug: post plug callback called after device.realize(true) and device + * reset * @unplug_request: unplug request callback. * Used as a means to initiate device unplug for devices that * require asynchronous unplug handling. @@ -61,6 +63,7 @@ typedef struct HotplugHandlerClass { /* <public> */ hotplug_fn pre_plug; hotplug_fn plug; + void (*post_plug)(HotplugHandler *plug_handler, DeviceState *plugged_dev); hotplug_fn unplug_request; hotplug_fn unplug; } HotplugHandlerClass; @@ -83,6 +86,14 @@ void hotplug_handler_pre_plug(HotplugHandler *plug_handler, DeviceState *plugged_dev, Error **errp); +/** + * hotplug_handler_post_plug: + * + * Call #HotplugHandlerClass.post_plug callback of @plug_handler. + */ +void hotplug_handler_post_plug(HotplugHandler *plug_handler, + DeviceState *plugged_dev); + /** * hotplug_handler_unplug_request: * diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c index 17ac986685..2253072d0e 100644 --- a/hw/core/hotplug.c +++ b/hw/core/hotplug.c @@ -35,6 +35,16 @@ void hotplug_handler_plug(HotplugHandler *plug_handler, } } +void hotplug_handler_post_plug(HotplugHandler *plug_handler, + DeviceState *plugged_dev) +{ + HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler); + + if (hdc->post_plug) { + hdc->post_plug(plug_handler, plugged_dev); + } +} + void hotplug_handler_unplug_request(HotplugHandler *plug_handler, DeviceState *plugged_dev, Error **errp) diff --git a/hw/core/qdev.c b/hw/core/qdev.c index cf0db4b6da..529b82de18 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -867,6 +867,10 @@ static void device_set_realized(Object *obj, bool value, Error **errp) device_reset(dev); } dev->pending_deleted_event = false; + + if (hotplug_ctrl) { + hotplug_handler_post_plug(hotplug_ctrl, dev); + } } else if (!value && dev->realized) { Error **local_errp = NULL; QLIST_FOREACH(bus, &dev->child_bus, sibling) { -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] virtio-scsi: fix hotplug ->reset() vs event race 2018-07-16 8:37 [Qemu-devel] [PATCH v2 0/2] virtio-scsi: fix hotplug ->reset() vs event race Stefan Hajnoczi 2018-07-16 8:37 ` [Qemu-devel] [PATCH v2 1/2] qdev: add HotplugHandler->post_plug() callback Stefan Hajnoczi @ 2018-07-16 8:37 ` Stefan Hajnoczi 2018-07-16 12:40 ` Michael S. Tsirkin 2018-07-16 9:28 ` [Qemu-devel] [PATCH v2 0/2] " Igor Mammedov 2 siblings, 1 reply; 6+ messages in thread From: Stefan Hajnoczi @ 2018-07-16 8:37 UTC (permalink / raw) To: qemu-devel Cc: l00284672, Igor Mammedov, Fam Zheng, Michael S. Tsirkin, Paolo Bonzini, Stefan Hajnoczi There is a race condition during hotplug when iothread is used. It occurs because virtio-scsi may be processing command queues in the iothread while the monitor performs SCSI device hotplug. When a SCSI device is hotplugged the HotplugHandler->plug() callback is invoked and virtio-scsi emits a rescan event to the guest. If the guest submits a SCSI command at this point then it may be cancelled before hotplug completes. This happens because ->reset() is called by hw/core/qdev.c:device_set_realized() after HotplugHandler->plug() has been called and hw/scsi/scsi-disk.c:scsi_disk_reset() purges all requests. This patch uses the new HotplugHandler->post_plug() callback to emit the rescan event after ->reset(). This eliminates the race conditions where requests could be cancelled. Reported-by: l00284672 <lizhengui@huawei.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Fam Zheng <famz@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/scsi/virtio-scsi.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index 3aa99717e2..5a3057d1f8 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -797,8 +797,16 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev, virtio_scsi_acquire(s); blk_set_aio_context(sd->conf.blk, s->ctx); virtio_scsi_release(s); - } +} + +/* Announce the new device after it has been plugged */ +static void virtio_scsi_post_hotplug(HotplugHandler *hotplug_dev, + DeviceState *dev) +{ + VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev); + VirtIOSCSI *s = VIRTIO_SCSI(vdev); + SCSIDevice *sd = SCSI_DEVICE(dev); if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) { virtio_scsi_acquire(s); @@ -968,6 +976,7 @@ static void virtio_scsi_class_init(ObjectClass *klass, void *data) vdc->start_ioeventfd = virtio_scsi_dataplane_start; vdc->stop_ioeventfd = virtio_scsi_dataplane_stop; hc->plug = virtio_scsi_hotplug; + hc->post_plug = virtio_scsi_post_hotplug; hc->unplug = virtio_scsi_hotunplug; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] virtio-scsi: fix hotplug ->reset() vs event race 2018-07-16 8:37 ` [Qemu-devel] [PATCH v2 2/2] virtio-scsi: fix hotplug ->reset() vs event race Stefan Hajnoczi @ 2018-07-16 12:40 ` Michael S. Tsirkin 2018-07-16 14:11 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2018-07-16 12:40 UTC (permalink / raw) To: Stefan Hajnoczi Cc: qemu-devel, l00284672, Igor Mammedov, Fam Zheng, Paolo Bonzini On Mon, Jul 16, 2018 at 09:37:32AM +0100, Stefan Hajnoczi wrote: > There is a race condition during hotplug when iothread is used. It > occurs because virtio-scsi may be processing command queues in the > iothread while the monitor performs SCSI device hotplug. > > When a SCSI device is hotplugged the HotplugHandler->plug() callback is > invoked and virtio-scsi emits a rescan event to the guest. > > If the guest submits a SCSI command at this point then it may be > cancelled before hotplug completes. This happens because ->reset() is > called by hw/core/qdev.c:device_set_realized() after > HotplugHandler->plug() has been called and > hw/scsi/scsi-disk.c:scsi_disk_reset() purges all requests. > > This patch uses the new HotplugHandler->post_plug() callback to emit the > rescan event after ->reset(). This eliminates the race conditions where > requests could be cancelled. > > Reported-by: l00284672 <lizhengui@huawei.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Fam Zheng <famz@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> Pls merge through scsi tree. > --- > hw/scsi/virtio-scsi.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c > index 3aa99717e2..5a3057d1f8 100644 > --- a/hw/scsi/virtio-scsi.c > +++ b/hw/scsi/virtio-scsi.c > @@ -797,8 +797,16 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev, > virtio_scsi_acquire(s); > blk_set_aio_context(sd->conf.blk, s->ctx); > virtio_scsi_release(s); > - > } > +} > + > +/* Announce the new device after it has been plugged */ > +static void virtio_scsi_post_hotplug(HotplugHandler *hotplug_dev, > + DeviceState *dev) > +{ > + VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev); > + VirtIOSCSI *s = VIRTIO_SCSI(vdev); > + SCSIDevice *sd = SCSI_DEVICE(dev); > > if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) { > virtio_scsi_acquire(s); > @@ -968,6 +976,7 @@ static void virtio_scsi_class_init(ObjectClass *klass, void *data) > vdc->start_ioeventfd = virtio_scsi_dataplane_start; > vdc->stop_ioeventfd = virtio_scsi_dataplane_stop; > hc->plug = virtio_scsi_hotplug; > + hc->post_plug = virtio_scsi_post_hotplug; > hc->unplug = virtio_scsi_hotunplug; > } > > -- > 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] virtio-scsi: fix hotplug ->reset() vs event race 2018-07-16 12:40 ` Michael S. Tsirkin @ 2018-07-16 14:11 ` Paolo Bonzini 0 siblings, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2018-07-16 14:11 UTC (permalink / raw) To: Michael S. Tsirkin, Stefan Hajnoczi Cc: qemu-devel, l00284672, Igor Mammedov, Fam Zheng On 16/07/2018 14:40, Michael S. Tsirkin wrote: > On Mon, Jul 16, 2018 at 09:37:32AM +0100, Stefan Hajnoczi wrote: >> There is a race condition during hotplug when iothread is used. It >> occurs because virtio-scsi may be processing command queues in the >> iothread while the monitor performs SCSI device hotplug. >> >> When a SCSI device is hotplugged the HotplugHandler->plug() callback is >> invoked and virtio-scsi emits a rescan event to the guest. >> >> If the guest submits a SCSI command at this point then it may be >> cancelled before hotplug completes. This happens because ->reset() is >> called by hw/core/qdev.c:device_set_realized() after >> HotplugHandler->plug() has been called and >> hw/scsi/scsi-disk.c:scsi_disk_reset() purges all requests. >> >> This patch uses the new HotplugHandler->post_plug() callback to emit the >> rescan event after ->reset(). This eliminates the race conditions where >> requests could be cancelled. >> >> Reported-by: l00284672 <lizhengui@huawei.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Fam Zheng <famz@redhat.com> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > Pls merge through scsi tree. Will do, thanks Stefan. Paolo > >> --- >> hw/scsi/virtio-scsi.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c >> index 3aa99717e2..5a3057d1f8 100644 >> --- a/hw/scsi/virtio-scsi.c >> +++ b/hw/scsi/virtio-scsi.c >> @@ -797,8 +797,16 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev, >> virtio_scsi_acquire(s); >> blk_set_aio_context(sd->conf.blk, s->ctx); >> virtio_scsi_release(s); >> - >> } >> +} >> + >> +/* Announce the new device after it has been plugged */ >> +static void virtio_scsi_post_hotplug(HotplugHandler *hotplug_dev, >> + DeviceState *dev) >> +{ >> + VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev); >> + VirtIOSCSI *s = VIRTIO_SCSI(vdev); >> + SCSIDevice *sd = SCSI_DEVICE(dev); >> >> if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) { >> virtio_scsi_acquire(s); >> @@ -968,6 +976,7 @@ static void virtio_scsi_class_init(ObjectClass *klass, void *data) >> vdc->start_ioeventfd = virtio_scsi_dataplane_start; >> vdc->stop_ioeventfd = virtio_scsi_dataplane_stop; >> hc->plug = virtio_scsi_hotplug; >> + hc->post_plug = virtio_scsi_post_hotplug; >> hc->unplug = virtio_scsi_hotunplug; >> } >> >> -- >> 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/2] virtio-scsi: fix hotplug ->reset() vs event race 2018-07-16 8:37 [Qemu-devel] [PATCH v2 0/2] virtio-scsi: fix hotplug ->reset() vs event race Stefan Hajnoczi 2018-07-16 8:37 ` [Qemu-devel] [PATCH v2 1/2] qdev: add HotplugHandler->post_plug() callback Stefan Hajnoczi 2018-07-16 8:37 ` [Qemu-devel] [PATCH v2 2/2] virtio-scsi: fix hotplug ->reset() vs event race Stefan Hajnoczi @ 2018-07-16 9:28 ` Igor Mammedov 2 siblings, 0 replies; 6+ messages in thread From: Igor Mammedov @ 2018-07-16 9:28 UTC (permalink / raw) To: Stefan Hajnoczi Cc: qemu-devel, l00284672, Fam Zheng, Michael S. Tsirkin, Paolo Bonzini On Mon, 16 Jul 2018 09:37:30 +0100 Stefan Hajnoczi <stefanha@redhat.com> wrote: > v2: > * Drop Error **errp argument to post_plug() handler [Paolo] > * Move post_plug() call outside if (dev->hotplugged) > > The virtio-scsi command virtqueues run during hotplug. This creates the > possibility of race conditions since the guest can submit commands while the > monitor is performing hotplug. > > See Patch 2 for a fix for the ->reset() vs event race condition that Zhengui Li > encountered. > > Stefan Hajnoczi (2): > qdev: add HotplugHandler->post_plug() callback > virtio-scsi: fix hotplug ->reset() vs event race > > include/hw/hotplug.h | 11 +++++++++++ > hw/core/hotplug.c | 10 ++++++++++ > hw/core/qdev.c | 4 ++++ > hw/scsi/virtio-scsi.c | 11 ++++++++++- > 4 files changed, 35 insertions(+), 1 deletion(-) > Reviewed-by: Igor Mammedov <imammedo@redhat.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-07-16 14:11 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-16 8:37 [Qemu-devel] [PATCH v2 0/2] virtio-scsi: fix hotplug ->reset() vs event race Stefan Hajnoczi 2018-07-16 8:37 ` [Qemu-devel] [PATCH v2 1/2] qdev: add HotplugHandler->post_plug() callback Stefan Hajnoczi 2018-07-16 8:37 ` [Qemu-devel] [PATCH v2 2/2] virtio-scsi: fix hotplug ->reset() vs event race Stefan Hajnoczi 2018-07-16 12:40 ` Michael S. Tsirkin 2018-07-16 14:11 ` Paolo Bonzini 2018-07-16 9:28 ` [Qemu-devel] [PATCH v2 0/2] " Igor Mammedov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.