qdev: Mark devices as non-hotpluggable by default

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 <[email protected]>

---
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(-)

hw/char/virtio-serial-bus.c +1 -0

index 17a1bb0..da26fc2 100644
@@ -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 = {

hw/core/qdev.c +4 -6

index d9ccce6..28fd92f 100644
@@ -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;
 }
 

hw/mem/nvdimm.c +3 -0

index 952fce5..02be9f3 100644
@@ -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;

hw/mem/pc-dimm.c +1 -0

index 66eace5..1f78567 100644
@@ -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;

hw/pci/pci.c +1 -0

index 1e6fb88..8db380d 100644
@@ -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;
 }
 

hw/ppc/spapr_cpu_core.c +1 -0

index c08ee75..720284e 100644
@@ -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);
 }

hw/s390x/ccw-device.c +1 -0

index f9bfa15..d1b6e6f 100644
@@ -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 = {

hw/scsi/scsi-bus.c +1 -0

index 977f7bc..6703b2a 100644
@@ -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)

hw/usb/bus.c +1 -0

index d910f84..16701aa 100644
@@ -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 = {

hw/usb/dev-smartcard-reader.c +1 -0

index bef1f03..e28d1ba 100644
@@ -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 = {

hw/xen/xen_backend.c +1 -0

index c46cbb0..96621fe 100644
@@ -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 = {

target/i386/cpu.c +2 -2

index 0aa28fc..122623b 100644
@@ -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 = {

target/s390x/cpu.c +1 -0

index 34538c3..ef61d19 100644
@@ -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)

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 <[email protected]>

---
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(-)

hw/char/virtio-serial-bus.c +1 -0

index 17a1bb0..da26fc2 100644
@@ -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 = {

hw/core/qdev.c +4 -6

index d9ccce6..28fd92f 100644
@@ -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;
  }
 

hw/mem/nvdimm.c +3 -0

index 952fce5..02be9f3 100644
@@ -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;

hw/mem/pc-dimm.c +1 -0

index 66eace5..1f78567 100644
@@ -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;

hw/pci/pci.c +1 -0

index 1e6fb88..8db380d 100644
@@ -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;
  }
 

hw/ppc/spapr_cpu_core.c +1 -0

index c08ee75..720284e 100644
@@ -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);
  }

hw/s390x/ccw-device.c +1 -0

index f9bfa15..d1b6e6f 100644
@@ -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 = {

hw/scsi/scsi-bus.c +1 -0

index 977f7bc..6703b2a 100644
@@ -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)

hw/usb/bus.c +1 -0

index d910f84..16701aa 100644
@@ -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 = {

hw/usb/dev-smartcard-reader.c +1 -0

index bef1f03..e28d1ba 100644
@@ -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 = {

hw/xen/xen_backend.c +1 -0

index c46cbb0..96621fe 100644
@@ -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 = {

target/i386/cpu.c +2 -2

index 0aa28fc..122623b 100644
@@ -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 = {

target/s390x/cpu.c +0 -0

index 34538c3..ef61d19 100644

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 <[email protected]>

---
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(-)

Quoted 19 lines · On Fri, 22 Sep 2017 11:16:34 +0200 Thomas Huth <[email protected]> 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 <[email protected]>

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

Quoted 14 lines

 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?

Quoted 39 lines · Cornelia Huck

Quoted 19 lines · On Fri, 22 Sep 2017 11:16:34 +0200 Thomas Huth <[email protected]> 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 <[email protected]>

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

Quoted 14 lines

 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:

Quoted 7 lines

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

Quoted 54 lines · On Mon, 25 Sep 2017 10:31:53 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 39 lines · Cornelia Huck

Quoted 19 lines · On Fri, 22 Sep 2017 11:16:34 +0200 Thomas Huth <[email protected]> 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 <[email protected]>

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

Quoted 14 lines

 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:

Quoted 7 lines

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

Quoted 3 lines

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.

Quoted 43 lines · Igor Mammedov

Quoted 40 lines · On Mon, 25 Sep 2017 10:31:53 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 39 lines · Cornelia Huck

Quoted 19 lines · On Fri, 22 Sep 2017 11:16:34 +0200 Thomas Huth <[email protected]> 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 <[email protected]>

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

Quoted 14 lines

 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:

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

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

Quoted 6 lines

Quoted 6 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

Not sure whether this works for the virtio-xxx-device devices, though, since they are marked as user_creatable = true currently...

Quoted 2 lines · Thomas Huth

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

Quoted 4 lines · Peter Maydell

Quoted 2 lines · Thomas Huth

Not sure whether this works for the virtio-xxx-device devices, though, since they are marked as user_creatable = true currently...

Quoted 3 lines

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

Quoted 9 lines · Thomas Huth

Quoted 7 lines · Peter Maydell

Quoted 2 lines · Thomas Huth

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.

Quoted 3 lines

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.

Quoted 3 lines · Peter Maydell

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

Quoted 5 lines · Eduardo Habkost

Quoted 3 lines · Peter Maydell

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.

Quoted 2 lines

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.

Quoted 6 lines · Peter Maydell

Quoted 5 lines · Eduardo Habkost

Quoted 3 lines · Peter Maydell

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 4 lines · Peter Maydell

Quoted 2 lines · Thomas Huth

Not sure whether this works for the virtio-xxx-device devices, though, since they are marked as user_creatable = true currently...

Quoted 3 lines

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

Quoted 5 lines

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.

Quoted 7 lines

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?

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

Quoted 2 lines · Eduardo Habkost

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?

Quoted 2 lines · Eduardo Habkost

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?

Quoted 8 lines · Peter Maydell

Quoted 2 lines · Eduardo Habkost

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:

Quoted 5 lines · Peter Maydell

On 25 September 2017 at 18:59, Eduardo Habkost <[email protected]> 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:

Quoted 5 lines · Thomas Huth

Quoted 4 lines · Peter Maydell

Quoted 2 lines · Thomas Huth

Not sure whether this works for the virtio-xxx-device devices, though, since they are marked as user_creatable = true currently...

Quoted 7 lines

Quoted 3 lines

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.

Quoted 9 lines

Quoted 5 lines

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:

Quoted 11 lines

Quoted 7 lines

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

Quoted 7 lines · Eduardo Habkost

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

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!

Quoted 2 lines · Thomas Huth

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

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.

Quoted 11 lines · 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.

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.