Re: [Qemu-devel] [PATCH v4 18/24] qdev: hotplug: provide do_unplug handler

[Igor Mammedov]

On Wed, 26 Sep 2018 11:42:13 +0200
David Hildenbrand <address@hidden> wrote:

> The unplug and unplug_request handlers are special: They are not
> executed when unrealizing a device, but rather trigger the removal of a
> device from device_del() via object_unparent() - to effectively
> unrealize a device.
> 
> If such a device has a child bus and another device attached to
> that bus (e.g. how virtio devices are created with their proxy device),
> we will not get a call to the unplug handler. As we want to support
> hotplug handlers (and especially also some unplug logic to undo resource
> assignment) for such devices, we cannot simply call the unplug handler
> when unrealizing - it has a different semantic ("trigger removal").
> 
> To handle this scenario, we need a do_unplug handler, that will be
> executed for all devices with a hotplug handler.
could you clarify what would be call flow for unplug in this case
starting from 'device_del'?

> 
> While at it, introduce hotplug_fn_nofail and fix a spelling mistake in
> a comment.
> 
> Signed-off-by: David Hildenbrand <address@hidden>
> ---
>  hw/core/hotplug.c    | 10 ++++++++++
>  hw/core/qdev.c       |  6 ++++++
>  include/hw/hotplug.h | 26 ++++++++++++++++++++++++--
>  3 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> index 2253072d0e..e7a68d5160 100644
> --- a/hw/core/hotplug.c
> +++ b/hw/core/hotplug.c
> @@ -45,6 +45,16 @@ void hotplug_handler_post_plug(HotplugHandler 
> *plug_handler,
>      }
>  }
>  
> +void hotplug_handler_do_unplug(HotplugHandler *plug_handler,
> +                                 DeviceState *plugged_dev)
> +{
> +    HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
> +
> +    if (hdc->do_unplug) {
> +        hdc->do_unplug(plug_handler, plugged_dev);
> +    }
> +}
> +
>  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 36b788a66b..dde2726099 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -873,6 +873,12 @@ static void device_set_realized(Object *obj, bool value, 
> Error **errp)
>          }
>      } else if (!value && dev->realized) {
>          Error **local_errp = NULL;
> +
> +        hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +        if (hotplug_ctrl) {
> +            hotplug_handler_do_unplug(hotplug_ctrl, dev);
> +        }
> +
>          QLIST_FOREACH(bus, &dev->child_bus, sibling) {
>              local_errp = local_err ? NULL : &local_err;
>              object_property_set_bool(OBJECT(bus), false, "realized",
> diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> index 51541d63e1..2fa5833cf1 100644
> --- a/include/hw/hotplug.h
> +++ b/include/hw/hotplug.h
> @@ -31,13 +31,21 @@ typedef struct HotplugHandler {
>  
>  /**
>   * hotplug_fn:
> - * @plug_handler: a device performing plug/uplug action
> + * @plug_handler: a device performing (un)plug action
>   * @plugged_dev: a device that has been (un)plugged
>   * @errp: returns an error if this function fails
>   */
>  typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
>                             DeviceState *plugged_dev, Error **errp);
>  
> +/**
> + * hotplug_fn_nofail:
> + * @plug_handler: a device performing un(plug) action
> + * @plugged_dev: a device that has been (un)plugged
> + */
> +typedef void (*hotplug_fn_nofail)(HotplugHandler *plug_handler,
> +                                  DeviceState *plugged_dev);
> +
>  /**
>   * HotplugDeviceClass:
>   *
> @@ -49,12 +57,17 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
>   * @plug: plug callback called at end of device.realize(true).
>   * @post_plug: post plug callback called after device.realize(true) and 
> device
>   *             reset
> + * @do_unplug: unplug callback called at start of device.realize(false)
>   * @unplug_request: unplug request callback.
>   *                  Used as a means to initiate device unplug for devices 
> that
>   *                  require asynchronous unplug handling.
>   * @unplug: unplug callback.
>   *          Used for device removal with devices that implement
>   *          asynchronous and synchronous (surprise) removal.
> + * Note: unplug_request and unplug are only called for devices to initiate
> + *       unplug of a device hierarchy (e.g. triggered by device_del). For
> + *       devices that will be removed along with this device hierarchy only
> + *       do_unplug will be called (e.g. to unassign resources).
>   */
>  typedef struct HotplugHandlerClass {
>      /* <private> */
> @@ -63,7 +76,8 @@ typedef struct HotplugHandlerClass {
>      /* <public> */
>      hotplug_fn pre_plug;
>      hotplug_fn plug;
> -    void (*post_plug)(HotplugHandler *plug_handler, DeviceState 
> *plugged_dev);
> +    hotplug_fn_nofail post_plug;
> +    hotplug_fn_nofail do_unplug;
>      hotplug_fn unplug_request;
>      hotplug_fn unplug;
>  } HotplugHandlerClass;
> @@ -94,6 +108,14 @@ void hotplug_handler_pre_plug(HotplugHandler 
> *plug_handler,
>  void hotplug_handler_post_plug(HotplugHandler *plug_handler,
>                                 DeviceState *plugged_dev);
>  
> +/**
> + * hotplug_handler_do_unplug:
> + *
> + * Call #HotplugHandlerClass.do_unplug callback of @plug_handler.
> + */
> +void hotplug_handler_do_unplug(HotplugHandler *plug_handler,
> +                               DeviceState *plugged_dev);
> +
>  /**
>   * hotplug_handler_unplug_request:
>   *