On 20.09.2017 12:57, Marcel Apfelbaum wrote: > On 20/09/2017 13:07, Peter Maydell wrote: >> On 20 September 2017 at 08:50, Marcel Apfelbaum <mar...@redhat.com> >> wrote: >>> Hi Thomas, >>> >>> >>> On 19/09/2017 11:55, Thomas Huth wrote: >>>> >>>> Historically we've marked all devices as hotpluggable by default. >>>> However, >>>> most devices are not hotpluggable, and you also need a >>>> HotplugHandler to >>>> support these devices. So if the user tries to "device_add" or >>>> "device_del" >>>> such a non-hotpluggable device during runtime, either nothing really >>>> usable >>>> happens, or QEMU even crashes/aborts unexpectedly (see for example >>>> commit >>>> 84ebd3e8c7d4fe955b - "Mark diag288 watchdog as non-hotpluggable"). >>>> So let's change this dangerous default behaviour and mark the >>>> devices as >>>> non-hotpluggable by default. Certain parent devices classes which are >>>> known >>>> as hotpluggable (e.g. PCI, USB, etc.) are marked with "hotpluggable = >>>> true", >>>> so that devices that are derived from these classes continue to work as >>>> expected. >>>> >>>> Signed-off-by: Thomas Huth <th...@redhat.com> >>>> --- >>>> Note: I've marked the patch as RFC since I'm not 100% sure >>>> whether I've >>>> correctly identified all devices that should still be marked as hot- >>>> pluggable. Feedback is welcome! >>>> >>>> hw/core/qdev.c | 10 ++++------ >>>> hw/cpu/core.c | 1 + >>>> hw/mem/nvdimm.c | 3 +++ >>>> hw/mem/pc-dimm.c | 1 + >>>> hw/pci/pci.c | 1 + >>>> hw/s390x/ccw-device.c | 1 + >>>> hw/scsi/scsi-bus.c | 1 + >>>> hw/usb/bus.c | 1 + >>>> 8 files changed, 13 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >>>> index 606ab53..c4f1902 100644 >>>> --- a/hw/core/qdev.c >>>> +++ b/hw/core/qdev.c >>>> @@ -1120,13 +1120,11 @@ static void device_class_init(ObjectClass >>>> *class, >>>> void *data) >>>> dc->realize = device_realize; >>>> dc->unrealize = device_unrealize; >>>> - /* by default all devices were considered as hotpluggable, >>>> - * so with intent to check it in generic qdev_unplug() / >>>> - * device_set_realized() functions make every device >>>> - * hotpluggable. Devices that shouldn't be hotpluggable, >>>> - * should override it in their class_init() >>>> + /* >>>> + * All devices are considered as cold-pluggable by default. The >>>> devices >>>> + * that are hotpluggable should override it in their class_init(). >>>> */ >>>> - dc->hotpluggable = true; >>>> + dc->hotpluggable = false; >>> >>> >>> I agree with the defensive mode as long as we don't >>> introduce any regression... >> >> Is it possible to hack together some kind of test code that >> can give us a list of the devices compiled in that have >> hotpluggable enabled? Then we could compare before and >> after to see which devices we've changed. >> > > Eduardo came up with some cool automated tests not long ago. > Eduardo, can your tests help?
You mean the scripts/device-crash-test script? That's only for cold-plugging ... but I could maybe use the device_add HMP test (see https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg00817.html ) to exercise the hotplugging of all devices and add some debug code to qdev_device_add() to see which devices are dc->hotpluggable and have a hotplug handler at the same time... I'll give it a try. Thomas
