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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4719 bytes --]

On Fri, Feb 26, 2021 at 11:16:19AM +0000, Alex Bennée wrote:
> In practice the protocol negotiation between vhost master and slave
> occurs before the final feature negotiation between backend and
> frontend. This has lead to an inconsistency between the rust-vmm vhost
> implementation and the libvhost-user library in their approaches to
> checking if all the requirements for REPLY_ACK processing were met.
> As this is purely a function of the protocol negotiation and not of
> interest to the frontend lets make the language clearer about the
> requirements for a successfully negotiated protocol feature.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Jiang Liu <gerry@linux.alibaba.com>
> ---
>  docs/interop/vhost-user.rst | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)

I had difficulty understanding this change and its purpose. I think it's
emphasizing what the spec already says: VHOST_USER_SET_PROTOCOL_FEATURES
can be sent after VHOST_USER_F_PROTOCOL_FEATURES was reported by
VHOST_USER_GET_FEATURES.

BTW Paolo has just sent a patch here to use the terms "frontend" and
"backend" with different meanings from how you are using them:
https://lists.nongnu.org/archive/html/qemu-devel/2021-02/msg08347.html

> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> index d6085f7045..3ac221a8c7 100644
> --- a/docs/interop/vhost-user.rst
> +++ b/docs/interop/vhost-user.rst
> @@ -301,12 +301,22 @@ If *slave* detects some error such as incompatible features, it may also
>  close the connection. This should only happen in exceptional circumstances.
>  
>  Any protocol extensions are gated by protocol feature bits, which
> -allows full backwards compatibility on both master and slave.  As
> -older slaves don't support negotiating protocol features, a feature
> +allows full backwards compatibility on both master and slave. As older
> +slaves don't support negotiating protocol features, a device feature
>  bit was dedicated for this purpose::
>  
>    #define VHOST_USER_F_PROTOCOL_FEATURES 30
>  
> +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.

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

> So the
> +enabling of protocol features need only require the advertising of the
> +feature by the slave and the successful get/set protocol features
> +sequence.

"the feature" == VHOST_USER_F_PROTOCOL_FEATURES?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]