| Message ID | 20180710155037.16834-2-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-scsi: fix hotplug ->reset() vs event race | expand |
On 10/07/2018 17:50, Stefan Hajnoczi wrote: > 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> Thanks Stefan. I looked a bit at the code last week, and it seemed to me that pretty much all "plug" callbacks should be split into a "pre_plug" (check) and a "post_plug" (commit). So the patch is okay, but please remove the Error** argument of post_plug, because all the checks should have happened already (there is none in the case of virtio-scsi). Thanks, Paolo > --- > include/hw/hotplug.h | 12 ++++++++++++ > hw/core/hotplug.c | 11 +++++++++++ > hw/core/qdev.c | 7 +++++++ > 3 files changed, 30 insertions(+) > > diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h > index 1a0516a479..1e8b1053b6 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; > + hotplug_fn post_plug; > hotplug_fn unplug_request; > hotplug_fn unplug; > } HotplugHandlerClass; > @@ -83,6 +86,15 @@ 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, > + Error **errp); > + > /** > * hotplug_handler_unplug_request: > * > diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c > index 17ac986685..ab34c19461 100644 > --- a/hw/core/hotplug.c > +++ b/hw/core/hotplug.c > @@ -35,6 +35,17 @@ void hotplug_handler_plug(HotplugHandler *plug_handler, > } > } > > +void hotplug_handler_post_plug(HotplugHandler *plug_handler, > + DeviceState *plugged_dev, > + Error **errp) > +{ > + HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler); > + > + if (hdc->post_plug) { > + hdc->post_plug(plug_handler, plugged_dev, errp); > + } > +} > + > 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..78c0e031ff 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -865,6 +865,13 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > } > if (dev->hotplugged) { > device_reset(dev); > + > + if (hotplug_ctrl) { > + hotplug_handler_post_plug(hotplug_ctrl, dev, &local_err); > + if (local_err != NULL) { > + goto child_realize_fail; > + } > + } > } > dev->pending_deleted_event = false; > } else if (!value && dev->realized) { >
On Tue, Jul 10, 2018 at 04:50:36PM +0100, Stefan Hajnoczi wrote: > 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 | 12 ++++++++++++ > hw/core/hotplug.c | 11 +++++++++++ > hw/core/qdev.c | 7 +++++++ > 3 files changed, 30 insertions(+) > > diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h > index 1a0516a479..1e8b1053b6 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; > + hotplug_fn post_plug; > hotplug_fn unplug_request; > hotplug_fn unplug; > } HotplugHandlerClass; > @@ -83,6 +86,15 @@ 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, > + Error **errp); > + > /** > * hotplug_handler_unplug_request: > * > diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c > index 17ac986685..ab34c19461 100644 > --- a/hw/core/hotplug.c > +++ b/hw/core/hotplug.c > @@ -35,6 +35,17 @@ void hotplug_handler_plug(HotplugHandler *plug_handler, > } > } > > +void hotplug_handler_post_plug(HotplugHandler *plug_handler, > + DeviceState *plugged_dev, > + Error **errp) > +{ > + HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler); > + > + if (hdc->post_plug) { > + hdc->post_plug(plug_handler, plugged_dev, errp); > + } > +} > + > 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..78c0e031ff 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -865,6 +865,13 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > } > if (dev->hotplugged) { > device_reset(dev); > + > + if (hotplug_ctrl) { In the final patch I will move this out of if (dev->hotplugged) since the other HotplugHandler callbacks are also invoked unconditionally. > + hotplug_handler_post_plug(hotplug_ctrl, dev, &local_err); > + if (local_err != NULL) { > + goto child_realize_fail; > + } > + } > } > dev->pending_deleted_event = false; > } else if (!value && dev->realized) { > -- > 2.17.1 > >
On 11/07/2018 15:29, Stefan Hajnoczi wrote: >> if (dev->hotplugged) { >> device_reset(dev); >> + >> + if (hotplug_ctrl) { > In the final patch I will move this out of if (dev->hotplugged) since > the other HotplugHandler callbacks are also invoked unconditionally. I'm not even sure why the reset is needed (removing it would also fix the bug!), and why it's only done on hotplug; however, it is probably there because it updates some bus-level state, so it's dangerous to remove it. Paolo >> + hotplug_handler_post_plug(hotplug_ctrl, dev, &local_err); >> + if (local_err != NULL) { >> + goto child_realize_fail; >> + } >> + } >> } >> dev->pending_deleted_event = false;
On Wed, 11 Jul 2018 15:32:12 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 11/07/2018 15:29, Stefan Hajnoczi wrote: > >> if (dev->hotplugged) { > >> device_reset(dev); > >> + > >> + if (hotplug_ctrl) { > > In the final patch I will move this out of if (dev->hotplugged) since > > the other HotplugHandler callbacks are also invoked unconditionally. > > I'm not even sure why the reset is needed (removing it would also fix > the bug!), and why it's only done on hotplug; however, it is probably > there because it updates some bus-level state, so it's dangerous to > remove it. it might be also so that each device won't have to call reset manually from their realize (5ab28c834) to initialize device into initial state. > Paolo > > >> + hotplug_handler_post_plug(hotplug_ctrl, dev, &local_err); > >> + if (local_err != NULL) { > >> + goto child_realize_fail; > >> + } > >> + } > >> } > >> dev->pending_deleted_event = false; >
On Tue, 10 Jul 2018 16:50:36 +0100 Stefan Hajnoczi <stefanha@redhat.com> wrote: > 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(). it seems other plug handlers would be also affected by this issue if monitor lock wouldn't block them when guest tried to processes hotplug notification and trapped back on MMIO happily waiting for device_add to release lock before proceeding. It also seems wrong to call _plug handler on maybe partially initialized device so perhaps we should first finish devices/children realization then do reset and only after that call _plug() handler something like following should fix the issue: diff --git a/hw/core/qdev.c b/hw/core/qdev.c index cf0db4b..a80372d 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -830,6 +830,18 @@ static void device_set_realized(Object *obj, bool value, Error **errp) goto fail; } + QLIST_FOREACH(bus, &dev->child_bus, sibling) { + object_property_set_bool(OBJECT(bus), true, "realized", + &local_err); + if (local_err != NULL) { + goto child_realize_fail; + } + } + + if (dev->hotplugged) { + device_reset(dev); + } + DEVICE_LISTENER_CALL(realize, Forward, dev); if (hotplug_ctrl) { @@ -837,7 +849,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp) } if (local_err != NULL) { - goto post_realize_fail; + goto child_realize_fail; } /* @@ -852,20 +864,9 @@ static void device_set_realized(Object *obj, bool value, Error **errp) dev->instance_id_alias, dev->alias_required_for_version, &local_err) < 0) { - goto post_realize_fail; - } - } - - QLIST_FOREACH(bus, &dev->child_bus, sibling) { - object_property_set_bool(OBJECT(bus), true, "realized", - &local_err); - if (local_err != NULL) { goto child_realize_fail; } } - if (dev->hotplugged) { - device_reset(dev); - } dev->pending_deleted_event = false; } else if (!value && dev->realized) { Error **local_errp = NULL; @@ -902,7 +903,6 @@ child_realize_fail: vmstate_unregister(dev, qdev_get_vmsd(dev), dev); } -post_realize_fail: g_free(dev->canonical_path); dev->canonical_path = NULL; if (dc->unrealize) { > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > include/hw/hotplug.h | 12 ++++++++++++ > hw/core/hotplug.c | 11 +++++++++++ > hw/core/qdev.c | 7 +++++++ > 3 files changed, 30 insertions(+) > > diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h > index 1a0516a479..1e8b1053b6 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; > + hotplug_fn post_plug; > hotplug_fn unplug_request; > hotplug_fn unplug; > } HotplugHandlerClass; > @@ -83,6 +86,15 @@ 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, > + Error **errp); > + > /** > * hotplug_handler_unplug_request: > * > diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c > index 17ac986685..ab34c19461 100644 > --- a/hw/core/hotplug.c > +++ b/hw/core/hotplug.c > @@ -35,6 +35,17 @@ void hotplug_handler_plug(HotplugHandler *plug_handler, > } > } > > +void hotplug_handler_post_plug(HotplugHandler *plug_handler, > + DeviceState *plugged_dev, > + Error **errp) > +{ > + HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler); > + > + if (hdc->post_plug) { > + hdc->post_plug(plug_handler, plugged_dev, errp); > + } > +} > + > 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..78c0e031ff 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -865,6 +865,13 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > } > if (dev->hotplugged) { > device_reset(dev); > + > + if (hotplug_ctrl) { > + hotplug_handler_post_plug(hotplug_ctrl, dev, &local_err); > + if (local_err != NULL) { > + goto child_realize_fail; > + } > + } > } > dev->pending_deleted_event = false; > } else if (!value && dev->realized) {
On 11/07/2018 17:22, Igor Mammedov wrote: > It also seems wrong to call _plug handler on maybe partially > initialized device so perhaps we should first finish devices/children > realization then do reset and only after that call _plug() handler I agree but this is too dangerous until we look at plug() handlers and remove the usage of Error **. Introducing a new callback helps the transition. Paolo
On Wed, 11 Jul 2018 18:48:21 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 11/07/2018 17:22, Igor Mammedov wrote: > > It also seems wrong to call _plug handler on maybe partially > > initialized device so perhaps we should first finish devices/children > > realization then do reset and only after that call _plug() handler > > I agree but this is too dangerous until we look at plug() handlers and I'll put on my TODO list for 3.1 > remove the usage of Error **. considering we have _per_plug now with all checks, we probably would be able to remove Error** from _plug but only after splitting/auditing existing _plug callbacks. > Introducing a new callback helps the > transition. sure, we can drop it later > Paolo >
diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h index 1a0516a479..1e8b1053b6 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; + hotplug_fn post_plug; hotplug_fn unplug_request; hotplug_fn unplug; } HotplugHandlerClass; @@ -83,6 +86,15 @@ 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, + Error **errp); + /** * hotplug_handler_unplug_request: * diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c index 17ac986685..ab34c19461 100644 --- a/hw/core/hotplug.c +++ b/hw/core/hotplug.c @@ -35,6 +35,17 @@ void hotplug_handler_plug(HotplugHandler *plug_handler, } } +void hotplug_handler_post_plug(HotplugHandler *plug_handler, + DeviceState *plugged_dev, + Error **errp) +{ + HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler); + + if (hdc->post_plug) { + hdc->post_plug(plug_handler, plugged_dev, errp); + } +} + 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..78c0e031ff 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -865,6 +865,13 @@ static void device_set_realized(Object *obj, bool value, Error **errp) } if (dev->hotplugged) { device_reset(dev); + + if (hotplug_ctrl) { + hotplug_handler_post_plug(hotplug_ctrl, dev, &local_err); + if (local_err != NULL) { + goto child_realize_fail; + } + } } dev->pending_deleted_event = false; } else if (!value && dev->realized) {
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 | 12 ++++++++++++ hw/core/hotplug.c | 11 +++++++++++ hw/core/qdev.c | 7 +++++++ 3 files changed, 30 insertions(+)