[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [for-2.11 PATCH 13/26] qdev: store DeviceState's canoni
From: |
Michael Roth |
Subject: |
Re: [Qemu-devel] [for-2.11 PATCH 13/26] qdev: store DeviceState's canonical path to use when unparenting |
Date: |
Wed, 26 Jul 2017 07:03:13 -0500 |
User-agent: |
alot/0.5.1 |
Quoting David Gibson (2017-07-26 00:24:43)
> On Tue, Jul 25, 2017 at 08:00:47PM +0200, Greg Kurz wrote:
> > From: Michael Roth <address@hidden>
> >
> > device_unparent(dev, ...) is called when a device is unparented,
> > either directly, or as a result of a parent device being
> > finalized, and handles some final cleanup for the device. Part
> > of this includes emiting a DEVICE_DELETED QMP event to notify
> > management, which includes the device's path in the composition
> > tree as provided by object_get_canonical_path().
> >
> > object_get_canonical_path() assumes the device is still connected
> > to the machine/root container, and will assert otherwise, but
> > in some situations this isn't the case:
> >
> > If the parent is finalized as a result of object_unparent(), it
> > will still be attached to the composition tree at the time any
> > children are unparented as a result of that same call to
> > object_unparent(). However, in some cases, object_unparent()
> > will complete without finalizing the parent device, due to
> > lingering references that won't be released till some time later.
> > One such example is if the parent has MemoryRegion children (which
> > take a ref on their parent), who in turn have AddressSpace's (which
> > take a ref on their regions), since those AddressSpaces get cleaned
> > up asynchronously by the RCU thread.
> >
> > In this case qdev:device_unparent() may be called for a child Device
> > that no longer has a path to the root/machine container, causing
> > object_get_canonical_path() to assert.
> >
> > Fix this by storing the canonical path during realize() so the
> > information will still be available for device_unparent() in such
> > cases.
>
> Hm. I'm no expert on the QOM model, but I'm not sure this is the
> right approach.
>
> I would have thought the right time to emit the DEVICE_DELETED message
> would be when the device leaves the main composition tree, even if it
> could be finalized later.
That probably is how it should've been approached originally, since
that's also the expectation for device_del on PCIDevices currently.
However, there was a recent discussion which suggests we should move
DEVICE_DELETED to finalize to fix a race with VFIO unplug in libvirt.
https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg06887.html
Since the QOM path will be broken by that point, we likely would need a
way to cache the path so it can be reported in the DEVICE_DELETED message.
I was actually planning on using this patch as a prereq for that. I'll
send that out soon.
>
> If we made that the case, does this problem go away?
>
> > Cc: Michael S. Tsirkin <address@hidden>
> > Cc: Paolo Bonzini <address@hidden>
> > Signed-off-by: Michael Roth <address@hidden>
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> > Changes since RFC:
> > - rebased against ppc-for-2.10
> > ---
> > hw/core/qdev.c | 15 ++++++++++++---
> > include/hw/qdev-core.h | 1 +
> > 2 files changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 606ab53c42cd..a64b35c16251 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -928,6 +928,12 @@ static void device_set_realized(Object *obj, bool
> > value, Error **errp)
> > goto post_realize_fail;
> > }
> >
> > + /* always re-initialize since we clean up in device_unparent()
> > instead
> > + * of unrealize()
> > + */
> > + g_free(dev->canonical_path);
> > + dev->canonical_path = object_get_canonical_path(OBJECT(dev));
> > +
> > if (qdev_get_vmsd(dev)) {
> > if (vmstate_register_with_alias_id(dev, -1,
> > qdev_get_vmsd(dev), dev,
> > dev->instance_id_alias,
> > @@ -984,6 +990,7 @@ child_realize_fail:
> > }
> >
> > post_realize_fail:
> > + g_free(dev->canonical_path);
> > if (dc->unrealize) {
> > dc->unrealize(dev, NULL);
> > }
> > @@ -1102,10 +1109,12 @@ static void device_unparent(Object *obj)
> >
> > /* Only send event if the device had been completely realized */
> > if (dev->pending_deleted_event) {
> > - gchar *path = object_get_canonical_path(OBJECT(dev));
> > + g_assert(dev->canonical_path);
> >
> > - qapi_event_send_device_deleted(!!dev->id, dev->id, path,
> > &error_abort);
> > - g_free(path);
> > + qapi_event_send_device_deleted(!!dev->id, dev->id,
> > dev->canonical_path,
> > + &error_abort);
> > + g_free(dev->canonical_path);
> > + dev->canonical_path = NULL;
> > }
> >
> > qemu_opts_del(dev->opts);
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index ae317286a480..9237b6849ff3 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -153,6 +153,7 @@ struct DeviceState {
> > /*< public >*/
> >
> > const char *id;
> > + char *canonical_path;
> > bool realized;
> > bool pending_deleted_event;
> > QemuOpts *opts;
> >
>
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
- Re: [Qemu-devel] [for-2.11 PATCH 08/26] spapr_drc: add unrealize method to physical DRC class, (continued)
- [Qemu-devel] [for-2.11 PATCH 09/26] spapr_drc: pass object ownership to parent/owner, Greg Kurz, 2017/07/25
- [Qemu-devel] [for-2.11 PATCH 10/26] spapr_iommu: pass object ownership to parent/owner, Greg Kurz, 2017/07/25
- [Qemu-devel] [for-2.11 PATCH 11/26] spapr_iommu: unregister vmstate at unrealize time, Greg Kurz, 2017/07/25
- [Qemu-devel] [for-2.11 PATCH 12/26] pci: allow cleanup/unregistration of PCI buses, Greg Kurz, 2017/07/25
- [Qemu-devel] [for-2.11 PATCH 13/26] qdev: store DeviceState's canonical path to use when unparenting, Greg Kurz, 2017/07/25
- [Qemu-devel] [for-2.11 PATCH 14/26] spapr_pci: add PHB unrealize, Greg Kurz, 2017/07/25
- [Qemu-devel] [for-2.11 PATCH 15/26] spapr: add pseries-2.11 machine type, Greg Kurz, 2017/07/25
- [Qemu-devel] [for-2.11 PATCH 16/26] spapr: enable PHB hotplug for pseries-2.11, Greg Kurz, 2017/07/25
[Qemu-devel] [for-2.11 PATCH 17/26] spapr_pci: introduce drc_id property, Greg Kurz, 2017/07/25