On 04.10.2017 13:36, Igor Mammedov wrote:
> On Tue, 3 Oct 2017 18:46:02 +0200
> Thomas Huth <th...@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 <th...@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
