Re: [PATCH 00/12] virtio: add vhost-user-generic and reduce copy and paste

["Alex Bennée" <alex.bennee@linaro.org>]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Fri, 14 Apr 2023 at 12:06, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> A lot of our vhost-user stubs are large chunks of boilerplate that do
>> (mostly) the same thing. This series attempts to fix that by defining
>> a new base class for vhost-user devices and then converting the rng
>> and gpio devices to be based off them. You can even use
>> vhost-user-device directly if you supply it with the right magic
>> numbers (which is helpful for development).
>>
>> However the final patch runs into the weeds because I don't yet have a
>> clean way to represent in QOM the fixing of certain properties for the
>> specialised classes.
>>
>> The series is a net reduction in code and an increase in
>> documentation but obviously needs to iron out a few more warts. I'm
>> open to suggestions on the best way to tweak the QOM stuff.
>
> --device vhost-user-device is not really possible because vhost-user
> devices are not full VIRTIO devices. vhost-user devices depend on
> device-specific code in the VMM by design.

What device specific code? You certainly need to instantiate stuff in
the DTB/ACPI tables for -M virt but everything else can be handed off to
the vhost-user daemon.

Indeed the split brain is a bit silly in some places. For example is
QEMU really the best arbiter of a block device config when the actual
backend is a separate process. We have config passing in the vhost-user
spec.

> The "subset of a VIRTIO device" design made sense for vhost_net.
> Nowadays there are other device types that are close to full VIRTIO
> devices, although the vhost-user protocol doesn't support the full
> VIRTIO device lifecycle.

What are we missing?

> I think a user-creatable --device vhost-user-device is not a good idea
> today. It creates confusion. Many people aren't aware of the
> architectural difference between vhost-user and VIRTIO devices. The
> result is that VMMs and vhost-user backends implement increasingly
> brittle VIRTIO configuration space and feature bit logic as they
> knowingly or unknowingly try to paper over the fact that a traditional
> vhost-user device isn't a full VIRTIO device.

I've always found the device feature gating in QEMU confusing. Surely we
can rely on the daemon to properly enumerate the features it supports?

> It is possible to resolve this difference and make --device
> vhost-user-device work properly for devices that want to be full
> VIRTIO devices. See "Making VMM device shims optional" here:
> https://blog.vmsplice.net/2020/09/on-unifying-vhost-user-and-virtio.html
>
> Even after extending the vhost-user protocol to solve the current
> limitations, existing backends would still only be partial VIRTIO
> devices that wouldn't work with --device vhost-user-device.

It works for RNG, GPIO (and soon I'll be testing i2c and SCSI). All we
need is the virtioid and the number of virtio queues.

> Reducing boilerplate is helpful, but I think --device
> vhost-user-device should not be user-creatable.

After this series lands it will certainly make adding new shims easier
but having a vhost-user-device will make testing of new backends easier.
Can we not simply document it as an advanced feature for those who know
what they are doing? I'm not intending to deprecate the existing shims.

>
> Stefan
>
>>
>> Alex.
>>
>> Alex Bennée (12):
>>   hw/virtio: fix typo in VIRTIO_CONFIG_IRQ_IDX comments
>>   include/hw/virtio: document virtio_notify_config
>>   include/hw/virtio: add kerneldoc for virtio_init
>>   include/hw/virtio: document some more usage of notifiers
>>   virtio: add generic vhost-user-device
>>   virtio: add PCI stub for vhost-user-device
>>   include: attempt to document device_class_set_props
>>   qom: allow for properties to become "fixed"
>>   hw/virtio: derive vhost-user-rng from vhost-user-device
>>   hw/virtio: add config support to vhost-user-device
>>   hw/virtio: derive vhost-user-gpio from vhost-user-device (!BROKEN)
>>   docs/system: add a basic enumeration of vhost-user devices
>>
>>  docs/system/devices/vhost-user-rng.rst |   2 +
>>  docs/system/devices/vhost-user.rst     |  41 +++
>>  qapi/qom.json                          |   2 +
>>  include/hw/qdev-core.h                 |   9 +
>>  include/hw/virtio/vhost-user-device.h  |  33 ++
>>  include/hw/virtio/vhost-user-gpio.h    |  23 +-
>>  include/hw/virtio/vhost-user-rng.h     |  11 +-
>>  include/hw/virtio/virtio.h             |  21 ++
>>  include/qom/object.h                   |  16 +-
>>  hw/display/vhost-user-gpu.c            |   4 +-
>>  hw/net/virtio-net.c                    |   4 +-
>>  hw/virtio/vhost-user-device-pci.c      |  71 +++++
>>  hw/virtio/vhost-user-device.c          | 359 ++++++++++++++++++++++
>>  hw/virtio/vhost-user-fs.c              |   4 +-
>>  hw/virtio/vhost-user-gpio.c            | 405 +------------------------
>>  hw/virtio/vhost-user-rng.c             | 264 +---------------
>>  hw/virtio/vhost-vsock-common.c         |   4 +-
>>  hw/virtio/virtio-crypto.c              |   4 +-
>>  qom/object.c                           |  14 +
>>  qom/object_interfaces.c                |   9 +-
>>  qom/qom-qmp-cmds.c                     |   1 +
>>  softmmu/qdev-monitor.c                 |   1 +
>>  hw/virtio/meson.build                  |   3 +
>>  23 files changed, 613 insertions(+), 692 deletions(-)
>>  create mode 100644 include/hw/virtio/vhost-user-device.h
>>  create mode 100644 hw/virtio/vhost-user-device-pci.c
>>  create mode 100644 hw/virtio/vhost-user-device.c
>>
>> --
>> 2.39.2
>>
>>


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro