Re: [Qemu-devel] [PATCH RFC] vhost-user: protocol extensions

[Olivier MATZ]

Hi Michael,

On 04/06/2015 04:48 PM, Michael S. Tsirkin wrote:
> This adds several extensions to the vhost user protocol:
> - protocol feature negotiation similar to virtio features
> - ability to report request failures
> - ability to start/stop specific rings
> 
> I went over all vhost-user implementations I could find,
> and this seems to be compatible with them all.
> 
> One thing that still bothers me is that the protocol
> as defined is not a transport in the virtio spec sense:
> for example, there is no concept of status bits,
> or config space. I'm still mulling whether this makes
> any sense.
> 
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>  docs/specs/vhost-user.txt | 105 
> +++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 95 insertions(+), 10 deletions(-)
> 
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 650bb18..025ffe9 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -2,6 +2,7 @@ Vhost-user Protocol
>  ===================
>  
>  Copyright (c) 2014 Virtual Open Systems Sarl.
> +Copyright (c) 2015 Red Hat, Inc.
>  
>  This work is licensed under the terms of the GNU GPL, version 2 or later.
>  See the COPYING file in the top-level directory.
> @@ -35,11 +36,28 @@ consists of 3 header fields and a payload:
>  
>   * Request: 32-bit type of the request
>   * Flags: 32-bit bit field:
> -   - Lower 2 bits are the version (currently 0x01)
> +   - Lower 2 bits are the version (currently 0x1 or 0x2)
>     - Bit 2 is the reply flag - needs to be sent on each reply from the slave
>   * Size - 32-bit size of the payload
>  
>  
> +Version negotiation:
> +---------------------
> +
> +Requester can set any version (at the moment 0x1 or 0x2) on all requests.
> +Responder has two options:
> +1. ignore version on requests, set version 0x1 on all replies. Such responder
> +   is allowed to assume it will only get request codes that were allowed
> +   to use version 0x1 (though requests themselves can use any version).
> +2. retrieve version from request and respond with same version in reply.
> +   Such responder must ignore unexpected requests (and respond with
> +   setting VHOST_USER_MESSAGE_ERROR if appropriate).
> +
> +In both cases, responder must ignore version for all other purposes,
> +for example, it must not check it and terminate the connection,
> +or ignore a message with an unexpected version value.
> +

If I understand well, it is the role of the requester to determine the
version of the responder. This would be done as soon as possible, for
instance in the VHOST_USER_GET_FEATURES message by setting the version=2
in the message and checking its value in the answer.

If the responder is version 1, the requester won't send any version=2
message (case 1.). As the VHOST_USER_MESSAGE_ERROR does not exist in
version 1, what is the expected reply from the responder for this first
message if it only supports version 1?


> +
>  Depending on the request type, payload can be:
>  
>   * A single 64-bit integer
> @@ -110,26 +128,45 @@ implementing vhost-user have an equivalent ioctl to the 
> kernel implementation.
>  
>  The communication consists of master sending message requests and slave 
> sending
>  message replies. Most of the requests don't require replies. Here is a list 
> of
> -the ones that do:
> +the ones that always do:
> +
> + * VHOST_USER_GET_FEATURES
> + * VHOST_USER_GET_VRING_BASE
> + * VHOST_USER_GET_PROTOCOL_FEATURES
> + * VHOST_USER_GET_RUNNING
> +
> +If slave is unable to respond, it must mirror the request received,
> +and set VHOST_USER_MESSAGE_ERROR bit in the request field.
>  
> - * VHOST_GET_FEATURES
> - * VHOST_GET_VRING_BASE
> +If VHOST_USER_PROTOCOL_F_REPLY_ALL protocol feature bit is negotiated, slave
> +must respond to all other messages by mirroring the requests received, and
> +setting the high bit VHOST_USER_MESSAGE_ERROR in Request field if the request
> +received is unsupported.
> +      VHOST_USER_MESSAGE_ERROR 31
>  
>  There are several messages that the master sends with file descriptors passed
>  in the ancillary data:
>  
> - * VHOST_SET_MEM_TABLE
> - * VHOST_SET_LOG_FD
> - * VHOST_SET_VRING_KICK
> - * VHOST_SET_VRING_CALL
> - * VHOST_SET_VRING_ERR
> + * VHOST_USER_SET_MEM_TABLE
> + * VHOST_USER_SET_LOG_FD
> + * VHOST_USER_SET_VRING_KICK
> + * VHOST_USER_SET_VRING_CALL
> + * VHOST_USER_SET_VRING_ERR
>  
>  If Master is unable to send the full message or receives a wrong reply it 
> will
>  close the connection. An optional reconnection mechanism can be implemented.
>  
> -Message types
> +Ring processing
>  -------------
> +virtio ring processing is documented in the virtio spec.
> +slave starts processing a specific ring after detecting 
> VHOST_USER_SET_RUNNING
> +request with VHOST_USER_RUNNING value, or (if VHOST_USER_PROTOCOL_F_MUST_RUN
> +has not been negotiated) after detecting a ring kick, and stops ring 
> processing
> +after detecting VHOST_USER_RESET_OWNER, or VHOST_USER_SET_RUNNING with 0x0
> +value.

So the role of VHOST_USER_SET_RUNNING is to enable or disable the
processing of a ring. Which component and what event would generate
this message? Does it come from the VM or from qemu?
 - VM -> qemu -> vhost-user-app?
 - or qemu -> vhost-user-app?

My underlying question is to understand how this flag will be used.

>  
> +Message types
> +-------------
>   * VHOST_USER_GET_FEATURES
>  
>        Id: 1
> @@ -264,3 +301,51 @@ Message types
>        Bits (0-7) of the payload contain the vring index. Bit 8 is the
>        invalid FD flag. This flag is set when there is no file descriptor
>        in the ancillary data.
> +
> + * VHOST_USER_GET_RUNNING
> +      Id: 15
> +      Ioctl: VHOST_NET_SET_BACKEND
> +      Master payload: vring state description
> +
> +      Not valid under protocol version 0x1.
> +      Only valid if VHOST_USER_PROTOCOL_F_MUST_RUN has been
> +      negotiated.
> +      Report whether backend is processing descriptors from a given queue.
> +      Values: num = 0x1 - VHOST_USER_RUNNING; num = 0x0 - not running
> +
> + * VHOST_USER_SET_RUNNING
> +      Id: 16
> +      Ioctl: VHOST_NET_SET_BACKEND
> +      Master payload: vring state description
> +
> +      Not valid under protocol version 0x1.
> +      Only valid if VHOST_USER_PROTOCOL_F_MUST_RUN has been
> +      negotiated.
> +      Stop/start processing descriptors from a given queue.
> +      Values: num = 0x1 - VHOST_USER_RUNNING; num = 0x0 - not running
> +
> + * VHOST_USER_GET_PROTOCOL_FEATURES
> +
> +      Id: 17
> +      Equivalent ioctl: N/A
> +      Master payload: N/A
> +      Slave payload: u64
> +
> +      Get the protocol features bitmask.
> +      Not valid under protocol version 0x1.
> +      At the moment two feature masks are defined:
> +      VHOST_USER_PROTOCOL_F_REPLY_ALL 0 - causes slave to reply to all 
> messages

I'm not sure to understand why such flag is needed. Could you
please give some more explanations?

> +      VHOST_USER_PROTOCOL_F_MUST_RUN 1  - slave won't process rings unless
> +      VHOST_USER_SET_RUNNING is called.
> +
> + * VHOST_USER_SET_PROTOCOL_FEATURES
> +
> +      Id: 18
> +      Ioctl: VHOST_SET_FEATURES
> +      Master payload: u64
> +
> +      Get the protocol features bitmask.
> +      Not valid under protocol version 0x1.
> +      At the moment a single feature mask bit is defined:
> +      VHOST_USER_PROTOCOL_F_REPLY_ALL 0
> +      If set, all messages will get a reply.

So VHOST_USER_PROTOCOL_F_MUST_RUN is a read-only property of the
responder, as it cannot be set in this command. Or is it a typo?
We can see above in the document "Only valid if
VHOST_USER_PROTOCOL_F_MUST_RUN has been negaciated".


Regards,
Olivier