Re: [PATCH v2] vhost-user.rst: add clarifying language about protocol negotiation

[Stefan Hajnoczi]

On Thu, Mar 04, 2021 at 06:11:26PM +0000, Alex Bennée wrote:
> 
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > On Wed, Mar 03, 2021 at 05:01:05PM -0500, Michael S. Tsirkin wrote:
> >> On Wed, Mar 03, 2021 at 02:50:11PM +0000, Alex Bennée wrote:
> >> Also, are we sure it's ok to send the messages and then send
> >> VHOST_USER_SET_FEATURES with VHOST_USER_F_PROTOCOL_FEATURES clear?
> >> Looks more like a violation to me ...
> >
> > Looking again I agree it would be a violation to omit
> > VHOST_USER_F_PROTOCOL_FEATURES in VHOST_USER_SET_FEATURES.
> >
> > Previously I only looked at VHOST_USER_SET_PROTOCOL_FEATURES where the
> > spec says:
> >
> >   Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in
> >   ``VHOST_USER_GET_FEATURES``.
> >
> > So negotiation is *not* necessary for sending
> > VHOST_USER_SET_PROTOCOL_FEATURES.
> >
> > However, I missed that other features *do* require negotiation of
> > VHOST_USER_F_PROTOCOL_FEATURES according to the spec. For example:
> >
> >   If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the
> >   ring is initialized in an enabled state.
> >
> > Now I think:
> >
> > 1. VHOST_USER_F_PROTOCOL_FEATURES *must* be included
> >    VHOST_USER_SET_FEATURES if the master supports it.
> 
> So added by the master - still invisible to the guest?

The relationship between a guest-visible VIRTIO device and a vhost-user
device is not covered by any specification AFAIK.

My opinion is that VMMs should not expose VHOST_USER_F_PROTOCOL_FEATURES
to the guest since that bit has a different meaning in VIRTIO.

If VMMs do expose it to the guest then they'll probably get away with it
since bit 30 is unused in VIRTIO. Drivers will probably not enable the
bit, even if the device reports it.

> >
> > 2. VHOST_USER_SET_PROTOCOL_FEATURES does not require negotiation,
> >    instead the master just needs to check that
> >    VHOST_USER_F_PROTOCOL_FEATURES is included in the
> >    VHOST_USER_GET_FEATURES reply. It's an exception.
> 
> OK I'm now thoroughly confused but I guess that's a good thing. However
> if we make the changes to QEMU to honour this won't we break with
> existing vhost-user receivers? We'll also need to track the state of a
> SET_FEATURES has happened and then gate the sending of things like
> reply_ack requests?

(To avoid confusion below when I write about negotiating
VHOST_USER_F_PROTOCOL_FEATURES it means sending VHOST_USER_SET_FEATURES
with the feature bit enabled. It shouldn't be confused with sending
VHOST_USER_SET_PROTOCOL_FEATURES to negotiate protocol features.)

I went through the spec and checked where VHOST_USER_F_PROTOCOL_FEATURES
must be negotiated. There is only one place: ring initialization state
(enabled/disabled). This makes sense: for backwards compatibility the
device backend needs to know whether or not the master supports
VHOST_USER_F_PROTOCOL_FEATURES.

There are also two messages where the spec says they "should" only be
sent when VHOST_USER_F_PROTOCOL_FEATURES has been negotiated:
VHOST_USER_SET_VRING_ENABLE and VHOST_USER_SET_SLAVE_REQ_FD. However,
QEMU's vhost-user master implementation actually sends
VHOST_USER_SET_SLAVE_REQ_FD before negotiating
VHOST_USER_F_PROTOCOL_FEATURES. I *think* VHOST_USER_SET_VRING_ENABLE is
really only sent after VHOST_USER_F_PROTOCOL_FEATURES has been
negotiated but I haven't checked all code paths.

All other mentions of VHOST_USER_F_PROTOCOL_FEATURES in the spec only
require checking VHOST_USER_GET_FEATURES, not negotiation.

So QEMU follows the current spec pretty closely. I don't anything needs
to be changed in QEMU. Have you found something?

Stefan

signature.asc
Description: PGP signature