[Rust-VMM] [virtio-dev] [VHOST USER SPEC PATCH] vhost-user.rst: add clarifying language about protocol negotiation

[Stefan Hajnoczi]

On Mon, Mar 01, 2021 at 11:38:47AM +0000, Alex Bennée wrote:
> Stefan Hajnoczi <stefanha at redhat.com> writes:
> > On Fri, Feb 26, 2021 at 11:16:19AM +0000, Alex Bennée wrote:
> >> +However as the protocol negotiation something that only occurs between
> >
> > Missing "is". Shortening the sentence fixes that without losing clarity:
> > s/something that/negotiation/
> >
> >> +parts of the backend implementation it is permissible to for the master
> >
> > "vhost-user device backend" is often used to refer to the slave (to
> > avoid saying the word "slave") but "backend" is being used in a
> > different sense here. That is confusing.
> >
> >> +to mask the feature bit from the guest.
> >
> > I think this sentence effectively says "the master MAY mask the
> > VHOST_USER_F_PROTOCOL_FEATURES bit from the VIRTIO feature bits". That
> > is not really accurate since VIRTIO devices do not advertise this
> > feature bit and so it can never be negotiated through the VIRTIO feature
> > negotiation process.
> >
> > How about referring to the details from the VIRTIO 1.1 specification
> > instead. Something like this:
> >
> >   Note that VHOST_USER_F_PROTOCOL_FEATURES is the UNUSED (30) feature
> >   bit defined in `VIRTIO 1.1 6.3 Legacy Interface: Reserved Feature Bits
> >   <https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-4130003>`_.
> >   VIRTIO devices do not advertise this feature bit and therefore VIRTIO
> >   drivers cannot negotiate it.
> >
> >   This reserved feature bit was reused by the vhost-user protocol to add
> >   vhost-user protocol feature negotiation in a backwards compatible
> >   fashion. Old vhost-user master and slave implementations continue to
> >   work even though they are not aware of vhost-user protocol feature
> >   negotiation.
> 
> OK - so does that mean that feature bit will remain UNUSED for ever
> more?

It's unlikely to be repurposed in VIRTIO. It can never be used by VIRTIO
in a situation that overlaps with vhost-user. That leaves cases that
don't overlap with vhost-user but that is unlikely too since the bit had
a previous meaning (before vhost-user) and repurposing it would cause
confusion for very old drivers or devices.

> What about other feature bits? Is it permissible for the
> master/requester/vhost-user front-end/QEMU to filter any other feature
> bits the slave/vhost-user backend/daemon may offer from being read by
> the guest driver when it reads the feature bits?

Yes, the vhost-user frontend can decide how it wants to expose
VHOST_USER_GET_FEATURES feature bits on the VIRTIO device:

1. Pass-through. Allow the vhost-user device backend to control the
   feature bit.
2. Disabling. Clear a feature bit because it cannot be supported for
   some reason (e.g. VIRTIO 1.1 packed vrings are not implemented and
   therefore enabling them would prevent live migration).
3. Enabling. Enable a feature bit that does not rely on vhost-user
   device backend support. For example, message-signalled interrupts
   for virtio-mmio.

> 
> >
> >> As noted for the
> >> +``VHOST_USER_GET_PROTOCOL_FEATURES`` and
> >> +``VHOST_USER_SET_PROTOCOL_FEATURES`` messages this occurs before a
> >> +final ``VHOST_USER_SET_FEATURES`` comes from the guest.
> >
> > I couldn't find any place where vhost-user.rst states that
> > VHOST_USER_SET_PROTOCOL_FEATURES has to come before
> > VHOST_USER_SET_FEATURES?
> >
> > The only order I found was:
> >
> > 1. VHOST_USER_GET_FEATURES to determine whether protocol features are
> >    supported.
> > 2. VHOST_USER_GET_PROTOCOL_FEATURES to fetch available protocol feature bits.
> > 3. VHOST_USER_SET_PROTOCOL_FEATURES to set protocol feature bits.
> > 4. Using functionality that depends on enabled protocol feature bits.
> >
> > Is the purpose of this sentence to add a new requirement to the spec
> > that "VHOST_USER_SET_PROTOCOL_FEATURES MUST be sent before
> > VHOST_USER_SET_FEATURES"?
> 
> No I don't want to add a new sequence requirement. But if SET_FEATURES
> doesn't acknowledge the VHOST_USER_F_PROTOCOL_FEATURES bit should that
> stop the processing of
> VHOST_USER_GET_PROTOCOL_FEATURES/VHOST_USER_SET_PROTOCOL_FEATURES
> messages? AFAICT SET_FEATURES should be irrelevant to the negotiation of
> the PROTOCOL_FEATURES right?

I agree, the value of VHOST_USER_F_PROTOCOL_FEATURES in
VHOST_USER_SET_FEATURES does not matter according to the spec:

  Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is
  present in ``VHOST_USER_GET_FEATURES``.

Since it does not mention "set in VHOST_USER_SET_FEATURES" we have to
assume existing vhost-user device backends do not care whether the
vhost-user frontend includes the bit in VHOST_USER_SET_FEATURES or not.

Stefan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.opendev.org/pipermail/rust-vmm/attachments/20210301/bb0ac464/attachment.sig>