Re: [PATCH v2 3/7] docs: document non-net VHOST_USER_GET_FEATURES behavior

[Michael S. Tsirkin]

On Tue, Jun 09, 2020 at 06:02:14PM +0100, Stefan Hajnoczi wrote:
> QEMU enabled several feature bits for non-net devices without allowing
> the device backend to control them. This only works when the device
> backend implements support for those features. It won't work for new
> features like the packed virtqueue layout, where proper feature
> negotiation will be needed.
> 
> Document the legacy behavior and specify that device backends must
> report features so that we can avoid problems in the future.
> 
> Cc: Ben Walker <benjamin.walker@intel.com>
> Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> CCing SPDK and cloud-hypervisor folks in case they are affected. DPDK
> isn't affected since vhost-user-net performs full feature negotiation.
> ---
>  docs/interop/vhost-user.rst | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> index 3b1b6602c7..dfadee411d 100644
> --- a/docs/interop/vhost-user.rst
> +++ b/docs/interop/vhost-user.rst
> @@ -290,6 +290,27 @@ bit was dedicated for this purpose::
>  
>    #define VHOST_USER_F_PROTOCOL_FEATURES 30
>  
> +Feature negotiation
> +-------------------
> +The master fetches features from the backend using the
> +``VHOST_USER_GET_FEATURES`` message. The feature bits correspond to those 
> from
> +the virtio specification, VHOST_F_LOG_ALL (26), and
> +``VHOST_USER_F_PROTOCOL_FEATURES`` (30).

Seems to partially duplicate the description of the message
VHOST_USER_GET_FEATURES, except that is missing
description of VHOST_F_LOG_ALL. How about tweaking that
instead of adding more text? BTW description of VHOST_USER_GET_FEATURES
has some typos worth fixing ...




> +Backends must report all supported feature bits. If a feature bit is set then
> +the master may set it in the ``VHOST_USER_SET_FEATURES`` message. If a 
> feature
> +bit is cleared then the master must not set it in the
> +``VHOST_USER_SET_FEATURES`` message.

Again let's extend description of VHOST_USER_SET_FEATURES if that's
unclear. BTW description of VHOST_USER_SET_FEATURES has
some typos worth fixing ...

> +
> +For devices other than the networking device, masters may assume the 
> following
> +feature bits are always set in ``VHOST_USER_GET_FEATURES`` for compatibility
> +with legacy backend implementations that do not report them correctly:
> +* ``VIRTIO_F_RING_INDIRECT_DESC``
> +* ``VIRTIO_F_RING_EVENT_IDX``
> +* ``VIRTIO_F_VERSION_1``
> +* ``VIRTIO_F_NOTIFY_ON_EMPTY``
> +* ``VIRTIO_F_ANY_LAYOUT``
> +
>  Starting and stopping rings
>  ---------------------------

How common are these backends? Anything shipped for a while?  IIUC we
are not talking about years of history here, so I really think we should
just enforce what spec always said, rather than work around some broken
clients.

> -- 
> 2.26.2
>