[Qemu-devel] [PATCH v4 0/2] vhost-user: Extend protocol to receive replies on any command.

[Qemu-devel] [PATCH v4 0/2] vhost-user: Extend protocol to receive replies on any command.

[Prerna Saxena]

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

*** BLURB HERE ***

vhost-user: Extend protocol to receive replies on any command.

The current vhost-user protocol requires the client to send responses to only a
few commands. For the remaining commands, it is impossible for QEMU to know the
status of the requested operation -- ie, did it succeed? If so, by 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).Note that SET_MEM_TABLE does not 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 does not 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.
Patch 1 introduces a protocol extension, VHOST_USER_PROTOCOL_F_REPLY_ACK. This feature, if negotiated, allows QEMU to request a response to any message by setting the newly introduced "need_response" flag. The application must then respond to qemu by providing a status about the requested operation.

Patch 2 adds a workaround for the race described above for clients that do not support REPLY_ACK
feature. it introduces  a get_features command to be sent before returning from set_mem_table. While this is not a complete fix, it will help client applications that strictly process messagesin order.

Changelog:
----------
Changes v3->v4:
1) Rearranged code in PATCH 1 to offset compiler warnings about missing declaration of vhost_user_read(). Fixed by moving process_message_reply() after definition of vhost_user_read()
2) Fixed minor suggestions in writeup for this protocol extension.

Changes v2->v3:
1) Swapped the patch numbers 1 & 2 from the previous series.
2) Patch 1 (previously patch 2 in v2): addresses MarcAndre's review comments and renames function 'process_message_response' to 'process_message_reply'
3) Patch 2 (ie patch 1 in v2) : Unchanged from v2.

Changes v1->v2:
1) Patch 1 : Ask for get_features before returning from set_mem_table(new).
2) Patch 2 : * Improve documentation.
          * Abstract out commonly used operations in the form of a function, process_message_response(). Also implement this only for SET_MEM_TABLE.

References:
v1 : https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07152.html
v2 : https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg00048.html
v3 : https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg01598.html

Prerna Saxena (2):
  vhost-user: Introduce a new protocol feature REPLY_ACK.
  vhost-user: Attempt to fix a race with set_mem_table.

 docs/specs/vhost-user.txt |  44 +++++++++++++++
 hw/virtio/vhost-user.c    | 133 ++++++++++++++++++++++++++++++----------------
 2 files changed, 130 insertions(+), 47 deletions(-)

-- 
1.8.1.2

Prerna Saxena (2):
  vhost-user: Introduce a new protocol feature REPLY_ACK.
  vhost-user: Attempt to fix a race with set_mem_table.

 docs/specs/vhost-user.txt |  41 ++++++++++++++
 hw/virtio/vhost-user.c    | 133 ++++++++++++++++++++++++++++++----------------
 2 files changed, 127 insertions(+), 47 deletions(-)

-- 
1.8.1.2

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

[Prerna Saxena]

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

This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK.

If negotiated, client applications should send a u64 payload in
response to any message that contains the "need_response" bit set
on the message flags. Setting the payload to "zero" indicates the
command finished successfully. Likewise, setting it to "non-zero"
indicates an error.

Currently implemented only for SET_MEM_TABLE.

Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com>
---
 docs/specs/vhost-user.txt | 41 +++++++++++++++++++++++++++++++++++++++++
 hw/virtio/vhost-user.c    | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)

diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 777c49c..57df586 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -37,6 +37,8 @@ consists of 3 header fields and a payload:
  * Flags: 32-bit bit field:
    - Lower 2 bits are the version (currently 0x01)
    - Bit 2 is the reply flag - needs to be sent on each reply from the slave
+   - Bit 3 is the need_response flag - see VHOST_USER_PROTOCOL_F_REPLY_ACK for
+     details.
  * Size - 32-bit size of the payload
 
 
@@ -126,6 +128,8 @@ the ones that do:
  * VHOST_GET_VRING_BASE
  * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD)
 
+[ Also see the section on REPLY_ACK protocol extension. ]
+
 There are several messages that the master sends with file descriptors passed
 in the ancillary data:
 
@@ -254,6 +258,7 @@ Protocol features
 #define VHOST_USER_PROTOCOL_F_MQ             0
 #define VHOST_USER_PROTOCOL_F_LOG_SHMFD      1
 #define VHOST_USER_PROTOCOL_F_RARP           2
+#define VHOST_USER_PROTOCOL_F_REPLY_ACK      3
 
 Message types
 -------------
@@ -464,3 +469,39 @@ Message types
       is present in VHOST_USER_GET_PROTOCOL_FEATURES.
       The first 6 bytes of the payload contain the mac address of the guest to
       allow the vhost user backend to construct and broadcast the fake RARP.
+
+VHOST_USER_PROTOCOL_F_REPLY_ACK:
+-------------------------------
+The original vhost-user specification only demands responses for certain
+commands. This differs from the vhost protocol implementation where commands
+are sent over an ioctl() call and block until the client has completed.
+
+With this protocol extension negotiated, the sender (QEMU) can set the newly
+introduced "need_response" [Bit 3] flag to any command. This indicates that
+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.
+In other words, response must be in the following format :
+
+------------------------------------
+| request | flags | size | payload |
+------------------------------------
+
+ * Request: 32-bit type of the request
+ * Flags: 32-bit bit field:
+ * Size: size of the payload ( see below)
+ * Payload : a u64 integer, where a non-zero value indicates a failure.
+
+This indicates to QEMU that the requested operation has deterministically
+been met or not. Today, QEMU is expected to terminate the main vhost-user
+loop upon receiving such errors. In future, qemu could be taught to be more
+resilient for selective requests.
+
+Note that as per the original vhost-user protocol, the following four messages
+anyway require distinct responses from the vhost-user client process:
+ * 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..0cdb918 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -31,6 +31,7 @@ enum VhostUserProtocolFeature {
     VHOST_USER_PROTOCOL_F_MQ = 0,
     VHOST_USER_PROTOCOL_F_LOG_SHMFD = 1,
     VHOST_USER_PROTOCOL_F_RARP = 2,
+    VHOST_USER_PROTOCOL_F_REPLY_ACK = 3,
 
     VHOST_USER_PROTOCOL_F_MAX
 };
@@ -84,6 +85,7 @@ typedef struct VhostUserMsg {
 
 #define VHOST_USER_VERSION_MASK     (0x3)
 #define VHOST_USER_REPLY_MASK       (0x1<<2)
+#define VHOST_USER_NEED_RESPONSE_MASK       (0x1 << 3)
     uint32_t flags;
     uint32_t size; /* the following payload size */
     union {
@@ -158,6 +160,25 @@ fail:
     return -1;
 }
 
+static int process_message_reply(struct vhost_dev *dev,
+                                    VhostUserRequest request)
+{
+    VhostUserMsg msg;
+
+    if (vhost_user_read(dev, &msg) < 0) {
+        return 0;
+    }
+
+    if (msg.request != request) {
+        error_report("Received unexpected msg type."
+                        "Expected %d received %d",
+                        request, msg.request);
+        return -1;
+    }
+
+    return msg.payload.u64 ? -1 : 0;
+}
+
 static bool vhost_user_one_time_request(VhostUserRequest request)
 {
     switch (request) {
@@ -239,11 +260,18 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
     int fds[VHOST_MEMORY_MAX_NREGIONS];
     int i, fd;
     size_t fd_num = 0;
+    bool reply_supported = virtio_has_feature(dev->protocol_features,
+                            VHOST_USER_PROTOCOL_F_REPLY_ACK);
+
     VhostUserMsg msg = {
         .request = VHOST_USER_SET_MEM_TABLE,
         .flags = VHOST_USER_VERSION,
     };
 
+    if (reply_supported) {
+        msg.flags |= VHOST_USER_NEED_RESPONSE_MASK;
+    }
+
     for (i = 0; i < dev->mem->nregions; ++i) {
         struct vhost_memory_region *reg = dev->mem->regions + i;
         ram_addr_t offset;
@@ -277,6 +305,10 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
 
     vhost_user_write(dev, &msg, fds, fd_num);
 
+    if (reply_supported) {
+        return process_message_reply(dev, msg.request);
+    }
+
     return 0;
 }
 
-- 
1.8.1.2

[Qemu-devel] [PATCH v4 2/2] vhost-user: Attempt to fix a race with set_mem_table.

[Prerna Saxena]

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

The set_mem_table command currently does not seek a reply. Hence, there is
no easy way for a remote application to notify to QEMU when it finished
setting up memory, or if there were errors doing so.

As an example:
(1) Qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net
application). SET_MEM_TABLE does not 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 has not yet remapped the memory, but it sees the I/O request.
(5) The application cannot satisfy the request because it does not know about those GPAs.

While a guaranteed fix would require a protocol extension (committed separately),
a best-effort workaround for existing applications is to send a GET_FEATURES
message before completing the vhost_user_set_mem_table() call.
Since GET_FEATURES requires a reply, an application that processes vhost-user
messages synchronously would probably have completed the SET_MEM_TABLE before replying.

Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com>
---
 hw/virtio/vhost-user.c | 123 ++++++++++++++++++++++++++-----------------------
 1 file changed, 65 insertions(+), 58 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 0cdb918..f96607e 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -254,64 +254,6 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
     return 0;
 }
 
-static int vhost_user_set_mem_table(struct vhost_dev *dev,
-                                    struct vhost_memory *mem)
-{
-    int fds[VHOST_MEMORY_MAX_NREGIONS];
-    int i, fd;
-    size_t fd_num = 0;
-    bool reply_supported = virtio_has_feature(dev->protocol_features,
-                            VHOST_USER_PROTOCOL_F_REPLY_ACK);
-
-    VhostUserMsg msg = {
-        .request = VHOST_USER_SET_MEM_TABLE,
-        .flags = VHOST_USER_VERSION,
-    };
-
-    if (reply_supported) {
-        msg.flags |= VHOST_USER_NEED_RESPONSE_MASK;
-    }
-
-    for (i = 0; i < dev->mem->nregions; ++i) {
-        struct vhost_memory_region *reg = dev->mem->regions + i;
-        ram_addr_t offset;
-        MemoryRegion *mr;
-
-        assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
-        mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
-                                     &offset);
-        fd = memory_region_get_fd(mr);
-        if (fd > 0) {
-            msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
-            msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
-            msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
-            msg.payload.memory.regions[fd_num].mmap_offset = offset;
-            assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
-            fds[fd_num++] = fd;
-        }
-    }
-
-    msg.payload.memory.nregions = fd_num;
-
-    if (!fd_num) {
-        error_report("Failed initializing vhost-user memory map, "
-                     "consider using -object memory-backend-file share=on");
-        return -1;
-    }
-
-    msg.size = sizeof(msg.payload.memory.nregions);
-    msg.size += sizeof(msg.payload.memory.padding);
-    msg.size += fd_num * sizeof(VhostUserMemoryRegion);
-
-    vhost_user_write(dev, &msg, fds, fd_num);
-
-    if (reply_supported) {
-        return process_message_reply(dev, msg.request);
-    }
-
-    return 0;
-}
-
 static int vhost_user_set_vring_addr(struct vhost_dev *dev,
                                      struct vhost_vring_addr *addr)
 {
@@ -514,6 +456,71 @@ static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features)
     return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features);
 }
 
+static int vhost_user_set_mem_table(struct vhost_dev *dev,
+                                    struct vhost_memory *mem)
+{
+    int fds[VHOST_MEMORY_MAX_NREGIONS];
+    int i, fd;
+    size_t fd_num = 0;
+    uint64_t features;
+    bool reply_supported = virtio_has_feature(dev->protocol_features,
+                            VHOST_USER_PROTOCOL_F_REPLY_ACK);
+
+    VhostUserMsg msg = {
+        .request = VHOST_USER_SET_MEM_TABLE,
+        .flags = VHOST_USER_VERSION,
+    };
+
+    if (reply_supported) {
+        msg.flags |= VHOST_USER_NEED_RESPONSE_MASK;
+    }
+
+    for (i = 0; i < dev->mem->nregions; ++i) {
+        struct vhost_memory_region *reg = dev->mem->regions + i;
+        ram_addr_t offset;
+        MemoryRegion *mr;
+
+        assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
+        mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
+                                     &offset);
+        fd = memory_region_get_fd(mr);
+        if (fd > 0) {
+            msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
+            msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
+            msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
+            msg.payload.memory.regions[fd_num].mmap_offset = offset;
+            assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
+            fds[fd_num++] = fd;
+        }
+    }
+
+    msg.payload.memory.nregions = fd_num;
+
+    if (!fd_num) {
+        error_report("Failed initializing vhost-user memory map, "
+                     "consider using -object memory-backend-file share=on");
+        return -1;
+    }
+
+    msg.size = sizeof(msg.payload.memory.nregions);
+    msg.size += sizeof(msg.payload.memory.padding);
+    msg.size += fd_num * sizeof(VhostUserMemoryRegion);
+
+    vhost_user_write(dev, &msg, fds, fd_num);
+
+    if (reply_supported) {
+        return process_message_reply(dev, msg.request);
+    } else {
+        /* Note: It is (yet) unknown when the client application has finished
+         * remapping the GPA.
+         * Attempt to prevent a race by sending a command that requires a reply.
+         */
+        vhost_user_get_features(dev, &features);
+    }
+
+    return 0;
+}
+
 static int vhost_user_set_owner(struct vhost_dev *dev)
 {
     VhostUserMsg msg = {
-- 
1.8.1.2

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

[Marc-André Lureau]

Hi

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

Why need_response and not "need reply"?

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

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

responses/replies

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

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

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

I would put right here for clarity:

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

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

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

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

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

reply

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



-- 
Marc-André Lureau

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

[Prerna Saxena]

Hi Marc,
Thanks, please find my reply inline.





On 27/07/16 4:35 pm, "Marc-André Lureau" <marcandre.lureau@gmail.com> wrote:

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

(I’d already pointed this out earlier, but looks like I was possibly not very clear.)
Before deciding on the right name for Bit 3, let us see the nomenclature for Bit 2 above : "Bit 2 is the reply flag - needs to be sent on each reply from the slave”.
So we already have a _reply_ flag in use. If the name Bit 3 as the _need_reply_ flag, don’t you think it would be ultra-confusing ? I found it confusing  when I reviewed the documentation with this different term.
So I chose the name need_response with much deliberation — it conveys the essence of what this flag means to achieve, but without adding to confusion.

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

If you feel strongly about it, will change it here.

>
>> +commands. This differs from the vhost protocol implementation where commands
>> +are sent over an ioctl() call and block until the client has completed.
>> +
>> +With this protocol extension negotiated, the sender (QEMU) can set the newly
>> +introduced "need_response" [Bit 3] flag to any command. This indicates that
>
>need reply, you can remove the "newly introduced" (it's not going to
>be so new after a while)

* need_reply = no I don’t agree, for reasons cited earlier.
* remove the “newly introduced” phrase = agree, will do.

>
>> +the client MUST respond with a Payload VhostUserMsg indicating success or
>
>I would put right here for clarity:
>
>...MUST respond with a Payload VhostUserMsg (unless the message has
>already an explicit reply body)...
>
>alternatively, I would forbid using the bit 3 on commands that have
>already an explicit reply.

I don’t currently have any code that raises an error for such cases.
The implementation silently ignores it.

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

Ok, will remove it for brevity.

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

Again, I disagree, for reasons cited above.

[..snip..] removing the rest.
>
>
>
>-- 
>Marc-André Lureau

Thanks once again for the quick review.
Let me know if this makes sense, so I’ll quickly spin a cleaned-up patch which includes the tiny documentation changes.

Regards,
Prerna

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

[Michael S. Tsirkin]

On Wed, Jul 27, 2016 at 12:56:18PM +0000, Prerna Saxena wrote:
> Hi Marc,
> Thanks, please find my reply inline.
> 
> 
> 
> 
> 
> On 27/07/16 4:35 pm, "Marc-André Lureau" <marcandre.lureau@gmail.com> wrote:
> 
> >Hi
> >
> >On Wed, Jul 27, 2016 at 1:52 PM, Prerna Saxena <saxenap.ltc@gmail.com> wrote:
> >> From: Prerna Saxena <prerna.saxena@nutanix.com>
> >>
> >> This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK.
> >>
> >> If negotiated, client applications should send a u64 payload in
> >> response to any message that contains the "need_response" bit set
> >> on the message flags. Setting the payload to "zero" indicates the
> >> command finished successfully. Likewise, setting it to "non-zero"
> >> indicates an error.
> >>
> >> Currently implemented only for SET_MEM_TABLE.
> >>
> >> Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com>
> >> ---
> >>  docs/specs/vhost-user.txt | 41 +++++++++++++++++++++++++++++++++++++++++
> >>  hw/virtio/vhost-user.c    | 32 ++++++++++++++++++++++++++++++++
> >>  2 files changed, 73 insertions(+)
> >>
> >> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> >> index 777c49c..57df586 100644
> >> --- a/docs/specs/vhost-user.txt
> >> +++ b/docs/specs/vhost-user.txt
> >> @@ -37,6 +37,8 @@ consists of 3 header fields and a payload:
> >>   * Flags: 32-bit bit field:
> >>     - Lower 2 bits are the version (currently 0x01)
> >>     - Bit 2 is the reply flag - needs to be sent on each reply from the slave
> >> +   - Bit 3 is the need_response flag - see VHOST_USER_PROTOCOL_F_REPLY_ACK for
> >> +     details.
> >
> >Why need_response and not "need reply"?
> 
> (I’d already pointed this out earlier, but looks like I was possibly not very clear.)
> Before deciding on the right name for Bit 3, let us see the nomenclature for Bit 2 above : "Bit 2 is the reply flag - needs to be sent on each reply from the slave”.
> So we already have a _reply_ flag in use. If the name Bit 3 as the _need_reply_ flag, don’t you think it would be ultra-confusing ? I found it confusing  when I reviewed the documentation with this different term.
> So I chose the name need_response with much deliberation — it conveys the essence of what this flag means to achieve, but without adding to confusion.

I don't see confusion, I think I agree with Marc André.


> >
> >btw, I wonder if it would be worth to introduce an enum at this point
> >
> >>   * Size - 32-bit size of the payload
> >>
> >>
> >> @@ -126,6 +128,8 @@ the ones that do:
> >>   * VHOST_GET_VRING_BASE
> >>   * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD)
> >>
> >> +[ Also see the section on REPLY_ACK protocol extension. ]
> >> +
> >>  There are several messages that the master sends with file descriptors passed
> >>  in the ancillary data:
> >>
> >> @@ -254,6 +258,7 @@ Protocol features
> >>  #define VHOST_USER_PROTOCOL_F_MQ             0
> >>  #define VHOST_USER_PROTOCOL_F_LOG_SHMFD      1
> >>  #define VHOST_USER_PROTOCOL_F_RARP           2
> >> +#define VHOST_USER_PROTOCOL_F_REPLY_ACK      3
> >>
> >>  Message types
> >>  -------------
> >> @@ -464,3 +469,39 @@ Message types
> >>        is present in VHOST_USER_GET_PROTOCOL_FEATURES.
> >>        The first 6 bytes of the payload contain the mac address of the guest to
> >>        allow the vhost user backend to construct and broadcast the fake RARP.
> >> +
> >> +VHOST_USER_PROTOCOL_F_REPLY_ACK:
> >> +-------------------------------
> >> +The original vhost-user specification only demands responses for certain
> >
> >responses/replies
> 
> If you feel strongly about it, will change it here.
> 
> >
> >> +commands. This differs from the vhost protocol implementation where commands
> >> +are sent over an ioctl() call and block until the client has completed.
> >> +
> >> +With this protocol extension negotiated, the sender (QEMU) can set the newly
> >> +introduced "need_response" [Bit 3] flag to any command. This indicates that
> >
> >need reply, you can remove the "newly introduced" (it's not going to
> >be so new after a while)
> 
> * need_reply = no I don’t agree, for reasons cited earlier.
> * remove the “newly introduced” phrase = agree, will do.
> 
> >
> >> +the client MUST respond with a Payload VhostUserMsg indicating success or
> >
> >I would put right here for clarity:
> >
> >...MUST respond with a Payload VhostUserMsg (unless the message has
> >already an explicit reply body)...
> >
> >alternatively, I would forbid using the bit 3 on commands that have
> >already an explicit reply.
> 
> I don’t currently have any code that raises an error for such cases.
> The implementation silently ignores it.
> 
> >
> >> +failure. The payload should be set to zero on success or non-zero on failure.
> >> +In other words, response must be in the following format :
> >> +
> >> +------------------------------------
> >> +| request | flags | size | payload |
> >> +------------------------------------
> >> +
> >> + * Request: 32-bit type of the request
> >> + * Flags: 32-bit bit field:
> >> + * Size: size of the payload ( see below)
> >> + * Payload : a u64 integer, where a non-zero value indicates a failure.
> >> +
> >> +This indicates to QEMU that the requested operation has deterministically
> >> +been met or not. Today, QEMU is expected to terminate the main vhost-user
> >> +loop upon receiving such errors. In future, qemu could be taught to be more
> >> +resilient for selective requests.
> >> +
> >> +Note that as per the original vhost-user protocol, the following four messages
> >> +anyway require distinct responses from the vhost-user client process:
> >
> >I don't think we need to repeat this list (already redundant with the
> >list in "Communication" part, and with the message specification, 2
> >times is enough imho)
> 
> Ok, will remove it for brevity.
> 
> >
> >> + * VHOST_GET_FEATURES
> >> + * VHOST_GET_PROTOCOL_FEATURES
> >> + * VHOST_GET_VRING_BASE
> >> + * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD)
> >> +
> >> +For these message types, the presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or
> >> +need_response bit being set brings no behaviourial change.
> >
> >Reply
> 
> Again, I disagree, for reasons cited above.
> 
> [..snip..] removing the rest.
> >
> >
> >
> >-- 
> >Marc-André Lureau
> 
> Thanks once again for the quick review.
> Let me know if this makes sense, so I’ll quickly spin a cleaned-up patch which includes the tiny documentation changes.
> 
> Regards,
> Prerna

Re: [Qemu-devel] [PATCH v4 2/2] vhost-user: Attempt to fix a race with set_mem_table.

[Michael S. Tsirkin]

On Wed, Jul 27, 2016 at 02:52:37AM -0700, Prerna Saxena wrote:
> From: Prerna Saxena <prerna.saxena@nutanix.com>
> 
> The set_mem_table command currently does not seek a reply. Hence, there is
> no easy way for a remote application to notify to QEMU when it finished
> setting up memory, or if there were errors doing so.
> 
> As an example:
> (1) Qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net
> application). SET_MEM_TABLE does not 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 has not yet remapped the memory, but it sees the I/O request.
> (5) The application cannot satisfy the request because it does not know about those GPAs.
> 
> While a guaranteed fix would require a protocol extension (committed separately),
> a best-effort workaround for existing applications is to send a GET_FEATURES
> message before completing the vhost_user_set_mem_table() call.
> Since GET_FEATURES requires a reply, an application that processes vhost-user
> messages synchronously would probably have completed the SET_MEM_TABLE before replying.
> 
> Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com>

Could you pls reorder patchset so this is 1/2?
1/1 is still under review but I'd like to make sure
we have some kind of fix in place for 2.7.

> ---
>  hw/virtio/vhost-user.c | 123 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 65 insertions(+), 58 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 0cdb918..f96607e 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -254,64 +254,6 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
>      return 0;
>  }
>  
> -static int vhost_user_set_mem_table(struct vhost_dev *dev,
> -                                    struct vhost_memory *mem)
> -{
> -    int fds[VHOST_MEMORY_MAX_NREGIONS];
> -    int i, fd;
> -    size_t fd_num = 0;
> -    bool reply_supported = virtio_has_feature(dev->protocol_features,
> -                            VHOST_USER_PROTOCOL_F_REPLY_ACK);
> -
> -    VhostUserMsg msg = {
> -        .request = VHOST_USER_SET_MEM_TABLE,
> -        .flags = VHOST_USER_VERSION,
> -    };
> -
> -    if (reply_supported) {
> -        msg.flags |= VHOST_USER_NEED_RESPONSE_MASK;
> -    }
> -
> -    for (i = 0; i < dev->mem->nregions; ++i) {
> -        struct vhost_memory_region *reg = dev->mem->regions + i;
> -        ram_addr_t offset;
> -        MemoryRegion *mr;
> -
> -        assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
> -        mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
> -                                     &offset);
> -        fd = memory_region_get_fd(mr);
> -        if (fd > 0) {
> -            msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
> -            msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
> -            msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
> -            msg.payload.memory.regions[fd_num].mmap_offset = offset;
> -            assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
> -            fds[fd_num++] = fd;
> -        }
> -    }
> -
> -    msg.payload.memory.nregions = fd_num;
> -
> -    if (!fd_num) {
> -        error_report("Failed initializing vhost-user memory map, "
> -                     "consider using -object memory-backend-file share=on");
> -        return -1;
> -    }
> -
> -    msg.size = sizeof(msg.payload.memory.nregions);
> -    msg.size += sizeof(msg.payload.memory.padding);
> -    msg.size += fd_num * sizeof(VhostUserMemoryRegion);
> -
> -    vhost_user_write(dev, &msg, fds, fd_num);
> -
> -    if (reply_supported) {
> -        return process_message_reply(dev, msg.request);
> -    }
> -
> -    return 0;
> -}
> -
>  static int vhost_user_set_vring_addr(struct vhost_dev *dev,
>                                       struct vhost_vring_addr *addr)
>  {
> @@ -514,6 +456,71 @@ static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features)
>      return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features);
>  }
>  
> +static int vhost_user_set_mem_table(struct vhost_dev *dev,
> +                                    struct vhost_memory *mem)
> +{
> +    int fds[VHOST_MEMORY_MAX_NREGIONS];
> +    int i, fd;
> +    size_t fd_num = 0;
> +    uint64_t features;
> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                            VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +
> +    VhostUserMsg msg = {
> +        .request = VHOST_USER_SET_MEM_TABLE,
> +        .flags = VHOST_USER_VERSION,
> +    };
> +
> +    if (reply_supported) {
> +        msg.flags |= VHOST_USER_NEED_RESPONSE_MASK;
> +    }
> +
> +    for (i = 0; i < dev->mem->nregions; ++i) {
> +        struct vhost_memory_region *reg = dev->mem->regions + i;
> +        ram_addr_t offset;
> +        MemoryRegion *mr;
> +
> +        assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
> +        mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
> +                                     &offset);
> +        fd = memory_region_get_fd(mr);
> +        if (fd > 0) {
> +            msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
> +            msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
> +            msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
> +            msg.payload.memory.regions[fd_num].mmap_offset = offset;
> +            assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
> +            fds[fd_num++] = fd;
> +        }
> +    }
> +
> +    msg.payload.memory.nregions = fd_num;
> +
> +    if (!fd_num) {
> +        error_report("Failed initializing vhost-user memory map, "
> +                     "consider using -object memory-backend-file share=on");
> +        return -1;
> +    }
> +
> +    msg.size = sizeof(msg.payload.memory.nregions);
> +    msg.size += sizeof(msg.payload.memory.padding);
> +    msg.size += fd_num * sizeof(VhostUserMemoryRegion);
> +
> +    vhost_user_write(dev, &msg, fds, fd_num);
> +
> +    if (reply_supported) {
> +        return process_message_reply(dev, msg.request);
> +    } else {
> +        /* Note: It is (yet) unknown when the client application has finished
> +         * remapping the GPA.
> +         * Attempt to prevent a race by sending a command that requires a reply.
> +         */
> +        vhost_user_get_features(dev, &features);
> +    }
> +
> +    return 0;
> +}
> +
>  static int vhost_user_set_owner(struct vhost_dev *dev)
>  {
>      VhostUserMsg msg = {
> -- 
> 1.8.1.2

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

[Prerna Saxena]

On 27/07/16 6:58 pm, "Michael S. Tsirkin" <mst@redhat.com> wrote:

>On Wed, Jul 27, 2016 at 12:56:18PM +0000, Prerna Saxena wrote:
>> Hi Marc,
>> Thanks, please find my reply inline.
>> 
>> 
>> 
>> 
>> 
>> On 27/07/16 4:35 pm, "Marc-André Lureau" <marcandre.lureau@gmail.com> wrote:
>> 
>> >Hi
>> >
>> >On Wed, Jul 27, 2016 at 1:52 PM, Prerna Saxena <saxenap.ltc@gmail.com> wrote:
>> >> From: Prerna Saxena <prerna.saxena@nutanix.com>
>> >>
>> >> This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK.
>> >>
>> >> If negotiated, client applications should send a u64 payload in
>> >> response to any message that contains the "need_response" bit set
>> >> on the message flags. Setting the payload to "zero" indicates the
>> >> command finished successfully. Likewise, setting it to "non-zero"
>> >> indicates an error.
>> >>
>> >> Currently implemented only for SET_MEM_TABLE.
>> >>
>> >> Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com>
>> >> ---
>> >>  docs/specs/vhost-user.txt | 41 +++++++++++++++++++++++++++++++++++++++++
>> >>  hw/virtio/vhost-user.c    | 32 ++++++++++++++++++++++++++++++++
>> >>  2 files changed, 73 insertions(+)
>> >>
>> >> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
>> >> index 777c49c..57df586 100644
>> >> --- a/docs/specs/vhost-user.txt
>> >> +++ b/docs/specs/vhost-user.txt
>> >> @@ -37,6 +37,8 @@ consists of 3 header fields and a payload:
>> >>   * Flags: 32-bit bit field:
>> >>     - Lower 2 bits are the version (currently 0x01)
>> >>     - Bit 2 is the reply flag - needs to be sent on each reply from the slave
>> >> +   - Bit 3 is the need_response flag - see VHOST_USER_PROTOCOL_F_REPLY_ACK for
>> >> +     details.
>> >
>> >Why need_response and not "need reply"?
>> 
>> (I’d already pointed this out earlier, but looks like I was possibly not very clear.)
>> Before deciding on the right name for Bit 3, let us see the nomenclature for Bit 2 above : "Bit 2 is the reply flag - needs to be sent on each reply from the slave”.
>> So we already have a _reply_ flag in use. If the name Bit 3 as the _need_reply_ flag, don’t you think it would be ultra-confusing ? I found it confusing  when I reviewed the documentation with this different term.
>> So I chose the name need_response with much deliberation — it conveys the essence of what this flag means to achieve, but without adding to confusion.
>
>I don't see confusion, I think I agree with Marc André.


Allright. Posted a new series with the reworded terminology and updated (more concise) documentation.

Regards,
Prerna

Re: [Qemu-devel] [PATCH v4 2/2] vhost-user: Attempt to fix a race with set_mem_table.

[Prerna Saxena]

On 27/07/16 7:00 pm, "Michael S. Tsirkin" <mst@redhat.com> wrote:



>On Wed, Jul 27, 2016 at 02:52:37AM -0700, Prerna Saxena wrote:
>> From: Prerna Saxena <prerna.saxena@nutanix.com>
>> 
>> The set_mem_table command currently does not seek a reply. Hence, there is
>> no easy way for a remote application to notify to QEMU when it finished
>> setting up memory, or if there were errors doing so.
>> 
>> As an example:
>> (1) Qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net
>> application). SET_MEM_TABLE does not 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 has not yet remapped the memory, but it sees the I/O request.
>> (5) The application cannot satisfy the request because it does not know about those GPAs.
>> 
>> While a guaranteed fix would require a protocol extension (committed separately),
>> a best-effort workaround for existing applications is to send a GET_FEATURES
>> message before completing the vhost_user_set_mem_table() call.
>> Since GET_FEATURES requires a reply, an application that processes vhost-user
>> messages synchronously would probably have completed the SET_MEM_TABLE before replying.
>> 
>> Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com>
>
>Could you pls reorder patchset so this is 1/2?
>1/1 is still under review but I'd like to make sure
>we have some kind of fix in place for 2.7.

Hi Michael,
The review comments for patch 1 were around documentation and the choice of name of flag.
There has been no recommendation/comment on the code itself.
I have fixed all of that and posted a new patch series. (Version v5.1)
Hope both the patches make it in time for 2.7.

Thanks, once again, for reviewing this.

Regards,
Prerna