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

[Eduardo Habkost <ehabkost@redhat.com>]

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

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.

-- 
Eduardo