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>
---
 v2: Add missing devices and dropped "RFC" status. See Eduardo's reply on
 the previous version of this patch for the rationale which devices need
 to be hotpluggable:
 https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg06128.html

 hw/char/virtio-serial-bus.c   |  1 +
 hw/core/qdev.c                | 10 ++++------
 hw/mem/nvdimm.c               |  3 +++
 hw/mem/pc-dimm.c              |  1 +
 hw/pci/pci.c                  |  1 +
 hw/ppc/spapr_cpu_core.c       |  1 +
 hw/s390x/ccw-device.c         |  1 +
 hw/scsi/scsi-bus.c            |  1 +
 hw/usb/bus.c                  |  1 +
 hw/usb/dev-smartcard-reader.c |  1 +
 hw/xen/xen_backend.c          |  1 +
 target/i386/cpu.c             |  4 ++--
 target/s390x/cpu.c            |  1 +
 13 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 17a1bb0..da26fc2 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -1097,6 +1097,7 @@ static void virtio_serial_port_class_init(ObjectClass *klass, void *data)
     k->realize = virtser_port_device_realize;
     k->unrealize = virtser_port_device_unrealize;
     k->props = virtser_props;
+    k->hotpluggable = true;
 }
 
 static const TypeInfo virtio_serial_port_type_info = {
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index d9ccce6..28fd92f 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1125,13 +1125,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/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/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index c08ee75..720284e 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -325,6 +325,7 @@ void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
     dc->realize = spapr_cpu_core_realize;
     dc->unrealize = spapr_cpu_core_unrealizefn;
     dc->props = spapr_cpu_core_properties;
+    dc->hotpluggable = true;
     scc->cpu_class = cpu_class_by_name(TYPE_POWERPC_CPU, data);
     g_assert(scc->cpu_class);
 }
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 977f7bc..6703b2a 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1741,6 +1741,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 = {
diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index bef1f03..e28d1ba 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -1499,6 +1499,7 @@ static void ccid_card_class_init(ObjectClass *klass, void *data)
     k->init = ccid_card_init;
     k->exit = ccid_card_exit;
     k->props = ccid_props;
+    k->hotpluggable = true;
 }
 
 static const TypeInfo ccid_card_type_info = {
diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
index c46cbb0..96621fe 100644
--- a/hw/xen/xen_backend.c
+++ b/hw/xen/xen_backend.c
@@ -620,6 +620,7 @@ static void xendev_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     /* xen-backend devices can be plugged/unplugged dynamically */
     dc->user_creatable = true;
+    dc->hotpluggable = true;
 }
 
 static const TypeInfo xendev_type_info = {
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 0aa28fc..122623b 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4170,6 +4170,8 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
     dc->realize = x86_cpu_realizefn;
     dc->unrealize = x86_cpu_unrealizefn;
     dc->props = x86_cpu_properties;
+    dc->user_creatable = true;
+    dc->hotpluggable = true;
 
     xcc->parent_reset = cc->reset;
     cc->reset = x86_cpu_reset;
@@ -4215,8 +4217,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
 #endif
     cc->cpu_exec_enter = x86_cpu_exec_enter;
     cc->cpu_exec_exit = x86_cpu_exec_exit;
-
-    dc->user_creatable = true;
 }
 
 static const TypeInfo x86_cpu_type_info = {
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 34538c3..ef61d19 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -462,6 +462,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     dc->realize = s390_cpu_realizefn;
     dc->props = s390x_cpu_properties;
     dc->user_creatable = true;
+    dc->hotpluggable = true;
 
     scc->parent_reset = cc->reset;
 #if !defined(CONFIG_USER_ONLY)
-- 
1.8.3.1

On Fri, Sep 22, 2017 at 11:16:34AM +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>

Seems reasonable to me.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  v2: Add missing devices and dropped "RFC" status. See Eduardo's reply on
>  the previous version of this patch for the rationale which devices need
>  to be hotpluggable:
>  https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg06128.html
> 
>  hw/char/virtio-serial-bus.c   |  1 +
>  hw/core/qdev.c                | 10 ++++------
>  hw/mem/nvdimm.c               |  3 +++
>  hw/mem/pc-dimm.c              |  1 +
>  hw/pci/pci.c                  |  1 +
>  hw/ppc/spapr_cpu_core.c       |  1 +
>  hw/s390x/ccw-device.c         |  1 +
>  hw/scsi/scsi-bus.c            |  1 +
>  hw/usb/bus.c                  |  1 +
>  hw/usb/dev-smartcard-reader.c |  1 +
>  hw/xen/xen_backend.c          |  1 +
>  target/i386/cpu.c             |  4 ++--
>  target/s390x/cpu.c            |  1 +
>  13 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 17a1bb0..da26fc2 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -1097,6 +1097,7 @@ static void virtio_serial_port_class_init(ObjectClass *klass, void *data)
>      k->realize = virtser_port_device_realize;
>      k->unrealize = virtser_port_device_unrealize;
>      k->props = virtser_props;
> +    k->hotpluggable = true;
>  }
>  
>  static const TypeInfo virtio_serial_port_type_info = {
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index d9ccce6..28fd92f 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1125,13 +1125,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/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/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index c08ee75..720284e 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -325,6 +325,7 @@ void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
>      dc->realize = spapr_cpu_core_realize;
>      dc->unrealize = spapr_cpu_core_unrealizefn;
>      dc->props = spapr_cpu_core_properties;
> +    dc->hotpluggable = true;
>      scc->cpu_class = cpu_class_by_name(TYPE_POWERPC_CPU, data);
>      g_assert(scc->cpu_class);
>  }
> 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 977f7bc..6703b2a 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -1741,6 +1741,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 = {
> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> index bef1f03..e28d1ba 100644
> --- a/hw/usb/dev-smartcard-reader.c
> +++ b/hw/usb/dev-smartcard-reader.c
> @@ -1499,6 +1499,7 @@ static void ccid_card_class_init(ObjectClass *klass, void *data)
>      k->init = ccid_card_init;
>      k->exit = ccid_card_exit;
>      k->props = ccid_props;
> +    k->hotpluggable = true;
>  }
>  
>  static const TypeInfo ccid_card_type_info = {
> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
> index c46cbb0..96621fe 100644
> --- a/hw/xen/xen_backend.c
> +++ b/hw/xen/xen_backend.c
> @@ -620,6 +620,7 @@ static void xendev_class_init(ObjectClass *klass, void *data)
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>      /* xen-backend devices can be plugged/unplugged dynamically */
>      dc->user_creatable = true;
> +    dc->hotpluggable = true;
>  }
>  
>  static const TypeInfo xendev_type_info = {
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 0aa28fc..122623b 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -4170,6 +4170,8 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>      dc->realize = x86_cpu_realizefn;
>      dc->unrealize = x86_cpu_unrealizefn;
>      dc->props = x86_cpu_properties;
> +    dc->user_creatable = true;
> +    dc->hotpluggable = true;
>  
>      xcc->parent_reset = cc->reset;
>      cc->reset = x86_cpu_reset;
> @@ -4215,8 +4217,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>  #endif
>      cc->cpu_exec_enter = x86_cpu_exec_enter;
>      cc->cpu_exec_exit = x86_cpu_exec_exit;
> -
> -    dc->user_creatable = true;
>  }
>  
>  static const TypeInfo x86_cpu_type_info = {
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 34538c3..ef61d19 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -462,6 +462,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>      dc->realize = s390_cpu_realizefn;
>      dc->props = s390x_cpu_properties;
>      dc->user_creatable = true;
> +    dc->hotpluggable = true;
>  
>      scc->parent_reset = cc->reset;
>  #if !defined(CONFIG_USER_ONLY)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

On 22/09/2017 12:16, 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>
> ---
>   v2: Add missing devices and dropped "RFC" status. See Eduardo's reply on
>   the previous version of this patch for the rationale which devices need
>   to be hotpluggable:
>   https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg06128.html
> 
>   hw/char/virtio-serial-bus.c   |  1 +
>   hw/core/qdev.c                | 10 ++++------
>   hw/mem/nvdimm.c               |  3 +++
>   hw/mem/pc-dimm.c              |  1 +
>   hw/pci/pci.c                  |  1 +
>   hw/ppc/spapr_cpu_core.c       |  1 +
>   hw/s390x/ccw-device.c         |  1 +
>   hw/scsi/scsi-bus.c            |  1 +
>   hw/usb/bus.c                  |  1 +
>   hw/usb/dev-smartcard-reader.c |  1 +
>   hw/xen/xen_backend.c          |  1 +
>   target/i386/cpu.c             |  4 ++--
>   target/s390x/cpu.c            |  1 +
>   13 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 17a1bb0..da26fc2 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -1097,6 +1097,7 @@ static void virtio_serial_port_class_init(ObjectClass *klass, void *data)
>       k->realize = virtser_port_device_realize;
>       k->unrealize = virtser_port_device_unrealize;
>       k->props = virtser_props;
> +    k->hotpluggable = true;
>   }
>   
>   static const TypeInfo virtio_serial_port_type_info = {
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index d9ccce6..28fd92f 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1125,13 +1125,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/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/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index c08ee75..720284e 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -325,6 +325,7 @@ void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
>       dc->realize = spapr_cpu_core_realize;
>       dc->unrealize = spapr_cpu_core_unrealizefn;
>       dc->props = spapr_cpu_core_properties;
> +    dc->hotpluggable = true;
>       scc->cpu_class = cpu_class_by_name(TYPE_POWERPC_CPU, data);
>       g_assert(scc->cpu_class);
>   }
> 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 977f7bc..6703b2a 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -1741,6 +1741,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 = {
> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> index bef1f03..e28d1ba 100644
> --- a/hw/usb/dev-smartcard-reader.c
> +++ b/hw/usb/dev-smartcard-reader.c
> @@ -1499,6 +1499,7 @@ static void ccid_card_class_init(ObjectClass *klass, void *data)
>       k->init = ccid_card_init;
>       k->exit = ccid_card_exit;
>       k->props = ccid_props;
> +    k->hotpluggable = true;
>   }
>   
>   static const TypeInfo ccid_card_type_info = {
> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
> index c46cbb0..96621fe 100644
> --- a/hw/xen/xen_backend.c
> +++ b/hw/xen/xen_backend.c
> @@ -620,6 +620,7 @@ static void xendev_class_init(ObjectClass *klass, void *data)
>       set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>       /* xen-backend devices can be plugged/unplugged dynamically */
>       dc->user_creatable = true;
> +    dc->hotpluggable = true;
>   }
>   
>   static const TypeInfo xendev_type_info = {
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 0aa28fc..122623b 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -4170,6 +4170,8 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>       dc->realize = x86_cpu_realizefn;
>       dc->unrealize = x86_cpu_unrealizefn;
>       dc->props = x86_cpu_properties;
> +    dc->user_creatable = true;
> +    dc->hotpluggable = true;
>   
>       xcc->parent_reset = cc->reset;
>       cc->reset = x86_cpu_reset;
> @@ -4215,8 +4217,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>   #endif
>       cc->cpu_exec_enter = x86_cpu_exec_enter;
>       cc->cpu_exec_exit = x86_cpu_exec_exit;
> -
> -    dc->user_creatable = true;
>   }
>   
>   static const TypeInfo x86_cpu_type_info = {
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 34538c3..ef61d19 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -462,6 +462,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>       dc->realize = s390_cpu_realizefn;
>       dc->props = s390x_cpu_properties;
>       dc->user_creatable = true;
> +    dc->hotpluggable = true;
>   
>       scc->parent_reset = cc->reset;
>   #if !defined(CONFIG_USER_ONLY)
> 

For the PCI related code:
     Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

On Fri, 22 Sep 2017 11:16:34 +0200
Thomas Huth <thuth@redhat.com> 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>
> ---
>  v2: Add missing devices and dropped "RFC" status. See Eduardo's reply on
>  the previous version of this patch for the rationale which devices need
>  to be hotpluggable:
>  https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg06128.html
> 
>  hw/char/virtio-serial-bus.c   |  1 +
>  hw/core/qdev.c                | 10 ++++------
>  hw/mem/nvdimm.c               |  3 +++
>  hw/mem/pc-dimm.c              |  1 +
>  hw/pci/pci.c                  |  1 +
>  hw/ppc/spapr_cpu_core.c       |  1 +
>  hw/s390x/ccw-device.c         |  1 +
>  hw/scsi/scsi-bus.c            |  1 +
>  hw/usb/bus.c                  |  1 +
>  hw/usb/dev-smartcard-reader.c |  1 +
>  hw/xen/xen_backend.c          |  1 +
>  target/i386/cpu.c             |  4 ++--
>  target/s390x/cpu.c            |  1 +
>  13 files changed, 19 insertions(+), 8 deletions(-)

Hm, this seems to break hotplug of virtio devices:

(qemu) device_add virtio-net-pci,id=xxx
Device 'virtio-net-device' does not support hotplugging

(qemu) device_add virtio-net-ccw,id=yyy
Device 'virtio-net-device' does not support hotplugging

We probably need to enable hotplug for virtio devices as well?

On Mon, Sep 25, 2017 at 01:53:16PM +0200, Cornelia Huck wrote:
> On Fri, 22 Sep 2017 11:16:34 +0200
> Thomas Huth <thuth@redhat.com> 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>
> > ---
> >  v2: Add missing devices and dropped "RFC" status. See Eduardo's reply on
> >  the previous version of this patch for the rationale which devices need
> >  to be hotpluggable:
> >  https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg06128.html
> > 
> >  hw/char/virtio-serial-bus.c   |  1 +
> >  hw/core/qdev.c                | 10 ++++------
> >  hw/mem/nvdimm.c               |  3 +++
> >  hw/mem/pc-dimm.c              |  1 +
> >  hw/pci/pci.c                  |  1 +
> >  hw/ppc/spapr_cpu_core.c       |  1 +
> >  hw/s390x/ccw-device.c         |  1 +
> >  hw/scsi/scsi-bus.c            |  1 +
> >  hw/usb/bus.c                  |  1 +
> >  hw/usb/dev-smartcard-reader.c |  1 +
> >  hw/xen/xen_backend.c          |  1 +
> >  target/i386/cpu.c             |  4 ++--
> >  target/s390x/cpu.c            |  1 +
> >  13 files changed, 19 insertions(+), 8 deletions(-)
> 
> Hm, this seems to break hotplug of virtio devices:
> 
> (qemu) device_add virtio-net-pci,id=xxx
> Device 'virtio-net-device' does not support hotplugging
> 
> (qemu) device_add virtio-net-ccw,id=yyy
> Device 'virtio-net-device' does not support hotplugging
> 
> We probably need to enable hotplug for virtio devices as well?

I've seen a similar case broken by "[PATCH] hw/core/qdev: Do not
allow hot-plugging without hotplug controller", because we're
blocking creation of devices created internally by other devices,
not just the ones created by device_add.  I need to find the
exact reproducer again.

It would probably simplify things if we move the check for
DeviceClass::hotpluggable to qmp_device_add(), instead of
device_set_realized().

-- 
Eduardo

On Mon, 25 Sep 2017 10:31:53 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Sep 25, 2017 at 01:53:16PM +0200, Cornelia Huck wrote:
> > On Fri, 22 Sep 2017 11:16:34 +0200
> > Thomas Huth <thuth@redhat.com> 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>
> > > ---
> > >  v2: Add missing devices and dropped "RFC" status. See Eduardo's reply on
> > >  the previous version of this patch for the rationale which devices need
> > >  to be hotpluggable:
> > >  https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg06128.html
> > > 
> > >  hw/char/virtio-serial-bus.c   |  1 +
> > >  hw/core/qdev.c                | 10 ++++------
> > >  hw/mem/nvdimm.c               |  3 +++
> > >  hw/mem/pc-dimm.c              |  1 +
> > >  hw/pci/pci.c                  |  1 +
> > >  hw/ppc/spapr_cpu_core.c       |  1 +
> > >  hw/s390x/ccw-device.c         |  1 +
> > >  hw/scsi/scsi-bus.c            |  1 +
> > >  hw/usb/bus.c                  |  1 +
> > >  hw/usb/dev-smartcard-reader.c |  1 +
> > >  hw/xen/xen_backend.c          |  1 +
> > >  target/i386/cpu.c             |  4 ++--
> > >  target/s390x/cpu.c            |  1 +
> > >  13 files changed, 19 insertions(+), 8 deletions(-)  
> > 
> > Hm, this seems to break hotplug of virtio devices:
> > 
> > (qemu) device_add virtio-net-pci,id=xxx
> > Device 'virtio-net-device' does not support hotplugging
> > 
> > (qemu) device_add virtio-net-ccw,id=yyy
> > Device 'virtio-net-device' does not support hotplugging
> > 
> > We probably need to enable hotplug for virtio devices as well?  
> 
> I've seen a similar case broken by "[PATCH] hw/core/qdev: Do not
> allow hot-plugging without hotplug controller", because we're
> blocking creation of devices created internally by other devices,
> not just the ones created by device_add.  I need to find the
> exact reproducer again.
> 
> It would probably simplify things if we move the check for
> DeviceClass::hotpluggable to qmp_device_add(), instead of
> device_set_realized().
if we would have to do that, than we are probably doing
something wrong and not using property right. Perhaps we
should fix property instead of trying find a place to check
it where it would hurt less.

On Mon, Sep 25, 2017 at 03:46:47PM +0200, Igor Mammedov wrote:
> On Mon, 25 Sep 2017 10:31:53 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Sep 25, 2017 at 01:53:16PM +0200, Cornelia Huck wrote:
> > > On Fri, 22 Sep 2017 11:16:34 +0200
> > > Thomas Huth <thuth@redhat.com> 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>
> > > > ---
> > > >  v2: Add missing devices and dropped "RFC" status. See Eduardo's reply on
> > > >  the previous version of this patch for the rationale which devices need
> > > >  to be hotpluggable:
> > > >  https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg06128.html
> > > > 
> > > >  hw/char/virtio-serial-bus.c   |  1 +
> > > >  hw/core/qdev.c                | 10 ++++------
> > > >  hw/mem/nvdimm.c               |  3 +++
> > > >  hw/mem/pc-dimm.c              |  1 +
> > > >  hw/pci/pci.c                  |  1 +
> > > >  hw/ppc/spapr_cpu_core.c       |  1 +
> > > >  hw/s390x/ccw-device.c         |  1 +
> > > >  hw/scsi/scsi-bus.c            |  1 +
> > > >  hw/usb/bus.c                  |  1 +
> > > >  hw/usb/dev-smartcard-reader.c |  1 +
> > > >  hw/xen/xen_backend.c          |  1 +
> > > >  target/i386/cpu.c             |  4 ++--
> > > >  target/s390x/cpu.c            |  1 +
> > > >  13 files changed, 19 insertions(+), 8 deletions(-)  
> > > 
> > > Hm, this seems to break hotplug of virtio devices:
> > > 
> > > (qemu) device_add virtio-net-pci,id=xxx
> > > Device 'virtio-net-device' does not support hotplugging
> > > 
> > > (qemu) device_add virtio-net-ccw,id=yyy
> > > Device 'virtio-net-device' does not support hotplugging
> > > 
> > > We probably need to enable hotplug for virtio devices as well?  
> > 
> > I've seen a similar case broken by "[PATCH] hw/core/qdev: Do not
> > allow hot-plugging without hotplug controller", because we're
> > blocking creation of devices created internally by other devices,
> > not just the ones created by device_add.  I need to find the
> > exact reproducer again.
> > 
> > It would probably simplify things if we move the check for
> > DeviceClass::hotpluggable to qmp_device_add(), instead of
> > device_set_realized().
> if we would have to do that, than we are probably doing
> something wrong and not using property right. Perhaps we
> should fix property instead of trying find a place to check
> it where it would hurt less.

I disagree.  It depends on the purpose and semantics we define
for DeviceClass::hotpluggable.  My goal is to have a reliable way
to tell the user if a device really supports device_add, and I
think it wouldn't make sense to report hotpluggable=true on a
device that is never instantiated directly by the user and only
used internally.

-- 
Eduardo

On 25.09.2017 16:34, Eduardo Habkost wrote:
> On Mon, Sep 25, 2017 at 03:46:47PM +0200, Igor Mammedov wrote:
>> On Mon, 25 Sep 2017 10:31:53 -0300
>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>
>>> On Mon, Sep 25, 2017 at 01:53:16PM +0200, Cornelia Huck wrote:
>>>> On Fri, 22 Sep 2017 11:16:34 +0200
>>>> Thomas Huth <thuth@redhat.com> 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>
>>>>> ---
>>>>>  v2: Add missing devices and dropped "RFC" status. See Eduardo's reply on
>>>>>  the previous version of this patch for the rationale which devices need
>>>>>  to be hotpluggable:
>>>>>  https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg06128.html
>>>>>
>>>>>  hw/char/virtio-serial-bus.c   |  1 +
>>>>>  hw/core/qdev.c                | 10 ++++------
>>>>>  hw/mem/nvdimm.c               |  3 +++
>>>>>  hw/mem/pc-dimm.c              |  1 +
>>>>>  hw/pci/pci.c                  |  1 +
>>>>>  hw/ppc/spapr_cpu_core.c       |  1 +
>>>>>  hw/s390x/ccw-device.c         |  1 +
>>>>>  hw/scsi/scsi-bus.c            |  1 +
>>>>>  hw/usb/bus.c                  |  1 +
>>>>>  hw/usb/dev-smartcard-reader.c |  1 +
>>>>>  hw/xen/xen_backend.c          |  1 +
>>>>>  target/i386/cpu.c             |  4 ++--
>>>>>  target/s390x/cpu.c            |  1 +
>>>>>  13 files changed, 19 insertions(+), 8 deletions(-)  
>>>>
>>>> Hm, this seems to break hotplug of virtio devices:
>>>>
>>>> (qemu) device_add virtio-net-pci,id=xxx
>>>> Device 'virtio-net-device' does not support hotplugging
>>>>
>>>> (qemu) device_add virtio-net-ccw,id=yyy
>>>> Device 'virtio-net-device' does not support hotplugging
>>>>
>>>> We probably need to enable hotplug for virtio devices as well?  

D'oh, I only checked "normal" PCI devices, not the ones like virtio
devices that consist of two devices internally :-/

>>> I've seen a similar case broken by "[PATCH] hw/core/qdev: Do not
>>> allow hot-plugging without hotplug controller", because we're
>>> blocking creation of devices created internally by other devices,
>>> not just the ones created by device_add.  I need to find the
>>> exact reproducer again.
>>>
>>> It would probably simplify things if we move the check for
>>> DeviceClass::hotpluggable to qmp_device_add(), instead of
>>> device_set_realized().
>> if we would have to do that, than we are probably doing
>> something wrong and not using property right. Perhaps we
>> should fix property instead of trying find a place to check
>> it where it would hurt less.
> 
> I disagree.  It depends on the purpose and semantics we define
> for DeviceClass::hotpluggable.  My goal is to have a reliable way
> to tell the user if a device really supports device_add, and I
> think it wouldn't make sense to report hotpluggable=true on a
> device that is never instantiated directly by the user and only
> used internally.

I think an alternative was would maybe be to check for "user_creatable"
in device_set_realized() in addition to "hotpluggable": If
user_creatable is false, we can be sure that it is an internal plugging
only. Not sure whether this works for the virtio-xxx-device devices,
though, since they are marked as user_creatable = true currently...

 Thomas

On 25 September 2017 at 16:19, Thomas Huth <thuth@redhat.com> wrote:
> Not sure whether this works for the virtio-xxx-device devices,
> though, since they are marked as user_creatable = true currently...

That's deliberate -- for the arm boards with virtio-mmio
the board creates a bunch of virtio-mmio transports and the
virtio-foo-device can be user created to plug into those.

If overall trying to do the 'right thing' is tricky here
then perhaps we can start by flipping the default to
not-hotpluggable and marking some devices hotpluggable
that in theory shouldn't be with a comment about why.

Incidentally I think there's still some advantage in the
"created as part of some other device" devices having
to be explicitly marked hotpluggable, because those
devices do still need some specific code in them to
handle it (ie code to release the resources that are
created in the device's realize method).

thanks
-- PMM

On 25.09.2017 17:26, Peter Maydell wrote:
> On 25 September 2017 at 16:19, Thomas Huth <thuth@redhat.com> wrote:
>> Not sure whether this works for the virtio-xxx-device devices,
>> though, since they are marked as user_creatable = true currently...
> 
> That's deliberate -- for the arm boards with virtio-mmio
> the board creates a bunch of virtio-mmio transports and the
> virtio-foo-device can be user created to plug into those.

Yes, I know ... I'm just wondering whether the virtio-xxx-device devices
should be non-user_creatable on the non-ARM targets, since they
apparently can't be used with "-device" there...?

> If overall trying to do the 'right thing' is tricky here
> then perhaps we can start by flipping the default to
> not-hotpluggable and marking some devices hotpluggable
> that in theory shouldn't be with a comment about why.

Yes, if Eduardo's idea to move the test to qmp_device_add() does not
work out (I'll try that next), your suggestion is certainly the best
thing we can do right now.

> Incidentally I think there's still some advantage in the
> "created as part of some other device" devices having
> to be explicitly marked hotpluggable, because those
> devices do still need some specific code in them to
> handle it (ie code to release the resources that are
> created in the device's realize method).

Ok ... by the way, does anybody know more devices like
virtio-xxx-device, i.e. devices that are indirectly plugged when adding
other devices?

 Thomas

On 25 September 2017 at 18:42, Thomas Huth <thuth@redhat.com> wrote:
> On 25.09.2017 17:26, Peter Maydell wrote:
>> On 25 September 2017 at 16:19, Thomas Huth <thuth@redhat.com> wrote:
>>> Not sure whether this works for the virtio-xxx-device devices,
>>> though, since they are marked as user_creatable = true currently...
>>
>> That's deliberate -- for the arm boards with virtio-mmio
>> the board creates a bunch of virtio-mmio transports and the
>> virtio-foo-device can be user created to plug into those.
>
> Yes, I know ... I'm just wondering whether the virtio-xxx-device devices
> should be non-user_creatable on the non-ARM targets, since they
> apparently can't be used with "-device" there...?

You should be able to on the command line for x86 do something
like -device virtio-pci,... -device virtio-foo-device,...
to manually create the pci transport and the backend.

thanks
-- PMM

On Mon, Sep 25, 2017 at 06:45:15PM +0100, Peter Maydell wrote:
> On 25 September 2017 at 18:42, Thomas Huth <thuth@redhat.com> wrote:
> > On 25.09.2017 17:26, Peter Maydell wrote:
> >> On 25 September 2017 at 16:19, Thomas Huth <thuth@redhat.com> wrote:
> >>> Not sure whether this works for the virtio-xxx-device devices,
> >>> though, since they are marked as user_creatable = true currently...
> >>
> >> That's deliberate -- for the arm boards with virtio-mmio
> >> the board creates a bunch of virtio-mmio transports and the
> >> virtio-foo-device can be user created to plug into those.
> >
> > Yes, I know ... I'm just wondering whether the virtio-xxx-device devices
> > should be non-user_creatable on the non-ARM targets, since they
> > apparently can't be used with "-device" there...?
> 
> You should be able to on the command line for x86 do something
> like -device virtio-pci,... -device virtio-foo-device,...
> to manually create the pci transport and the backend.

virtio-pci is abstract, so this is not possible.  (The same
applies to virtio-ccw-device).

-- 
Eduardo

On 25 September 2017 at 18:51, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Sep 25, 2017 at 06:45:15PM +0100, Peter Maydell wrote:
>> You should be able to on the command line for x86 do something
>> like -device virtio-pci,... -device virtio-foo-device,...
>> to manually create the pci transport and the backend.
>
> virtio-pci is abstract, so this is not possible.  (The same
> applies to virtio-ccw-device).

Did I use the wrong device name? I meant the transport
layer device (which virtio-pci-blk creates along with
virtio-blk-device internally), not the abstract device
that's a base class for the pci devices.

thanks
-- PMM

On Mon, Sep 25, 2017 at 07:02:06PM +0100, Peter Maydell wrote:
> On 25 September 2017 at 18:51, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Mon, Sep 25, 2017 at 06:45:15PM +0100, Peter Maydell wrote:
> >> You should be able to on the command line for x86 do something
> >> like -device virtio-pci,... -device virtio-foo-device,...
> >> to manually create the pci transport and the backend.
> >
> > virtio-pci is abstract, so this is not possible.  (The same
> > applies to virtio-ccw-device).
> 
> Did I use the wrong device name? I meant the transport
> layer device (which virtio-pci-blk creates along with
> virtio-blk-device internally), not the abstract device
> that's a base class for the pci devices.

AFAIK, virtio-pci/virtio-pci-blk itself is the transport layer
device.  Internally, it creates two objects: a virtio-pci-bus
(which is a 1-device bus, not creatable using -device), and a
virtio-blk-device attached to that bus.

-- 
Eduardo

On 25 September 2017 at 19:20, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Sep 25, 2017 at 07:02:06PM +0100, Peter Maydell wrote:
>> On 25 September 2017 at 18:51, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > On Mon, Sep 25, 2017 at 06:45:15PM +0100, Peter Maydell wrote:
>> >> You should be able to on the command line for x86 do something
>> >> like -device virtio-pci,... -device virtio-foo-device,...
>> >> to manually create the pci transport and the backend.
>> >
>> > virtio-pci is abstract, so this is not possible.  (The same
>> > applies to virtio-ccw-device).
>>
>> Did I use the wrong device name? I meant the transport
>> layer device (which virtio-pci-blk creates along with
>> virtio-blk-device internally), not the abstract device
>> that's a base class for the pci devices.
>
> AFAIK, virtio-pci/virtio-pci-blk itself is the transport layer
> device.  Internally, it creates two objects: a virtio-pci-bus
> (which is a 1-device bus, not creatable using -device), and a
> virtio-blk-device attached to that bus.

Hmm, I thought the way we structured this was that
virtio-pci-blk is the convenience wrapper device,
which creates both the endpoint device (virtio-blk-device)
and the transport device (which is the thing that
has a PCI bus interface on one end and a virtio bus
on the other). But maybe we didn't restructure the
pci virtio devices to do that...

thanks
-- PMM

On Mon, Sep 25, 2017 at 07:42:13PM +0200, Thomas Huth wrote:
> On 25.09.2017 17:26, Peter Maydell wrote:
> > On 25 September 2017 at 16:19, Thomas Huth <thuth@redhat.com> wrote:
> >> Not sure whether this works for the virtio-xxx-device devices,
> >> though, since they are marked as user_creatable = true currently...
> > 
> > That's deliberate -- for the arm boards with virtio-mmio
> > the board creates a bunch of virtio-mmio transports and the
> > virtio-foo-device can be user created to plug into those.
> 
> Yes, I know ... I'm just wondering whether the virtio-xxx-device devices
> should be non-user_creatable on the non-ARM targets, since they
> apparently can't be used with "-device" there...?

I wouldn't like to make DeviceClass fields depend on the target.
Being user-creatable doesn't mean they will work on all machines,
anyway.  If user/management need more specific info, they need
something like the query-device-slots command I've proposed some
time ago.

> 
> > If overall trying to do the 'right thing' is tricky here
> > then perhaps we can start by flipping the default to
> > not-hotpluggable and marking some devices hotpluggable
> > that in theory shouldn't be with a comment about why.
> 
> Yes, if Eduardo's idea to move the test to qmp_device_add() does not
> work out (I'll try that next), your suggestion is certainly the best
> thing we can do right now.

I think it would work, but we would lose the feature Peter
mentions below:

> 
> > Incidentally I think there's still some advantage in the
> > "created as part of some other device" devices having
> > to be explicitly marked hotpluggable, because those
> > devices do still need some specific code in them to
> > handle it (ie code to release the resources that are
> > created in the device's realize method).
> 
> Ok ... by the way, does anybody know more devices like
> virtio-xxx-device, i.e. devices that are indirectly plugged when adding
> other devices?

"make check" found a few candidates:
https://travis-ci.org/ehabkost/qemu/jobs/278743999

  Initialization of device dpcd failed: Device 'dpcd' does not support hotplugging
[...]
  Initialization of device nand failed: Device 'nand' does not support hotplugging

Finding the full list of devices that can be instantiated
internally at hotplug-time sounds tricky.

-- 
Eduardo

On 25 September 2017 at 18:59, Eduardo Habkost <ehabkost@redhat.com> wrote:
> Finding the full list of devices that can be instantiated
> internally at hotplug-time sounds tricky.

If we just diff "list of devices marked hotplug before this patch"
against "list of devices marked hotplug after this patch" how
big is the list? Can we just eyeball it to see what needs
to be specialcased?

thanks
-- PMM

On Mon, Sep 25, 2017 at 07:05:26PM +0100, Peter Maydell wrote:
> On 25 September 2017 at 18:59, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > Finding the full list of devices that can be instantiated
> > internally at hotplug-time sounds tricky.
> 
> If we just diff "list of devices marked hotplug before this patch"
> against "list of devices marked hotplug after this patch" how
> big is the list? Can we just eyeball it to see what needs
> to be specialcased?

I was working on a query-device-type QMP command some time ago.
I will rebase the work in progress and use it to build those
lists.

-- 
Eduardo

On Mon, Sep 25, 2017 at 07:05:26PM +0100, Peter Maydell wrote:
> On 25 September 2017 at 18:59, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > Finding the full list of devices that can be instantiated
> > internally at hotplug-time sounds tricky.
> 
> If we just diff "list of devices marked hotplug before this patch"
> against "list of devices marked hotplug after this patch" how
> big is the list? Can we just eyeball it to see what needs
> to be specialcased?

So, the full list quite big, ~1800 device types are affected by
this patch:

https://gist.github.com/ehabkost/bd8e25c6811ac81d947ad8ad5b557f5c#file-dev-types-diff-json

If we ignore the "-cpu" classes, there ~640 affected device
types.

However, if we look only at the direct children of TYPE_DEVICE,
we have:

$ grep '"parent": "device"' dev-types-diff.json 
-{"return": {"abstract": true, "name": "adb-device", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"abstract": true, "name": "apic-common", "parent": "device", "user-creatable": false, "hotpluggable": true}}
-{"return": {"abstract": true, "name": "aspeed-soc", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"abstract": true, "name": "aux-slave", "parent": "device", "user-creatable": true, "hotpluggable": true}}
+{"return": {"abstract": true, "name": "adb-device", "parent": "device", "user-creatable": true, "hotpluggable": false}}
+{"return": {"abstract": true, "name": "apic-common", "parent": "device", "user-creatable": false, "hotpluggable": false}}
+{"return": {"abstract": true, "name": "aspeed-soc", "parent": "device", "user-creatable": true, "hotpluggable": false}}
+{"return": {"abstract": true, "name": "aux-slave", "parent": "device", "user-creatable": true, "hotpluggable": false}}
 {"return": {"abstract": true, "name": "ccid-card", "parent": "device", "user-creatable": true, "hotpluggable": true}}
 {"return": {"abstract": true, "name": "ccw-device", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"abstract": true, "name": "cpu-core", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"abstract": true, "name": "cpu", "parent": "device", "user-creatable": false, "hotpluggable": true}}
+{"return": {"abstract": true, "name": "cpu-core", "parent": "device", "user-creatable": true, "hotpluggable": false}}
+{"return": {"abstract": true, "name": "cpu", "parent": "device", "user-creatable": false, "hotpluggable": false}}
-{"return": {"abstract": true, "name": "hda-codec", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"abstract": true, "name": "i2c-slave", "parent": "device", "user-creatable": true, "hotpluggable": true}}
+{"return": {"abstract": true, "name": "hda-codec", "parent": "device", "user-creatable": true, "hotpluggable": false}}
+{"return": {"abstract": true, "name": "i2c-slave", "parent": "device", "user-creatable": true, "hotpluggable": false}}
-{"return": {"abstract": true, "name": "ics-base", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"abstract": true, "name": "ide-device", "parent": "device", "user-creatable": true, "hotpluggable": true}}
+{"return": {"abstract": true, "name": "ics-base", "parent": "device", "user-creatable": true, "hotpluggable": false}}
+{"return": {"abstract": true, "name": "ide-device", "parent": "device", "user-creatable": true, "hotpluggable": false}}
-{"return": {"abstract": true, "name": "ipack-device", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"abstract": true, "name": "ipmi-bmc", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"abstract": true, "name": "isa-device", "parent": "device", "user-creatable": true, "hotpluggable": true}}
+{"return": {"abstract": true, "name": "ipack-device", "parent": "device", "user-creatable": true, "hotpluggable": false}}
+{"return": {"abstract": true, "name": "ipmi-bmc", "parent": "device", "user-creatable": true, "hotpluggable": false}}
+{"return": {"abstract": true, "name": "isa-device", "parent": "device", "user-creatable": true, "hotpluggable": false}}
 {"return": {"abstract": true, "name": "pci-device", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"abstract": true, "name": "pcmcia-card", "parent": "device", "user-creatable": true, "hotpluggable": true}}
+{"return": {"abstract": true, "name": "pcmcia-card", "parent": "device", "user-creatable": true, "hotpluggable": false}}
-{"return": {"abstract": true, "name": "s390-sclp-event-type", "parent": "device", "user-creatable": true, "hotpluggable": true}}
+{"return": {"abstract": true, "name": "s390-sclp-event-type", "parent": "device", "user-creatable": true, "hotpluggable": false}}
 {"return": {"abstract": true, "name": "s390-skeys", "parent": "device", "user-creatable": true, "hotpluggable": false}}
 {"return": {"abstract": true, "name": "s390-storage_attributes", "parent": "device", "user-creatable": true, "hotpluggable": false}}
 {"return": {"abstract": true, "name": "scsi-device", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"abstract": true, "name": "spapr-dr-connector", "parent": "device", "user-creatable": false, "hotpluggable": true}}
-{"return": {"abstract": true, "name": "ssi-slave", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"abstract": true, "name": "sys-bus-device", "parent": "device", "user-creatable": false, "hotpluggable": true}}
+{"return": {"abstract": true, "name": "spapr-dr-connector", "parent": "device", "user-creatable": false, "hotpluggable": false}}
+{"return": {"abstract": true, "name": "ssi-slave", "parent": "device", "user-creatable": true, "hotpluggable": false}}
+{"return": {"abstract": true, "name": "sys-bus-device", "parent": "device", "user-creatable": false, "hotpluggable": false}}
 {"return": {"abstract": true, "name": "usb-device", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"abstract": true, "name": "vio-spapr-device", "parent": "device", "user-creatable": true, "hotpluggable": true}}
+{"return": {"abstract": true, "name": "vio-spapr-device", "parent": "device", "user-creatable": true, "hotpluggable": false}}
-{"return": {"abstract": true, "name": "virtio-device", "parent": "device", "user-creatable": true, "hotpluggable": true}}
+{"return": {"abstract": true, "name": "virtio-device", "parent": "device", "user-creatable": true, "hotpluggable": false}}
 {"return": {"abstract": true, "name": "virtio-serial-port", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"name": "allwinner-a10", "parent": "device", "user-creatable": false, "hotpluggable": true}}
+{"return": {"name": "allwinner-a10", "parent": "device", "user-creatable": false, "hotpluggable": false}}
-{"return": {"name": "aux-to-i2c-bridge", "parent": "device", "user-creatable": true, "hotpluggable": true}}
+{"return": {"name": "aux-to-i2c-bridge", "parent": "device", "user-creatable": true, "hotpluggable": false}}
 {"return": {"name": "diag288", "parent": "device", "user-creatable": true, "hotpluggable": false}}
-{"return": {"name": "digic", "parent": "device", "user-creatable": false, "hotpluggable": true}}
+{"return": {"name": "digic", "parent": "device", "user-creatable": false, "hotpluggable": false}}
-{"return": {"name": "floppy", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"name": "fsl,imx25", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"name": "fsl,imx31", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"name": "fsl,imx6", "parent": "device", "user-creatable": true, "hotpluggable": true}}
+{"return": {"name": "floppy", "parent": "device", "user-creatable": true, "hotpluggable": false}}
+{"return": {"name": "fsl,imx25", "parent": "device", "user-creatable": true, "hotpluggable": false}}
+{"return": {"name": "fsl,imx31", "parent": "device", "user-creatable": true, "hotpluggable": false}}
+{"return": {"name": "fsl,imx6", "parent": "device", "user-creatable": true, "hotpluggable": false}}
-{"return": {"name": "icp", "parent": "device", "user-creatable": true, "hotpluggable": true}}
+{"return": {"name": "icp", "parent": "device", "user-creatable": true, "hotpluggable": false}}
-{"return": {"name": "loader", "parent": "device", "user-creatable": true, "hotpluggable": true}}
+{"return": {"name": "loader", "parent": "device", "user-creatable": true, "hotpluggable": false}}
-{"return": {"name": "migration", "parent": "device", "user-creatable": false, "hotpluggable": true}}
-{"return": {"name": "mmio_interface", "parent": "device", "user-creatable": false, "hotpluggable": true}}
+{"return": {"name": "migration", "parent": "device", "user-creatable": false, "hotpluggable": false}}
+{"return": {"name": "mmio_interface", "parent": "device", "user-creatable": false, "hotpluggable": false}}
-{"return": {"name": "nand", "parent": "device", "user-creatable": true, "hotpluggable": true}}
+{"return": {"name": "nand", "parent": "device", "user-creatable": true, "hotpluggable": false}}
-{"return": {"name": "or-irq", "parent": "device", "user-creatable": false, "hotpluggable": true}}
+{"return": {"name": "or-irq", "parent": "device", "user-creatable": false, "hotpluggable": false}}
 {"return": {"name": "pc-dimm", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"name": "pnv-lpc", "parent": "device", "user-creatable": true, "hotpluggable": true}}
-{"return": {"name": "pnv-occ", "parent": "device", "user-creatable": true, "hotpluggable": true}}
+{"return": {"name": "pnv-lpc", "parent": "device", "user-creatable": true, "hotpluggable": false}}
+{"return": {"name": "pnv-occ", "parent": "device", "user-creatable": true, "hotpluggable": false}}
-{"return": {"name": "qemu,register", "parent": "device", "user-creatable": false, "hotpluggable": true}}
+{"return": {"name": "qemu,register", "parent": "device", "user-creatable": false, "hotpluggable": false}}
-{"return": {"name": "s390-ipl", "parent": "device", "user-creatable": false, "hotpluggable": true}}
+{"return": {"name": "s390-ipl", "parent": "device", "user-creatable": false, "hotpluggable": false}}
 {"return": {"name": "sclp", "parent": "device", "user-creatable": true, "hotpluggable": false}}
-{"return": {"name": "sd-card", "parent": "device", "user-creatable": true, "hotpluggable": true}}
+{"return": {"name": "sd-card", "parent": "device", "user-creatable": true, "hotpluggable": false}}
 {"return": {"name": "spapr-rng", "parent": "device", "user-creatable": true, "hotpluggable": false}}
-{"return": {"name": "spapr-rtc", "parent": "device", "user-creatable": false, "hotpluggable": true}}
-{"return": {"name": "spapr-tce-table", "parent": "device", "user-creatable": false, "hotpluggable": true}}
+{"return": {"name": "spapr-rtc", "parent": "device", "user-creatable": false, "hotpluggable": false}}
+{"return": {"name": "spapr-tce-table", "parent": "device", "user-creatable": false, "hotpluggable": false}}
 {"return": {"name": "vmgenid", "parent": "device", "user-creatable": true, "hotpluggable": false}}
-{"return": {"name": "xlnx,zynqmp", "parent": "device", "user-creatable": true, "hotpluggable": true}}
+{"return": {"name": "xlnx,zynqmp", "parent": "device", "user-creatable": true, "hotpluggable": false}}
-{"return": {"name": "zpci", "parent": "device", "user-creatable": true, "hotpluggable": true}}
+{"return": {"name": "zpci", "parent": "device", "user-creatable": true, "hotpluggable": false}}

-- 
Eduardo

On 26.09.2017 04:59, Eduardo Habkost wrote:
> On Mon, Sep 25, 2017 at 07:05:26PM +0100, Peter Maydell wrote:
>> On 25 September 2017 at 18:59, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>> Finding the full list of devices that can be instantiated
>>> internally at hotplug-time sounds tricky.
>>
>> If we just diff "list of devices marked hotplug before this patch"
>> against "list of devices marked hotplug after this patch" how
>> big is the list? Can we just eyeball it to see what needs
>> to be specialcased?
> 
> So, the full list quite big, ~1800 device types are affected by
> this patch:
> 
> https://gist.github.com/ehabkost/bd8e25c6811ac81d947ad8ad5b557f5c#file-dev-types-diff-json
> 
> If we ignore the "-cpu" classes, there ~640 affected device
> types.
> 
> However, if we look only at the direct children of TYPE_DEVICE,
> we have:

OK, thanks a lot for that list! But do you think that we can assume that
the devices which are not direct children of TYPE_DEVICE can not be
hotplugged internally during runtime? ... I currently don't think so
(but at least they are good candidates which need to be examined more
carefully).

But anyway, how can we continue here? Set hotpluggable = true on all 640
(or even all 1800) affected devices? That sounds very, very ugly to me.
Maybe we should just do it for the virtio-*-device devices and the
others that break during "make check" (btw. sorry for not noticing this
... I normally run "make check" regularly, but this time I apparently
missed to run it for aarch64), and if we later notice some more
problems, we know that we lack a "make check" test for that case, too,
and we should add such a test? That would at least eventually improve
our test coverage a little bit, I guess...

 Thomas

On 26 September 2017 at 03:59, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Sep 25, 2017 at 07:05:26PM +0100, Peter Maydell wrote:
>> On 25 September 2017 at 18:59, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> If we just diff "list of devices marked hotplug before this patch"
>> against "list of devices marked hotplug after this patch" how
>> big is the list? Can we just eyeball it to see what needs
>> to be specialcased?
>
> So, the full list quite big, ~1800 device types are affected by
> this patch:

Thanks for producing the list. These arm ones definitely shouldn't
be hotpluggable, certainly:

aspeed-soc, allwinner-a10, fsl,imx21, fsl,imx6, fsl,imx25, fsl,imx31,
xlnx,zynqmp, digic

thanks
-- PMM

On 25.09.2017 19:59, Eduardo Habkost wrote:
> On Mon, Sep 25, 2017 at 07:42:13PM +0200, Thomas Huth wrote:
>> On 25.09.2017 17:26, Peter Maydell wrote:
>>> On 25 September 2017 at 16:19, Thomas Huth <thuth@redhat.com> wrote:
>>>> Not sure whether this works for the virtio-xxx-device devices,
>>>> though, since they are marked as user_creatable = true currently...
>>>
>>> That's deliberate -- for the arm boards with virtio-mmio
>>> the board creates a bunch of virtio-mmio transports and the
>>> virtio-foo-device can be user created to plug into those.
>>
>> Yes, I know ... I'm just wondering whether the virtio-xxx-device devices
>> should be non-user_creatable on the non-ARM targets, since they
>> apparently can't be used with "-device" there...?
> 
> I wouldn't like to make DeviceClass fields depend on the target.
> Being user-creatable doesn't mean they will work on all machines,
> anyway.  If user/management need more specific info, they need
> something like the query-device-slots command I've proposed some
> time ago.
> 
>>
>>> If overall trying to do the 'right thing' is tricky here
>>> then perhaps we can start by flipping the default to
>>> not-hotpluggable and marking some devices hotpluggable
>>> that in theory shouldn't be with a comment about why.
>>
>> Yes, if Eduardo's idea to move the test to qmp_device_add() does not
>> work out (I'll try that next), your suggestion is certainly the best
>> thing we can do right now.
> 
> I think it would work, but we would lose the feature Peter
> mentions below:
> 
>>
>>> Incidentally I think there's still some advantage in the
>>> "created as part of some other device" devices having
>>> to be explicitly marked hotpluggable, because those
>>> devices do still need some specific code in them to
>>> handle it (ie code to release the resources that are
>>> created in the device's realize method).
>>
>> Ok ... by the way, does anybody know more devices like
>> virtio-xxx-device, i.e. devices that are indirectly plugged when adding
>> other devices?
> 
> "make check" found a few candidates:
> https://travis-ci.org/ehabkost/qemu/jobs/278743999
> 
>   Initialization of device dpcd failed: Device 'dpcd' does not support hotplugging
> [...]
>   Initialization of device nand failed: Device 'nand' does not support hotplugging

I've now had a look at those, and I now feel like the whole
"hotpluggable = false by default" idea was either just wrong or there
are other smart ideas necessary to solve this issue: These devices are
created during the instance_init() function of another device, e.g.
"dpcd" is created in the xlnx_dp_init() function, which is the
instance_init of TYPE_XLNX_DP. Now, instance_init() can be called at any
time during runtime, even without really adding the device, e.g. for
parameter introspection (try "device_add xlnx.v-dp,help" at the HMP
prompt for example).

But just setting "hotpluggable = true" for a device (e.g. "dpcd") which
could be created during instance_init also does not sound very
attractive to me... Not sure about any good alternative way to tackle
this right now, maybe I've eventually got to check user_creatable in
device_set_realized, too, or move the hotpluggable checks to
qmp_device_add() instead... I'll think about that for a while...
suggestions welcome!

 Thomas

On 26 September 2017 at 18:27, Thomas Huth <thuth@redhat.com> wrote:
> On 25.09.2017 19:59, Eduardo Habkost wrote:
>> On Mon, Sep 25, 2017 at 07:42:13PM +0200, Thomas Huth wrote:
>> "make check" found a few candidates:
>> https://travis-ci.org/ehabkost/qemu/jobs/278743999
>>
>>   Initialization of device dpcd failed: Device 'dpcd' does not support hotplugging
>> [...]
>>   Initialization of device nand failed: Device 'nand' does not support hotplugging
>
> I've now had a look at those, and I now feel like the whole
> "hotpluggable = false by default" idea was either just wrong or there
> are other smart ideas necessary to solve this issue: These devices are
> created during the instance_init() function of another device, e.g.
> "dpcd" is created in the xlnx_dp_init() function, which is the
> instance_init of TYPE_XLNX_DP. Now, instance_init() can be called at any
> time during runtime, even without really adding the device, e.g. for
> parameter introspection (try "device_add xlnx.v-dp,help" at the HMP
> prompt for example).

This is a bit weird though, because it means we have a lot of devices
which are "it's OK to instance_init this but not to actually create
(realize) it". I don't think that that should count as hotpluggable.
(Is it actually useful to be able to call "help" for a device
that it is not possible to create?)

> But just setting "hotpluggable = true" for a device (e.g. "dpcd") which
> could be created during instance_init also does not sound very
> attractive to me... Not sure about any good alternative way to tackle
> this right now, maybe I've eventually got to check user_creatable in
> device_set_realized, too, or move the hotpluggable checks to
> qmp_device_add() instead... I'll think about that for a while...
> suggestions welcome!

I think the general principle of defaulting to "you need to
specifically mark the device if you want it to be permitted to
hotplug it" seems right -- even devices which are created under
the hood by the real hotpluggable device still need to be
written to support it (ie by having a finalize method or
whatever to clean up). So they should probably be marked as
"hotpluggable but not user createable" or similar.

We just need to get the details right.

thanks
-- PMM

On Mon, Sep 25, 2017 at 04:26:44PM +0100, Peter Maydell wrote:
> On 25 September 2017 at 16:19, Thomas Huth <thuth@redhat.com> wrote:
> > Not sure whether this works for the virtio-xxx-device devices,
> > though, since they are marked as user_creatable = true currently...
> 
> That's deliberate -- for the arm boards with virtio-mmio
> the board creates a bunch of virtio-mmio transports and the
> virtio-foo-device can be user created to plug into those.

Do they support device_add virtio-xxx-device, though?  If they
don't, should we report it as hotpluggable on a future QMP
query-device-type command?

> 
> If overall trying to do the 'right thing' is tricky here
> then perhaps we can start by flipping the default to
> not-hotpluggable and marking some devices hotpluggable
> that in theory shouldn't be with a comment about why.

Finding the full list of those devices is the tricky part.

> 
> Incidentally I think there's still some advantage in the
> "created as part of some other device" devices having
> to be explicitly marked hotpluggable, because those
> devices do still need some specific code in them to
> handle it (ie code to release the resources that are
> created in the device's realize method).

I agree this is still useful.

I was trying represent something different: I was looking for a
flag meaning "supports device_add", while the current meaning is
"supports late instantiation".

On all devices, device_add support requires late instantiation.
On most user-creatable devices, late instantiation support
implies device_add support (virtio-*-device is the only
exception).

This confusion can be solved by documenting
DeviceClass::hotpluggable as "may support device_add, as long as
the machine and bus lets you do it", so there's no harm in
setting it to true on virtio-*-device.

This won't allow us to automatically tell if a device can be
safely instantiated without a hotplug controller, though.

-- 
Eduardo

On Fri, Sep 22, 2017 at 11:16:34AM +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.

I see that the discussion has moved on, but want to note here that
CPU hotplug on pseries breaks with this patch.

(qemu) device_add host-spapr-cpu-core,core-id=8,id=core8
Device 'host-powerpc64-cpu' does not support hotplugging

(qemu) device_add POWER8E_v2.1-spapr-cpu-core,id=core8,core-id=8
Device 'POWER8E_v2.1-powerpc64-cpu' does not support hotplugging

Hope I am not missing anything.

Regards,
Bharata.

On 26.09.2017 07:26, Bharata B Rao wrote:
> On Fri, Sep 22, 2017 at 11:16:34AM +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.
> 
> I see that the discussion has moved on, but want to note here that
> CPU hotplug on pseries breaks with this patch.
> 
> (qemu) device_add host-spapr-cpu-core,core-id=8,id=core8
> Device 'host-powerpc64-cpu' does not support hotplugging
> 
> (qemu) device_add POWER8E_v2.1-spapr-cpu-core,id=core8,core-id=8
> Device 'POWER8E_v2.1-powerpc64-cpu' does not support hotplugging

Sorry, my bad (again) - I missed that the spapr-cpu-core tries to
internally create some more devices, i.e. the powerpc64-cpu device and a
icp device. So they need to be marked as hotpluggable, too. I'll fix
this in my next version of the patch...
Anyway, thanks a lot for testing! (and memo ty myself: Do more testing
on your own before sending patches like this...)

> Hope I am not missing anything.

You just missed to add a proper qtest for "make check" for hot-pluggable
CPUs (hint, hint!) ;-) Apart from that, this failure was completely my
fault...

 Cheers,
  Thomas