Stefan Hajnoczi <stefa...@redhat.com> writes:
> On Fri, Aug 02, 2024 at 10:01:20AM +0200, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefa...@redhat.com> writes:
>>
>> > The QMP device_add monitor command converts the QDict arguments to
>> > QemuOpts and then back again to QDict. This process only supports scalar
>> > types. Device properties like virtio-blk-pci's iothread-vq-mapping (an
>> > array of objects) are silently dropped by qemu_opts_from_qdict() during
>> > the QemuOpts conversion even though QAPI is capable of validating them.
>> > As a result, hotplugging virtio-blk-pci devices with the
>> > iothread-vq-mapping property does not work as expected (the property is
>> > ignored).
>> >
>> > Get rid of the QemuOpts conversion in qmp_device_add() and call
>> > qdev_device_add_from_qdict() with from_json=true. Using the QMP
>> > command's QDict arguments directly allows non-scalar properties.
>> >
>> > The HMP is also adjusted since qmp_device_add()'s now expects properly
>> > typed JSON arguments and cannot be used from HMP anymore. Move the code
>> > that was previously in qmp_device_add() (with QemuOpts conversion and
>> > from_json=false) into hmp_device_add() so that its behavior is
>> > unchanged.
>> >
>> > This patch changes the behavior of QMP device_add but not HMP
>> > device_add. QMP clients that sent incorrectly typed device_add QMP
>> > commands no longer work. This is a breaking change but clients should be
>> > using the correct types already. See the netdev_add QAPIfication in
>> > commit db2a380c8457 for similar reasoning and object-add in commit
>> > 9151e59a8b6e. Unlike those commits, we continue to rely on 'gen': false
>> > for the time being.
>> >
>> > Move the drain_call_rcu() invocation into qdev_device_add_from_qdict()
>> > so all callers benefit from it automatically. This avoids code
>> > duplication.
>> >
>> > Markus helped me figure this out and even provided a draft patch. The
>> > code ended up very close to what he suggested.
>> >
>> > Suggested-by: Markus Armbruster <arm...@redhat.com>
>> > Cc: Daniel P. Berrangé <berra...@redhat.com>
>> > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
>> > ---
>> > system/qdev-monitor.c | 56 ++++++++++++++++++++++---------------------
>> > 1 file changed, 29 insertions(+), 27 deletions(-)
>> >
>> > diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
>> > index 6af6ef7d66..8a756b1a91 100644
>> > --- a/system/qdev-monitor.c
>> > +++ b/system/qdev-monitor.c
>> > @@ -725,6 +725,17 @@ err_del_dev:
>> > if (dev) {
>> > object_unparent(OBJECT(dev));
>> > object_unref(OBJECT(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();
>> > }
>> > return NULL;
>> > }
>>
>> Moving this from qmp_device_add() adds RCU draining to call chains not
>> going through qmp_device_add().
>>
>> Can adding it hurt? I guess it can't.
>>
>> Can it fix bugs? I don't know.
>>
>> Let's review the callers of qdev_device_add_from_qdict():
>>
>> * qdev_device_add()
>>
>> - called from qmp_device_add()
>>
>> No change.
>>
>> - called from device_init_func() called from qemu_create_cli_devices()
>>
>> See below.
>>
>> - called from usbback_portid_add() called from usbback_process_port()
>> called from usbback_backend_changed()
>>
>> · called from usbback_init()
>>
>> · called as XenDevOps method backend_changed()
>>
>> This is Xen. We now drain pending RCU callbacks. Impact? Beats
>> me.
>>
>> * qemu_create_cli_devices() called from qmp_x_exit_preconfig()
>>
>> - as QMP command with -preconfig, phase must be
>> PHASE_MACHINE_INITIALIZED
>>
>> - called from qemu_init() without -preconfig
>>
>> We now drain pending RCU callbacks. Can any be pending at this
>> early point? If not, the change is a no-op.
>>
>> * failover_add_primary() called from virtio_net_set_features() called as
>> VirtioDeviceClass method set_features()
>>
>> This is virtio-net failover. We now drain pending RCU callbacks.
>> Impact? Beats me.
>>
>> My gut feeling is "improvement, possibly even a bug fix". It deserves
>> its own commit, doesn't it?
>
> I thought I'd checked it, but now that I have reviewed the functions you
> looked at, I no longer think it's safe to move drain_call_rcu() into
> qdev_device_add_from_qdict(). drain_call_rcu() drops the BQL and must
> not be called from non-reentrant multi-threaded code paths.
>
> For example, if drain_call_rcu() is called from device emulation code
> then another vCPU thread could execute that same device's emulation code
> while the BQL is dropped. That's usually unsafe because most device
> emulation code only expects to run in one thread at any given time
> thanks to the BQL.
>
> Here are the cases:
> - qemu_create_cli_devices: safe because this happens before device
> emulation runs in vCPU threads
> - Xen: safe because there are no vCPU threads when QEMU just provides
> device emulation in a normal Xen system. But I'm not sure if usbback
> can also be used in a QEMU's Xen HVM guest mode where KVM handles
> vCPUs, it might be a problem there.
> - virtio_net_set_features() -> failover_add_primary() is unsafe. Another
> vCPU thread could enter virtio-net emulation code while the thread
> running virtio_net_set_features() drops the lock.
I see.
Can you make an argument why the reason for RCU draining does not apply
in failover_add_primary()?
If it doesn't apply, capturing the argument in a comment would be nice.
If it does apply, a FIXME comment seems in order.
Also, RCU draining / BQL dropping vs. non-draining/dropping behavior of
non-static functions should be documented in function comments.
All this can be done on top.
> I think it's better to duplicate drain_call_rcu() into hmp_device_add()
> than to risk breaking the other code paths.
Makes sense.
