| Message ID | 1509617407-21191-3-git-send-email-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2 Nov 2017 11:10:06 +0100 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), and can currently for example also be > triggered like this: > > $ s390x-softmmu/qemu-system-s390x -M none -nographic > QEMU 2.10.50 monitor - type 'help' for more information > (qemu) device_add qemu-s390x-cpu,id=x > (qemu) device_del x > ** > ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl) > Aborted (core dumped) > > So devices clearly need a hotplug controller when they should be usable > with 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 yet. 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. > > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/core/qdev.c | 28 ++++++++++++++++++++-------- > include/hw/qdev-core.h | 1 + > qdev-monitor.c | 5 +++++ > 3 files changed, 26 insertions(+), 8 deletions(-) Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Thomas Huth <thuth@redhat.com> writes: > 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), and can currently for example also be > triggered like this: > > $ s390x-softmmu/qemu-system-s390x -M none -nographic > QEMU 2.10.50 monitor - type 'help' for more information > (qemu) device_add qemu-s390x-cpu,id=x > (qemu) device_del x > ** > ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl) > Aborted (core dumped) > > So devices clearly need a hotplug controller when they should be usable > with device_add. > The code in qdev_device_add() already checks whether the bus has a proper > hotplug controller, Where? Hmm, I guess it's this one: if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) { error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name); return NULL; } > but for devices that do not have a corresponding bus, > there is no appropriate check available yet. 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. > > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/core/qdev.c | 28 ++++++++++++++++++++-------- > include/hw/qdev-core.h | 1 + > qdev-monitor.c | 5 +++++ > 3 files changed, 26 insertions(+), 8 deletions(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 1111295..f739753 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; > } qdev_get_machine_hotplug_handler() factored out of qdev_get_hotplug_handler(). Okay. Announcing it in the commit message could've saved me a few review brainwaves. > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 0a71bf8..51473ee 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -286,6 +286,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 9188d20..38c0fc2 100644 > --- a/qdev-monitor.c > +++ b/qdev-monitor.c > @@ -614,6 +614,11 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) > if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) { error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name); return NULL; } if (!migration_is_idle()) { error_setg(errp, "device_add not allowed while migrating"); return NULL; } /* create device */ dev = DEVICE(object_new(driver)); > if (bus) { > qdev_set_parent_bus(dev, bus); > + } else if (qdev_hotplug && !qdev_get_machine_hotplug_handler(dev)) { > + /* No bus, no machine hotplug handler --> device is not hotpluggable */ Long line. > + error_setg(&err, "Device '%s' can not be hotplugged on this machine", > + driver); > + goto err_del_dev; > } > > qdev_set_id(dev, qemu_opts_id(opts)); Hmm. We need to check "can hotplug" in two separate ways, with bus and without bus. Can we keep the two ways on one place? Something like if (qdev_hotplug) { if (bus && !qbus_is_hotpluggable(bus)) { error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name); return NULL; } if (!bus && !qdev_get_machine_hotplug_handler(dev)) { error_setg(&err, "Machine doesn't support hot-plugging device '%s'" driver); return NULL; } }
On 13.11.2017 15:00, Markus Armbruster wrote: > Thomas Huth <thuth@redhat.com> writes: > >> 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), and can currently for example also be >> triggered like this: >> >> $ s390x-softmmu/qemu-system-s390x -M none -nographic >> QEMU 2.10.50 monitor - type 'help' for more information >> (qemu) device_add qemu-s390x-cpu,id=x >> (qemu) device_del x >> ** >> ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl) >> Aborted (core dumped) >> >> So devices clearly need a hotplug controller when they should be usable >> with device_add. >> The code in qdev_device_add() already checks whether the bus has a proper >> hotplug controller, > > Where? Hmm, I guess it's this one: > > if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) { > error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name); > return NULL; > } Right. >> but for devices that do not have a corresponding bus, >> there is no appropriate check available yet. 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. [...] >> diff --git a/qdev-monitor.c b/qdev-monitor.c >> index 9188d20..38c0fc2 100644 >> --- a/qdev-monitor.c >> +++ b/qdev-monitor.c >> @@ -614,6 +614,11 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) >> > if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) { > error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name); > return NULL; > } > > if (!migration_is_idle()) { > error_setg(errp, "device_add not allowed while migrating"); > return NULL; > } > > /* create device */ > dev = DEVICE(object_new(driver)); > >> if (bus) { >> qdev_set_parent_bus(dev, bus); >> + } else if (qdev_hotplug && !qdev_get_machine_hotplug_handler(dev)) { >> + /* No bus, no machine hotplug handler --> device is not hotpluggable */ > > Long line. Less than 80 columns, so that's fine. >> + error_setg(&err, "Device '%s' can not be hotplugged on this machine", >> + driver); >> + goto err_del_dev; >> } >> >> qdev_set_id(dev, qemu_opts_id(opts)); > > Hmm. We need to check "can hotplug" in two separate ways, with bus and > without bus. Can we keep the two ways on one place? Something like > > if (qdev_hotplug) { > if (bus && !qbus_is_hotpluggable(bus)) { > error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name); > return NULL; > } > if (!bus && !qdev_get_machine_hotplug_handler(dev)) { > error_setg(&err, > "Machine doesn't support hot-plugging device '%s'" > driver); > return NULL; > } > } We likely could ... but I don't like that change since in that case you've got to always do "dev = DEVICE(object_new(driver))" (and the code needs to be at the end of the function, so it would need "goto err_del_dev" instead of "return NULL"). If we keep the checks separate, the first check can bail out without creating the device first, so I'd really prefer to keep my patch that way. Thomas
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 1111295..f739753 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 0a71bf8..51473ee 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -286,6 +286,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 9188d20..38c0fc2 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -614,6 +614,11 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) if (bus) { qdev_set_parent_bus(dev, bus); + } else if (qdev_hotplug && !qdev_get_machine_hotplug_handler(dev)) { + /* No bus, no machine hotplug handler --> device is not hotpluggable */ + error_setg(&err, "Device '%s' can not be hotplugged on this machine", + driver); + goto err_del_dev; } qdev_set_id(dev, qemu_opts_id(opts));