qmp_device_add() and hmp_device_add() drain pending RCU callbacks after a failed device_add, since some bus teardown (e.g. bus_remove_child()) is deferred to call_rcu(). -device on the command line reaches the same qdev_device_add_from_qdict() with errp pointing at error_fatal, whose ERRP_GUARD() exits before control returns to the caller, so the caller's drain_call_rcu() never runs.
Move the drain into qdev_device_add_from_qdict()'s own err_del_dev path and drop the now-redundant calls in qmp_device_add() and hmp_device_add(). Gate it behind a new drain_rcu parameter, because virtio-net failover can reach qdev_device_add_from_qdict() from an MMIO write dispatched under address_space_write()'s RCU read lock. Signed-off-by: Marc-André Lureau <[email protected]> --- include/monitor/qdev.h | 3 ++- hw/audio/intel-hda.c | 2 +- hw/net/virtio-net.c | 2 +- system/qdev-monitor.c | 42 ++++++++++++++++-------------------------- 4 files changed, 20 insertions(+), 29 deletions(-) diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h index f85f25738d58..72d2fa4654d3 100644 --- a/include/monitor/qdev.h +++ b/include/monitor/qdev.h @@ -12,7 +12,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp); int qdev_device_help(QemuOpts *opts); DeviceState *qdev_device_add(QemuOpts *opts, Error **errp); DeviceState *qdev_device_add_from_qdict(const QDict *opts, - bool from_json, Error **errp); + bool from_json, bool drain_rcu, + Error **errp); BusState *qdev_find_default_bus(DeviceClass *dc, Error **errp); /** diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c index 3d361a4976c6..128c5ee612ce 100644 --- a/hw/audio/intel-hda.c +++ b/hw/audio/intel-hda.c @@ -1313,7 +1313,7 @@ static void intel_hda_and_codec_init(const char *audiodev) BusState *hdabus; qdict_put_str(props, "driver", "intel-hda"); - intel_hda = qdev_device_add_from_qdict(props, false, &error_fatal); + intel_hda = qdev_device_add_from_qdict(props, false, true, &error_fatal); hdabus = QLIST_FIRST(&intel_hda->child_bus); codec = qdev_new("hda-duplex"); diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index f0e3beb29032..0c5da8cb71af 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -916,7 +916,7 @@ static void failover_add_primary(VirtIONet *n, Error **errp) dev = qdev_device_add_from_qdict(n->primary_opts, n->primary_opts_from_json, - &err); + false, &err); if (err) { qobject_unref(n->primary_opts); n->primary_opts = NULL; diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c index 00fed791cce1..77508bb1c3f1 100644 --- a/system/qdev-monitor.c +++ b/system/qdev-monitor.c @@ -650,7 +650,8 @@ BusState *qdev_find_default_bus(DeviceClass *dc, Error **errp) } DeviceState *qdev_device_add_from_qdict(const QDict *opts, - bool from_json, Error **errp) + bool from_json, bool drain_rcu, + Error **errp) { ERRP_GUARD(); DeviceClass *dc; @@ -746,6 +747,18 @@ err_del_dev: object_unparent(OBJECT(dev)); object_unref(OBJECT(dev)); + /* + * Some bus teardown (e.g. bus_remove_child()) is deferred via + * call_rcu(). When safe, drain those callbacks so the failed + * device is fully torn down before we return. Callers that are + * already inside an RCU read-side critical section (e.g. MMIO + * dispatch during virtio-net failover) must pass drain_rcu=false + * to avoid deadlocking on the grace period. + */ + if (drain_rcu) { + drain_call_rcu(); + } + return NULL; } @@ -755,7 +768,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) QDict *qdict = qemu_opts_to_qdict(opts, NULL); DeviceState *ret; - ret = qdev_device_add_from_qdict(qdict, false, errp); + ret = qdev_device_add_from_qdict(qdict, false, true, errp); if (ret) { qemu_opts_del(opts); } @@ -869,19 +882,7 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) { DeviceState *dev; - dev = qdev_device_add_from_qdict(qdict, true, errp); - if (!dev) { - /* - * Drain all pending RCU callbacks. This is done because - * some bus related operations can delay a device removal - * (in this case this can happen if device is added and then - * removed due to a configuration error) - * to a RCU callback, but user might expect that this interface - * will finish its job completely once qmp command returns result - * to the user - */ - drain_call_rcu(); - } + dev = qdev_device_add_from_qdict(qdict, true, true, errp); object_unref(OBJECT(dev)); } @@ -1017,17 +1018,6 @@ void hmp_device_add(Monitor *mon, const QDict *qdict) } dev = qdev_device_add(opts, &err); if (!dev) { - /* - * Drain all pending RCU callbacks. This is done because - * some bus related operations can delay a device removal - * (in this case this can happen if device is added and then - * removed due to a configuration error) - * to a RCU callback, but user might expect that this interface - * will finish its job completely once qmp command returns result - * to the user - */ - drain_call_rcu(); - qemu_opts_del(opts); } object_unref(dev); -- 2.55.0
