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

Message ID 20180710155037.16834-2-stefanha@redhat.com
State New
Headers show
Series
  • virtio-scsi: fix hotplug ->reset() vs event race
Related show

Commit Message

Stefan Hajnoczi July 10, 2018, 3:50 p.m.
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(+)

Comments

Paolo Bonzini July 11, 2018, 11:01 a.m. | #1
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) {
>
Stefan Hajnoczi July 11, 2018, 1:29 p.m. | #2
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
> 
>
Paolo Bonzini July 11, 2018, 1:32 p.m. | #3
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;
Igor Mammedov July 11, 2018, 2:15 p.m. | #4
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;  
>
Igor Mammedov July 11, 2018, 3:22 p.m. | #5
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) {
Paolo Bonzini July 11, 2018, 4:48 p.m. | #6
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
Igor Mammedov July 12, 2018, 9:04 a.m. | #7
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
>

Patch

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