| Message ID | 1539696820-273275-1-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | call HotplugHandler->plug() as the last step in device realization | expand |
On Tue, 16 Oct 2018 15:33:40 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > When [2] was fixed it was agreed that adding and calling post_plug() > callback after device_reset() was low risk approach to hotfix issue > right before release. So it was merged instead of moving already > existing plug() callback after device_reset() is called which would > be more risky and require all plug() callbacks audit. > > Looking at the current plug() callbacks, it doesn't seem that moving > plug() callback after device_reset() is breaking anything, so here > goes agreed upon [3] proper fix which essentially reverts [1][2] > and moves plug() callback after device_reset(). > This way devices always comes to plug() stage, after it's been fully > initialized (including being reset), which fixes race condition [2] > without need for an extra post_plug() callback. > > 1. (25e897881 "qdev: add HotplugHandler->post_plug() callback") > 2. (8449bcf94 "virtio-scsi: fix hotplug ->reset() vs event race") > 3. https://www.mail-archive.com/qemu-devel@nongnu.org/msg549915.html > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > TODO: > remove usage of Error** from plug() callback, we need to factor out > pre_plug part from plug() callbacks, before proceeding with it. > DavidH has recently finished it for pc-dimm/memory_devices, cpus > mostly have pre_plug parts factored out, but there still are parts > that could fail so it needs some more work to eliminate failure points > from plug() callbacks. Meanwhile, I'll plan to treat other misc > handlers (pci[e]/acpi/usb/...) and introduce pre_plug() where > necessary. Forgot to CC spapr/s390 folks to give it an extra scrutiny.
On Tue, Oct 16, 2018 at 03:33:40PM +0200, Igor Mammedov wrote: > When [2] was fixed it was agreed that adding and calling post_plug() > callback after device_reset() was low risk approach to hotfix issue > right before release. So it was merged instead of moving already > existing plug() callback after device_reset() is called which would > be more risky and require all plug() callbacks audit. > > Looking at the current plug() callbacks, it doesn't seem that moving > plug() callback after device_reset() is breaking anything, so here > goes agreed upon [3] proper fix which essentially reverts [1][2] > and moves plug() callback after device_reset(). > This way devices always comes to plug() stage, after it's been fully > initialized (including being reset), which fixes race condition [2] > without need for an extra post_plug() callback. > > 1. (25e897881 "qdev: add HotplugHandler->post_plug() callback") > 2. (8449bcf94 "virtio-scsi: fix hotplug ->reset() vs event race") > 3. https://www.mail-archive.com/qemu-devel@nongnu.org/msg549915.html > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > TODO: > remove usage of Error** from plug() callback, we need to factor out > pre_plug part from plug() callbacks, before proceeding with it. > DavidH has recently finished it for pc-dimm/memory_devices, cpus > mostly have pre_plug parts factored out, but there still are parts > that could fail so it needs some more work to eliminate failure points > from plug() callbacks. Meanwhile, I'll plan to treat other misc > handlers (pci[e]/acpi/usb/...) and introduce pre_plug() where > necessary. > --- > include/hw/hotplug.h | 11 ----------- > hw/core/hotplug.c | 10 ---------- > hw/core/qdev.c | 16 ++++++---------- > hw/scsi/virtio-scsi.c | 11 +---------- > 4 files changed, 7 insertions(+), 41 deletions(-) Thanks. I think we're okay from the virtio-scsi perspective. I have not audited all devices. Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Tue, 16 Oct 2018 15:33:40 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > When [2] was fixed it was agreed that adding and calling post_plug() > callback after device_reset() was low risk approach to hotfix issue > right before release. So it was merged instead of moving already > existing plug() callback after device_reset() is called which would > be more risky and require all plug() callbacks audit. > > Looking at the current plug() callbacks, it doesn't seem that moving > plug() callback after device_reset() is breaking anything, so here > goes agreed upon [3] proper fix which essentially reverts [1][2] > and moves plug() callback after device_reset(). > This way devices always comes to plug() stage, after it's been fully > initialized (including being reset), which fixes race condition [2] > without need for an extra post_plug() callback. > > 1. (25e897881 "qdev: add HotplugHandler->post_plug() callback") > 2. (8449bcf94 "virtio-scsi: fix hotplug ->reset() vs event race") > 3. https://www.mail-archive.com/qemu-devel@nongnu.org/msg549915.html > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > TODO: > remove usage of Error** from plug() callback, we need to factor out > pre_plug part from plug() callbacks, before proceeding with it. > DavidH has recently finished it for pc-dimm/memory_devices, cpus > mostly have pre_plug parts factored out, but there still are parts > that could fail so it needs some more work to eliminate failure points > from plug() callbacks. Meanwhile, I'll plan to treat other misc > handlers (pci[e]/acpi/usb/...) and introduce pre_plug() where > necessary. > --- > include/hw/hotplug.h | 11 ----------- > hw/core/hotplug.c | 10 ---------- > hw/core/qdev.c | 16 ++++++---------- > hw/scsi/virtio-scsi.c | 11 +---------- > 4 files changed, 7 insertions(+), 41 deletions(-) I've looked at the s390x users of this interface, and it seems to be sane. The one I'm a bit unsure about is zPCI with its bridge enumeration code as introduced in d2f07120a35a ("s390x/pci: handle PCIBridge bus number"). I *think* that one is fine as well. Pierre, can you confirm? > > diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h > index 51541d6..1a0516a 100644 > --- a/include/hw/hotplug.h > +++ b/include/hw/hotplug.h > @@ -47,8 +47,6 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler, > * @parent: Opaque parent interface. > * @pre_plug: pre plug callback called at start of device.realize(true) > * @plug: plug callback called at end of device.realize(true). > - * @post_plug: post plug callback called after device.realize(true) and device > - * reset > * @unplug_request: unplug request callback. > * Used as a means to initiate device unplug for devices that > * require asynchronous unplug handling. > @@ -63,7 +61,6 @@ typedef struct HotplugHandlerClass { > /* <public> */ > hotplug_fn pre_plug; > hotplug_fn plug; > - void (*post_plug)(HotplugHandler *plug_handler, DeviceState *plugged_dev); > hotplug_fn unplug_request; > hotplug_fn unplug; > } HotplugHandlerClass; > @@ -87,14 +84,6 @@ void hotplug_handler_pre_plug(HotplugHandler *plug_handler, > Error **errp); > > /** > - * hotplug_handler_post_plug: > - * > - * Call #HotplugHandlerClass.post_plug callback of @plug_handler. > - */ > -void hotplug_handler_post_plug(HotplugHandler *plug_handler, > - DeviceState *plugged_dev); > - > -/** > * hotplug_handler_unplug_request: > * > * Calls #HotplugHandlerClass.unplug_request callback of @plug_handler. > diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c > index 2253072..17ac986 100644 > --- a/hw/core/hotplug.c > +++ b/hw/core/hotplug.c > @@ -35,16 +35,6 @@ void hotplug_handler_plug(HotplugHandler *plug_handler, > } > } > > -void hotplug_handler_post_plug(HotplugHandler *plug_handler, > - DeviceState *plugged_dev) > -{ > - HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler); > - > - if (hdc->post_plug) { > - hdc->post_plug(plug_handler, plugged_dev); > - } > -} > - > void hotplug_handler_unplug_request(HotplugHandler *plug_handler, > DeviceState *plugged_dev, > Error **errp) > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 046d8f1..6b3cc55 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -832,14 +832,6 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > > DEVICE_LISTENER_CALL(realize, Forward, dev); > > - if (hotplug_ctrl) { > - hotplug_handler_plug(hotplug_ctrl, dev, &local_err); > - } > - > - if (local_err != NULL) { > - goto post_realize_fail; > - } > - > /* > * always free/re-initialize here since the value cannot be cleaned up > * in device_unrealize due to its usage later on in the unplug path > @@ -869,8 +861,12 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > dev->pending_deleted_event = false; > > if (hotplug_ctrl) { > - hotplug_handler_post_plug(hotplug_ctrl, dev); > - } > + hotplug_handler_plug(hotplug_ctrl, dev, &local_err); > + if (local_err != NULL) { > + goto child_realize_fail; > + } > + } > + > } else if (!value && dev->realized) { > Error **local_errp = NULL; > QLIST_FOREACH(bus, &dev->child_bus, sibling) { > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c > index 5a3057d..3aa9971 100644 > --- a/hw/scsi/virtio-scsi.c > +++ b/hw/scsi/virtio-scsi.c > @@ -797,16 +797,8 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev, > virtio_scsi_acquire(s); > blk_set_aio_context(sd->conf.blk, s->ctx); > virtio_scsi_release(s); > - } > -} > > -/* Announce the new device after it has been plugged */ > -static void virtio_scsi_post_hotplug(HotplugHandler *hotplug_dev, > - DeviceState *dev) > -{ > - VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev); > - VirtIOSCSI *s = VIRTIO_SCSI(vdev); > - SCSIDevice *sd = SCSI_DEVICE(dev); > + } > > if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) { > virtio_scsi_acquire(s); > @@ -976,7 +968,6 @@ static void virtio_scsi_class_init(ObjectClass *klass, void *data) > vdc->start_ioeventfd = virtio_scsi_dataplane_start; > vdc->stop_ioeventfd = virtio_scsi_dataplane_stop; > hc->plug = virtio_scsi_hotplug; > - hc->post_plug = virtio_scsi_post_hotplug; > hc->unplug = virtio_scsi_hotunplug; > } >
On 16/10/2018 15:33, Igor Mammedov wrote: > TODO: > remove usage of Error** from plug() callback, we need to factor out > pre_plug part from plug() callbacks, before proceeding with it. > DavidH has recently finished it for pc-dimm/memory_devices, cpus > mostly have pre_plug parts factored out, but there still are parts > that could fail so it needs some more work to eliminate failure points > from plug() callbacks. Meanwhile, I'll plan to treat other misc > handlers (pci[e]/acpi/usb/...) and introduce pre_plug() where > necessary. I am not sure it's a good idea to do this first, rather than last (so that we risk introducing a ping-pong of bugs that appear now and are fixed when the other changes are made), but if others disagree I am okay with the patch. I agree that virtio-scsi is fine here. Paolo
On 17/10/2018 12:56, Cornelia Huck wrote: > On Tue, 16 Oct 2018 15:33:40 +0200 > Igor Mammedov <imammedo@redhat.com> wrote: > >> When [2] was fixed it was agreed that adding and calling post_plug() >> callback after device_reset() was low risk approach to hotfix issue >> right before release. So it was merged instead of moving already >> existing plug() callback after device_reset() is called which would >> be more risky and require all plug() callbacks audit. >> >> Looking at the current plug() callbacks, it doesn't seem that moving >> plug() callback after device_reset() is breaking anything, so here >> goes agreed upon [3] proper fix which essentially reverts [1][2] >> and moves plug() callback after device_reset(). >> This way devices always comes to plug() stage, after it's been fully >> initialized (including being reset), which fixes race condition [2] >> without need for an extra post_plug() callback. >> >> 1. (25e897881 "qdev: add HotplugHandler->post_plug() callback") >> 2. (8449bcf94 "virtio-scsi: fix hotplug ->reset() vs event race") >> 3. https://www.mail-archive.com/qemu-devel@nongnu.org/msg549915.html >> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >> --- >> TODO: >> remove usage of Error** from plug() callback, we need to factor out >> pre_plug part from plug() callbacks, before proceeding with it. >> DavidH has recently finished it for pc-dimm/memory_devices, cpus >> mostly have pre_plug parts factored out, but there still are parts >> that could fail so it needs some more work to eliminate failure points >> from plug() callbacks. Meanwhile, I'll plan to treat other misc >> handlers (pci[e]/acpi/usb/...) and introduce pre_plug() where >> necessary. >> --- >> include/hw/hotplug.h | 11 ----------- >> hw/core/hotplug.c | 10 ---------- >> hw/core/qdev.c | 16 ++++++---------- >> hw/scsi/virtio-scsi.c | 11 +---------- >> 4 files changed, 7 insertions(+), 41 deletions(-) > > I've looked at the s390x users of this interface, and it seems to be > sane. The one I'm a bit unsure about is zPCI with its bridge > enumeration code as introduced in d2f07120a35a ("s390x/pci: handle > PCIBridge bus number"). I *think* that one is fine as well. Pierre, can > you confirm? Yes, I can confirm. We have no problem with this patch in VFIO_PCI nor in VIRTIO_PCI with zPCI emulation, for devices bridge or bus. I played the patch and tested locally. all fine. For the S390: Tested-by: Pierre Morel<pmorel@linux.ibm.com> also Acked-by: Pierre Morel<pmorel@linux.ibm.com> Regards, Pierre
On Wed, 17 Oct 2018 13:40:00 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 16/10/2018 15:33, Igor Mammedov wrote: > > TODO: > > remove usage of Error** from plug() callback, we need to factor out > > pre_plug part from plug() callbacks, before proceeding with it. > > DavidH has recently finished it for pc-dimm/memory_devices, cpus > > mostly have pre_plug parts factored out, but there still are parts > > that could fail so it needs some more work to eliminate failure points > > from plug() callbacks. Meanwhile, I'll plan to treat other misc > > handlers (pci[e]/acpi/usb/...) and introduce pre_plug() where > > necessary. > > I am not sure it's a good idea to do this first, rather than last (so > that we risk introducing a ping-pong of bugs that appear now and are > fixed when the other changes are made), but if others disagree I am okay > with the patch. It will take a time to cleanup Error** handling, but it's not must have req for moving plug() handler to the right place, even if error gets triggered on this patch ever it cleanly fails device_realize(). I'm posting this first, so post_plug() won't confuse people and won't introduce a new code that would depend on it or even worse some plug handler would depend on plug() happening before reset. This patch should prevent that from happening. (Error** concerns are secondary here and I'll deal with it later on, hopefully during 3.2 cycle) PS: post_plug() is recurring idea which indicates that current plug() is misplaced. (previous instance that's slipped in, was nvdimm's 75b0713e1 which was removed later on by c7f8d0f3a5). [...]
On 18/10/2018 11:57, Igor Mammedov wrote: > > I'm posting this first, so post_plug() won't confuse people and > won't introduce a new code that would depend on it or even worse > some plug handler would depend on plug() happening before reset. > This patch should prevent that from happening. (Error** concerns > are secondary here and I'll deal with it later on, hopefully > during 3.2 cycle) > > PS: > post_plug() is recurring idea which indicates that current plug() > is misplaced. (previous instance that's slipped in, was nvdimm's > 75b0713e1 which was removed later on by c7f8d0f3a5). Ok, queued. Paolo
On 16.10.18 15:56, Igor Mammedov wrote: > On Tue, 16 Oct 2018 15:33:40 +0200 > Igor Mammedov <imammedo@redhat.com> wrote: > >> When [2] was fixed it was agreed that adding and calling post_plug() >> callback after device_reset() was low risk approach to hotfix issue >> right before release. So it was merged instead of moving already >> existing plug() callback after device_reset() is called which would >> be more risky and require all plug() callbacks audit. >> >> Looking at the current plug() callbacks, it doesn't seem that moving >> plug() callback after device_reset() is breaking anything, so here >> goes agreed upon [3] proper fix which essentially reverts [1][2] >> and moves plug() callback after device_reset(). >> This way devices always comes to plug() stage, after it's been fully >> initialized (including being reset), which fixes race condition [2] >> without need for an extra post_plug() callback. >> >> 1. (25e897881 "qdev: add HotplugHandler->post_plug() callback") >> 2. (8449bcf94 "virtio-scsi: fix hotplug ->reset() vs event race") >> 3. https://www.mail-archive.com/qemu-devel@nongnu.org/msg549915.html >> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >> --- >> TODO: >> remove usage of Error** from plug() callback, we need to factor out >> pre_plug part from plug() callbacks, before proceeding with it. >> DavidH has recently finished it for pc-dimm/memory_devices, cpus >> mostly have pre_plug parts factored out, but there still are parts >> that could fail so it needs some more work to eliminate failure points >> from plug() callbacks. Meanwhile, I'll plan to treat other misc >> handlers (pci[e]/acpi/usb/...) and introduce pre_plug() where >> necessary. Saw this mail just now. I guess we should do more. Especially what seems to be fragile is errors during unrealize() and unplug(). Errors during unplug() should only ever happen if it was not triggered via unplug_request(). Otherwise, unplug_request() should check for all possible errors and unplug() will not result in errors. Also, errors during unrealize() should definitely be avoided. Or even forbidden. E.g. looking at hw/core/qdev.c:device_set_realized, 1. failing to unrealize might already have resulted in the unplug handler getting called. 2. will result in a unparent of the device and therefore removal 3. will have already eventually unrealized child devices or buses. To summarize, failing in unrealize() is a bad idea and might leave the rest of the system in a very unpredictable state. Failing in unplug() is a bad idea unless we don't have a unplug_request(). > > Forgot to CC spapr/s390 folks to give it an extra scrutiny. >
On Wed, 24 Oct 2018 15:09:36 +0200 David Hildenbrand <david@redhat.com> wrote: > On 16.10.18 15:56, Igor Mammedov wrote: > > On Tue, 16 Oct 2018 15:33:40 +0200 > > Igor Mammedov <imammedo@redhat.com> wrote: > > > >> When [2] was fixed it was agreed that adding and calling post_plug() > >> callback after device_reset() was low risk approach to hotfix issue > >> right before release. So it was merged instead of moving already > >> existing plug() callback after device_reset() is called which would > >> be more risky and require all plug() callbacks audit. > >> > >> Looking at the current plug() callbacks, it doesn't seem that moving > >> plug() callback after device_reset() is breaking anything, so here > >> goes agreed upon [3] proper fix which essentially reverts [1][2] > >> and moves plug() callback after device_reset(). > >> This way devices always comes to plug() stage, after it's been fully > >> initialized (including being reset), which fixes race condition [2] > >> without need for an extra post_plug() callback. > >> > >> 1. (25e897881 "qdev: add HotplugHandler->post_plug() callback") > >> 2. (8449bcf94 "virtio-scsi: fix hotplug ->reset() vs event race") > >> 3. https://www.mail-archive.com/qemu-devel@nongnu.org/msg549915.html > >> > >> Signed-off-by: Igor Mammedov <imammedo@redhat.com> > >> --- > >> TODO: > >> remove usage of Error** from plug() callback, we need to factor out > >> pre_plug part from plug() callbacks, before proceeding with it. > >> DavidH has recently finished it for pc-dimm/memory_devices, cpus > >> mostly have pre_plug parts factored out, but there still are parts > >> that could fail so it needs some more work to eliminate failure points > >> from plug() callbacks. Meanwhile, I'll plan to treat other misc > >> handlers (pci[e]/acpi/usb/...) and introduce pre_plug() where > >> necessary. > > Saw this mail just now. I guess we should do more. Especially what seems > to be fragile is errors during unrealize() and unplug(). > > Errors during unplug() should only ever happen if it was not triggered > via unplug_request(). Otherwise, unplug_request() should check for all > possible errors and unplug() will not result in errors. Not sure that I read it right, I see 2 cases: 1: unplug_request == NULL (i.e. where surprise device removal is supported) should not fail 2: unplug_request != NULL && unplug() is called by guest side, it's caller policy to decide what to do on failure. One use case is NOP, i.e. caller should not destroy object if unplug() fails and unplug() should fail without side effects, and possibly notify guest about it if such mechanism is supported. Could be tricky to handle errors in case of chained handlers. > Also, errors during unrealize() should definitely be avoided. Or even > forbidden. E.g. looking at hw/core/qdev.c:device_set_realized, > > 1. failing to unrealize might already have resulted in the unplug > handler getting called. > 2. will result in a unparent of the device and therefore removal > 3. will have already eventually unrealized child devices or buses. > > To summarize, failing in unrealize() is a bad idea and might leave the > rest of the system in a very unpredictable state. failure path isn't really usable here, I'd assume that initial idea was that unrealize() could fail cleanly and let caller to decide on what to do. But with child bus handling and co it won't really work. BTW what devices are the users of DeviceState::child_bus, I don't think that we have any composite devices that actually exercise realize() part of it and maybe unrealize() part. ...
On 30.10.18 16:15, Igor Mammedov wrote: > On Wed, 24 Oct 2018 15:09:36 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 16.10.18 15:56, Igor Mammedov wrote: >>> On Tue, 16 Oct 2018 15:33:40 +0200 >>> Igor Mammedov <imammedo@redhat.com> wrote: >>> >>>> When [2] was fixed it was agreed that adding and calling post_plug() >>>> callback after device_reset() was low risk approach to hotfix issue >>>> right before release. So it was merged instead of moving already >>>> existing plug() callback after device_reset() is called which would >>>> be more risky and require all plug() callbacks audit. >>>> >>>> Looking at the current plug() callbacks, it doesn't seem that moving >>>> plug() callback after device_reset() is breaking anything, so here >>>> goes agreed upon [3] proper fix which essentially reverts [1][2] >>>> and moves plug() callback after device_reset(). >>>> This way devices always comes to plug() stage, after it's been fully >>>> initialized (including being reset), which fixes race condition [2] >>>> without need for an extra post_plug() callback. >>>> >>>> 1. (25e897881 "qdev: add HotplugHandler->post_plug() callback") >>>> 2. (8449bcf94 "virtio-scsi: fix hotplug ->reset() vs event race") >>>> 3. https://www.mail-archive.com/qemu-devel@nongnu.org/msg549915.html >>>> >>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>>> --- >>>> TODO: >>>> remove usage of Error** from plug() callback, we need to factor out >>>> pre_plug part from plug() callbacks, before proceeding with it. >>>> DavidH has recently finished it for pc-dimm/memory_devices, cpus >>>> mostly have pre_plug parts factored out, but there still are parts >>>> that could fail so it needs some more work to eliminate failure points >>>> from plug() callbacks. Meanwhile, I'll plan to treat other misc >>>> handlers (pci[e]/acpi/usb/...) and introduce pre_plug() where >>>> necessary. >> >> Saw this mail just now. I guess we should do more. Especially what seems >> to be fragile is errors during unrealize() and unplug(). >> >> Errors during unplug() should only ever happen if it was not triggered >> via unplug_request(). Otherwise, unplug_request() should check for all >> possible errors and unplug() will not result in errors. > Not sure that I read it right, I see 2 cases: > 1: unplug_request == NULL (i.e. where surprise device removal is supported) > should not fail So for now, we should not report errors. > 2: unplug_request != NULL && unplug() is called by guest side, it's caller > policy to decide what to do on failure. One use case is NOP, i.e. caller > should not destroy object if unplug() fails and unplug() should fail without > side effects, and possibly notify guest about it if such mechanism is supported. > Could be tricky to handle errors in case of chained handlers. My assumption would be that unplug_request() performs all necessary checks and triggers the guest. The guest action should eventually result in an unplug(), which should in my opinion also never fail. The code calling into unplug() should perform checks if the guest is actually doing something sane and/or something that was actually requested. Right now I am not sure if unplug() should be allowed to fail at all. Maybe I am missing once piece. Chained handlers for memory devices should not be a problem as far as I can see. > > >> Also, errors during unrealize() should definitely be avoided. Or even >> forbidden. E.g. looking at hw/core/qdev.c:device_set_realized, >> >> 1. failing to unrealize might already have resulted in the unplug >> handler getting called. >> 2. will result in a unparent of the device and therefore removal >> 3. will have already eventually unrealized child devices or buses. >> >> To summarize, failing in unrealize() is a bad idea and might leave the >> rest of the system in a very unpredictable state. > failure path isn't really usable here, I'd assume that initial idea > was that unrealize() could fail cleanly and let caller to decide on > what to do. But with child bus handling and co it won't really work. Exactly, we should maybe clean that up. It seems to easy for people that implement devices to just stop unplug by trying to fail unrealize(). But it will break things. > > BTW what devices are the users of DeviceState::child_bus, > I don't think that we have any composite devices that actually > exercise realize() part of it and maybe unrealize() part. For unrealize(): This should be called for all virtio proxy devices to unrealize the child virtio device. So recursive unrealize is in place. For realize(): Not really used yet. See the comment in bus_set_realized() on realize part: "TODO: recursive realization". That's why e.g. virtio proxy devices explicitly trigger "realize=true" for the child on device realization. I remember Andreas used to work on this a long time ago but we never completed it.
diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h index 51541d6..1a0516a 100644 --- a/include/hw/hotplug.h +++ b/include/hw/hotplug.h @@ -47,8 +47,6 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler, * @parent: Opaque parent interface. * @pre_plug: pre plug callback called at start of device.realize(true) * @plug: plug callback called at end of device.realize(true). - * @post_plug: post plug callback called after device.realize(true) and device - * reset * @unplug_request: unplug request callback. * Used as a means to initiate device unplug for devices that * require asynchronous unplug handling. @@ -63,7 +61,6 @@ typedef struct HotplugHandlerClass { /* <public> */ hotplug_fn pre_plug; hotplug_fn plug; - void (*post_plug)(HotplugHandler *plug_handler, DeviceState *plugged_dev); hotplug_fn unplug_request; hotplug_fn unplug; } HotplugHandlerClass; @@ -87,14 +84,6 @@ void hotplug_handler_pre_plug(HotplugHandler *plug_handler, Error **errp); /** - * hotplug_handler_post_plug: - * - * Call #HotplugHandlerClass.post_plug callback of @plug_handler. - */ -void hotplug_handler_post_plug(HotplugHandler *plug_handler, - DeviceState *plugged_dev); - -/** * hotplug_handler_unplug_request: * * Calls #HotplugHandlerClass.unplug_request callback of @plug_handler. diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c index 2253072..17ac986 100644 --- a/hw/core/hotplug.c +++ b/hw/core/hotplug.c @@ -35,16 +35,6 @@ void hotplug_handler_plug(HotplugHandler *plug_handler, } } -void hotplug_handler_post_plug(HotplugHandler *plug_handler, - DeviceState *plugged_dev) -{ - HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler); - - if (hdc->post_plug) { - hdc->post_plug(plug_handler, plugged_dev); - } -} - void hotplug_handler_unplug_request(HotplugHandler *plug_handler, DeviceState *plugged_dev, Error **errp) diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 046d8f1..6b3cc55 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -832,14 +832,6 @@ static void device_set_realized(Object *obj, bool value, Error **errp) DEVICE_LISTENER_CALL(realize, Forward, dev); - if (hotplug_ctrl) { - hotplug_handler_plug(hotplug_ctrl, dev, &local_err); - } - - if (local_err != NULL) { - goto post_realize_fail; - } - /* * always free/re-initialize here since the value cannot be cleaned up * in device_unrealize due to its usage later on in the unplug path @@ -869,8 +861,12 @@ static void device_set_realized(Object *obj, bool value, Error **errp) dev->pending_deleted_event = false; if (hotplug_ctrl) { - hotplug_handler_post_plug(hotplug_ctrl, dev); - } + hotplug_handler_plug(hotplug_ctrl, dev, &local_err); + if (local_err != NULL) { + goto child_realize_fail; + } + } + } else if (!value && dev->realized) { Error **local_errp = NULL; QLIST_FOREACH(bus, &dev->child_bus, sibling) { diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index 5a3057d..3aa9971 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -797,16 +797,8 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev, virtio_scsi_acquire(s); blk_set_aio_context(sd->conf.blk, s->ctx); virtio_scsi_release(s); - } -} -/* Announce the new device after it has been plugged */ -static void virtio_scsi_post_hotplug(HotplugHandler *hotplug_dev, - DeviceState *dev) -{ - VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev); - VirtIOSCSI *s = VIRTIO_SCSI(vdev); - SCSIDevice *sd = SCSI_DEVICE(dev); + } if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) { virtio_scsi_acquire(s); @@ -976,7 +968,6 @@ static void virtio_scsi_class_init(ObjectClass *klass, void *data) vdc->start_ioeventfd = virtio_scsi_dataplane_start; vdc->stop_ioeventfd = virtio_scsi_dataplane_stop; hc->plug = virtio_scsi_hotplug; - hc->post_plug = virtio_scsi_post_hotplug; hc->unplug = virtio_scsi_hotunplug; }
When [2] was fixed it was agreed that adding and calling post_plug() callback after device_reset() was low risk approach to hotfix issue right before release. So it was merged instead of moving already existing plug() callback after device_reset() is called which would be more risky and require all plug() callbacks audit. Looking at the current plug() callbacks, it doesn't seem that moving plug() callback after device_reset() is breaking anything, so here goes agreed upon [3] proper fix which essentially reverts [1][2] and moves plug() callback after device_reset(). This way devices always comes to plug() stage, after it's been fully initialized (including being reset), which fixes race condition [2] without need for an extra post_plug() callback. 1. (25e897881 "qdev: add HotplugHandler->post_plug() callback") 2. (8449bcf94 "virtio-scsi: fix hotplug ->reset() vs event race") 3. https://www.mail-archive.com/qemu-devel@nongnu.org/msg549915.html Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- TODO: remove usage of Error** from plug() callback, we need to factor out pre_plug part from plug() callbacks, before proceeding with it. DavidH has recently finished it for pc-dimm/memory_devices, cpus mostly have pre_plug parts factored out, but there still are parts that could fail so it needs some more work to eliminate failure points from plug() callbacks. Meanwhile, I'll plan to treat other misc handlers (pci[e]/acpi/usb/...) and introduce pre_plug() where necessary. --- include/hw/hotplug.h | 11 ----------- hw/core/hotplug.c | 10 ---------- hw/core/qdev.c | 16 ++++++---------- hw/scsi/virtio-scsi.c | 11 +---------- 4 files changed, 7 insertions(+), 41 deletions(-)