Re: [qemu-s390x] [Qemu-ppc] [PATCH v3 03/18] qdev: let machine hotplug handler to override bus hotplug handler

[Murilo Opsfelder Araujo]

On Mon, May 14, 2018 at 12:00:08PM +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)
>       }
>   }

Hi, David.

I might be misreading, but isn't it more like the following?

    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)
        } else {
            /* 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>
> ---
>  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,
> -- 
> 2.14.3
> 
> 

-- 
Murilo