Re: [Qemu-devel] [PATCH v4 1/2] vhost-user: Introduce a new protocol feature REPLY_ACK.

[Marc-André Lureau]

Hi

On Wed, Jul 27, 2016 at 1:52 PM, Prerna Saxena <address@hidden> wrote:
> From: Prerna Saxena <address@hidden>
>
> This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK.
>
> If negotiated, client applications should send a u64 payload in
> response to any message that contains the "need_response" bit set
> on the message flags. Setting the payload to "zero" indicates the
> command finished successfully. Likewise, setting it to "non-zero"
> indicates an error.
>
> Currently implemented only for SET_MEM_TABLE.
>
> Signed-off-by: Prerna Saxena <address@hidden>
> ---
>  docs/specs/vhost-user.txt | 41 +++++++++++++++++++++++++++++++++++++++++
>  hw/virtio/vhost-user.c    | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 73 insertions(+)
>
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 777c49c..57df586 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -37,6 +37,8 @@ consists of 3 header fields and a payload:
>   * Flags: 32-bit bit field:
>     - Lower 2 bits are the version (currently 0x01)
>     - Bit 2 is the reply flag - needs to be sent on each reply from the slave
> +   - Bit 3 is the need_response flag - see VHOST_USER_PROTOCOL_F_REPLY_ACK 
> for
> +     details.

Why need_response and not "need reply"?

btw, I wonder if it would be worth to introduce an enum at this point

>   * Size - 32-bit size of the payload
>
>
> @@ -126,6 +128,8 @@ the ones that do:
>   * VHOST_GET_VRING_BASE
>   * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD)
>
> +[ Also see the section on REPLY_ACK protocol extension. ]
> +
>  There are several messages that the master sends with file descriptors passed
>  in the ancillary data:
>
> @@ -254,6 +258,7 @@ Protocol features
>  #define VHOST_USER_PROTOCOL_F_MQ             0
>  #define VHOST_USER_PROTOCOL_F_LOG_SHMFD      1
>  #define VHOST_USER_PROTOCOL_F_RARP           2
> +#define VHOST_USER_PROTOCOL_F_REPLY_ACK      3
>
>  Message types
>  -------------
> @@ -464,3 +469,39 @@ Message types
>        is present in VHOST_USER_GET_PROTOCOL_FEATURES.
>        The first 6 bytes of the payload contain the mac address of the guest 
> to
>        allow the vhost user backend to construct and broadcast the fake RARP.
> +
> +VHOST_USER_PROTOCOL_F_REPLY_ACK:
> +-------------------------------
> +The original vhost-user specification only demands responses for certain

responses/replies

> +commands. This differs from the vhost protocol implementation where commands
> +are sent over an ioctl() call and block until the client has completed.
> +
> +With this protocol extension negotiated, the sender (QEMU) can set the newly
> +introduced "need_response" [Bit 3] flag to any command. This indicates that

need reply, you can remove the "newly introduced" (it's not going to
be so new after a while)

> +the client MUST respond with a Payload VhostUserMsg indicating success or

I would put right here for clarity:

...MUST respond with a Payload VhostUserMsg (unless the message has
already an explicit reply body)...

alternatively, I would forbid using the bit 3 on commands that have
already an explicit reply.

> +failure. The payload should be set to zero on success or non-zero on failure.
> +In other words, response must be in the following format :
> +
> +------------------------------------
> +| request | flags | size | payload |
> +------------------------------------
> +
> + * Request: 32-bit type of the request
> + * Flags: 32-bit bit field:
> + * Size: size of the payload ( see below)
> + * Payload : a u64 integer, where a non-zero value indicates a failure.
> +
> +This indicates to QEMU that the requested operation has deterministically
> +been met or not. Today, QEMU is expected to terminate the main vhost-user
> +loop upon receiving such errors. In future, qemu could be taught to be more
> +resilient for selective requests.
> +
> +Note that as per the original vhost-user protocol, the following four 
> messages
> +anyway require distinct responses from the vhost-user client process:

I don't think we need to repeat this list (already redundant with the
list in "Communication" part, and with the message specification, 2
times is enough imho)

> + * VHOST_GET_FEATURES
> + * VHOST_GET_PROTOCOL_FEATURES
> + * VHOST_GET_VRING_BASE
> + * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD)
> +
> +For these message types, the presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or
> +need_response bit being set brings no behaviourial change.

reply

> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 495e09f..0cdb918 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -31,6 +31,7 @@ enum VhostUserProtocolFeature {
>      VHOST_USER_PROTOCOL_F_MQ = 0,
>      VHOST_USER_PROTOCOL_F_LOG_SHMFD = 1,
>      VHOST_USER_PROTOCOL_F_RARP = 2,
> +    VHOST_USER_PROTOCOL_F_REPLY_ACK = 3,
>
>      VHOST_USER_PROTOCOL_F_MAX
>  };
> @@ -84,6 +85,7 @@ typedef struct VhostUserMsg {
>
>  #define VHOST_USER_VERSION_MASK     (0x3)
>  #define VHOST_USER_REPLY_MASK       (0x1<<2)
> +#define VHOST_USER_NEED_RESPONSE_MASK       (0x1 << 3)
>      uint32_t flags;
>      uint32_t size; /* the following payload size */
>      union {
> @@ -158,6 +160,25 @@ fail:
>      return -1;
>  }
>
> +static int process_message_reply(struct vhost_dev *dev,
> +                                    VhostUserRequest request)
> +{
> +    VhostUserMsg msg;
> +
> +    if (vhost_user_read(dev, &msg) < 0) {
> +        return 0;
> +    }
> +
> +    if (msg.request != request) {
> +        error_report("Received unexpected msg type."
> +                        "Expected %d received %d",
> +                        request, msg.request);
> +        return -1;
> +    }
> +
> +    return msg.payload.u64 ? -1 : 0;
> +}
> +
>  static bool vhost_user_one_time_request(VhostUserRequest request)
>  {
>      switch (request) {
> @@ -239,11 +260,18 @@ static int vhost_user_set_mem_table(struct vhost_dev 
> *dev,
>      int fds[VHOST_MEMORY_MAX_NREGIONS];
>      int i, fd;
>      size_t fd_num = 0;
> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                            VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +
>      VhostUserMsg msg = {
>          .request = VHOST_USER_SET_MEM_TABLE,
>          .flags = VHOST_USER_VERSION,
>      };
>
> +    if (reply_supported) {
> +        msg.flags |= VHOST_USER_NEED_RESPONSE_MASK;
> +    }
> +
>      for (i = 0; i < dev->mem->nregions; ++i) {
>          struct vhost_memory_region *reg = dev->mem->regions + i;
>          ram_addr_t offset;
> @@ -277,6 +305,10 @@ static int vhost_user_set_mem_table(struct vhost_dev 
> *dev,
>
>      vhost_user_write(dev, &msg, fds, fd_num);
>
> +    if (reply_supported) {
> +        return process_message_reply(dev, msg.request);
> +    }
> +
>      return 0;
>  }
>
> --
> 1.8.1.2
>



-- 
Marc-André Lureau