hw/core/qdev: Do not allow hot-plugging without hotplug controller
PATCH 1/1 hw/core/qdev: Do not allow hot-plugging without hotplug controller
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!
--- hw/core/qdev.c | 5 +++++ 1 file changed, 5 insertions(+)
hw/core/qdev.c +5 -0
@@ -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) {
Signature · 1 line
1.8.3.1
Quoted 28 lines · On Thu, 7 Sep 2017 11:22:42 +0200 Thomas Huth <[email protected]> wrote:
qdev_unplug()bails out with an assertion if the user tries todevice_dela hot-plugged device that does not have a hotplug controller. Unfortunately, our devices are all marked with hotpluggable = true by default (see thedevice_class_init()function inqdev.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 <[email protected]>Signed-off-by Thomas Huth <[email protected]>--- hw/core/qdev.c | 5 +++++ 1 file changed, 5 insertions(+)hw/core/qdev.c +0
-0@@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)if (local_err !=NULL ) {
hw/core/qdev.c static void device_set_realized(Object *obj, bool value, Error **errp) · line 918
Quoted 10 lines
hw/core/qdev.c +5
-0goto 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
Quoted 44 lines · Igor Mammedov
Quoted 28 lines · On Thu, 7 Sep 2017 11:22:42 +0200 Thomas Huth <[email protected]> wrote:
qdev_unplug()bails out with an assertion if the user tries todevice_dela hot-plugged device that does not have a hotplug controller. Unfortunately, our devices are all marked with hotpluggable = true by default (see thedevice_class_init()function inqdev.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 <[email protected]>Signed-off-by Thomas Huth <[email protected]>--- hw/core/qdev.c | 5 +++++ 1 file changed, 5 insertions(+)hw/core/qdev.c +0
-0@@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)if (local_err !=NULL ) {hw/core/qdev.c static void device_set_realized(Object *obj, bool value, Error **errp) · line 918
Quoted 10 lines
hw/core/qdev.c +5
-0goto 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
Quoted 8 lines
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
Quoted 51 lines · On Mon, 11 Sep 2017 16:31:39 +0200 Thomas Huth <[email protected]> wrote:
Quoted 50 lines · Igor Mammedov
Quoted 28 lines · On Thu, 7 Sep 2017 11:22:42 +0200 Thomas Huth <[email protected]> wrote:
qdev_unplug()bails out with an assertion if the user tries todevice_dela hot-plugged device that does not have a hotplug controller. Unfortunately, our devices are all marked with hotpluggable = true by default (see thedevice_class_init()function inqdev.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 <[email protected]>Signed-off-by Thomas Huth <[email protected]>--- hw/core/qdev.c | 5 +++++ 1 file changed, 5 insertions(+)hw/core/qdev.c +0
-0@@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)if (local_err !=NULL ) {hw/core/qdev.c static void device_set_realized(Object *obj, bool value, Error **errp) · line 918
Quoted 10 lines
hw/core/qdev.c +5
-0goto 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; }
Quoted 7 lines
Quoted 2 lines
instead of leaving device broken, like in yours
84ebd3e watchdog/wdt_diag288: Mark diag288 watchdog as non-hotpluggableNo, 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.
Quoted 1 line
Thomas
Quoted 65 lines · Igor Mammedov
Quoted 51 lines · On Mon, 11 Sep 2017 16:31:39 +0200 Thomas Huth <[email protected]> wrote:
Quoted 50 lines · Igor Mammedov
Quoted 28 lines · On Thu, 7 Sep 2017 11:22:42 +0200 Thomas Huth <[email protected]> wrote:
qdev_unplug()bails out with an assertion if the user tries todevice_dela hot-plugged device that does not have a hotplug controller. Unfortunately, our devices are all marked with hotpluggable = true by default (see thedevice_class_init()function inqdev.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 <[email protected]>Signed-off-by Thomas Huth <[email protected]>--- hw/core/qdev.c | 5 +++++ 1 file changed, 5 insertions(+)hw/core/qdev.c +0
-0@@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)if (local_err !=NULL ) {hw/core/qdev.c static void device_set_realized(Object *obj, bool value, Error **errp) · line 918
Quoted 10 lines
hw/core/qdev.c +5
-0goto 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; }Quoted 7 lines
Quoted 2 lines
instead of leaving device broken, like in yours
84ebd3e watchdog/wdt_diag288: Mark diag288 watchdog as non-hotpluggableNo, 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.
Quoted 6 lines
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 setdc->hotpluggableproperty.
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.
Signature · 1 line
Eduardo
Igor Mammedov
Reviewed-byQuoted 73 lines · On Tue, 12 Sep 2017 15:05:56 -0300 Eduardo Habkost <[email protected]> wrote:
Quoted 65 lines · Igor Mammedov
Quoted 51 lines · On Mon, 11 Sep 2017 16:31:39 +0200 Thomas Huth <[email protected]> wrote:
Quoted 50 lines · Igor Mammedov
Quoted 28 lines · On Thu, 7 Sep 2017 11:22:42 +0200 Thomas Huth <[email protected]> wrote:
qdev_unplug()bails out with an assertion if the user tries todevice_dela hot-plugged device that does not have a hotplug controller. Unfortunately, our devices are all marked with hotpluggable = true by default (see thedevice_class_init()function inqdev.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 <[email protected]>Signed-off-by Thomas Huth <[email protected]>--- hw/core/qdev.c | 5 +++++ 1 file changed, 5 insertions(+)hw/core/qdev.c +0
-0@@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)if (local_err !=NULL ) {hw/core/qdev.c static void device_set_realized(Object *obj, bool value, Error **errp) · line 918
Quoted 10 lines
hw/core/qdev.c +5
-0goto 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; }Quoted 7 lines
Quoted 2 lines
instead of leaving device broken, like in yours
84ebd3e watchdog/wdt_diag288: Mark diag288 watchdog as non-hotpluggableNo, 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.
Quoted 6 lines
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 setdc->hotpluggableproperty.
Quoted 7 lines
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,
Quoted 5 lines · Thomas Huth
qdev_unplug()bails out with an assertion if the user tries todevice_dela hot-plugged device that does not have a hotplug controller. Unfortunately, our devices are all marked with hotpluggable = true by default (see thedevice_class_init()function inqdev.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
Quoted 14 lines · Thomas Huth
qdev_unplug()bails out with an assertion if the user tries todevice_dela hot-plugged device that does not have a hotplug controller. Unfortunately, our devices are all marked with hotpluggable = true by default (see thedevice_class_init()function inqdev.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)
Quoted 5 lines
Hotplugging devices without a hotplug controller does not make much sense, so we should disallow this during the
device_addprocess already!Suggested-by Paolo Bonzini <[email protected]>Signed-off-by Thomas Huth <[email protected]>
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?
Signature · 1 line
Eduardo
Quoted 18 lines · Eduardo Habkost
Quoted 17 lines · Thomas Huth
qdev_unplug()bails out with an assertion if the user tries todevice_dela hot-plugged device that does not have a hotplug controller. Unfortunately, our devices are all marked with hotpluggable = true by default (see thedevice_class_init()function inqdev.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_addprocess already!
Quoted 6 lines
Quoted 2 lines
Suggested-by Paolo Bonzini <[email protected]>Signed-off-by Thomas Huth <[email protected]>I'm queueing this on machine-next. We still want it even if we apply the patch that changes
TYPE_DEVICEto 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
Quoted 18 lines · Eduardo Habkost
Quoted 17 lines · Thomas Huth
qdev_unplug()bails out with an assertion if the user tries todevice_dela hot-plugged device that does not have a hotplug controller. Unfortunately, our devices are all marked with hotpluggable = true by default (see thedevice_class_init()function inqdev.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_addprocess already!
Quoted 6 lines
Quoted 2 lines
Suggested-by Paolo Bonzini <[email protected]>Signed-off-by Thomas Huth <[email protected]>I'm queueing this on machine-next. We still want it even if we apply the patch that changes
TYPE_DEVICEto 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
Quoted 22 lines · Thomas Huth
Quoted 21 lines · Eduardo Habkost
Quoted 19 lines · Thomas Huth
qdev_unplug()bails out with an assertion if the user tries todevice_dela hot-plugged device that does not have a hotplug controller. Unfortunately, our devices are all marked with hotpluggable = true by default (see thedevice_class_init()function inqdev.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_addprocess already!Suggested-by Paolo Bonzini <[email protected]>Signed-off-by Thomas Huth <[email protected]>
Quoted 8 lines
Quoted 3 lines
I'm queueing this on machine-next. We still want it even if we apply the patch that changes
TYPE_DEVICEto 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.
Quoted 4 lines
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?
Signature · 1 line
Eduardo
Quoted 35 lines · Eduardo Habkost
Quoted 22 lines · Thomas Huth
Quoted 21 lines · Eduardo Habkost
Quoted 19 lines · Thomas Huth
qdev_unplug()bails out with an assertion if the user tries todevice_dela hot-plugged device that does not have a hotplug controller. Unfortunately, our devices are all marked with hotpluggable = true by default (see thedevice_class_init()function inqdev.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_addprocess already!Suggested-by Paolo Bonzini <[email protected]>Signed-off-by Thomas Huth <[email protected]>Quoted 8 lines
Quoted 3 lines
I'm queueing this on machine-next. We still want it even if we apply the patch that changes
TYPE_DEVICEto 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.
Quoted 1 line
Quoted 7 lines
Quoted 3 lines
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_addmust 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