Re: [PATCH v2 00/15] virtio-gpio and various virtio cleanups

[Stefan Hajnoczi]

On Thu, 7 Jul 2022 at 14:42, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
> > [[PGP Signed Part:Undecided]]
> > On Tue, May 24, 2022 at 04:40:41PM +0100, Alex Bennée wrote:
> >> Hi,
> >>
> >> This series ostensibly adds virtio-user-gpio stubs to the build for
> >> use with an external vhost-user daemon. We've been testing it with our
> >> rust daemons from:
> >>
> >>   https://github.com/rust-vmm/vhost-device
> >>
> >> Getting the test enabled took some doing most likely because the need
> >> for CONFIG support exercised additional paths in the code that were
> >> not used for the simpler virtio-net tests. As a result the series has
> >> a number of cleanup and documentation patches.
> >>
> >> The final thing that needed fixing was the ensuring that
> >> VHOST_USER_F_PROTOCOL_FEATURES didn't get squashed in the negotiation
> >> process. This was the hardest thing to track down as we store the
> >> feature bits in several places variously as:
> >>
> >>   in VirtIODevice as:
> >>     uint64_t guest_features;
> >>     uint64_t host_features;
> >>     uint64_t backend_features;
> >
> > None of these know about VHOST_USER_F_PROTOCOL_FEATURES and vhost-user's
> > unfiltered feature bits should never be passed to VirtIODevice.
> >
> >>
> >>  in vhost_dev as:
> >>     uint64_t features;
> >>     uint64_t acked_features;
> >>     uint64_t backend_features;
> >
> > I don't think these should know about VHOST_USER_F_PROTOCOL_FEATURES
> > either. AFAIK vhost_dev deals with VIRTIO feature bits, not raw
> > vhost-user GET_FEATURES.
>
> So where does VHOST_USER_F_PROTOCOL_FEATURES get set before it's set
> with the VHOST_USER_SET_FEATURES message? Currently it's fed via:
>
>     uint64_t features = vhost_dev->acked_features;
>
> in vhost_dev_set_features() which does apply a few extra bits
> (VHOST_F_LOG_ALL/VIRTIO_F_IOMMU_PLATFORM). Maybe it should be adding
> VHOST_USER_F_PROTOCOL_FEATURES here? How should it be signalled by the
> vhost-user backend that this should be done? Overload the function?

A modern vhost-user server replies to VHOST_USER_GET_FEATURES with
VHOST_USER_F_PROTOCOL_FEATURES set. That's when the vhost-user client
encounters this bit.

The vhost-user client should then filter out
VHOST_USER_F_PROTOCOL_FEATURES because it belongs to the vhost-user
protocol and isn't a real VIRTIO feature bit. The client uses the
filtered VIRTIO feature bits and it now knows whether the vhost-user
server supports the VHOST_USER_GET_PROTOCOL_FEATURES and
VHOST_USER_SET_PROTOCOL_FEATURES messages.

I think vhost_user_set_features() should set
VHOST_USER_F_PROTOCOL_FEATURES if the server returned it from
VHOST_USER_GET_FEATURES. At the moment vhost_user_backend_init()
stores VHOST_USER_F_PROTOCOL_FEATURES in struct
vhost_dev->backend_features, which only seems to be used by vhost-net
code.

The other vhost-user devices set acked_features = guest_features and
ignore backend_features. As a result I guess they don't set
VHOST_USER_F_PROTOCOL_FEATURES in the VHOST_USER_SET_FEATURES message.
Most vhost-user servers probably don't care and still respond to
VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES
messages (although the vhost-user protocol spec mentions other
protocol differences when VHOST_USER_F_PROTOCOL_FEATURES is not
negotiated).

Does this match what you've found? The code is a maze so I may have
gotten something wrong. In general I think hw/virtio/vhost-user.c
should be responsible for VHOST_USER_F_PROTOCOL_FEATURES and no other
part of the QEMU codebase should ever see the bit since it's a
vhost-user protocol detail and not part of VIRTIO or even a common
part of vhost.

Stefan