[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] call HotplugHandler->plug() as the last step in
From: |
Cornelia Huck |
Subject: |
Re: [Qemu-devel] [PATCH] call HotplugHandler->plug() as the last step in device realization |
Date: |
Wed, 17 Oct 2018 12:56:38 +0200 |
On Tue, 16 Oct 2018 15:33:40 +0200
Igor Mammedov <address@hidden> 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/address@hidden/msg549915.html
>
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> 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;
> }
>