Message ID | 1504776162-31400-1-git-send-email-thuth@redhat.com |
---|---|
State | New |
Headers | show |
Series | hw/core/qdev: Do not allow hot-plugging without hotplug controller | expand |
On Thu, 7 Sep 2017 11:22:42 +0200 Thomas Huth <thuth@redhat.com> wrote: > qdev_unplug() bails out with an assertion if the user tries to device_del > a hot-plugged device that does not have a hotplug controller. Unfortunately, > our devices are all marked with hotpluggable = true by default (see the > device_class_init() function in qdev.c), so it currently can happen that > the user runs into this situation and QEMU gets terminated unexpectedly: > > $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S > QEMU 2.10.50 monitor - type 'help' for more information > (qemu) device_add aux-to-i2c-bridge,id=x > (qemu) device_del x > ** > ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl) > Aborted (core dumped) > Hotplugging devices without a hotplug controller does not make much sense, > so we should disallow this during the device_add process already! > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/core/qdev.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 606ab53..d9ccce6 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > if (local_err != NULL) { > goto fail; > } > + } else if (dev->hotplugged) { > + /* Hot-plugged device without hotplug controller? No way! */ > + error_setg(&local_err, QERR_DEVICE_NO_HOTPLUG, > + object_get_typename(obj)); > + goto fail; > } > > if (dc->realize) { maybe it should be other way around, i.e, fix device so that following would work device_set_realized() if (dev->hotplugged && !dc->hotpluggable) { error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj)); return; } instead of leaving device broken, like in yours 84ebd3e watchdog/wdt_diag288: Mark diag288 watchdog as non-hotpluggable
On 11.09.2017 14:53, Igor Mammedov wrote: > On Thu, 7 Sep 2017 11:22:42 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> qdev_unplug() bails out with an assertion if the user tries to device_del >> a hot-plugged device that does not have a hotplug controller. Unfortunately, >> our devices are all marked with hotpluggable = true by default (see the >> device_class_init() function in qdev.c), so it currently can happen that >> the user runs into this situation and QEMU gets terminated unexpectedly: >> >> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S >> QEMU 2.10.50 monitor - type 'help' for more information >> (qemu) device_add aux-to-i2c-bridge,id=x >> (qemu) device_del x >> ** >> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl) >> Aborted (core dumped) >> Hotplugging devices without a hotplug controller does not make much sense, >> so we should disallow this during the device_add process already! >> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> hw/core/qdev.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >> index 606ab53..d9ccce6 100644 >> --- a/hw/core/qdev.c >> +++ b/hw/core/qdev.c >> @@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp) >> if (local_err != NULL) { >> goto fail; >> } >> + } else if (dev->hotplugged) { >> + /* Hot-plugged device without hotplug controller? No way! */ >> + error_setg(&local_err, QERR_DEVICE_NO_HOTPLUG, >> + object_get_typename(obj)); >> + goto fail; >> } >> >> if (dc->realize) { > > maybe it should be other way around, i.e, fix device so that following would work > > device_set_realized() > if (dev->hotplugged && !dc->hotpluggable) { > error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj)); > return; > } > > instead of leaving device broken, like in yours > 84ebd3e watchdog/wdt_diag288: Mark diag288 watchdog as non-hotpluggable No, that apparently does not work right for new devices since people keep forgetting to set hotpluggable = false there. Both, Paolo and Peter suggested that we should not allow hot-plugging if there's no hot plug controller - it indeed does not make sense, so we should not allow it. Thomas
On Mon, 11 Sep 2017 16:31:39 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 11.09.2017 14:53, Igor Mammedov wrote: > > On Thu, 7 Sep 2017 11:22:42 +0200 > > Thomas Huth <thuth@redhat.com> wrote: > > > >> qdev_unplug() bails out with an assertion if the user tries to device_del > >> a hot-plugged device that does not have a hotplug controller. Unfortunately, > >> our devices are all marked with hotpluggable = true by default (see the > >> device_class_init() function in qdev.c), so it currently can happen that > >> the user runs into this situation and QEMU gets terminated unexpectedly: > >> > >> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S > >> QEMU 2.10.50 monitor - type 'help' for more information > >> (qemu) device_add aux-to-i2c-bridge,id=x > >> (qemu) device_del x > >> ** > >> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl) > >> Aborted (core dumped) > >> Hotplugging devices without a hotplug controller does not make much sense, > >> so we should disallow this during the device_add process already! > >> > >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > >> --- > >> hw/core/qdev.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c > >> index 606ab53..d9ccce6 100644 > >> --- a/hw/core/qdev.c > >> +++ b/hw/core/qdev.c > >> @@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > >> if (local_err != NULL) { > >> goto fail; > >> } > >> + } else if (dev->hotplugged) { > >> + /* Hot-plugged device without hotplug controller? No way! */ > >> + error_setg(&local_err, QERR_DEVICE_NO_HOTPLUG, > >> + object_get_typename(obj)); > >> + goto fail; > >> } > >> > >> if (dc->realize) { > > > > maybe it should be other way around, i.e, fix device so that following would work > > > > device_set_realized() > > if (dev->hotplugged && !dc->hotpluggable) { > > error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj)); > > return; > > } > > > > instead of leaving device broken, like in yours > > 84ebd3e watchdog/wdt_diag288: Mark diag288 watchdog as non-hotpluggable > > No, that apparently does not work right for new devices since people > keep forgetting to set hotpluggable = false there. Both, Paolo and Peter > suggested that we should not allow hot-plugging if there's no hot plug > controller - it indeed does not make sense, so we should not allow it. historically all devices were hotpluggble and conversion to hotplug controller didn't fix it which os fine as far as user did not attempt unreasonable things. However it should be fixedfor code to work correctly. I'd suggest to flip default dc->hotpluggable = false; and set it to true explicitly for devices that support hotplug, it obviously harder to do than this patch as it requires audit of all devices, but it looks more correct than fixing symptoms of incorrectly set dc->hotpluggable property. > Thomas
On Mon, Sep 11, 2017 at 05:06:00PM +0200, Igor Mammedov wrote: > On Mon, 11 Sep 2017 16:31:39 +0200 > Thomas Huth <thuth@redhat.com> wrote: > > > On 11.09.2017 14:53, Igor Mammedov wrote: > > > On Thu, 7 Sep 2017 11:22:42 +0200 > > > Thomas Huth <thuth@redhat.com> wrote: > > > > > >> qdev_unplug() bails out with an assertion if the user tries to device_del > > >> a hot-plugged device that does not have a hotplug controller. Unfortunately, > > >> our devices are all marked with hotpluggable = true by default (see the > > >> device_class_init() function in qdev.c), so it currently can happen that > > >> the user runs into this situation and QEMU gets terminated unexpectedly: > > >> > > >> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S > > >> QEMU 2.10.50 monitor - type 'help' for more information > > >> (qemu) device_add aux-to-i2c-bridge,id=x > > >> (qemu) device_del x > > >> ** > > >> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl) > > >> Aborted (core dumped) > > >> Hotplugging devices without a hotplug controller does not make much sense, > > >> so we should disallow this during the device_add process already! > > >> > > >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > >> --- > > >> hw/core/qdev.c | 5 +++++ > > >> 1 file changed, 5 insertions(+) > > >> > > >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > >> index 606ab53..d9ccce6 100644 > > >> --- a/hw/core/qdev.c > > >> +++ b/hw/core/qdev.c > > >> @@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > > >> if (local_err != NULL) { > > >> goto fail; > > >> } > > >> + } else if (dev->hotplugged) { > > >> + /* Hot-plugged device without hotplug controller? No way! */ > > >> + error_setg(&local_err, QERR_DEVICE_NO_HOTPLUG, > > >> + object_get_typename(obj)); > > >> + goto fail; > > >> } > > >> > > >> if (dc->realize) { > > > > > > maybe it should be other way around, i.e, fix device so that following would work > > > > > > device_set_realized() > > > if (dev->hotplugged && !dc->hotpluggable) { > > > error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj)); > > > return; > > > } > > > > > > instead of leaving device broken, like in yours > > > 84ebd3e watchdog/wdt_diag288: Mark diag288 watchdog as non-hotpluggable > > > > No, that apparently does not work right for new devices since people > > keep forgetting to set hotpluggable = false there. Both, Paolo and Peter > > suggested that we should not allow hot-plugging if there's no hot plug > > controller - it indeed does not make sense, so we should not allow it. > historically all devices were hotpluggble and conversion to hotplug > controller didn't fix it which os fine as far as user did not attempt > unreasonable things. However it should be fixedfor code to work correctly. > > I'd suggest to flip default > dc->hotpluggable = false; > and set it to true explicitly for devices that support hotplug, > it obviously harder to do than this patch as it requires audit > of all devices, but it looks more correct than fixing symptoms of > incorrectly set dc->hotpluggable property. I agree we should do this. If we have any device-type that is not hotpluggable on any machine because no machine will return a hotplug controller for it, we shouldn't report it as hotpluggable through QMP and HMP. But this patch also seems to be required, for cases where not all machine-types accept hotplug of a given device type.
On Tue, 12 Sep 2017 15:05:56 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Sep 11, 2017 at 05:06:00PM +0200, Igor Mammedov wrote: > > On Mon, 11 Sep 2017 16:31:39 +0200 > > Thomas Huth <thuth@redhat.com> wrote: > > > > > On 11.09.2017 14:53, Igor Mammedov wrote: > > > > On Thu, 7 Sep 2017 11:22:42 +0200 > > > > Thomas Huth <thuth@redhat.com> wrote: > > > > > > > >> qdev_unplug() bails out with an assertion if the user tries to device_del > > > >> a hot-plugged device that does not have a hotplug controller. Unfortunately, > > > >> our devices are all marked with hotpluggable = true by default (see the > > > >> device_class_init() function in qdev.c), so it currently can happen that > > > >> the user runs into this situation and QEMU gets terminated unexpectedly: > > > >> > > > >> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S > > > >> QEMU 2.10.50 monitor - type 'help' for more information > > > >> (qemu) device_add aux-to-i2c-bridge,id=x > > > >> (qemu) device_del x > > > >> ** > > > >> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl) > > > >> Aborted (core dumped) > > > >> Hotplugging devices without a hotplug controller does not make much sense, > > > >> so we should disallow this during the device_add process already! > > > >> > > > >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > > > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > > >> --- > > > >> hw/core/qdev.c | 5 +++++ > > > >> 1 file changed, 5 insertions(+) > > > >> > > > >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > > >> index 606ab53..d9ccce6 100644 > > > >> --- a/hw/core/qdev.c > > > >> +++ b/hw/core/qdev.c > > > >> @@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > > > >> if (local_err != NULL) { > > > >> goto fail; > > > >> } > > > >> + } else if (dev->hotplugged) { > > > >> + /* Hot-plugged device without hotplug controller? No way! */ > > > >> + error_setg(&local_err, QERR_DEVICE_NO_HOTPLUG, > > > >> + object_get_typename(obj)); > > > >> + goto fail; > > > >> } > > > >> > > > >> if (dc->realize) { > > > > > > > > maybe it should be other way around, i.e, fix device so that following would work > > > > > > > > device_set_realized() > > > > if (dev->hotplugged && !dc->hotpluggable) { > > > > error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj)); > > > > return; > > > > } > > > > > > > > instead of leaving device broken, like in yours > > > > 84ebd3e watchdog/wdt_diag288: Mark diag288 watchdog as non-hotpluggable > > > > > > No, that apparently does not work right for new devices since people > > > keep forgetting to set hotpluggable = false there. Both, Paolo and Peter > > > suggested that we should not allow hot-plugging if there's no hot plug > > > controller - it indeed does not make sense, so we should not allow it. > > historically all devices were hotpluggble and conversion to hotplug > > controller didn't fix it which os fine as far as user did not attempt > > unreasonable things. However it should be fixedfor code to work correctly. > > > > I'd suggest to flip default > > dc->hotpluggable = false; > > and set it to true explicitly for devices that support hotplug, > > it obviously harder to do than this patch as it requires audit > > of all devices, but it looks more correct than fixing symptoms of > > incorrectly set dc->hotpluggable property. > > I agree we should do this. If we have any device-type that is > not hotpluggable on any machine because no machine will return a > hotplug controller for it, we shouldn't report it as hotpluggable > through QMP and HMP. > > But this patch also seems to be required, for cases where not all > machine-types accept hotplug of a given device type. Agreed, Reviewed-by: Igor Mammedov <imammedo@redhat.com>
On 7 September 2017 at 10:22, Thomas Huth <thuth@redhat.com> wrote: > qdev_unplug() bails out with an assertion if the user tries to device_del > a hot-plugged device that does not have a hotplug controller. Unfortunately, > our devices are all marked with hotpluggable = true by default (see the > device_class_init() function in qdev.c), so it currently can happen that > the user runs into this situation and QEMU gets terminated unexpectedly: Why are our devices hotpluggable by default? Hotpluggable is the unusual case which requires the device author to take special case (notably writing a function to undo anything done in realize so we don't leak stuff on unplug), so it should be the case that requires the device author to mark the device as hotplugged specifically. We should just change the default (and effectively whitelist devices which do hotplug successfully), or we'll be continually playing whack-a-mole with devices or classes of devices or machines that accidentally forgot to disable hotplugging. thanks -- PMM
On Thu, Sep 07, 2017 at 11:22:42AM +0200, Thomas Huth wrote: > qdev_unplug() bails out with an assertion if the user tries to device_del > a hot-plugged device that does not have a hotplug controller. Unfortunately, > our devices are all marked with hotpluggable = true by default (see the > device_class_init() function in qdev.c), so it currently can happen that > the user runs into this situation and QEMU gets terminated unexpectedly: > > $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S > QEMU 2.10.50 monitor - type 'help' for more information > (qemu) device_add aux-to-i2c-bridge,id=x > (qemu) device_del x > ** > ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl) > Aborted (core dumped) > > Hotplugging devices without a hotplug controller does not make much sense, > so we should disallow this during the device_add process already! > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> I'm queueing this on machine-next. We still want it even if we apply the patch that changes TYPE_DEVICE to hotpluggable=false by default, right?
On 21.09.2017 19:56, Eduardo Habkost wrote: > On Thu, Sep 07, 2017 at 11:22:42AM +0200, Thomas Huth wrote: >> qdev_unplug() bails out with an assertion if the user tries to device_del >> a hot-plugged device that does not have a hotplug controller. Unfortunately, >> our devices are all marked with hotpluggable = true by default (see the >> device_class_init() function in qdev.c), so it currently can happen that >> the user runs into this situation and QEMU gets terminated unexpectedly: >> >> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S >> QEMU 2.10.50 monitor - type 'help' for more information >> (qemu) device_add aux-to-i2c-bridge,id=x >> (qemu) device_del x >> ** >> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl) >> Aborted (core dumped) >> >> Hotplugging devices without a hotplug controller does not make much sense, >> so we should disallow this during the device_add process already! >> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > I'm queueing this on machine-next. We still want it even if we > apply the patch that changes TYPE_DEVICE to hotpluggable=false by > default, right? As far as I can see, yes. There might be machine types where e.g. the PCI bus comes without a hotplug controller, so in that case we'd need this. Thomas
On 21.09.2017 19:56, Eduardo Habkost wrote: > On Thu, Sep 07, 2017 at 11:22:42AM +0200, Thomas Huth wrote: >> qdev_unplug() bails out with an assertion if the user tries to device_del >> a hot-plugged device that does not have a hotplug controller. Unfortunately, >> our devices are all marked with hotpluggable = true by default (see the >> device_class_init() function in qdev.c), so it currently can happen that >> the user runs into this situation and QEMU gets terminated unexpectedly: >> >> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S >> QEMU 2.10.50 monitor - type 'help' for more information >> (qemu) device_add aux-to-i2c-bridge,id=x >> (qemu) device_del x >> ** >> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl) >> Aborted (core dumped) >> >> Hotplugging devices without a hotplug controller does not make much sense, >> so we should disallow this during the device_add process already! >> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > I'm queueing this on machine-next. We still want it even if we > apply the patch that changes TYPE_DEVICE to hotpluggable=false by > default, right? OK, just to be sure: Please de-queue it again. As you already mentioned in another mail, there are situations where this does not work (e.g. when one hot-pluggable device wants to instantiate another hot-pluggable device internally). But maybe we could add the test for the availability of a hot-plug controller to qmp_device_add() instead? (I haven't had a closer look at that yet, though). Thomas
On Tue, Sep 26, 2017 at 12:05:47PM +0200, Thomas Huth wrote: > On 21.09.2017 19:56, Eduardo Habkost wrote: > > On Thu, Sep 07, 2017 at 11:22:42AM +0200, Thomas Huth wrote: > >> qdev_unplug() bails out with an assertion if the user tries to device_del > >> a hot-plugged device that does not have a hotplug controller. Unfortunately, > >> our devices are all marked with hotpluggable = true by default (see the > >> device_class_init() function in qdev.c), so it currently can happen that > >> the user runs into this situation and QEMU gets terminated unexpectedly: > >> > >> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S > >> QEMU 2.10.50 monitor - type 'help' for more information > >> (qemu) device_add aux-to-i2c-bridge,id=x > >> (qemu) device_del x > >> ** > >> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl) > >> Aborted (core dumped) > >> > >> Hotplugging devices without a hotplug controller does not make much sense, > >> so we should disallow this during the device_add process already! > >> > >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > > > I'm queueing this on machine-next. We still want it even if we > > apply the patch that changes TYPE_DEVICE to hotpluggable=false by > > default, right? > > OK, just to be sure: Please de-queue it again. As you already mentioned > in another mail, there are situations where this does not work (e.g. > when one hot-pluggable device wants to instantiate another hot-pluggable > device internally). I just dequeued it. > > But maybe we could add the test for the availability of a hot-plug > controller to qmp_device_add() instead? (I haven't had a closer look at > that yet, though). I'm unsure about this. Now we know we have some devices that work without a hotplug controller, so what's the reason device_add must always require one?
On 26.09.2017 15:41, Eduardo Habkost wrote: > On Tue, Sep 26, 2017 at 12:05:47PM +0200, Thomas Huth wrote: >> On 21.09.2017 19:56, Eduardo Habkost wrote: >>> On Thu, Sep 07, 2017 at 11:22:42AM +0200, Thomas Huth wrote: >>>> qdev_unplug() bails out with an assertion if the user tries to device_del >>>> a hot-plugged device that does not have a hotplug controller. Unfortunately, >>>> our devices are all marked with hotpluggable = true by default (see the >>>> device_class_init() function in qdev.c), so it currently can happen that >>>> the user runs into this situation and QEMU gets terminated unexpectedly: >>>> >>>> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S >>>> QEMU 2.10.50 monitor - type 'help' for more information >>>> (qemu) device_add aux-to-i2c-bridge,id=x >>>> (qemu) device_del x >>>> ** >>>> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl) >>>> Aborted (core dumped) >>>> >>>> Hotplugging devices without a hotplug controller does not make much sense, >>>> so we should disallow this during the device_add process already! >>>> >>>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> >>> I'm queueing this on machine-next. We still want it even if we >>> apply the patch that changes TYPE_DEVICE to hotpluggable=false by >>> default, right? >> >> OK, just to be sure: Please de-queue it again. As you already mentioned >> in another mail, there are situations where this does not work (e.g. >> when one hot-pluggable device wants to instantiate another hot-pluggable >> device internally). > > I just dequeued it. > >> >> But maybe we could add the test for the availability of a hot-plug >> controller to qmp_device_add() instead? (I haven't had a closer look at >> that yet, though). > > I'm unsure about this. Now we know we have some devices that > work without a hotplug controller, so what's the reason > device_add must always require one? The problem is that you can also "device_del" these devices again. And qdev_unplug() currently has this piece of code in it: /* hotpluggable device MUST have HotplugHandler, if it doesn't * then something is very wrong with it */ g_assert(hotplug_ctrl); So if you do a device_add followed by a device_del with one of these devices, QEMU aborts and kills your guest. One solution is maybe to remove the assert here. OTOH, considering that we have the assert in qdev_monitor.c->qdev_unplug(), adding a check for the availability of a hotplug_ctrl in qdev_monitor.c->qmp_device_add() currently sounds like the better solution to this problem to me. Anybody got another opinion here? If not, I'll have a try and send a patch... Thomas
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 606ab53..d9ccce6 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp) if (local_err != NULL) { goto fail; } + } else if (dev->hotplugged) { + /* Hot-plugged device without hotplug controller? No way! */ + error_setg(&local_err, QERR_DEVICE_NO_HOTPLUG, + object_get_typename(obj)); + goto fail; } if (dc->realize) {
qdev_unplug() bails out with an assertion if the user tries to device_del a hot-plugged device that does not have a hotplug controller. Unfortunately, our devices are all marked with hotpluggable = true by default (see the device_class_init() function in qdev.c), so it currently can happen that the user runs into this situation and QEMU gets terminated unexpectedly: $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S QEMU 2.10.50 monitor - type 'help' for more information (qemu) device_add aux-to-i2c-bridge,id=x (qemu) device_del x ** ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl) Aborted (core dumped) Hotplugging devices without a hotplug controller does not make much sense, so we should disallow this during the device_add process already! Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Thomas Huth <thuth@redhat.com> --- hw/core/qdev.c | 5 +++++ 1 file changed, 5 insertions(+)