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

[Stefan Hajnoczi]

On Mon, Apr 17, 2023 at 05:14:59PM +0100, Alex Bennée wrote:
> 
> 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.

There are vhost-user device types that lack functionality entirely, like
vhost-user-net's lack of the controlq virtqueue and configuration space.
It is not even possible to query the MAC address from a vhost-user-net
device. It's not a full virtio-net device and the VMM has to fill in the
gaps to emulate the missing virtqueues and configuration space.

There are device type-specific vhost-user messages like
VHOST_USER_SEND_RARP and VHOST_USER_CREATE_CRYPTO_SESSION that can't
really be supported by a generic --device vhost-user-device.

Live migration is another device-specific aspect that is handled by the
vhost-user frontend.

> 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.

Optional vhost-user messages like configuration space access may be in
the spec, but existing device types cannot rely on them for backwards
compatibility reasons. Therefore most existing device types don't use
the configuration space. Those that do may only access parts of the
configuration space from the backend and emulate the rest.

The blog post I linked mentions a new protocol feature bit
(VHOST_USER_PROTOCOL_F_VDPA) to distinguish new vhost-user backends that
are full VIRTIO devices. This doesn't exist today, but I think something
like that is necessary to detect devices that will work with --device
vhost-user-device.

> > 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?

vhost-user needs:
- A GET_DEVICE_ID message.
- A GET_CONFIG_SIZE message. Today it is assumed that the vhost-user
  frontend already knows the configuration space size.
- A protocol feature bit indicating that the device is a full VIRTIO
  device. These devices also need to implement the SET_STATUS message,
  which is rarely implemented today.

> > 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?

The backend's features cannot be passed through to the guest because
there is also an emulated VIRTIO transport (virtio-pci, virtio-mmio,
virtio-ccw) involved. The transport may support a different feature set
from the vhost-user backend. For example, the transport may support
Packed Virtqueues but the backend may not. So some filtering is
necessary in the VMM.

Since some of the device-specific functionality may be handled by the
VMM, then this extends beyond just the transport feature bits.

But I agree with you that it's ugly and complex. I have to re-read the
code every time because it works in a strange way.

> > 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.

There is already a vhost-user message for querying the number of
virtqueues: VHOST_USER_GET_QUEUE_NUM.

See above for other missing stuff.

> > 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.

If you add the missing pieces to vhost-user then --device
vhost-user-device can be done properly. Two messages and one protocol
feature bit are required, it's not that bad.

Stefan

signature.asc
Description: PGP signature