Re: [qemu-s390x] [PATCH v4 03/14] qdev: let machine hotplug handler to override bus hotplug handler

[David Gibson]

On Thu, May 17, 2018 at 10:15:16AM +0200, David Hildenbrand wrote:
> From: Igor Mammedov <address@hidden>
> 
> it will allow to return another hotplug handler than the default
> one for a specific bus based device type. Which is needed to handle
> non trivial plug/unplug sequences that need the access to resources
> configured outside of bus where device is attached.
> 
> That will allow for returned hotplug handler to orchestrate wiring
> in arbitrary order, by chaining other hotplug handlers when
> it's needed.
> 
> PS:
> It could be used for hybrid virtio-mem and virtio-pmem devices
> where it will return machine as hotplug handler which will do
> necessary wiring at machine level and then pass control down
> the chain to bus specific hotplug handler.
> 
> Example of top level hotplug handler override and custom plug sequence:
> 
>   some_machine_get_hotplug_handler(machine){
>       if (object_dynamic_cast(OBJECT(dev), TYPE_SOME_BUS_DEVICE)) {
>           return HOTPLUG_HANDLER(machine);
>       }
>       return NULL;
>   }
> 
>   some_machine_device_plug(hotplug_dev, dev) {
>       if (object_dynamic_cast(OBJECT(dev), TYPE_SOME_BUS_DEVICE)) {
>           /* do machine specific initialization */
>           some_machine_init_special_device(dev)
> 
>           /* pass control to bus specific handler */
>           hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev)
>       }
>   }
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> Signed-off-by: David Hildenbrand <address@hidden>

Reviewed-by: David Gibson <address@hidden>

I've been considering a similar change for a while; we'll also need
something like this in order to do hoplug for PCI devices under p2p
bridges on pseries (there's a PAPR specific hotplug model that we need
to use instead of SHP).

> ---
>  hw/core/qdev.c         |  6 ++----
>  include/hw/qdev-core.h | 11 +++++++++++
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index f6f92473b8..885286f579 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -261,12 +261,10 @@ HotplugHandler 
> *qdev_get_machine_hotplug_handler(DeviceState *dev)
>  
>  HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
>  {
> -    HotplugHandler *hotplug_ctrl;
> +    HotplugHandler *hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
>  
> -    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> +    if (hotplug_ctrl == NULL && dev->parent_bus) {
>          hotplug_ctrl = dev->parent_bus->hotplug_handler;
> -    } else {
> -        hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
>      }
>      return hotplug_ctrl;
>  }
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 9453588160..e6a8eca558 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -286,6 +286,17 @@ void qdev_init_nofail(DeviceState *dev);
>  void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>                                   int required_for_version);
>  HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
> +/**
> + * qdev_get_hotplug_handler: Get handler responsible for device wiring
> + *
> + * Find HOTPLUG_HANDLER for @dev that provides [pre|un]plug callbacks for it.
> + *
> + * Note: in case @dev has a parent bus, it will be returned as handler unless
> + * machine handler overrides it.
> + *
> + * Returns: pointer to object that implements TYPE_HOTPLUG_HANDLER interface
> + *          or NULL if there aren't any.
> + */
>  HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
>  void qdev_unplug(DeviceState *dev, Error **errp);
>  void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature