Re: [Qemu-devel] [PATCH] qdev: Check for the availability of a hotplug controller before adding a device

[Thomas Huth <thuth@redhat.com>]

On 04.10.2017 13:36, Igor Mammedov wrote:
> On Tue,  3 Oct 2017 18:46:02 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> The qdev_unplug() function contains a g_assert(hotplug_ctrl) statement,
>> so QEMU crashes when the user tries to device_add + device_del a device
>> that does not have a corresponding hotplug controller. This could be
>> provoked for a couple of devices in the past (see commit 4c93950659487c7ad
>> or 84ebd3e8c7d4fe955 for example). So devices clearly need a hotplug
>> controller when they are suitable for device_add.
>> The code in qdev_device_add() already checks whether the bus has a proper
>> hotplug controller, but for devices that do not have a corresponding bus,
>> there is no appropriate check available. In that case we should check
>> whether the machine itself provides a suitable hotplug controller and
>> refuse to plug the device if none is available.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  This is the follow-up patch from my earlier try "hw/core/qdev: Do not
>>  allow hot-plugging without hotplug controller" ... AFAICS the function
>>  qdev_device_add() is now the right spot to do the check.
>>
>>  hw/core/qdev.c         | 28 ++++++++++++++++++++--------
>>  include/hw/qdev-core.h |  1 +
>>  qdev-monitor.c         |  9 +++++++++
>>  3 files changed, 30 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 606ab53..a953ec9 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -253,19 +253,31 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>>      dev->alias_required_for_version = required_for_version;
>>  }
>>  
>> +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
>> +{
>> +    MachineState *machine;
>> +    MachineClass *mc;
>> +    Object *m_obj = qdev_get_machine();
>> +
>> +    if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
>> +        machine = MACHINE(m_obj);
>> +        mc = MACHINE_GET_CLASS(machine);
>> +        if (mc->get_hotplug_handler) {
>> +            return mc->get_hotplug_handler(machine, dev);
>> +        }
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>>  HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
>>  {
>> -    HotplugHandler *hotplug_ctrl = NULL;
>> +    HotplugHandler *hotplug_ctrl;
>>  
>>      if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
>>          hotplug_ctrl = dev->parent_bus->hotplug_handler;
>> -    } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
>> -        MachineState *machine = MACHINE(qdev_get_machine());
>> -        MachineClass *mc = MACHINE_GET_CLASS(machine);
>> -
>> -        if (mc->get_hotplug_handler) {
>> -            hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
>> -        }
>> +    } 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 0891461..5aa536d 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -285,6 +285,7 @@ DeviceState *qdev_try_create(BusState *bus, const char *name);
>>  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);
>>  HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
>>  void qdev_unplug(DeviceState *dev, Error **errp);
>>  void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 8fd6df9..2891dde 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -626,6 +626,15 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>          return NULL;
>>      }
>>  
>> +    /* In case we don't have a bus, there must be a machine hotplug handler */
>> +    if (qdev_hotplug && !bus && !qdev_get_machine_hotplug_handler(dev)) {
> current machine hotplug handler serves both cold and hot-plug so in reality it's
> just  'plug' handler.
> 
> Is there a point -device/device_add devices on board that doesn't have 'hotplug'
> handler that would wire device up properly?

Sorry, I did not get that question ... do you mean whether there's any
code that uses qdev_device_add() to add a device without hotplug
controller? I don't think so. It's currently only used by
qmp_device_add() for the QMP/HMP command, in vl.c for -device and in the
USB code for xen-usb host device. So this function currently really only
makes sense for devices that have a hotplug controller.

 Thomas