Re: [Qemu-devel] [PATCH 1/2] qdev: add HotplugHandler->post_plug() callback

[Igor Mammedov]

On Tue, 10 Jul 2018 16:50:36 +0100
Stefan Hajnoczi <address@hidden> 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 <address@hidden>
> ---
>  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) {