On Thu, Jul 27, 2017 at 06:50:42PM +0200, Greg Kurz wrote: > On Wed, 26 Jul 2017 15:24:43 +1000 > David Gibson <da...@gibson.dropbear.id.au> wrote: > > > On Tue, Jul 25, 2017 at 08:00:47PM +0200, Greg Kurz wrote: > > > From: Michael Roth <mdr...@linux.vnet.ibm.com> > > > > > > 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. > > > > If we made that the case, does this problem go away? > > > > I'm no expert either and I confess I took this patch simply because it was > in Michael's original patchset. :) > > But according to Michael's answer, it seems that the issue has a broader > scope than just PHB hotplug...
Ok. I see Michael has posted this and a couple of other things separately. Let's hope that can get resolved upstream, and rebase this series on top of the result. > > > > Cc: Michael S. Tsirkin <m...@redhat.com> > > > Cc: Paolo Bonzini <pbonz...@redhat.com> > > > Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com> > > > Signed-off-by: Greg Kurz <gr...@kaod.org> > > > --- > > > 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
signature.asc
Description: PGP signature