[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.7 v5.1 1/2] vhost-user: Introduce a new pr
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH for-2.7 v5.1 1/2] vhost-user: Introduce a new protocol feature REPLY_ACK. |
Date: |
Fri, 29 Jul 2016 16:47:30 +0400 |
Hi
On Thu, Jul 28, 2016 at 11:07 AM, 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_reply" 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 | 26 ++++++++++++++++++++++++++
> hw/virtio/vhost-user.c | 32 ++++++++++++++++++++++++++++++++
> 2 files changed, 58 insertions(+)
>
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 777c49c..54b5c8f 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_reply flag - see VHOST_USER_PROTOCOL_F_REPLY_ACK for
> + details.
> * 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,24 @@ 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 replies 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.
> +
> +With this protocol extension negotiated, the sender (QEMU) can set the
> +"need_reply" [Bit 3] flag to any command. This indicates that
> +the client MUST respond with a Payload VhostUserMsg indicating success or
> +failure. The payload should be set to zero on success or non-zero on failure.
> +(Unless the message already has an explicit reply body)
Unless/unless (for consistency, the rest of the document doesn't use
Upper-case inside parentheses)
> +
> +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.
> +
> +For the message types that already solicit a reply from the client, the
> +presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or need_reply bit being set
> brings
> +no behaviourial change. (See the 'Communication' section for details.)
See/see
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 495e09f..86e7ae0 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_REPLY_MASK (0x1 << 3)
You could align it, and use the same style as the line above
> 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)
bad indentation
> +{
> + VhostUserMsg msg;
> +
> + if (vhost_user_read(dev, &msg) < 0) {
> + return 0;
return -1
> + }
> +
> + if (msg.request != request) {
> + error_report("Received unexpected msg type."
> + "Expected %d received %d",
> + request, msg.request);
bad indentation
> + 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);
bad indentation
> +
> VhostUserMsg msg = {
> .request = VHOST_USER_SET_MEM_TABLE,
> .flags = VHOST_USER_VERSION,
> };
>
> + if (reply_supported) {
> + msg.flags |= VHOST_USER_NEED_REPLY_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
>
Other than that, looks good to me, with those fixes
Reviewed-by: Marc-André Lureau <address@hidden>
--
Marc-André Lureau
[Qemu-devel] [PATCH for-2.7 v5.1 2/2] vhost-user: Attempt to fix a race with set_mem_table., Prerna Saxena, 2016/07/28