Re: [Qemu-devel] [PATCH 1/2] qdev: add HotplugHandler->post_plug() callback
On Wed, 11 Jul 2018 18:48:21 +0200 Paolo Bonzini 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 >
Re: [Qemu-devel] [PATCH 1/2] qdev: add HotplugHandler->post_plug() callback
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
Re: [Qemu-devel] [PATCH 1/2] qdev: add HotplugHandler->post_plug() callback
On Tue, 10 Jul 2018 16:50:36 +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().
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, >child_bus, sibling) {
+object_property_set_bool(OBJECT(bus), true, "realized",
+ _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,
_err) < 0) {
-goto post_realize_fail;
-}
-}
-
-QLIST_FOREACH(bus, >child_bus, sibling) {
-object_property_set_bool(OBJECT(bus), true, "realized",
- _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
> ---
> 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 {
> /* */
> 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,
> +
Re: [Qemu-devel] [PATCH 1/2] qdev: add HotplugHandler->post_plug() callback
On Wed, 11 Jul 2018 15:32:12 +0200
Paolo Bonzini 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, _err);
> >> +if (local_err != NULL) {
> >> +goto child_realize_fail;
> >> +}
> >> +}
> >> }
> >> dev->pending_deleted_event = false;
>
Re: [Qemu-devel] [PATCH 1/2] qdev: add HotplugHandler->post_plug() callback
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, _err);
>> +if (local_err != NULL) {
>> +goto child_realize_fail;
>> +}
>> +}
>> }
>> dev->pending_deleted_event = false;
Re: [Qemu-devel] [PATCH 1/2] qdev: add HotplugHandler->post_plug() callback
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
> ---
> 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 {
> /* */
> 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, _err);
> +if (local_err != NULL) {
> +goto child_realize_fail;
> +}
> +}
> }
> dev->pending_deleted_event = false;
> } else if (!value && dev->realized) {
> --
> 2.17.1
>
>
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH 1/2] qdev: add HotplugHandler->post_plug() callback
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
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 {
> /* */
> 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, _err);
> +if (local_err != NULL) {
> +goto child_realize_fail;
> +}
> +}
> }
> dev->pending_deleted_event = false;
> } else if (!value && dev->realized) {
>
[Qemu-devel] [PATCH 1/2] qdev: add HotplugHandler->post_plug() callback
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
---
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 {
/* */
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, _err);
+if (local_err != NULL) {
+goto child_realize_fail;
+}
+}
}
dev->pending_deleted_event = false;
} else if (!value && dev->realized) {
--
2.17.1
