Message ID | 20171009170607.4155-4-mdroth@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
Series | qdev/vfio: defer DEVICE_DEL to avoid races with libvirt | expand |
On Mon, Oct 09, 2017 at 12:06:07PM -0500, Michael Roth wrote: > DEVICE_DEL is currently emitted when a Device is unparented, as > opposed to when it is finalized. The main design motivation for this > seems to be that after unparent()/unrealize(), the Device is no > longer visible to the guest, and thus the operation is complete > from the perspective of management. > > However, there are cases where remaining host-side cleanup is also > pertinent to management. The is generally handled by treating these > resources as aspects of the "backend", which can be managed via > separate interfaces/events, such as blockdev_add/del, netdev_add/del, > object_add/del, etc, but some devices do not have this level of > compartmentalization, namely vfio-pci, and possibly to lend themselves > well to it. > > In the case of vfio-pci, the "backend" cleanup happens as part of > the finalization of the vfio-pci device itself, in particular the > cleanup of the VFIO group FD. Failing to wait for this cleanup can > result in tools like libvirt attempting to rebind the device to > the host while it's still being used by VFIO, which can result in > host crashes or other misbehavior depending on the host driver. > > Deferring DEVICE_DEL still affords us the ability to manage backends > explicitly, while also addressing cases like vfio-pci's, so we > implement that approach here. > > An alternative proposal involving having VFIO emit a separate event > to denote completion of host-side cleanup was discussed, but the > prevailing opinion seems to be that it is not worth the added > complexity, and leaves the issue open for other Device implementations > solve in the future. > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > Reviewed-by: Greg Kurz <groug@kaod.org> > Tested-by: Eric Auger <eric.auger@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/core/qdev.c | 23 ++++++++++++----------- > 1 file changed, 12 insertions(+), 11 deletions(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index f7c66d9bd0..8b7b8c3280 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -1068,7 +1068,6 @@ static void device_finalize(Object *obj) > NamedGPIOList *ngl, *next; > > DeviceState *dev = DEVICE(obj); > - qemu_opts_del(dev->opts); > > QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) { > QLIST_REMOVE(ngl, node); > @@ -1079,6 +1078,18 @@ static void device_finalize(Object *obj) > * here > */ > } > + > + /* Only send event if the device had been completely realized */ > + if (dev->pending_deleted_event) { > + g_assert(dev->canonical_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); > } > > static void device_class_base_init(ObjectClass *class, void *data) > @@ -1108,16 +1119,6 @@ static void device_unparent(Object *obj) > object_unref(OBJECT(dev->parent_bus)); > dev->parent_bus = NULL; > } > - > - /* Only send event if the device had been completely realized */ > - if (dev->pending_deleted_event) { > - g_assert(dev->canonical_path); > - > - qapi_event_send_device_deleted(!!dev->id, dev->id, dev->canonical_path, > - &error_abort); > - g_free(dev->canonical_path); > - dev->canonical_path = NULL; > - } > } > > static void device_class_init(ObjectClass *class, void *data)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index f7c66d9bd0..8b7b8c3280 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -1068,7 +1068,6 @@ static void device_finalize(Object *obj) NamedGPIOList *ngl, *next; DeviceState *dev = DEVICE(obj); - qemu_opts_del(dev->opts); QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) { QLIST_REMOVE(ngl, node); @@ -1079,6 +1078,18 @@ static void device_finalize(Object *obj) * here */ } + + /* Only send event if the device had been completely realized */ + if (dev->pending_deleted_event) { + g_assert(dev->canonical_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); } static void device_class_base_init(ObjectClass *class, void *data) @@ -1108,16 +1119,6 @@ static void device_unparent(Object *obj) object_unref(OBJECT(dev->parent_bus)); dev->parent_bus = NULL; } - - /* Only send event if the device had been completely realized */ - if (dev->pending_deleted_event) { - g_assert(dev->canonical_path); - - qapi_event_send_device_deleted(!!dev->id, dev->id, dev->canonical_path, - &error_abort); - g_free(dev->canonical_path); - dev->canonical_path = NULL; - } } static void device_class_init(ObjectClass *class, void *data)