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

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

[Prerna Saxena]

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

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

The current vhost-user protocol requires the client to send reply 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 reply to any message by setting
the newly introduced "need_reply" 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 v5->v5.1 :
1) Patch 1 : no change
2) Patch 2 : fixes a tiny typo I'd accidentally introduced while creating v5 from v4. The code itself is unchanged from v4.

Changes v4->v5:
1) Patch 1 :
* Reword 'response' to 'reply' on public demand.
* Documentation is more concise.
Patch 2 : unchanged

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
v4 : https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06173.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

[Qemu-devel] [PATCH for-2.7 v5.1 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_reply" bit set
on the message flags. Setting the payload to "zero" indicates the
command finished successfully. Likewise, setting it to "non-zero"
indicates an error.

Currently implemented only for SET_MEM_TABLE.

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

diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 777c49c..54b5c8f 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -37,6 +37,8 @@ consists of 3 header fields and a payload:
  * Flags: 32-bit bit field:
    - Lower 2 bits are the version (currently 0x01)
    - Bit 2 is the reply flag - needs to be sent on each reply from the slave
+   - Bit 3 is the need_reply flag - see VHOST_USER_PROTOCOL_F_REPLY_ACK for
+     details.
  * Size - 32-bit size of the payload
 
 
@@ -126,6 +128,8 @@ the ones that do:
  * VHOST_GET_VRING_BASE
  * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD)
 
+[ Also see the section on REPLY_ACK protocol extension. ]
+
 There are several messages that the master sends with file descriptors passed
 in the ancillary data:
 
@@ -254,6 +258,7 @@ Protocol features
 #define VHOST_USER_PROTOCOL_F_MQ             0
 #define VHOST_USER_PROTOCOL_F_LOG_SHMFD      1
 #define VHOST_USER_PROTOCOL_F_RARP           2
+#define VHOST_USER_PROTOCOL_F_REPLY_ACK      3
 
 Message types
 -------------
@@ -464,3 +469,24 @@ Message types
       is present in VHOST_USER_GET_PROTOCOL_FEATURES.
       The first 6 bytes of the payload contain the mac address of the guest to
       allow the vhost user backend to construct and broadcast the fake RARP.
+
+VHOST_USER_PROTOCOL_F_REPLY_ACK:
+-------------------------------
+The original vhost-user specification only demands replies for certain
+commands. This differs from the vhost protocol implementation where commands
+are sent over an ioctl() call and block until the client has completed.
+
+With this protocol extension negotiated, the sender (QEMU) can set the
+"need_reply" [Bit 3] flag to any command. This indicates that
+the client MUST respond with a Payload VhostUserMsg indicating success or
+failure. The payload should be set to zero on success or non-zero on failure.
+(Unless the message already has an explicit reply body)
+
+This indicates to QEMU that the requested operation has deterministically
+been met or not. Today, QEMU is expected to terminate the main vhost-user
+loop upon receiving such errors. In future, qemu could be taught to be more
+resilient for selective requests.
+
+For the message types that already solicit a reply from the client, the
+presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or need_reply bit being set brings
+no behaviourial change. (See the 'Communication' section for details.)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 495e09f..86e7ae0 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -31,6 +31,7 @@ enum VhostUserProtocolFeature {
     VHOST_USER_PROTOCOL_F_MQ = 0,
     VHOST_USER_PROTOCOL_F_LOG_SHMFD = 1,
     VHOST_USER_PROTOCOL_F_RARP = 2,
+    VHOST_USER_PROTOCOL_F_REPLY_ACK = 3,
 
     VHOST_USER_PROTOCOL_F_MAX
 };
@@ -84,6 +85,7 @@ typedef struct VhostUserMsg {
 
 #define VHOST_USER_VERSION_MASK     (0x3)
 #define VHOST_USER_REPLY_MASK       (0x1<<2)
+#define VHOST_USER_NEED_REPLY_MASK       (0x1 << 3)
     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_REPLY_MASK;
+    }
+
     for (i = 0; i < dev->mem->nregions; ++i) {
         struct vhost_memory_region *reg = dev->mem->regions + i;
         ram_addr_t offset;
@@ -277,6 +305,10 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
 
     vhost_user_write(dev, &msg, fds, fd_num);
 
+    if (reply_supported) {
+        return process_message_reply(dev, msg.request);
+    }
+
     return 0;
 }
 
-- 
1.8.1.2

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

[Marc-André Lureau]

Hi

On Thu, Jul 28, 2016 at 11:07 AM, 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_reply" bit set
> on the message flags. Setting the payload to "zero" indicates the
> command finished successfully. Likewise, setting it to "non-zero"
> indicates an error.
>
> Currently implemented only for SET_MEM_TABLE.
>
> Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com>
> ---
>  docs/specs/vhost-user.txt | 26 ++++++++++++++++++++++++++
>  hw/virtio/vhost-user.c    | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
>
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 777c49c..54b5c8f 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -37,6 +37,8 @@ consists of 3 header fields and a payload:
>   * Flags: 32-bit bit field:
>     - Lower 2 bits are the version (currently 0x01)
>     - Bit 2 is the reply flag - needs to be sent on each reply from the slave
> +   - Bit 3 is the need_reply flag - see VHOST_USER_PROTOCOL_F_REPLY_ACK for
> +     details.
>   * Size - 32-bit size of the payload
>
>
> @@ -126,6 +128,8 @@ the ones that do:
>   * VHOST_GET_VRING_BASE
>   * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD)
>
> +[ Also see the section on REPLY_ACK protocol extension. ]
> +
>  There are several messages that the master sends with file descriptors passed
>  in the ancillary data:
>
> @@ -254,6 +258,7 @@ Protocol features
>  #define VHOST_USER_PROTOCOL_F_MQ             0
>  #define VHOST_USER_PROTOCOL_F_LOG_SHMFD      1
>  #define VHOST_USER_PROTOCOL_F_RARP           2
> +#define VHOST_USER_PROTOCOL_F_REPLY_ACK      3
>
>  Message types
>  -------------
> @@ -464,3 +469,24 @@ Message types
>        is present in VHOST_USER_GET_PROTOCOL_FEATURES.
>        The first 6 bytes of the payload contain the mac address of the guest to
>        allow the vhost user backend to construct and broadcast the fake RARP.
> +
> +VHOST_USER_PROTOCOL_F_REPLY_ACK:
> +-------------------------------
> +The original vhost-user specification only demands replies for certain
> +commands. This differs from the vhost protocol implementation where commands
> +are sent over an ioctl() call and block until the client has completed.
> +
> +With this protocol extension negotiated, the sender (QEMU) can set the
> +"need_reply" [Bit 3] flag to any command. This indicates that
> +the client MUST respond with a Payload VhostUserMsg indicating success or
> +failure. The payload should be set to zero on success or non-zero on failure.
> +(Unless the message already has an explicit reply body)

Unless/unless (for consistency, the rest of the document doesn't use
Upper-case inside parentheses)

> +
> +This indicates to QEMU that the requested operation has deterministically
> +been met or not. Today, QEMU is expected to terminate the main vhost-user
> +loop upon receiving such errors. In future, qemu could be taught to be more
> +resilient for selective requests.
> +
> +For the message types that already solicit a reply from the client, the
> +presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or need_reply bit being set brings
> +no behaviourial change. (See the 'Communication' section for details.)

See/see

> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 495e09f..86e7ae0 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -31,6 +31,7 @@ enum VhostUserProtocolFeature {
>      VHOST_USER_PROTOCOL_F_MQ = 0,
>      VHOST_USER_PROTOCOL_F_LOG_SHMFD = 1,
>      VHOST_USER_PROTOCOL_F_RARP = 2,
> +    VHOST_USER_PROTOCOL_F_REPLY_ACK = 3,
>
>      VHOST_USER_PROTOCOL_F_MAX
>  };
> @@ -84,6 +85,7 @@ typedef struct VhostUserMsg {
>
>  #define VHOST_USER_VERSION_MASK     (0x3)
>  #define VHOST_USER_REPLY_MASK       (0x1<<2)
> +#define VHOST_USER_NEED_REPLY_MASK       (0x1 << 3)

You could align it, and use the same style as the line above

>      uint32_t flags;
>      uint32_t size; /* the following payload size */
>      union {
> @@ -158,6 +160,25 @@ fail:
>      return -1;
>  }
>
> +static int process_message_reply(struct vhost_dev *dev,
> +                                    VhostUserRequest request)

bad indentation

> +{
> +    VhostUserMsg msg;
> +
> +    if (vhost_user_read(dev, &msg) < 0) {
> +        return 0;

return -1

> +    }
> +
> +    if (msg.request != request) {
> +        error_report("Received unexpected msg type."
> +                        "Expected %d received %d",
> +                        request, msg.request);

bad indentation

> +        return -1;
> +    }
> +
> +    return msg.payload.u64 ? -1 : 0;
> +}
> +
>  static bool vhost_user_one_time_request(VhostUserRequest request)
>  {
>      switch (request) {
> @@ -239,11 +260,18 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
>      int fds[VHOST_MEMORY_MAX_NREGIONS];
>      int i, fd;
>      size_t fd_num = 0;
> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                            VHOST_USER_PROTOCOL_F_REPLY_ACK);

bad indentation

> +
>      VhostUserMsg msg = {
>          .request = VHOST_USER_SET_MEM_TABLE,
>          .flags = VHOST_USER_VERSION,
>      };
>
> +    if (reply_supported) {
> +        msg.flags |= VHOST_USER_NEED_REPLY_MASK;
> +    }
> +
>      for (i = 0; i < dev->mem->nregions; ++i) {
>          struct vhost_memory_region *reg = dev->mem->regions + i;
>          ram_addr_t offset;
> @@ -277,6 +305,10 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
>
>      vhost_user_write(dev, &msg, fds, fd_num);
>
> +    if (reply_supported) {
> +        return process_message_reply(dev, msg.request);
> +    }
> +
>      return 0;
>  }
>
> --
> 1.8.1.2
>

Other than that, looks good to me, with those fixes


Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



-- 
Marc-André Lureau

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

[Felipe Franciosi]

Heya,

On 29 Jul 2016, at 13:47, Marc-André Lureau <marcandre.lureau@gmail.com<mailto:marcandre.lureau@gmail.com>> wrote:

Hi

On Thu, Jul 28, 2016 at 11:07 AM, Prerna Saxena <saxenap.ltc@gmail.com<mailto:saxenap.ltc@gmail.com>> wrote:
From: Prerna Saxena <prerna.saxena@nutanix.com<mailto: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_reply" bit set
on the message flags. Setting the payload to "zero" indicates the
command finished successfully. Likewise, setting it to "non-zero"
indicates an error.

Currently implemented only for SET_MEM_TABLE.

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

diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 777c49c..54b5c8f 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -37,6 +37,8 @@ consists of 3 header fields and a payload:
 * Flags: 32-bit bit field:
   - Lower 2 bits are the version (currently 0x01)
   - Bit 2 is the reply flag - needs to be sent on each reply from the slave
+   - Bit 3 is the need_reply flag - see VHOST_USER_PROTOCOL_F_REPLY_ACK for
+     details.
 * Size - 32-bit size of the payload


@@ -126,6 +128,8 @@ the ones that do:
 * VHOST_GET_VRING_BASE
 * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD)

+[ Also see the section on REPLY_ACK protocol extension. ]
+
There are several messages that the master sends with file descriptors passed
in the ancillary data:

@@ -254,6 +258,7 @@ Protocol features
#define VHOST_USER_PROTOCOL_F_MQ             0
#define VHOST_USER_PROTOCOL_F_LOG_SHMFD      1
#define VHOST_USER_PROTOCOL_F_RARP           2
+#define VHOST_USER_PROTOCOL_F_REPLY_ACK      3

Message types
-------------
@@ -464,3 +469,24 @@ Message types
      is present in VHOST_USER_GET_PROTOCOL_FEATURES.
      The first 6 bytes of the payload contain the mac address of the guest to
      allow the vhost user backend to construct and broadcast the fake RARP.
+
+VHOST_USER_PROTOCOL_F_REPLY_ACK:
+-------------------------------
+The original vhost-user specification only demands replies for certain
+commands. This differs from the vhost protocol implementation where commands
+are sent over an ioctl() call and block until the client has completed.
+
+With this protocol extension negotiated, the sender (QEMU) can set the
+"need_reply" [Bit 3] flag to any command. This indicates that
+the client MUST respond with a Payload VhostUserMsg indicating success or
+failure. The payload should be set to zero on success or non-zero on failure.
+(Unless the message already has an explicit reply body)

Unless/unless (for consistency, the rest of the document doesn't use
Upper-case inside parentheses)

Actually, if the sentence starts inside the parenthesis it should be capital.
See rule 2a:
http://www.grammarbook.com/punctuation/parens.asp

Prerna's text looks correct to me. If it's wrong in other places we should probably fix it there separately.


+
+This indicates to QEMU that the requested operation has deterministically
+been met or not. Today, QEMU is expected to terminate the main vhost-user
+loop upon receiving such errors. In future, qemu could be taught to be more
+resilient for selective requests.
+
+For the message types that already solicit a reply from the client, the
+presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or need_reply bit being set brings
+no behaviourial change. (See the 'Communication' section for details.)

See/see

Same as my comment above.

Cheers,
Felipe


diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 495e09f..86e7ae0 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -31,6 +31,7 @@ enum VhostUserProtocolFeature {
    VHOST_USER_PROTOCOL_F_MQ = 0,
    VHOST_USER_PROTOCOL_F_LOG_SHMFD = 1,
    VHOST_USER_PROTOCOL_F_RARP = 2,
+    VHOST_USER_PROTOCOL_F_REPLY_ACK = 3,

    VHOST_USER_PROTOCOL_F_MAX
};
@@ -84,6 +85,7 @@ typedef struct VhostUserMsg {

#define VHOST_USER_VERSION_MASK     (0x3)
#define VHOST_USER_REPLY_MASK       (0x1<<2)
+#define VHOST_USER_NEED_REPLY_MASK       (0x1 << 3)

You could align it, and use the same style as the line above

    uint32_t flags;
    uint32_t size; /* the following payload size */
    union {
@@ -158,6 +160,25 @@ fail:
    return -1;
}

+static int process_message_reply(struct vhost_dev *dev,
+                                    VhostUserRequest request)

bad indentation

+{
+    VhostUserMsg msg;
+
+    if (vhost_user_read(dev, &msg) < 0) {
+        return 0;

return -1

+    }
+
+    if (msg.request != request) {
+        error_report("Received unexpected msg type."
+                        "Expected %d received %d",
+                        request, msg.request);

bad indentation

+        return -1;
+    }
+
+    return msg.payload.u64 ? -1 : 0;
+}
+
static bool vhost_user_one_time_request(VhostUserRequest request)
{
    switch (request) {
@@ -239,11 +260,18 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
    int fds[VHOST_MEMORY_MAX_NREGIONS];
    int i, fd;
    size_t fd_num = 0;
+    bool reply_supported = virtio_has_feature(dev->protocol_features,
+                            VHOST_USER_PROTOCOL_F_REPLY_ACK);

bad indentation

+
    VhostUserMsg msg = {
        .request = VHOST_USER_SET_MEM_TABLE,
        .flags = VHOST_USER_VERSION,
    };

+    if (reply_supported) {
+        msg.flags |= VHOST_USER_NEED_REPLY_MASK;
+    }
+
    for (i = 0; i < dev->mem->nregions; ++i) {
        struct vhost_memory_region *reg = dev->mem->regions + i;
        ram_addr_t offset;
@@ -277,6 +305,10 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,

    vhost_user_write(dev, &msg, fds, fd_num);

+    if (reply_supported) {
+        return process_message_reply(dev, msg.request);
+    }
+
    return 0;
}

--
1.8.1.2


Other than that, looks good to me, with those fixes


Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com<mailto:marcandre.lureau@redhat.com>>



--
Marc-André Lureau

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1989 bytes --]

On 07/29/2016 09:31 AM, Felipe Franciosi wrote:
> Heya,
> 
> On 29 Jul 2016, at 13:47, Marc-André Lureau <marcandre.lureau@gmail.com<mailto:marcandre.lureau@gmail.com>> wrote:
> 
> Hi
> 
> On Thu, Jul 28, 2016 at 11:07 AM, Prerna Saxena <saxenap.ltc@gmail.com<mailto:saxenap.ltc@gmail.com>> wrote:
> From: Prerna Saxena <prerna.saxena@nutanix.com<mailto:prerna.saxena@nutanix.com>>

Wow, your mailer's poor choice of quoting style makes it VERY difficult
to read your reply.


> +With this protocol extension negotiated, the sender (QEMU) can set the
> +"need_reply" [Bit 3] flag to any command. This indicates that
> +the client MUST respond with a Payload VhostUserMsg indicating success or
> +failure. The payload should be set to zero on success or non-zero on failure.
> +(Unless the message already has an explicit reply body)
> 
> Unless/unless (for consistency, the rest of the document doesn't use
> Upper-case inside parentheses)
> 
> Actually, if the sentence starts inside the parenthesis it should be capital.
> See rule 2a:
> http://www.grammarbook.com/punctuation/parens.asp
> 
> Prerna's text looks correct to me. If it's wrong in other places we should probably fix it there separately.
> 

Based on the number of '>' inserted by my mailer, it appears that you
wrote all four of the above paragraphs. In reality, Prerna wrote the
first paragraph (quoted from the patch, Marc-André wrote the second, and
you wrote the third and fourth.  In fact, your mailer ACTIVELY stripped
the '>' that was already present in Marc-André's mail when he quoted
Prerna, rather than the usual paradigm of adding yet another layer of '>'.

You really need to get that fixed.  We should not have to hunt and
compare multiple emails in order to determine what you are adding to the
conversation, nor mis-attribute it to the wrong author.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Felipe Franciosi]

(Intentionally top-posting:)

Hey Eric,

That's really odd. I don't recall changing anything, but I just checked my (other) e-mail on the nongnu.org's archive and indeed it looks awful. (It looks fine on both clients I use, though.) Thanks for letting me know, I'll sort this out.

Cheers,
Felipe


> On 29 Jul 2016, at 21:42, Eric Blake <eblake@redhat.com> wrote:
> 
> On 07/29/2016 09:31 AM, Felipe Franciosi wrote:
>> Heya,
>> 
>> On 29 Jul 2016, at 13:47, Marc-André Lureau <marcandre.lureau@gmail.com<mailto:marcandre.lureau@gmail.com>> wrote:
>> 
>> Hi
>> 
>> On Thu, Jul 28, 2016 at 11:07 AM, Prerna Saxena <saxenap.ltc@gmail.com<mailto:saxenap.ltc@gmail.com>> wrote:
>> From: Prerna Saxena <prerna.saxena@nutanix.com<mailto:prerna.saxena@nutanix.com>>
> 
> Wow, your mailer's poor choice of quoting style makes it VERY difficult
> to read your reply.
> 
> 
>> +With this protocol extension negotiated, the sender (QEMU) can set the
>> +"need_reply" [Bit 3] flag to any command. This indicates that
>> +the client MUST respond with a Payload VhostUserMsg indicating success or
>> +failure. The payload should be set to zero on success or non-zero on failure.
>> +(Unless the message already has an explicit reply body)
>> 
>> Unless/unless (for consistency, the rest of the document doesn't use
>> Upper-case inside parentheses)
>> 
>> Actually, if the sentence starts inside the parenthesis it should be capital.
>> See rule 2a:
>> http://www.grammarbook.com/punctuation/parens.asp
>> 
>> Prerna's text looks correct to me. If it's wrong in other places we should probably fix it there separately.
>> 
> 
> Based on the number of '>' inserted by my mailer, it appears that you
> wrote all four of the above paragraphs. In reality, Prerna wrote the
> first paragraph (quoted from the patch, Marc-André wrote the second, and
> you wrote the third and fourth.  In fact, your mailer ACTIVELY stripped
> the '>' that was already present in Marc-André's mail when he quoted
> Prerna, rather than the usual paradigm of adding yet another layer of '>'.
> 
> You really need to get that fixed.  We should not have to hunt and
> compare multiple emails in order to determine what you are adding to the
> conversation, nor mis-attribute it to the wrong author.
> 
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

On 07/28/2016 01:07 AM, Prerna Saxena wrote:
> From: Prerna Saxena <prerna.saxena@nutanix.com>
> 
> This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK.
> 

> +
> +With this protocol extension negotiated, the sender (QEMU) can set the
> +"need_reply" [Bit 3] flag to any command. This indicates that
> +the client MUST respond with a Payload VhostUserMsg indicating success or
> +failure. The payload should be set to zero on success or non-zero on failure.
> +(Unless the message already has an explicit reply body)

Rather than make this parenthetical, I would go with:

The payload should be set to zero on success or non-zero on failure,
unless the message already has an explicit reply body.

> +
> +This indicates to QEMU that the requested operation has deterministically
> +been met or not. Today, QEMU is expected to terminate the main vhost-user

Reads awkwardly; maybe:

The response payload gives QEMU a deterministic indication of the result
of the command.

> +loop upon receiving such errors. In future, qemu could be taught to be more
> +resilient for selective requests.
> +
> +For the message types that already solicit a reply from the client, the
> +presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or need_reply bit being set brings
> +no behaviourial change. (See the 'Communication' section for details.)

s/behaviourial/behavioural/ (or if the document widely favors US
spelling, behavioral)


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Prerna Saxena]

On 30/07/16 2:19 am, "Eric Blake" <eblake@redhat.com> wrote:

>On 07/28/2016 01:07 AM, Prerna Saxena wrote:
>> From: Prerna Saxena <prerna.saxena@nutanix.com>
>> 
>> This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK.
>> 
>
>> +
>> +With this protocol extension negotiated, the sender (QEMU) can set the
>> +"need_reply" [Bit 3] flag to any command. This indicates that
>> +the client MUST respond with a Payload VhostUserMsg indicating success or
>> +failure. The payload should be set to zero on success or non-zero on failure.
>> +(Unless the message already has an explicit reply body)
>
>Rather than make this parenthetical, I would go with:
>
>The payload should be set to zero on success or non-zero on failure,
>unless the message already has an explicit reply body.

Hi Eric,
Thank you for taking a look, but I think you possibly missed the latest patchset posted last night.
This had already been incorporated in v6 that I’d posted last night before your message.
See https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06772.html


>
>> +
>> +This indicates to QEMU that the requested operation has deterministically
>> +been met or not. Today, QEMU is expected to terminate the main vhost-user
>
>Reads awkwardly; maybe:
>
>The response payload gives QEMU a deterministic indication of the result
>of the command.

Hmm, it is more of personal taste, so I’ll refrain from commenting either way.

>
>> +loop upon receiving such errors. In future, qemu could be taught to be more
>> +resilient for selective requests.
>> +
>> +For the message types that already solicit a reply from the client, the
>> +presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or need_reply bit being set brings
>> +no behaviourial change. (See the 'Communication' section for details.)
>
>s/behaviourial/behavioural/ (or if the document widely favors US
>spelling, behavioral)


The last 3 iterations of this patchset have only seen review comments focussed on documentation suggestions and indentation of code, but nothing on the idea/code itself. This gives me hope that the patch is possibly close to merging within 2.7 timeframe :-)
May I request the maintainers to please correct this tiny spelling typo as this is checked in?

Regards,
Prerna

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

[Michael S. Tsirkin]

On Sat, Jul 30, 2016 at 06:38:23AM +0000, Prerna Saxena wrote:
> 
> 
> 
> 
> 
> On 30/07/16 2:19 am, "Eric Blake" <eblake@redhat.com> wrote:
> 
> >On 07/28/2016 01:07 AM, Prerna Saxena wrote:
> >> From: Prerna Saxena <prerna.saxena@nutanix.com>
> >> 
> >> This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK.
> >> 
> >
> >> +
> >> +With this protocol extension negotiated, the sender (QEMU) can set the
> >> +"need_reply" [Bit 3] flag to any command. This indicates that
> >> +the client MUST respond with a Payload VhostUserMsg indicating success or
> >> +failure. The payload should be set to zero on success or non-zero on failure.
> >> +(Unless the message already has an explicit reply body)
> >
> >Rather than make this parenthetical, I would go with:
> >
> >The payload should be set to zero on success or non-zero on failure,
> >unless the message already has an explicit reply body.
> 
> Hi Eric,
> Thank you for taking a look, but I think you possibly missed the latest patchset posted last night.
> This had already been incorporated in v6 that I’d posted last night before your message.
> See https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06772.html
> 
> 
> >
> >> +
> >> +This indicates to QEMU that the requested operation has deterministically
> >> +been met or not. Today, QEMU is expected to terminate the main vhost-user
> >
> >Reads awkwardly; maybe:
> >
> >The response payload gives QEMU a deterministic indication of the result
> >of the command.
> 
> Hmm, it is more of personal taste, so I’ll refrain from commenting either way.

I prefer Eric's form too. "that ... or not" isn't very clear.

> >
> >> +loop upon receiving such errors. In future, qemu could be taught to be more
> >> +resilient for selective requests.
> >> +
> >> +For the message types that already solicit a reply from the client, the
> >> +presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or need_reply bit being set brings
> >> +no behaviourial change. (See the 'Communication' section for details.)
> >
> >s/behaviourial/behavioural/ (or if the document widely favors US
> >spelling, behavioral)
> 
> 
> The last 3 iterations of this patchset have only seen review comments focussed on documentation suggestions and indentation of code, but nothing on the idea/code itself. This gives me hope that the patch is possibly close to merging within 2.7 timeframe :-)
> May I request the maintainers to please correct this tiny spelling typo as this is checked in?
> 
> Regards,
> Prerna

Probably easier to post v7 with above minor things.

-- 
MST

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

[Prerna Saxena]

On 04/08/16 9:41 am, "Michael S. Tsirkin" <mst@redhat.com> wrote:



>On Sat, Jul 30, 2016 at 06:38:23AM +0000, Prerna Saxena wrote:
>> 
>> 
>> 
>> 
>> 
>> On 30/07/16 2:19 am, "Eric Blake" <eblake@redhat.com> wrote:
>> 
>> >On 07/28/2016 01:07 AM, Prerna Saxena wrote:
>> >> From: Prerna Saxena <prerna.saxena@nutanix.com>
>> >> 
>> >> This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK.
>> >> 
>> >
>> >> +
>> >> +With this protocol extension negotiated, the sender (QEMU) can set the
>> >> +"need_reply" [Bit 3] flag to any command. This indicates that
>> >> +the client MUST respond with a Payload VhostUserMsg indicating success or
>> >> +failure. The payload should be set to zero on success or non-zero on failure.
>> >> +(Unless the message already has an explicit reply body)
>> >
>> >Rather than make this parenthetical, I would go with:
>> >
>> >The payload should be set to zero on success or non-zero on failure,
>> >unless the message already has an explicit reply body.
>> 
>> Hi Eric,
>> Thank you for taking a look, but I think you possibly missed the latest patchset posted last night.
>> This had already been incorporated in v6 that I’d posted last night before your message.
>> See https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06772.html
>> 
>> 
>> >
>> >> +
>> >> +This indicates to QEMU that the requested operation has deterministically
>> >> +been met or not. Today, QEMU is expected to terminate the main vhost-user
>> >
>> >Reads awkwardly; maybe:
>> >
>> >The response payload gives QEMU a deterministic indication of the result
>> >of the command.
>> 
>> Hmm, it is more of personal taste, so I’ll refrain from commenting either way.
>
>I prefer Eric's form too. "that ... or not" isn't very clear.

Done.

>
>> >
>> >> +loop upon receiving such errors. In future, qemu could be taught to be more
>> >> +resilient for selective requests.
>> >> +
>> >> +For the message types that already solicit a reply from the client, the
>> >> +presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or need_reply bit being set brings
>> >> +no behaviourial change. (See the 'Communication' section for details.)
>> >
>> >s/behaviourial/behavioural/ (or if the document widely favors US
>> >spelling, behavioral)
>> 
>> 
>> The last 3 iterations of this patchset have only seen review comments focussed on documentation suggestions and indentation of code, but nothing on the idea/code itself. This gives me hope that the patch is possibly close to merging within 2.7 timeframe :-)
>> May I request the maintainers to please correct this tiny spelling typo as this is checked in?
>> 
>> Regards,
>> Prerna
>
>Probably easier to post v7 with above minor things.

Posted a v7 which incorporates all suggestions made by Eric.
https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01027.html

Regards,

[Qemu-devel] [PATCH for-2.7 v5.1 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 86e7ae0..d0dafa0 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_REPLY_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_REPLY_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