[Qemu-devel] [PATCH 0/1] vhost-user: Add a protocol extension for client responses to vhost commands.

[Qemu-devel] [PATCH 0/1] vhost-user: Add a protocol extension for client responses to vhost commands.

[Prerna Saxena]

From: Prerna Saxena <prerna.saxena@nutanix.com>

The current vhost-user protocol requires the client to send responses to only few commands. For the remaining commands, it is impossible for QEMU to know the status of the requested operation -- ie, did it succeed at all, and if so, at what time.

This is inconvenient, and can also lead to races. As an example:

(1) qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net application) and SET_MEM_TABLE doesn't require a reply according to the spec.
(2) qemu commits the memory to the guest.
(3) guest issues an I/O operation over a new memory region which was configured on (1)
(4) The application hasn't yet remapped the memory, but it sees the I/O request.
(5) The application cannot satisfy the request because it doesn't know about those GPAs

Note that the kernel implementation does not suffer from this limitation since messages are sent via an ioctl(). The ioctl() blocks until the backend (eg. vhost-net) completes the command and returns (with an error code).

Changing the behaviour of current vhost-user commands would break existing applications. This patch introduces a protocol extension, VHOST_USER_PROTOCOL_F_REPLY_ACK. This feature, if negotiated, allows QEMU to annotate messages to the application that it seeks a response for. The application must then respond to qemu by providing a status about the requested operation.


Prerna Saxena (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 containin status code for each
               requested operation.          
               Status codes are '0' for success, and non-zero for error.

 docs/specs/vhost-user.txt |  36 +++++++++++
 hw/virtio/vhost-user.c    | 153 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 186 insertions(+), 3 deletions(-)

-- 
1.8.1.2

[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.

[Prerna Saxena]

From: Prerna Saxena <prerna.saxena@nutanix.com>

Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com>
---
 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.
+
+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.
+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.
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;
 }
-- 
1.8.1.2

Re: [Qemu-devel] [PATCH 0/1] vhost-user: Add a protocol extension for client responses to vhost commands.

[Prerna]

CC'ing MarcAndre & Michael S Tsirkin.

On Fri, Jun 24, 2016 at 1:47 PM, Prerna Saxena <saxenap.ltc@gmail.com>
wrote:

> From: Prerna Saxena <prerna.saxena@nutanix.com>
>
> The current vhost-user protocol requires the client to send responses to
> only few commands. For the remaining commands, it is impossible for QEMU to
> know the status of the requested operation -- ie, did it succeed at all,
> and if so, at what time.
>
> This is inconvenient, and can also lead to races. As an example:
>
> (1) qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net
> application) and SET_MEM_TABLE doesn't require a reply according to the
> spec.
> (2) qemu commits the memory to the guest.
> (3) guest issues an I/O operation over a new memory region which was
> configured on (1)
> (4) The application hasn't yet remapped the memory, but it sees the I/O
> request.
> (5) The application cannot satisfy the request because it doesn't know
> about those GPAs
>
> Note that the kernel implementation does not suffer from this limitation
> since messages are sent via an ioctl(). The ioctl() blocks until the
> backend (eg. vhost-net) completes the command and returns (with an error
> code).
>
> Changing the behaviour of current vhost-user commands would break existing
> applications. This patch introduces a protocol extension,
> VHOST_USER_PROTOCOL_F_REPLY_ACK. This feature, if negotiated, allows QEMU
> to annotate messages to the application that it seeks a response for. The
> application must then respond to qemu by providing a status about the
> requested operation.
>
>
> Prerna Saxena (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 containin status code for each
>                requested operation.
>                Status codes are '0' for success, and non-zero for error.
>
>  docs/specs/vhost-user.txt |  36 +++++++++++
>  hw/virtio/vhost-user.c    | 153
> +++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 186 insertions(+), 3 deletions(-)
>
> --
> 1.8.1.2
>
>

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.

[Prerna]

CC'ing MarcAndre and Michael S Tsirkin.

On Fri, Jun 24, 2016 at 1:47 PM, Prerna Saxena <saxenap.ltc@gmail.com>
wrote:

> From: Prerna Saxena <prerna.saxena@nutanix.com>
>
> Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com>
> ---
>  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.
> +
> +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.
> +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.
> 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;
>  }
> --
> 1.8.1.2
>
>

Re: [Qemu-devel] [PATCH 0/1] vhost-user: Add a protocol extension for client responses to vhost commands.

[Marc-André Lureau]

Hi

On Fri, Jun 24, 2016 at 10:17 AM, Prerna Saxena <saxenap.ltc@gmail.com> wrote:
> From: Prerna Saxena <prerna.saxena@nutanix.com>
>
> The current vhost-user protocol requires the client to send responses to only few commands. For the remaining commands, it is impossible for QEMU to know the status of the requested operation -- ie, did it succeed at all, and if so, at what time.
>
> This is inconvenient, and can also lead to races. As an example:
>
> (1) qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net application) and SET_MEM_TABLE doesn't require a reply according to the spec.
> (2) qemu commits the memory to the guest.
> (3) guest issues an I/O operation over a new memory region which was configured on (1)
> (4) The application hasn't yet remapped the memory, but it sees the I/O request.
> (5) The application cannot satisfy the request because it doesn't know about those GPAs
>
> Note that the kernel implementation does not suffer from this limitation since messages are sent via an ioctl(). The ioctl() blocks until the backend (eg. vhost-net) completes the command and returns (with an error code).
>
> Changing the behaviour of current vhost-user commands would break existing applications. This patch introduces a protocol extension, VHOST_USER_PROTOCOL_F_REPLY_ACK. This feature, if negotiated, allows QEMU to annotate messages to the application that it seeks a response for. The application must then respond to qemu by providing a status about the requested operation.

I like the idea, as I encountered a similar issue in my
"vhost-user-gpu" development (which I worked around by sending a dump
GET_FEATURES.. to sync things). But I question the need to have a flag
per message. I think if the protocol feature is negociated, all
messages should have a reply. Why do you want it to be per-message?

thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 0/1] vhost-user: Add a protocol extension for client responses to vhost commands.

[Felipe Franciosi]

We talked to MST on IRC a while back and he brainstormed the idea of doing this per-message.
(I even recall proposing to call this feature REPLY_ALL and he suggested REPLY_ANY due to that.)

I agree with doing it per message, as the protocol itself should be flexible in that sense.
(Even if qemu today will probably want to ask for a reply in all messages.)

Felipe

On 24/06/2016, 14:59, "Qemu-devel on behalf of Marc-André Lureau" <qemu-devel-bounces+felipe=nutanix.com@nongnu.org on behalf of marcandre.lureau@gmail.com> wrote:

Hi

On Fri, Jun 24, 2016 at 10:17 AM, Prerna Saxena <saxenap.ltc@gmail.com> wrote:
> From: Prerna Saxena <prerna.saxena@nutanix.com>
>
> The current vhost-user protocol requires the client to send responses to only few commands. For the remaining commands, it is impossible for QEMU to know the status of the requested operation -- ie, did it succeed at all, and if so, at what time.
>
> This is inconvenient, and can also lead to races. As an example:
>
> (1) qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net application) and SET_MEM_TABLE doesn't require a reply according to the spec.
> (2) qemu commits the memory to the guest.
> (3) guest issues an I/O operation over a new memory region which was configured on (1)
> (4) The application hasn't yet remapped the memory, but it sees the I/O request.
> (5) The application cannot satisfy the request because it doesn't know about those GPAs
>
> Note that the kernel implementation does not suffer from this limitation since messages are sent via an ioctl(). The ioctl() blocks until the backend (eg. vhost-net) completes the command and returns (with an error code).
>
> Changing the behaviour of current vhost-user commands would break existing applications. This patch introduces a protocol extension, VHOST_USER_PROTOCOL_F_REPLY_ACK. This feature, if negotiated, allows QEMU to annotate messages to the application that it seeks a response for. The application must then respond to qemu by providing a status about the requested operation.

I like the idea, as I encountered a similar issue in my
"vhost-user-gpu" development (which I worked around by sending a dump
GET_FEATURES.. to sync things). But I question the need to have a flag
per message. I think if the protocol feature is negociated, all
messages should have a reply. Why do you want it to be per-message?

thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 0/1] vhost-user: Add a protocol extension for client responses to vhost commands.

[Prerna Saxena]

On 24/06/16 9:15 pm, "Felipe Franciosi" <felipe@nutanix.com> wrote:

>We talked to MST on IRC a while back and he brainstormed the idea of doing this per-message.
>(I even recall proposing to call this feature REPLY_ALL and he suggested REPLY_ANY due to that.)
>
>I agree with doing it per message, as the protocol itself should be flexible in that sense.
>(Even if qemu today will probably want to ask for a reply in all messages.)

In fact, the current implementation does exactly this. If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, the current QEMU patch sets the NEED_RESPONSE flag bit for all outgoing messages — basically enforcing the vhost-user application to respond to all messages.

>
>On 24/06/2016, 14:59, "Qemu-devel on behalf of Marc-André Lureau" <qemu-devel-bounces+felipe=nutanix.com@nongnu.org on behalf of marcandre.lureau@gmail.com> wrote:
>
>Hi
>
>On Fri, Jun 24, 2016 at 10:17 AM, Prerna Saxena <saxenap.ltc@gmail.com> wrote:
>> From: Prerna Saxena <prerna.saxena@nutanix.com>
>>
>> The current vhost-user protocol requires the client to send responses to only few commands. For the remaining commands, it is impossible for QEMU to know the status of the requested operation -- ie, did it succeed at all, and if so, at what time.
>>
>> This is inconvenient, and can also lead to races. As an example:
>>
>> (1) qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net application) and SET_MEM_TABLE doesn't require a reply according to the spec.
>> (2) qemu commits the memory to the guest.
>> (3) guest issues an I/O operation over a new memory region which was configured on (1)
>> (4) The application hasn't yet remapped the memory, but it sees the I/O request.
>> (5) The application cannot satisfy the request because it doesn't know about those GPAs
>>
>> Note that the kernel implementation does not suffer from this limitation since messages are sent via an ioctl(). The ioctl() blocks until the backend (eg. vhost-net) completes the command and returns (with an error code).
>>
>> Changing the behaviour of current vhost-user commands would break existing applications. This patch introduces a protocol extension, VHOST_USER_PROTOCOL_F_REPLY_ACK. This feature, if negotiated, allows QEMU to annotate messages to the application that it seeks a response for. The application must then respond to qemu by providing a status about the requested operation.
>
>I like the idea, as I encountered a similar issue in my
>"vhost-user-gpu" development (which I worked around by sending a dump
>GET_FEATURES.. to sync things). But I question the need to have a flag
>per message. I think if the protocol feature is negociated, all
>messages should have a reply. Why do you want it to be per-message?
>
>thanks
>
>-- 
>Marc-André Lureau
>
>
>

Re: [Qemu-devel] [PATCH 0/1] vhost-user: Add a protocol extension for client responses to vhost commands.

[Michael S. Tsirkin]

On Fri, Jun 24, 2016 at 01:17:07AM -0700, Prerna Saxena wrote:
> From: Prerna Saxena <prerna.saxena@nutanix.com>
> 
> The current vhost-user protocol requires the client to send responses to only few commands. For the remaining commands, it is impossible for QEMU to know the status of the requested operation -- ie, did it succeed at all, and if so, at what time.
> 
> This is inconvenient, and can also lead to races. As an example:
> 
> (1) qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net application) and SET_MEM_TABLE doesn't require a reply according to the spec.
> (2) qemu commits the memory to the guest.
> (3) guest issues an I/O operation over a new memory region which was configured on (1)
> (4) The application hasn't yet remapped the memory, but it sees the I/O request.
> (5) The application cannot satisfy the request because it doesn't know about those GPAs

OK, but there's a simple way to fix the race: submit
another request e.g. GET_FEATURES and wait for a response.


> 
> Note that the kernel implementation does not suffer from this limitation since messages are sent via an ioctl(). The ioctl() blocks until the backend (eg. vhost-net) completes the command and returns (with an error code).
> 
> Changing the behaviour of current vhost-user commands would break existing applications. This patch introduces a protocol extension, VHOST_USER_PROTOCOL_F_REPLY_ACK. This feature, if negotiated, allows QEMU to annotate messages to the application that it seeks a response for. The application must then respond to qemu by providing a status about the requested operation.


I would like this structured like this:

1. fix races by sending GET_FEATURES and waiting for it
2. add VHOST_USER_PROTOCOL_F_REPLY_ACK


> 
> Prerna Saxena (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 containin status code for each
>                requested operation.          
>                Status codes are '0' for success, and non-zero for error.
> 
>  docs/specs/vhost-user.txt |  36 +++++++++++
>  hw/virtio/vhost-user.c    | 153 +++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 186 insertions(+), 3 deletions(-)
> 
> -- 
> 1.8.1.2
> 

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 <prerna.saxena@nutanix.com>
> 
> Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com>
> ---
>  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
> 

Re: [Qemu-devel] [PATCH 0/1] vhost-user: Add a protocol extension for client responses to vhost commands.

[Michael S. Tsirkin]

On Fri, Jun 24, 2016 at 05:39:31PM +0000, Prerna Saxena wrote:
> 
> 
> On 24/06/16 9:15 pm, "Felipe Franciosi" <felipe@nutanix.com> wrote:
> 
> >We talked to MST on IRC a while back and he brainstormed the idea of doing this per-message.
> >(I even recall proposing to call this feature REPLY_ALL and he suggested REPLY_ANY due to that.)
> >
> >I agree with doing it per message, as the protocol itself should be flexible in that sense.
> >(Even if qemu today will probably want to ask for a reply in all messages.)
> 
> In fact, the current implementation does exactly this. If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, the current QEMU patch sets the NEED_RESPONSE flag bit for all outgoing messages — basically enforcing the vhost-user application to respond to all messages.


This seems unnecessary. Let's only do that for messages that actually
need to be synchronous.

> >
> >On 24/06/2016, 14:59, "Qemu-devel on behalf of Marc-André Lureau" <qemu-devel-bounces+felipe=nutanix.com@nongnu.org on behalf of marcandre.lureau@gmail.com> wrote:
> >
> >Hi
> >
> >On Fri, Jun 24, 2016 at 10:17 AM, Prerna Saxena <saxenap.ltc@gmail.com> wrote:
> >> From: Prerna Saxena <prerna.saxena@nutanix.com>
> >>
> >> The current vhost-user protocol requires the client to send responses to only few commands. For the remaining commands, it is impossible for QEMU to know the status of the requested operation -- ie, did it succeed at all, and if so, at what time.
> >>
> >> This is inconvenient, and can also lead to races. As an example:
> >>
> >> (1) qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net application) and SET_MEM_TABLE doesn't require a reply according to the spec.
> >> (2) qemu commits the memory to the guest.
> >> (3) guest issues an I/O operation over a new memory region which was configured on (1)
> >> (4) The application hasn't yet remapped the memory, but it sees the I/O request.
> >> (5) The application cannot satisfy the request because it doesn't know about those GPAs
> >>
> >> Note that the kernel implementation does not suffer from this limitation since messages are sent via an ioctl(). The ioctl() blocks until the backend (eg. vhost-net) completes the command and returns (with an error code).
> >>
> >> Changing the behaviour of current vhost-user commands would break existing applications. This patch introduces a protocol extension, VHOST_USER_PROTOCOL_F_REPLY_ACK. This feature, if negotiated, allows QEMU to annotate messages to the application that it seeks a response for. The application must then respond to qemu by providing a status about the requested operation.
> >
> >I like the idea, as I encountered a similar issue in my
> >"vhost-user-gpu" development (which I worked around by sending a dump
> >GET_FEATURES.. to sync things). But I question the need to have a flag
> >per message. I think if the protocol feature is negociated, all
> >messages should have a reply. Why do you want it to be per-message?
> >
> >thanks
> >
> >-- 
> >Marc-André Lureau
> >
> >
> >

Re: [Qemu-devel] [PATCH 0/1] vhost-user: Add a protocol extension for client responses to vhost commands.

[Prerna Saxena]

On 25/06/16 4:43 am, "Michael S. Tsirkin" <mst@redhat.com> wrote:

>On Fri, Jun 24, 2016 at 05:39:31PM +0000, Prerna Saxena wrote:
>> 
>> 
>> On 24/06/16 9:15 pm, "Felipe Franciosi" <felipe@nutanix.com> wrote:
>> 
>> >We talked to MST on IRC a while back and he brainstormed the idea of doing this per-message.
>> >(I even recall proposing to call this feature REPLY_ALL and he suggested REPLY_ANY due to that.)
>> >
>> >I agree with doing it per message, as the protocol itself should be flexible in that sense.
>> >(Even if qemu today will probably want to ask for a reply in all messages.)
>> 
>> In fact, the current implementation does exactly this. If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, the current QEMU patch sets the NEED_RESPONSE flag bit for all outgoing messages — basically enforcing the vhost-user application to respond to all messages.
>
>
>This seems unnecessary. Let's only do that for messages that actually
>need to be synchronous.

It would be nice to distinguish the vhost-user protocol itself from its QEMU implementation.
The protocol should, in theory, have provision for an implementation (such as QEMU’s vhost-user implementation) to seek response for _any_ command. However, we can choose to be selective in our QEMU implementation and just have limited commands currently send a response, such as SET_MEM_TABLE. 

In other words, we will still require the NEED_RESPONSE flag bit defined, but we can just set it to 1 it for SET_MEM_TABLE command in our QEMU implementation. All other vhost-user commands are sent from QEMU setting this to 0, so the application does not send an ack.

Michael, Does that correctly summarize what you were meaning to suggest here ?

Regards,
Prerna


>
>> >
>> >On 24/06/2016, 14:59, "Qemu-devel on behalf of Marc-André Lureau" <qemu-devel-bounces+felipe=nutanix.com@nongnu.org on behalf of marcandre.lureau@gmail.com> wrote:
>> >
>> >Hi
>> >
>> >On Fri, Jun 24, 2016 at 10:17 AM, Prerna Saxena <saxenap.ltc@gmail.com> wrote:
>> >> From: Prerna Saxena <prerna.saxena@nutanix.com>
>> >>
>> >> The current vhost-user protocol requires the client to send responses to only few commands. For the remaining commands, it is impossible for QEMU to know the status of the requested operation -- ie, did it succeed at all, and if so, at what time.
>> >>
>> >> This is inconvenient, and can also lead to races. As an example:
>> >>
>> >> (1) qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net application) and SET_MEM_TABLE doesn't require a reply according to the spec.
>> >> (2) qemu commits the memory to the guest.
>> >> (3) guest issues an I/O operation over a new memory region which was configured on (1)
>> >> (4) The application hasn't yet remapped the memory, but it sees the I/O request.
>> >> (5) The application cannot satisfy the request because it doesn't know about those GPAs
>> >>
>> >> Note that the kernel implementation does not suffer from this limitation since messages are sent via an ioctl(). The ioctl() blocks until the backend (eg. vhost-net) completes the command and returns (with an error code).
>> >>
>> >> Changing the behaviour of current vhost-user commands would break existing applications. This patch introduces a protocol extension, VHOST_USER_PROTOCOL_F_REPLY_ACK. This feature, if negotiated, allows QEMU to annotate messages to the application that it seeks a response for. The application must then respond to qemu by providing a status about the requested operation.
>> >
>> >I like the idea, as I encountered a similar issue in my
>> >"vhost-user-gpu" development (which I worked around by sending a dump
>> >GET_FEATURES.. to sync things). But I question the need to have a flag
>> >per message. I think if the protocol feature is negociated, all
>> >messages should have a reply. Why do you want it to be per-message?
>> >
>> >thanks
>> >
>> >-- 
>> >Marc-André Lureau
>> >
>> >
>> >

Re: [Qemu-devel] [PATCH 0/1] vhost-user: Add a protocol extension for client responses to vhost commands.

[Michael S. Tsirkin]

On Sat, Jun 25, 2016 at 03:13:54AM +0000, Prerna Saxena wrote:
> 
> 
> 
> 
> 
> On 25/06/16 4:43 am, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> >On Fri, Jun 24, 2016 at 05:39:31PM +0000, Prerna Saxena wrote:
> >> 
> >> 
> >> On 24/06/16 9:15 pm, "Felipe Franciosi" <felipe@nutanix.com> wrote:
> >> 
> >> >We talked to MST on IRC a while back and he brainstormed the idea of doing this per-message.
> >> >(I even recall proposing to call this feature REPLY_ALL and he suggested REPLY_ANY due to that.)
> >> >
> >> >I agree with doing it per message, as the protocol itself should be flexible in that sense.
> >> >(Even if qemu today will probably want to ask for a reply in all messages.)
> >> 
> >> In fact, the current implementation does exactly this. If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, the current QEMU patch sets the NEED_RESPONSE flag bit for all outgoing messages — basically enforcing the vhost-user application to respond to all messages.
> >
> >
> >This seems unnecessary. Let's only do that for messages that actually
> >need to be synchronous.
> 
> It would be nice to distinguish the vhost-user protocol itself from its QEMU implementation.
> The protocol should, in theory, have provision for an implementation (such as QEMU’s vhost-user implementation) to seek response for _any_ command. However, we can choose to be selective in our QEMU implementation and just have limited commands currently send a response, such as SET_MEM_TABLE. 
> 
> In other words, we will still require the NEED_RESPONSE flag bit defined, but we can just set it to 1 it for SET_MEM_TABLE command in our QEMU implementation. All other vhost-user commands are sent from QEMU setting this to 0, so the application does not send an ack.
> 
> Michael, Does that correctly summarize what you were meaning to suggest here ?
> 
> Regards,
> Prerna

Exactly.

> 
> >
> >> >
> >> >On 24/06/2016, 14:59, "Qemu-devel on behalf of Marc-André Lureau" <qemu-devel-bounces+felipe=nutanix.com@nongnu.org on behalf of marcandre.lureau@gmail.com> wrote:
> >> >
> >> >Hi
> >> >
> >> >On Fri, Jun 24, 2016 at 10:17 AM, Prerna Saxena <saxenap.ltc@gmail.com> wrote:
> >> >> From: Prerna Saxena <prerna.saxena@nutanix.com>
> >> >>
> >> >> The current vhost-user protocol requires the client to send responses to only few commands. For the remaining commands, it is impossible for QEMU to know the status of the requested operation -- ie, did it succeed at all, and if so, at what time.
> >> >>
> >> >> This is inconvenient, and can also lead to races. As an example:
> >> >>
> >> >> (1) qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net application) and SET_MEM_TABLE doesn't require a reply according to the spec.
> >> >> (2) qemu commits the memory to the guest.
> >> >> (3) guest issues an I/O operation over a new memory region which was configured on (1)
> >> >> (4) The application hasn't yet remapped the memory, but it sees the I/O request.
> >> >> (5) The application cannot satisfy the request because it doesn't know about those GPAs
> >> >>
> >> >> Note that the kernel implementation does not suffer from this limitation since messages are sent via an ioctl(). The ioctl() blocks until the backend (eg. vhost-net) completes the command and returns (with an error code).
> >> >>
> >> >> Changing the behaviour of current vhost-user commands would break existing applications. This patch introduces a protocol extension, VHOST_USER_PROTOCOL_F_REPLY_ACK. This feature, if negotiated, allows QEMU to annotate messages to the application that it seeks a response for. The application must then respond to qemu by providing a status about the requested operation.
> >> >
> >> >I like the idea, as I encountered a similar issue in my
> >> >"vhost-user-gpu" development (which I worked around by sending a dump
> >> >GET_FEATURES.. to sync things). But I question the need to have a flag
> >> >per message. I think if the protocol feature is negociated, all
> >> >messages should have a reply. Why do you want it to be per-message?
> >> >
> >> >thanks
> >> >
> >> >-- 
> >> >Marc-André Lureau
> >> >
> >> >
> >> >

Re: [Qemu-devel] [PATCH 0/1] vhost-user: Add a protocol extension for client responses to vhost commands.

[Prerna Saxena]

On 26/06/16 8:15 am, "Michael S. Tsirkin" <mst@redhat.com> wrote:

>On Sat, Jun 25, 2016 at 03:13:54AM +0000, Prerna Saxena wrote:
>> 
>> 
>> 
>> 
>> 
>> On 25/06/16 4:43 am, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> >On Fri, Jun 24, 2016 at 05:39:31PM +0000, Prerna Saxena wrote:
>> >> 
>> >> 
>> >> On 24/06/16 9:15 pm, "Felipe Franciosi" <felipe@nutanix.com> wrote:
>> >> 
>> >> >We talked to MST on IRC a while back and he brainstormed the idea of doing this per-message.
>> >> >(I even recall proposing to call this feature REPLY_ALL and he suggested REPLY_ANY due to that.)
>> >> >
>> >> >I agree with doing it per message, as the protocol itself should be flexible in that sense.
>> >> >(Even if qemu today will probably want to ask for a reply in all messages.)
>> >> 
>> >> In fact, the current implementation does exactly this. If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, the current QEMU patch sets the NEED_RESPONSE flag bit for all outgoing messages — basically enforcing the vhost-user application to respond to all messages.
>> >
>> >
>> >This seems unnecessary. Let's only do that for messages that actually
>> >need to be synchronous.
>> 
>> It would be nice to distinguish the vhost-user protocol itself from its QEMU implementation.
>> The protocol should, in theory, have provision for an implementation (such as QEMU’s vhost-user implementation) to seek response for _any_ command. However, we can choose to be selective in our QEMU implementation and just have limited commands currently send a response, such as SET_MEM_TABLE. 
>> 
>> In other words, we will still require the NEED_RESPONSE flag bit defined, but we can just set it to 1 it for SET_MEM_TABLE command in our QEMU implementation. All other vhost-user commands are sent from QEMU setting this to 0, so the application does not send an ack.
>> 
>> Michael, Does that correctly summarize what you were meaning to suggest here ?
>> 
>> Regards,
>> Prerna
>
>Exactly.

Thanks for your response. I will rework and send out a patch to that end.

Regards,
Prerna

>
>> 
>> >
>> >> >
>> >> >On 24/06/2016, 14:59, "Qemu-devel on behalf of Marc-André Lureau" <qemu-devel-bounces+felipe=nutanix.com@nongnu.org on behalf of marcandre.lureau@gmail.com> wrote:
>> >> >
>> >> >Hi
>> >> >
>> >> >On Fri, Jun 24, 2016 at 10:17 AM, Prerna Saxena <saxenap.ltc@gmail.com> wrote:
>> >> >> From: Prerna Saxena <prerna.saxena@nutanix.com>
>> >> >>
>> >> >> The current vhost-user protocol requires the client to send responses to only few commands. For the remaining commands, it is impossible for QEMU to know the status of the requested operation -- ie, did it succeed at all, and if so, at what time.
>> >> >>
>> >> >> This is inconvenient, and can also lead to races. As an example:
>> >> >>
>> >> >> (1) qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net application) and SET_MEM_TABLE doesn't require a reply according to the spec.
>> >> >> (2) qemu commits the memory to the guest.
>> >> >> (3) guest issues an I/O operation over a new memory region which was configured on (1)
>> >> >> (4) The application hasn't yet remapped the memory, but it sees the I/O request.
>> >> >> (5) The application cannot satisfy the request because it doesn't know about those GPAs
>> >> >>
>> >> >> Note that the kernel implementation does not suffer from this limitation since messages are sent via an ioctl(). The ioctl() blocks until the backend (eg. vhost-net) completes the command and returns (with an error code).
>> >> >>
>> >> >> Changing the behaviour of current vhost-user commands would break existing applications. This patch introduces a protocol extension, VHOST_USER_PROTOCOL_F_REPLY_ACK. This feature, if negotiated, allows QEMU to annotate messages to the application that it seeks a response for. The application must then respond to qemu by providing a status about the requested operation.
>> >> >
>> >> >I like the idea, as I encountered a similar issue in my
>> >> >"vhost-user-gpu" development (which I worked around by sending a dump
>> >> >GET_FEATURES.. to sync things). But I question the need to have a flag
>> >> >per message. I think if the protocol feature is negociated, all
>> >> >messages should have a reply. Why do you want it to be per-message?
>> >> >
>> >> >thanks
>> >> >
>> >> >-- 
>> >> >Marc-André Lureau
>> >> >
>> >> >
>> >> >

Re: [Qemu-devel] [PATCH 0/1] vhost-user: Add a protocol extension for client responses to vhost commands.

[Michael S. Tsirkin]

On Sun, Jun 26, 2016 at 02:48:09AM +0000, Prerna Saxena wrote:
> 
> 
> 
> 
> 
> On 26/06/16 8:15 am, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> >On Sat, Jun 25, 2016 at 03:13:54AM +0000, Prerna Saxena wrote:
> >> 
> >> 
> >> 
> >> 
> >> 
> >> On 25/06/16 4:43 am, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> 
> >> >On Fri, Jun 24, 2016 at 05:39:31PM +0000, Prerna Saxena wrote:
> >> >> 
> >> >> 
> >> >> On 24/06/16 9:15 pm, "Felipe Franciosi" <felipe@nutanix.com> wrote:
> >> >> 
> >> >> >We talked to MST on IRC a while back and he brainstormed the idea of doing this per-message.
> >> >> >(I even recall proposing to call this feature REPLY_ALL and he suggested REPLY_ANY due to that.)
> >> >> >
> >> >> >I agree with doing it per message, as the protocol itself should be flexible in that sense.
> >> >> >(Even if qemu today will probably want to ask for a reply in all messages.)
> >> >> 
> >> >> In fact, the current implementation does exactly this. If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, the current QEMU patch sets the NEED_RESPONSE flag bit for all outgoing messages — basically enforcing the vhost-user application to respond to all messages.
> >> >
> >> >
> >> >This seems unnecessary. Let's only do that for messages that actually
> >> >need to be synchronous.
> >> 
> >> It would be nice to distinguish the vhost-user protocol itself from its QEMU implementation.
> >> The protocol should, in theory, have provision for an implementation (such as QEMU’s vhost-user implementation) to seek response for _any_ command. However, we can choose to be selective in our QEMU implementation and just have limited commands currently send a response, such as SET_MEM_TABLE. 
> >> 
> >> In other words, we will still require the NEED_RESPONSE flag bit defined, but we can just set it to 1 it for SET_MEM_TABLE command in our QEMU implementation. All other vhost-user commands are sent from QEMU setting this to 0, so the application does not send an ack.
> >> 
> >> Michael, Does that correctly summarize what you were meaning to suggest here ?
> >> 
> >> Regards,
> >> Prerna
> >
> >Exactly.
> 
> Thanks for your response. I will rework and send out a patch to that end.
> 
> Regards,
> Prerna

And if the feature is not supported, work around that
by sending e.g. GET_FEATURES afterwards.

> >
> >> 
> >> >
> >> >> >
> >> >> >On 24/06/2016, 14:59, "Qemu-devel on behalf of Marc-André Lureau" <qemu-devel-bounces+felipe=nutanix.com@nongnu.org on behalf of marcandre.lureau@gmail.com> wrote:
> >> >> >
> >> >> >Hi
> >> >> >
> >> >> >On Fri, Jun 24, 2016 at 10:17 AM, Prerna Saxena <saxenap.ltc@gmail.com> wrote:
> >> >> >> From: Prerna Saxena <prerna.saxena@nutanix.com>
> >> >> >>
> >> >> >> The current vhost-user protocol requires the client to send responses to only few commands. For the remaining commands, it is impossible for QEMU to know the status of the requested operation -- ie, did it succeed at all, and if so, at what time.
> >> >> >>
> >> >> >> This is inconvenient, and can also lead to races. As an example:
> >> >> >>
> >> >> >> (1) qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net application) and SET_MEM_TABLE doesn't require a reply according to the spec.
> >> >> >> (2) qemu commits the memory to the guest.
> >> >> >> (3) guest issues an I/O operation over a new memory region which was configured on (1)
> >> >> >> (4) The application hasn't yet remapped the memory, but it sees the I/O request.
> >> >> >> (5) The application cannot satisfy the request because it doesn't know about those GPAs
> >> >> >>
> >> >> >> Note that the kernel implementation does not suffer from this limitation since messages are sent via an ioctl(). The ioctl() blocks until the backend (eg. vhost-net) completes the command and returns (with an error code).
> >> >> >>
> >> >> >> Changing the behaviour of current vhost-user commands would break existing applications. This patch introduces a protocol extension, VHOST_USER_PROTOCOL_F_REPLY_ACK. This feature, if negotiated, allows QEMU to annotate messages to the application that it seeks a response for. The application must then respond to qemu by providing a status about the requested operation.
> >> >> >
> >> >> >I like the idea, as I encountered a similar issue in my
> >> >> >"vhost-user-gpu" development (which I worked around by sending a dump
> >> >> >GET_FEATURES.. to sync things). But I question the need to have a flag
> >> >> >per message. I think if the protocol feature is negociated, all
> >> >> >messages should have a reply. Why do you want it to be per-message?
> >> >> >
> >> >> >thanks
> >> >> >
> >> >> >-- 
> >> >> >Marc-André Lureau
> >> >> >
> >> >> >
> >> >> >

Re: [Qemu-devel] [PATCH 0/1] vhost-user: Add a protocol extension for client responses to vhost commands.

[Felipe Franciosi]

> On 24 Jun 2016, at 23:43, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Fri, Jun 24, 2016 at 01:17:07AM -0700, Prerna Saxena wrote:
>> From: Prerna Saxena <prerna.saxena@nutanix.com>
>> 
>> The current vhost-user protocol requires the client to send responses to only few commands. For the remaining commands, it is impossible for QEMU to know the status of the requested operation -- ie, did it succeed at all, and if so, at what time.
>> 
>> This is inconvenient, and can also lead to races. As an example:
>> 
>> (1) qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net application) and SET_MEM_TABLE doesn't require a reply according to the spec.
>> (2) qemu commits the memory to the guest.
>> (3) guest issues an I/O operation over a new memory region which was configured on (1)
>> (4) The application hasn't yet remapped the memory, but it sees the I/O request.
>> (5) The application cannot satisfy the request because it doesn't know about those GPAs
> 
> OK, but there's a simple way to fix the race: submit
> another request e.g. GET_FEATURES and wait for a response.

Actually, that doesn't guarantee to fix the problem. I argue it's quite a poor workaround.

The protocol doesn't specify that messages need to be processed in series. I could easily see a client application receiving a SET_MEM_TABLE message and dispatching a task to process it asynchronously. The same application could respond to a following GET_FEATURES in line, since the command requires a response, but no extra processing. In summary, qemu could get a response for GET_FEATURES while SET_MEM_TABLE is being processed in the background at the client application.

It might seem like this is unlikely to happen, but consider that a multi-threaded application might need to take a lock in order to process SET_MEM_TABLE. For example, waiting for all virtqueues being processed to quiesce.

> 
> 
>> 
>> Note that the kernel implementation does not suffer from this limitation since messages are sent via an ioctl(). The ioctl() blocks until the backend (eg. vhost-net) completes the command and returns (with an error code).
>> 
>> Changing the behaviour of current vhost-user commands would break existing applications. This patch introduces a protocol extension, VHOST_USER_PROTOCOL_F_REPLY_ACK. This feature, if negotiated, allows QEMU to annotate messages to the application that it seeks a response for. The application must then respond to qemu by providing a status about the requested operation.
> 
> 
> I would like this structured like this:
> 
> 1. fix races by sending GET_FEATURES and waiting for it

We can still add this first if you think it will help. But is it worth it? Asking for a reply to certain commands is clearly a better fix.

Thanks,
Felipe


> 2. add VHOST_USER_PROTOCOL_F_REPLY_ACK
> 
> 
>> 
>> Prerna Saxena (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 containin status code for each
>>               requested operation.          
>>               Status codes are '0' for success, and non-zero for error.
>> 
>> docs/specs/vhost-user.txt |  36 +++++++++++
>> hw/virtio/vhost-user.c    | 153 +++++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 186 insertions(+), 3 deletions(-)
>> 
>> -- 
>> 1.8.1.2
>> 
> 

Re: [Qemu-devel] [PATCH 0/1] vhost-user: Add a protocol extension for client responses to vhost commands.

[Marc-André Lureau]

Hi

On Mon, Jun 27, 2016 at 12:45 PM, Felipe Franciosi <felipe@nutanix.com> wrote:
>
>> On 24 Jun 2016, at 23:43, Michael S. Tsirkin <mst@redhat.com> wrote:
>> I would like this structured like this:
>>
>> 1. fix races by sending GET_FEATURES and waiting for it
>
> We can still add this first if you think it will help. But is it worth it? Asking for a reply to certain commands is clearly a better fix.


fwiw, I agree it's not worth, using GET_FEATURES is a hack (that I
used it in vhost-user-gpu rfc series) and could suffer the issues
Felipe described, unless the spec is constrained in backward
incompatible ways.


>
> Thanks,
> Felipe
>
>
>> 2. add VHOST_USER_PROTOCOL_F_REPLY_ACK
>>
>>
>>>
>>> Prerna Saxena (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 containin status code for each
>>>               requested operation.
>>>               Status codes are '0' for success, and non-zero for error.
>>>
>>> docs/specs/vhost-user.txt |  36 +++++++++++
>>> hw/virtio/vhost-user.c    | 153 +++++++++++++++++++++++++++++++++++++++++++++-
>>> 2 files changed, 186 insertions(+), 3 deletions(-)
>>>
>>> --
>>> 1.8.1.2
>>>
>>
>
>



-- 
Marc-André Lureau