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 <ehabk...@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 <th...@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 <th...@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
