[Qemu-devel] [PATCH 0/2] virtio-scsi: fix hotplug ->reset() vs event race

[Qemu-devel] [PATCH 0/2] virtio-scsi: fix hotplug ->reset() vs event race

[Stefan Hajnoczi]

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.

Zhengui Li: Sorry it took me so long to look into this.  Please let me know if
this fixes the issue you are seeing.  Thanks!

Stefan Hajnoczi (2):
  qdev: add HotplugHandler->post_plug() callback
  virtio-scsi: fix hotplug ->reset() vs event race

 include/hw/hotplug.h  | 12 ++++++++++++
 hw/core/hotplug.c     | 11 +++++++++++
 hw/core/qdev.c        |  7 +++++++
 hw/scsi/virtio-scsi.c | 11 ++++++++++-
 4 files changed, 40 insertions(+), 1 deletion(-)

-- 
2.17.1

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

[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 | 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) {
-- 
2.17.1

[Qemu-devel] [PATCH 2/2] virtio-scsi: fix hotplug ->reset() vs event race

[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..1ec808e6fd 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, Error **errp)
+{
+    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

Re: [Qemu-devel] [PATCH 0/2] virtio-scsi: fix hotplug ->reset() vs event race

[Fam Zheng]

On Tue, 07/10 16:50, Stefan Hajnoczi wrote:
> 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.
> 
> Zhengui Li: Sorry it took me so long to look into this.  Please let me know if
> this fixes the issue you are seeing.  Thanks!
> 
> Stefan Hajnoczi (2):
>   qdev: add HotplugHandler->post_plug() callback
>   virtio-scsi: fix hotplug ->reset() vs event race
> 
>  include/hw/hotplug.h  | 12 ++++++++++++
>  hw/core/hotplug.c     | 11 +++++++++++
>  hw/core/qdev.c        |  7 +++++++
>  hw/scsi/virtio-scsi.c | 11 ++++++++++-
>  4 files changed, 40 insertions(+), 1 deletion(-)
> 
> -- 
> 2.17.1

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 0/2] virtio-scsi: fix hotplug ->reset() vs event race

[l00284672]

[-- Attachment #1: Type: text/plain, Size: 853 bytes --]

Ok, I will test your patch soon.


On 2018/7/10 23:50, Stefan Hajnoczi wrote:
> 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.
>
> Zhengui Li: Sorry it took me so long to look into this.  Please let me know if
> this fixes the issue you are seeing.  Thanks!
>
> Stefan Hajnoczi (2):
>    qdev: add HotplugHandler->post_plug() callback
>    virtio-scsi: fix hotplug ->reset() vs event race
>
>   include/hw/hotplug.h  | 12 ++++++++++++
>   hw/core/hotplug.c     | 11 +++++++++++
>   hw/core/qdev.c        |  7 +++++++
>   hw/scsi/virtio-scsi.c | 11 ++++++++++-
>   4 files changed, 40 insertions(+), 1 deletion(-)
>


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: lizhengui.vcf --]
[-- Type: text/x-vcard; name="lizhengui.vcf", Size: 4 bytes --]

null

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

[Paolo Bonzini]

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) {
> 

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3846 bytes --]

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
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Paolo Bonzini]

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;

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

[Igor Mammedov]

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;  
> 

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 <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) {

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

[Paolo Bonzini]

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

[Igor Mammedov]

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
>