Re: [Qemu-devel] [RFC PATCH] qdev: Mark devices as non-hotpluggable by default

[Marcel Apfelbaum <marcel@redhat.com>]

On 20/09/2017 13:07, Peter Maydell wrote:
> On 20 September 2017 at 08:50, Marcel Apfelbaum <marcel@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 <thuth@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?

Thanks,
Marcel

> thanks
> -- PMM
>