Date: Fri, 22 Sep 2017 09:38:24 +0200
From: Cornelia Huck <cohuck@redhat.com>
Message-ID: <20170922093824.58a83358.cohuck@redhat.com>
In-Reply-To: <20170921185028.GK21016@localhost.localdomain>
References: <1505811353-29151-1-git-send-email-thuth@redhat.com>
	<20170921185028.GK21016@localhost.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [RFC PATCH] qdev: Mark devices as non-hotpluggable
 by default
To: Eduardo Habkost <ehabkost@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>, qemu-devel@nongnu.org, Igor Mammedov <imammedo@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Stefano Stabellini <sstabellini@kernel.org>, Xiao Guangrong <guangrong.xiao@linux.intel.com>, "Michael S. Tsirkin" <mst@redhat.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Gerd Hoffmann <kraxel@redhat.com>, Anthony Perard <anthony.perard@citrix.com>, Marcel Apfelbaum <marcel@redhat.com>

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

> On Tue, Sep 19, 2017 at 10:55:53AM +0200, Thomas Huth wrote:
> > Historically we've marked all devices as hotpluggable by default. However,
> > most devices are not hotpluggable, and you also need a HotplugHandler to
> > support these devices. So if the user tries to "device_add" or "device_del"
> > such a non-hotpluggable device during runtime, either nothing really usable
> > happens, or QEMU even crashes/aborts unexpectedly (see for example commit
> > 84ebd3e8c7d4fe955b - "Mark diag288 watchdog as non-hotpluggable").
> > So let's change this dangerous default behaviour and mark the devices as
> > non-hotpluggable by default. Certain parent devices classes which are known
> > as hotpluggable (e.g. PCI, USB, etc.) are marked with "hotpluggable = true",
> > so that devices that are derived from these classes continue to work as
> > expected.  
> 
> These seem to be missing:
> * TYPE_CPU (or at least TYPE_X86_CPU and TYPE_S390_CPU)

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

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

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

Nod.

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

Yes, it is.

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

Yes.