[v2,0/2] qdev-monitor: avoid QemuOpts in QMP device_add

Message

Stefan Hajnoczi Aug. 1, 2024, 2:05 p.m. UTC

v2:
- Rename Patch 1 to indicate that we're avoiding QemuOpts rather than doing a
  full conversion to QAPI. Also mention that 'gen': false is still being used.
  [Markus]
- Add Patch 2 to address a TODO comment suggesting that
  qemu_create_cli_devices() should call qmp_device_add(). [Markus]
- Move drain_call_rcu() into qdev_device_add_from_qdict() to avoid code
  duplication. [Markus]

This series enables non-scalar parameter parsing in device_add (e.g.
virtio-blk-pci,iothread-vq-mapping=). Stop converting from QDict to QemuOpts
and back again as this loses type information and cannot represent non-scalars.

Stefan Hajnoczi (2):
  qdev-monitor: avoid QemuOpts in QMP device_add
  vl: use qmp_device_add() in qemu_create_cli_devices()

 system/qdev-monitor.c | 56 ++++++++++++++++++++++---------------------
 system/vl.c           | 14 ++++-------
 2 files changed, 33 insertions(+), 37 deletions(-)

Comments

Markus Armbruster Aug. 2, 2024, 8:10 a.m. UTC | #1

Can we additionally cut out the QemuOpts middleman in
usbback_portid_add()?

    qdict = qdict_new();
    qdict_put_str(qdict, "driver", "usb-host");
    tmp = g_strdup_printf("%s.0", usbif->xendev.qdev.id);
    qdict_put_str(qdict, "bus", tmp);
    g_free(tmp);
    tmp = g_strdup_printf("%s-%u", usbif->xendev.qdev.id, port);
    qdict_put_str(qdict, "id", tmp);
    g_free(tmp);
    qdict_put_int(qdict, "port", port);
    qdict_put_int(qdict, "hostbus", atoi(busid));
    qdict_put_str(qdict, "hostport", portname);
    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict,
                                &error_abort);
    usbif->ports[port - 1].dev = USB_DEVICE(qdev_device_add(opts, &local_err));

Trying this is up to you!

Stefan Hajnoczi Aug. 12, 2024, 6:15 p.m. UTC | #2

On Fri, Aug 02, 2024 at 10:10:43AM +0200, Markus Armbruster wrote:
> Can we additionally cut out the QemuOpts middleman in
> usbback_portid_add()?
> 
>     qdict = qdict_new();
>     qdict_put_str(qdict, "driver", "usb-host");
>     tmp = g_strdup_printf("%s.0", usbif->xendev.qdev.id);
>     qdict_put_str(qdict, "bus", tmp);
>     g_free(tmp);
>     tmp = g_strdup_printf("%s-%u", usbif->xendev.qdev.id, port);
>     qdict_put_str(qdict, "id", tmp);
>     g_free(tmp);
>     qdict_put_int(qdict, "port", port);
>     qdict_put_int(qdict, "hostbus", atoi(busid));
>     qdict_put_str(qdict, "hostport", portname);
>     opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict,
>                                 &error_abort);
>     usbif->ports[port - 1].dev = USB_DEVICE(qdev_device_add(opts, &local_err));
> 
> Trying this is up to you!

Paul or Anthony: Do you know how to run usbback_portid_add() for
testing? I would like to make sure that suggested the code change works
and don't have experience running the Xen code in QEMU.

Thanks,
Stefan

Paul Durrant Aug. 13, 2024, 8:18 a.m. UTC | #3

On 12/08/2024 19:15, Stefan Hajnoczi wrote:
> On Fri, Aug 02, 2024 at 10:10:43AM +0200, Markus Armbruster wrote:
>> Can we additionally cut out the QemuOpts middleman in
>> usbback_portid_add()?
>>
>>      qdict = qdict_new();
>>      qdict_put_str(qdict, "driver", "usb-host");
>>      tmp = g_strdup_printf("%s.0", usbif->xendev.qdev.id);
>>      qdict_put_str(qdict, "bus", tmp);
>>      g_free(tmp);
>>      tmp = g_strdup_printf("%s-%u", usbif->xendev.qdev.id, port);
>>      qdict_put_str(qdict, "id", tmp);
>>      g_free(tmp);
>>      qdict_put_int(qdict, "port", port);
>>      qdict_put_int(qdict, "hostbus", atoi(busid));
>>      qdict_put_str(qdict, "hostport", portname);
>>      opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict,
>>                                  &error_abort);
>>      usbif->ports[port - 1].dev = USB_DEVICE(qdev_device_add(opts, &local_err));
>>
>> Trying this is up to you!
> 
> Paul or Anthony: Do you know how to run usbback_portid_add() for
> testing? I would like to make sure that suggested the code change works
> and don't have experience running the Xen code in QEMU.

Sorry, PV USB is not something I'm familiar with. 
https://wiki.xenproject.org/wiki/Xen_USB_Passthrough suggests that `xl 
usbdev-attach` might be the way to test... but you'd need a system with 
Xen installed and suitably configured guest, so not trivial to set up.

Stefan Hajnoczi Aug. 27, 2024, 7:20 p.m. UTC | #4

On Tue, Aug 13, 2024 at 09:18:46AM +0100, Paul Durrant wrote:
> On 12/08/2024 19:15, Stefan Hajnoczi wrote:
> > On Fri, Aug 02, 2024 at 10:10:43AM +0200, Markus Armbruster wrote:
> > > Can we additionally cut out the QemuOpts middleman in
> > > usbback_portid_add()?
> > > 
> > >      qdict = qdict_new();
> > >      qdict_put_str(qdict, "driver", "usb-host");
> > >      tmp = g_strdup_printf("%s.0", usbif->xendev.qdev.id);
> > >      qdict_put_str(qdict, "bus", tmp);
> > >      g_free(tmp);
> > >      tmp = g_strdup_printf("%s-%u", usbif->xendev.qdev.id, port);
> > >      qdict_put_str(qdict, "id", tmp);
> > >      g_free(tmp);
> > >      qdict_put_int(qdict, "port", port);
> > >      qdict_put_int(qdict, "hostbus", atoi(busid));
> > >      qdict_put_str(qdict, "hostport", portname);
> > >      opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict,
> > >                                  &error_abort);
> > >      usbif->ports[port - 1].dev = USB_DEVICE(qdev_device_add(opts, &local_err));
> > > 
> > > Trying this is up to you!
> > 
> > Paul or Anthony: Do you know how to run usbback_portid_add() for
> > testing? I would like to make sure that suggested the code change works
> > and don't have experience running the Xen code in QEMU.
> 
> Sorry, PV USB is not something I'm familiar with.
> https://wiki.xenproject.org/wiki/Xen_USB_Passthrough suggests that `xl
> usbdev-attach` might be the way to test... but you'd need a system with Xen
> installed and suitably configured guest, so not trivial to set up.

Thanks for the pointer! I will leave the usbback_portid_add()
refactoring because I don't have a setup for testing it.

Stefan