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

[David Hildenbrand]

On 15.05.2018 02:00, Murilo Opsfelder Araujo wrote:
> 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)
>         }
>     }

There are various ways how to implement it ,this example if from Igor
and most probably not complete (only showcases how things are passed on
to the next handler).

I decided to implement it slightly different, to make it more flexible.
Best you have a look at the following patches in this series.

Thanks!

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


-- 

Thanks,

David / dhildenb