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

[Thomas Huth]

On 04.10.2017 23:21, Eduardo Habkost wrote:
> On Wed, Oct 04, 2017 at 09:29:54PM +0200, Thomas Huth wrote:
>> On 04.10.2017 13:36, Igor Mammedov wrote:
>>> On Tue,  3 Oct 2017 18:46:02 +0200
>>> Thomas Huth <address@hidden> 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 <address@hidden>
>>>> ---
>>>>  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.
> 
> I assume you are talking only about hotpluggable devices.  With
> -device, qdev_device_add() is also used for devices that don't
> have a hotplug controller, but this is supposed to be true only
> for non-hotpluggable devices.

Right, the cold-pluggable devices should be fine because of the
"qdev_hotplug" check, forgot to mention that, sorry.

 Thomas