From: Michael Roth <mdroth@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.
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Greg Kurz <groug@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;
On Tue, Jul 25, 2017 at 08:00:47PM +0200, Greg Kurz wrote:
> From: Michael Roth <mdroth@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?
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Greg Kurz <groug@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
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 <mdroth@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.
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 <mst@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > Signed-off-by: Greg Kurz <groug@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
On Wed, 26 Jul 2017 15:24:43 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Tue, Jul 25, 2017 at 08:00:47PM +0200, Greg Kurz wrote:
> > From: Michael Roth <mdroth@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...
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > Signed-off-by: Greg Kurz <groug@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;
> >
>
On Thu, Jul 27, 2017 at 06:50:42PM +0200, Greg Kurz wrote:
> On Wed, 26 Jul 2017 15:24:43 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Tue, Jul 25, 2017 at 08:00:47PM +0200, Greg Kurz wrote:
> > > From: Michael Roth <mdroth@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 <mst@redhat.com>
> > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > Signed-off-by: Greg Kurz <groug@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
© 2016 - 2025 Red Hat, Inc.