Re: [Qemu-devel] [PATCH 1/1] vhost-user : Introduce a new feature VHOST_USER_PROTOCOL_F_REPLY_ACK. This feature, if negotiated, forces the remote vhost-user process to send a u64 reply containing a status code for each requested operation. Status codes are '0' for success, and non-zero for error.

[Michael S. Tsirkin]

On Fri, Jun 24, 2016 at 01:17:08AM -0700, Prerna Saxena wrote:
> From: Prerna Saxena <address@hidden>
> 
> Signed-off-by: Prerna Saxena <address@hidden>
> ---
>  docs/specs/vhost-user.txt |  36 +++++++++++
>  hw/virtio/vhost-user.c    | 153 
> +++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 186 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 777c49c..e5388b2 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -37,6 +37,7 @@ 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.
>   * Size - 32-bit size of the payload
>  
>  
> @@ -126,6 +127,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 +257,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 +468,35 @@ 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
> +commands. This differs from the vhost protocol implementation where commands
> +are sent over an ioctl() call and block until the client has completed. Not
> +receiving a response for commands like VHOST_SET_MEM_TABLE makes the sender
> +unable to tell when the client has finished (re)mapping the GPA, or whether 
> it
> +has failed altogether.

I'd just drop this from spec.


> +
> +With this protocol extension negotiated, the sender can set the newly
> +introduced "need_response" [Bit 3] flag to any command. This indicates that
> +the client MUST to respond with a Payload VhostUserMsg indicating success or
> +failure. The payload should be set to zero on success or non-zero on failure.

Are there any cases where qemu can handle failure?
If not, please specify that it's for debugging.
Let's add a string for debugging as well?

> +In other words, response must be in the following format :
> +------------------------------------
> +| request | flags | size | payload |
> +------------------------------------
> +
> + * Request: 32-bit type of the original request which is being responded to.
> + * Flags: 32-bit bit field: (VHOST_USER_VERSION | VHOST_USER_REPLY_MASK)
> + * Size: size of the payload ( see below)
> + * Payload : a u64 integer, where a non-zero value indicates a failure.
> +
> +Note that as per the original vhost-user protocol, the following four 
> messages anyway
> +require distinct responses from the vhost-user client process :
> + * 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.

This needs to be reworded to avoid distinction between original and new.
Basically compare behaviour when VHOST_USER_PROTOCOL_F_REPLY_ACK is
negotiated and not.


> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 495e09f..f01ebb4 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
>  };
> @@ -82,8 +83,9 @@ typedef struct VhostUserLog {
>  typedef struct VhostUserMsg {
>      VhostUserRequest request;
>  
> -#define VHOST_USER_VERSION_MASK     (0x3)
> -#define VHOST_USER_REPLY_MASK       (0x1<<2)
> +#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 {
> @@ -239,10 +241,17 @@ 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,
>      };
> +    VhostUserRequest request = msg.request;
> +
> +    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;
> @@ -277,6 +286,20 @@ static int vhost_user_set_mem_table(struct vhost_dev 
> *dev,
>  
>      vhost_user_write(dev, &msg, fds, fd_num);
>  
> +    if (reply_supported) {
> +        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;
> +    }
> +
>      return 0;
>  }
>  
> @@ -289,9 +312,29 @@ static int vhost_user_set_vring_addr(struct vhost_dev 
> *dev,
>          .payload.addr = *addr,
>          .size = sizeof(msg.payload.addr),
>      };
> +    VhostUserRequest request = msg.request;
> +
> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                                    VHOST_USER_PROTOCOL_F_REPLY_ACK);
>  
> +    if (reply_supported) {
> +        msg.flags |= VHOST_USER_NEED_RESPONSE_MASK;
> +    }
>      vhost_user_write(dev, &msg, NULL, 0);
>  
> +    if (reply_supported) {
> +        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;
> +    }
>      return 0;
>  }
>  
> @@ -313,8 +356,28 @@ static int vhost_set_vring(struct vhost_dev *dev,
>          .size = sizeof(msg.payload.state),
>      };
>  
> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                                    VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +
> +    if (reply_supported) {
> +        msg.flags |= VHOST_USER_NEED_RESPONSE_MASK;
> +    }
> +
>      vhost_user_write(dev, &msg, NULL, 0);
>  
> +    if (reply_supported) {
> +        if (vhost_user_read(dev, &msg) < 0) {
> +            return 0;
> +        }
> +
> +        if (msg.request != request) {
> +            error_report("Received unexpected msg type."
> +                            "Expected %lu received %lu",
> +                            request, msg.request);
> +            return -1;
> +        }
> +        return msg.payload.u64 ? -1 : 0;
> +    }
>      return 0;
>  }
>  
> @@ -395,6 +458,13 @@ static int vhost_set_vring_file(struct vhost_dev *dev,
>          .size = sizeof(msg.payload.u64),
>      };
>  
> +    bool reply_Supported = virtio_has_feature(dev->protocol_features,
> +                                    VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +
> +    if (reply_supported) {
> +        msg.flags |= VHOST_USER_NEED_RESPONSE_MASK;
> +    }
> +
>      if (ioeventfd_enabled() && file->fd > 0) {
>          fds[fd_num++] = file->fd;
>      } else {
> @@ -403,6 +473,20 @@ static int vhost_set_vring_file(struct vhost_dev *dev,
>  
>      vhost_user_write(dev, &msg, fds, fd_num);
>  
> +    if (reply_supported) {
> +        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;
> +    }
> +
>      return 0;
>  }
>  
> @@ -489,8 +573,30 @@ static int vhost_user_set_owner(struct vhost_dev *dev)
>          .flags = VHOST_USER_VERSION,
>      };
>  
> +    VhostUserRequest request = msg.request;
> +
> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                                    VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +
> +    if (reply_supported) {
> +        msg.flags |= VHOST_USER_NEED_RESPONSE_MASK;
> +    }
> +
>      vhost_user_write(dev, &msg, NULL, 0);
>  
> +    if (reply_supported) {
> +        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;
> +    }
>      return 0;
>  }
>  
> @@ -500,9 +606,30 @@ static int vhost_user_reset_device(struct vhost_dev *dev)
>          .request = VHOST_USER_RESET_OWNER,
>          .flags = VHOST_USER_VERSION,
>      };
> +    VhostUserRequest request = msg.request;
> +
> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                                    VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +
> +    if (reply_supported) {
> +        msg.flags |= VHOST_USER_NEED_RESPONSE_MASK;
> +    }
>  
>      vhost_user_write(dev, &msg, NULL, 0);
>  
> +    if (reply_suported) {
> +        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;
> +    }
>      return 0;
>  }
>  
> @@ -589,9 +716,15 @@ static int vhost_user_migration_done(struct vhost_dev 
> *dev, char* mac_addr)
>  {
>      VhostUserMsg msg = { 0 };
>      int err;
> +    VhostUserRequest request = msg.request;
> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                                    VHOST_USER_PROTOCOL_F_REPLY_ACK);
>  
>      assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
>  
> +    if (reply_supported) {
> +        msg.flags |= VHOST_USER_NEED_RESPONSE_MASK;
> +    }
>      /* If guest supports GUEST_ANNOUNCE do nothing */
>      if (virtio_has_feature(dev->acked_features, 
> VIRTIO_NET_F_GUEST_ANNOUNCE)) {
>          return 0;
> @@ -606,7 +739,21 @@ static int vhost_user_migration_done(struct vhost_dev 
> *dev, char* mac_addr)
>          msg.size = sizeof(msg.payload.u64);
>  
>          err = vhost_user_write(dev, &msg, NULL, 0);
> -        return err;
> +        if (reply_supported) {
> +            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;
> +        } else {
> +            return err;
> +        }
>      }
>      return -1;

Please add comments documenting why we are waiting
for a response here.

Also, pls add a helper to reduce code duplication, and also to support
!VHOST_USER_PROTOCOL_F_REPLY_ACK.

>  }
> -- 
> 1.8.1.2
>