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

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

[Thomas Huth]

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;
     dc->user_creatable = true;
 }
 
diff --git a/hw/cpu/core.c b/hw/cpu/core.c
index bd578ab..01660aa 100644
--- a/hw/cpu/core.c
+++ b/hw/cpu/core.c
@@ -82,6 +82,7 @@ static void cpu_core_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     set_bit(DEVICE_CATEGORY_CPU, dc->categories);
+    dc->hotpluggable = true;
 }
 
 static const TypeInfo cpu_core_type_info = {
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 952fce5..02be9f3 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -148,9 +148,12 @@ static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
 
 static void nvdimm_class_init(ObjectClass *oc, void *data)
 {
+    DeviceClass *dc = DEVICE_CLASS(oc);
     PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
     NVDIMMClass *nvc = NVDIMM_CLASS(oc);
 
+    dc->hotpluggable = true;
+
     ddc->realize = nvdimm_realize;
     ddc->get_memory_region = nvdimm_get_memory_region;
     ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 66eace5..1f78567 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -443,6 +443,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
     dc->unrealize = pc_dimm_unrealize;
     dc->props = pc_dimm_properties;
     dc->desc = "DIMM memory module";
+    dc->hotpluggable = true;
 
     ddc->get_memory_region = pc_dimm_get_memory_region;
     ddc->get_vmstate_memory_region = pc_dimm_get_vmstate_memory_region;
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 1e6fb88..8db380d 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2534,6 +2534,7 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
     k->unrealize = pci_qdev_unrealize;
     k->bus_type = TYPE_PCI_BUS;
     k->props = pci_props;
+    k->hotpluggable = true;
     pc->realize = pci_default_realize;
 }
 
diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
index f9bfa15..d1b6e6f 100644
--- a/hw/s390x/ccw-device.c
+++ b/hw/s390x/ccw-device.c
@@ -48,6 +48,7 @@ static void ccw_device_class_init(ObjectClass *klass, void *data)
     k->realize = ccw_device_realize;
     k->refill_ids = ccw_device_refill_ids;
     dc->props = ccw_device_properties;
+    dc->hotpluggable = true;
 }
 
 const VMStateDescription vmstate_ccw_dev = {
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index e364410..338180d 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -2123,6 +2123,7 @@ static void scsi_device_class_init(ObjectClass *klass, void *data)
     k->realize   = scsi_qdev_realize;
     k->unrealize = scsi_qdev_unrealize;
     k->props     = scsi_props;
+    k->hotpluggable = true;
 }
 
 static void scsi_dev_instance_init(Object *obj)
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index d910f84..16701aa 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -793,6 +793,7 @@ static void usb_device_class_init(ObjectClass *klass, void *data)
     k->realize  = usb_qdev_realize;
     k->unrealize = usb_qdev_unrealize;
     k->props    = usb_props;
+    k->hotpluggable = true;
 }
 
 static const TypeInfo usb_device_type_info = {
-- 
1.8.3.1

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

[Marcel Apfelbaum]

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...

By the way, in case a base class of a "family" of devices
would set the "hotpluggable" field to true (e.g. PCI devices),
we would still have the same problem on the specific sub-tree.
If the sub-tree is wide enough, this patch might not have the
intended impact. (we will still have the same bugs as the one
you mentioned in the commit message)

Thanks,
Marcel

>       dc->user_creatable = true;
>   }
>   
> diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> index bd578ab..01660aa 100644
> --- a/hw/cpu/core.c
> +++ b/hw/cpu/core.c
> @@ -82,6 +82,7 @@ static void cpu_core_class_init(ObjectClass *oc, void *data)
>       DeviceClass *dc = DEVICE_CLASS(oc);
>   
>       set_bit(DEVICE_CATEGORY_CPU, dc->categories);
> +    dc->hotpluggable = true;
>   }
>   
>   static const TypeInfo cpu_core_type_info = {
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 952fce5..02be9f3 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -148,9 +148,12 @@ static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
>   
>   static void nvdimm_class_init(ObjectClass *oc, void *data)
>   {
> +    DeviceClass *dc = DEVICE_CLASS(oc);
>       PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
>       NVDIMMClass *nvc = NVDIMM_CLASS(oc);
>   
> +    dc->hotpluggable = true;
> +
>       ddc->realize = nvdimm_realize;
>       ddc->get_memory_region = nvdimm_get_memory_region;
>       ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 66eace5..1f78567 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -443,6 +443,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
>       dc->unrealize = pc_dimm_unrealize;
>       dc->props = pc_dimm_properties;
>       dc->desc = "DIMM memory module";
> +    dc->hotpluggable = true;
>   
>       ddc->get_memory_region = pc_dimm_get_memory_region;
>       ddc->get_vmstate_memory_region = pc_dimm_get_vmstate_memory_region;
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 1e6fb88..8db380d 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2534,6 +2534,7 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
>       k->unrealize = pci_qdev_unrealize;
>       k->bus_type = TYPE_PCI_BUS;
>       k->props = pci_props;
> +    k->hotpluggable = true;
>       pc->realize = pci_default_realize;
>   }
>   
> diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
> index f9bfa15..d1b6e6f 100644
> --- a/hw/s390x/ccw-device.c
> +++ b/hw/s390x/ccw-device.c
> @@ -48,6 +48,7 @@ static void ccw_device_class_init(ObjectClass *klass, void *data)
>       k->realize = ccw_device_realize;
>       k->refill_ids = ccw_device_refill_ids;
>       dc->props = ccw_device_properties;
> +    dc->hotpluggable = true;
>   }
>   
>   const VMStateDescription vmstate_ccw_dev = {
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index e364410..338180d 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -2123,6 +2123,7 @@ static void scsi_device_class_init(ObjectClass *klass, void *data)
>       k->realize   = scsi_qdev_realize;
>       k->unrealize = scsi_qdev_unrealize;
>       k->props     = scsi_props;
> +    k->hotpluggable = true;
>   }
>   
>   static void scsi_dev_instance_init(Object *obj)
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index d910f84..16701aa 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -793,6 +793,7 @@ static void usb_device_class_init(ObjectClass *klass, void *data)
>       k->realize  = usb_qdev_realize;
>       k->unrealize = usb_qdev_unrealize;
>       k->props    = usb_props;
> +    k->hotpluggable = true;
>   }
>   
>   static const TypeInfo usb_device_type_info = {
> 

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

[Peter Maydell]

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.

thanks
-- PMM

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

[Marcel Apfelbaum]

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
> 

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

[Thomas Huth]

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 <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?

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

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

[Thomas Huth]

On 20.09.2017 13:17, Thomas Huth wrote:
> On 20.09.2017 12:57, Marcel Apfelbaum wrote:
>> On 20/09/2017 13:07, Peter Maydell wrote:
[...]
>>> 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.

Ok, done that now, and it looks like I missed devices of type
TYPE_X86_CPU and TYPE_S390_CPU at least... Unlike the ppc64 CPUs, they
are not derived from TYPE_CPU_CORE, but from TYPE_CPU ... is that on
purpose? I thought hot-pluggable CPUs would need to be derived from
TYPE_CPU_CORE... well, maybe I just understood that wrong.
I'll send a new version of my patch.

 Thomas

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

[David Hildenbrand]

On 20.09.2017 18:10, Thomas Huth wrote:
> On 20.09.2017 13:17, Thomas Huth wrote:
>> On 20.09.2017 12:57, Marcel Apfelbaum wrote:
>>> On 20/09/2017 13:07, Peter Maydell wrote:
> [...]
>>>> 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.
> 
> Ok, done that now, and it looks like I missed devices of type
> TYPE_X86_CPU and TYPE_S390_CPU at least... Unlike the ppc64 CPUs, they
> are not derived from TYPE_CPU_CORE, but from TYPE_CPU ... is that on
> purpose? I thought hot-pluggable CPUs would need to be derived from
> TYPE_CPU_CORE... well, maybe I just understood that wrong.
> I'll send a new version of my patch.
> 
>  Thomas
> 

Guess CORE was introduced for special thread handling on PPC. I am not
aware of that hotplug restriction. And it seems to work just fine :)

-- 

Thanks,

David

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

[Igor Mammedov]

On Wed, 20 Sep 2017 18:10:48 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 20.09.2017 13:17, Thomas Huth wrote:
> > On 20.09.2017 12:57, Marcel Apfelbaum wrote:  
> >> On 20/09/2017 13:07, Peter Maydell wrote:  
> [...]
> >>> 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.  
> 
> Ok, done that now, and it looks like I missed devices of type
> TYPE_X86_CPU and TYPE_S390_CPU at least... Unlike the ppc64 CPUs, they
> are not derived from TYPE_CPU_CORE, but from TYPE_CPU ... is that on
> purpose? I thought hot-pluggable CPUs would need to be derived from
> TYPE_CPU_CORE... well, maybe I just understood that wrong.
> I'll send a new version of my patch.
cpu hotplug on x86 existed before ppc started to support it and
hotplug is done at thread level there. core was introduced later
for ppc as guest expects hotplugged cpu in core granularity.

So as you noticed some leaf CPU types are indeed hotpluggble.


> 
>  Thomas

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

[Anthony PERARD]

On Tue, Sep 19, 2017 at 10:55:53AM +0200, 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>


Hi,

xen-backend is needed to be hotpluggable, otherwise I have this error
message:
qemu-system-i386: Initialization of device xen-backend failed: Device 'xen-backend' does not support hotplugging

Also, when I try to add more cpus:
QMP command: { "execute": "cpu-add", "id": 2, "arguments": { "id": 2 } }
error message: Device 'qemu32-i386-cpu' does not support hotplugging


I've tested all I could think of that would involve hotplug.

Thanks,

-- 
Anthony PERARD

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

[Eduardo Habkost]

On Tue, Sep 19, 2017 at 10:55:53AM +0200, 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.

These seem to be missing:
* TYPE_CPU (or at least TYPE_X86_CPU and TYPE_S390_CPU)
* TYPE_VIRTIO_SERIAL_PORT
* TYPE_CCID_CARD
* TYPE_XENSYSDEV

Also, I don't think we need to set it for TYPE_CPU_CORE, just for
TYPE_SPAPR_CPU_CORE.

For reference, the full list of qbus_set*hotplug_handler() callers is:

hw/acpi/piix4.c=439=static void piix4_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
hw/acpi/piix4.c:445:    qbus_set_hotplug_handler(BUS(pci_bus), DEVICE(s), &error_abort);
hw/char/virtio-serial-bus.c=1015=static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
hw/char/virtio-serial-bus.c:1045:    qbus_set_hotplug_handler(BUS(&vser->bus), DEVICE(vser), errp);
hw/char/virtio-serial-bus.c=1111=static void virtio_serial_device_unrealize(DeviceState *dev, Error **errp)
hw/char/virtio-serial-bus.c:1128:    qbus_set_hotplug_handler(BUS(&vser->bus), NULL, errp);
hw/pci/pcie.c=392=void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot)
hw/pci/pcie.c:445:    qbus_set_hotplug_handler(BUS(pci_bridge_get_sec_bus(PCI_BRIDGE(dev))),
hw/pci/shpc.c=585=int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar,
hw/pci/shpc.c:651:    qbus_set_hotplug_handler(BUS(sec_bus), DEVICE(d), NULL);
hw/ppc/spapr_pci.c=1508=static void spapr_phb_realize(DeviceState *dev, Error **errp)
hw/ppc/spapr_pci.c:1657:    qbus_set_hotplug_handler(BUS(phb->bus), DEVICE(sphb), NULL);
hw/s390x/css-bridge.c=94=VirtualCssBus *virtual_css_bus_init(void)
hw/s390x/css-bridge.c:110:    qbus_set_hotplug_handler(bus, dev, &error_abort);
hw/s390x/s390-pci-bus.c=548=static int s390_pcihost_init(SysBusDevice *dev)
hw/s390x/s390-pci-bus.c:564:    qbus_set_hotplug_handler(bus, DEVICE(dev), NULL);
hw/s390x/s390-pci-bus.c:568:    qbus_set_hotplug_handler(BUS(s->bus), DEVICE(s), NULL);
hw/s390x/s390-pci-bus.c=660=static void s390_pcihost_hot_plug(HotplugHandler *hotplug_dev,
hw/s390x/s390-pci-bus.c:676:        qbus_set_hotplug_handler(bus, DEVICE(s), errp);
hw/scsi/scsi-bus.c=91=void scsi_bus_new(SCSIBus *bus, size_t bus_size, DeviceState *host,
hw/scsi/scsi-bus.c:97:    qbus_set_bus_hotplug_handler(BUS(bus), &error_abort);
hw/scsi/virtio-scsi.c=877=static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)
hw/scsi/virtio-scsi.c:896:    qbus_set_hotplug_handler(BUS(&s->bus), dev, &error_abort);
hw/scsi/virtio-scsi.c=910=static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp)
hw/scsi/virtio-scsi.c:914:    qbus_set_hotplug_handler(BUS(&s->bus), NULL, &error_abort);
hw/scsi/vmw_pvscsi.c=1107=pvscsi_realizefn(PCIDevice *pci_dev, Error **errp)
hw/scsi/vmw_pvscsi.c:1145:    qbus_set_hotplug_handler(BUS(&s->bus), DEVICE(s), &error_abort);
hw/usb/bus.c=88=void usb_bus_new(USBBus *bus, size_t bus_size,
hw/usb/bus.c:92:    qbus_set_bus_hotplug_handler(BUS(bus), &error_abort);
hw/usb/dev-smartcard-reader.c=1332=static void ccid_realize(USBDevice *dev, Error **errp)
hw/usb/dev-smartcard-reader.c:1340:    qbus_set_hotplug_handler(BUS(&s->bus), DEVICE(dev), &error_abort);
hw/xen/xen_backend.c=520=int xen_be_init(void)
hw/xen/xen_backend.c:538:    qbus_set_bus_hotplug_handler(xen_sysbus, &error_abort);

The full list of machine-provided hotplug handler callbacks is:

hw/i386/pc.c:2302:    pcmc->get_hotplug_handler = mc->get_hotplug_handler;
hw/i386/pc.c:2317:    mc->get_hotplug_handler = pc_get_hotpug_handler;
hw/ppc/spapr.c:3582:    mc->get_hotplug_handler = spapr_get_hotplug_handler;
hw/s390x/s390-virtio-ccw.c:446:    mc->get_hotplug_handler = s390_get_hotplug_handler;


Details for each item:

hw/i386/pc.c:2302:    pcmc->get_hotplug_handler = mc->get_hotplug_handler;
hw/i386/pc.c:2317:    mc->get_hotplug_handler = pc_get_hotpug_handler;
pc_get_hotpug_handler accepts:
* TYPE_PC_DIMM
* TYPE_CPU

hw/ppc/spapr.c:3582:    mc->get_hotplug_handler = spapr_get_hotplug_handler;
spapr_get_hotplug_handler accepts:
* TYPE_PC_DIMM
* TYPE_SPAPR_CPU_CORE

hw/s390x/s390-virtio-ccw.c:446:    mc->get_hotplug_handler = s390_get_hotplug_handler;
s390_get_hotplug_handler accepts:
* TYPE_CPU

hw/acpi/piix4.c=439=static void piix4_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
hw/acpi/piix4.c:445:    qbus_set_hotplug_handler(BUS(pci_bus), DEVICE(s), &error_abort);
* It looks like this is set only for PCI buses, but
  piix4_device_plug_cb() handles: TYPE_PC_DIMM, TYPE_NVDIMM,
  TYPE_PCI_DEVICE, and TYPE_CPU.  It isn't clear how
  piix4_device_plug_cb() ends up getting called for non-PCI
  devices.  But this patch handle all of them except TYPE_CPU,
  anyway.

hw/char/virtio-serial-bus.c=1015=static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
hw/char/virtio-serial-bus.c:1045:    qbus_set_hotplug_handler(BUS(&vser->bus), DEVICE(vser), errp);
hw/char/virtio-serial-bus.c=1111=static void virtio_serial_device_unrealize(DeviceState *dev, Error **errp)
hw/char/virtio-serial-bus.c:1128:    qbus_set_hotplug_handler(BUS(&vser->bus), NULL, errp);
* vser->bus is VirtIOSerialBus, which accepts hotplug of
  TYPE_VIRTIO_SERIAL_PORT devices.  Missing from this patch.

hw/pci/pcie.c=392=void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot)
hw/pci/pcie.c:445:    qbus_set_hotplug_handler(BUS(pci_bridge_get_sec_bus(PCI_BRIDGE(dev))),
hw/pci/shpc.c=585=int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar,
hw/pci/shpc.c:651:    qbus_set_hotplug_handler(BUS(sec_bus), DEVICE(d), NULL);
hw/ppc/spapr_pci.c=1508=static void spapr_phb_realize(DeviceState *dev, Error **errp)
hw/ppc/spapr_pci.c:1657:    qbus_set_hotplug_handler(BUS(phb->bus), DEVICE(sphb), NULL);
* These are a bit confusing because the actual
  TYPE_HOTPLUG_HANDLER method implementations are not at the same
  files as the qbus_set_hotplug_handler() calls, but it seems to
  work, and they only affect TYPE_PCI anyway.

hw/s390x/css-bridge.c=94=VirtualCssBus *virtual_css_bus_init(void)
hw/s390x/css-bridge.c:110:    qbus_set_hotplug_handler(bus, dev, &error_abort);
* This seems to be for TYPE_CCW_DEVICE

hw/s390x/s390-pci-bus.c=548=static int s390_pcihost_init(SysBusDevice *dev)
hw/s390x/s390-pci-bus.c:564:    qbus_set_hotplug_handler(bus, DEVICE(dev), NULL);
hw/s390x/s390-pci-bus.c:568:    qbus_set_hotplug_handler(BUS(s->bus), DEVICE(s), NULL);
hw/s390x/s390-pci-bus.c=660=static void s390_pcihost_hot_plug(HotplugHandler *hotplug_dev,
hw/s390x/s390-pci-bus.c:676:        qbus_set_hotplug_handler(bus, DEVICE(s), errp);
* TYPE_PCI only, it seems.

hw/scsi/scsi-bus.c=91=void scsi_bus_new(SCSIBus *bus, size_t bus_size, DeviceState *host,
hw/scsi/scsi-bus.c:97:    qbus_set_bus_hotplug_handler(BUS(bus), &error_abort);
* For TYPE_SCSI_DEVICE

hw/scsi/virtio-scsi.c=877=static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)
hw/scsi/virtio-scsi.c:896:    qbus_set_hotplug_handler(BUS(&s->bus), dev, &error_abort);
hw/scsi/virtio-scsi.c=910=static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp)
hw/scsi/virtio-scsi.c:914:    qbus_set_hotplug_handler(BUS(&s->bus), NULL, &error_abort);
* s->bus is a SCSIBus.  TYPE_SCSI_DEVICE only, it seems.

hw/scsi/vmw_pvscsi.c=1107=pvscsi_realizefn(PCIDevice *pci_dev, Error **errp)
hw/scsi/vmw_pvscsi.c:1145:    qbus_set_hotplug_handler(BUS(&s->bus), DEVICE(s), &error_abort);
* s->bus is a SCSIBus.  TYPE_SCSI_DEVICE only, it seems.

hw/usb/bus.c=88=void usb_bus_new(USBBus *bus, size_t bus_size,
hw/usb/bus.c:92:    qbus_set_bus_hotplug_handler(BUS(bus), &error_abort);
* For TYPE_USB_DEVICE

hw/usb/dev-smartcard-reader.c=1332=static void ccid_realize(USBDevice *dev, Error **errp)
hw/usb/dev-smartcard-reader.c:1340:    qbus_set_hotplug_handler(BUS(&s->bus), DEVICE(dev), &error_abort);
* s->bus is a CCIDBus.  This is for TYPE_CCID_CARD.  Missing from
  this patch.

hw/xen/xen_backend.c=520=int xen_be_init(void)
hw/xen/xen_backend.c:538:    qbus_set_bus_hotplug_handler(xen_sysbus, &error_abort);
* xen_sysbus is a TYPE_XENSYSBUS, which accepts hotplug of
  TYPE_XENSYSDEV devices.  Missing from this patch.





> 
> 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;
>      dc->user_creatable = true;
>  }
>  
> diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> index bd578ab..01660aa 100644
> --- a/hw/cpu/core.c
> +++ b/hw/cpu/core.c
> @@ -82,6 +82,7 @@ static void cpu_core_class_init(ObjectClass *oc, void *data)
>      DeviceClass *dc = DEVICE_CLASS(oc);
>  
>      set_bit(DEVICE_CATEGORY_CPU, dc->categories);
> +    dc->hotpluggable = true;
>  }
>  
>  static const TypeInfo cpu_core_type_info = {
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 952fce5..02be9f3 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -148,9 +148,12 @@ static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
>  
>  static void nvdimm_class_init(ObjectClass *oc, void *data)
>  {
> +    DeviceClass *dc = DEVICE_CLASS(oc);
>      PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
>      NVDIMMClass *nvc = NVDIMM_CLASS(oc);
>  
> +    dc->hotpluggable = true;
> +
>      ddc->realize = nvdimm_realize;
>      ddc->get_memory_region = nvdimm_get_memory_region;
>      ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 66eace5..1f78567 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -443,6 +443,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
>      dc->unrealize = pc_dimm_unrealize;
>      dc->props = pc_dimm_properties;
>      dc->desc = "DIMM memory module";
> +    dc->hotpluggable = true;
>  
>      ddc->get_memory_region = pc_dimm_get_memory_region;
>      ddc->get_vmstate_memory_region = pc_dimm_get_vmstate_memory_region;
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 1e6fb88..8db380d 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2534,6 +2534,7 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
>      k->unrealize = pci_qdev_unrealize;
>      k->bus_type = TYPE_PCI_BUS;
>      k->props = pci_props;
> +    k->hotpluggable = true;
>      pc->realize = pci_default_realize;
>  }
>  
> diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
> index f9bfa15..d1b6e6f 100644
> --- a/hw/s390x/ccw-device.c
> +++ b/hw/s390x/ccw-device.c
> @@ -48,6 +48,7 @@ static void ccw_device_class_init(ObjectClass *klass, void *data)
>      k->realize = ccw_device_realize;
>      k->refill_ids = ccw_device_refill_ids;
>      dc->props = ccw_device_properties;
> +    dc->hotpluggable = true;
>  }
>  
>  const VMStateDescription vmstate_ccw_dev = {
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index e364410..338180d 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -2123,6 +2123,7 @@ static void scsi_device_class_init(ObjectClass *klass, void *data)
>      k->realize   = scsi_qdev_realize;
>      k->unrealize = scsi_qdev_unrealize;
>      k->props     = scsi_props;
> +    k->hotpluggable = true;
>  }
>  
>  static void scsi_dev_instance_init(Object *obj)
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index d910f84..16701aa 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -793,6 +793,7 @@ static void usb_device_class_init(ObjectClass *klass, void *data)
>      k->realize  = usb_qdev_realize;
>      k->unrealize = usb_qdev_unrealize;
>      k->props    = usb_props;
> +    k->hotpluggable = true;
>  }
>  
>  static const TypeInfo usb_device_type_info = {
> -- 
> 1.8.3.1
> 
> 

-- 
Eduardo

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

[Cornelia Huck]

On Thu, 21 Sep 2017 15:50:28 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Sep 19, 2017 at 10:55:53AM +0200, 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.  
> 
> These seem to be missing:
> * TYPE_CPU (or at least TYPE_X86_CPU and TYPE_S390_CPU)

I think it would be better to set it for TYPE_CPU (and have
architectures override if needed).

> * TYPE_VIRTIO_SERIAL_PORT
> * TYPE_CCID_CARD
> * TYPE_XENSYSDEV
> 
> Also, I don't think we need to set it for TYPE_CPU_CORE, just for
> TYPE_SPAPR_CPU_CORE.

> hw/s390x/s390-virtio-ccw.c:446:    mc->get_hotplug_handler = s390_get_hotplug_handler;
> s390_get_hotplug_handler accepts:
> * TYPE_CPU

Nod.

> hw/s390x/css-bridge.c=94=VirtualCssBus *virtual_css_bus_init(void)
> hw/s390x/css-bridge.c:110:    qbus_set_hotplug_handler(bus, dev, &error_abort);
> * This seems to be for TYPE_CCW_DEVICE

Yes, it is.

> 
> hw/s390x/s390-pci-bus.c=548=static int s390_pcihost_init(SysBusDevice *dev)
> hw/s390x/s390-pci-bus.c:564:    qbus_set_hotplug_handler(bus, DEVICE(dev), NULL);
> hw/s390x/s390-pci-bus.c:568:    qbus_set_hotplug_handler(BUS(s->bus), DEVICE(s), NULL);
> hw/s390x/s390-pci-bus.c=660=static void s390_pcihost_hot_plug(HotplugHandler *hotplug_dev,
> hw/s390x/s390-pci-bus.c:676:        qbus_set_hotplug_handler(bus, DEVICE(s), errp);
> * TYPE_PCI only, it seems.

Yes.

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

[Thomas Huth]

On 22.09.2017 09:38, Cornelia Huck wrote:
> On Thu, 21 Sep 2017 15:50:28 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
>> On Tue, Sep 19, 2017 at 10:55:53AM +0200, 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.  
>>
>> These seem to be missing:
>> * TYPE_CPU (or at least TYPE_X86_CPU and TYPE_S390_CPU)
> 
> I think it would be better to set it for TYPE_CPU (and have
> architectures override if needed).

Hmm, no, I think TYPE_CPU should stay non-notpluggable, and we should
only do this for TYPE_X86_CPU, TYPE_S390_CPU and TYPE_SPAPR_CPU_CORE.
Most CPU types are not hot-pluggable, so that should be the default.
Having seen all those "device_add" crashes in the past weeks, I'm afraid
that we'll run into weird problems again otherwise since people will
keep forgetting to add "hotpluggable = false" for new CPU types that are
not hotpluggable.

 Thomas

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

[Thomas Huth]

On 21.09.2017 20:50, Eduardo Habkost wrote:
> On Tue, Sep 19, 2017 at 10:55:53AM +0200, 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.
> 
> These seem to be missing:
> * TYPE_CPU (or at least TYPE_X86_CPU and TYPE_S390_CPU)
> * TYPE_VIRTIO_SERIAL_PORT
> * TYPE_CCID_CARD
> * TYPE_XENSYSDEV

Thanks for the detailed examination, Eduardo! I'll rework my patch
accordingly...

> Also, I don't think we need to set it for TYPE_CPU_CORE, just for
> TYPE_SPAPR_CPU_CORE.

Ok - you're likely right. There is one other consumer of TYPE_CPU_CORE
beside spapr, which is the pnv machine, and as far as I can see, it does
not support CPU hotplugging yet. So it indeed makes more sense to set
this in TYPE_SPAPR_CPU_CORE only.

 Thomas

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

[Cédric Le Goater]

On 09/22/2017 09:47 AM, Thomas Huth wrote:
> On 21.09.2017 20:50, Eduardo Habkost wrote:
>> On Tue, Sep 19, 2017 at 10:55:53AM +0200, 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.
>>
>> These seem to be missing:
>> * TYPE_CPU (or at least TYPE_X86_CPU and TYPE_S390_CPU)
>> * TYPE_VIRTIO_SERIAL_PORT
>> * TYPE_CCID_CARD
>> * TYPE_XENSYSDEV
> 
> Thanks for the detailed examination, Eduardo! I'll rework my patch
> accordingly...
> 
>> Also, I don't think we need to set it for TYPE_CPU_CORE, just for
>> TYPE_SPAPR_CPU_CORE.
> 
> Ok - you're likely right. There is one other consumer of TYPE_CPU_CORE
> beside spapr, which is the pnv machine, and as far as I can see, it does
> not support CPU hotplugging yet. So it indeed makes more sense to set
> this in TYPE_SPAPR_CPU_CORE only.

I don't think pnv will ever support CPU hotplugging. The HW platform 
doesn't but CPU hot unplugging should be as CPU can fail and that
is supported by the firmware and Linux.

But I guess the right decision for pnv now is to just not support
CPU hotplug.

Thanks,

C.