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

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

[Prerna Saxena]

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

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


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 v3 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 | 44 ++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio/vhost-user.c    | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)

diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 777c49c..26dbe71 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,42 @@ 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 aids debugging the application's responses from QEMU. More
+importantly, it indicates to QEMU that the requested operation has
+deterministically (not) been met. 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.
+The response from the client is identical whether or not the REPLY_ACK feature
+has been negotiated.
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 495e09f..899f354 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 {
@@ -107,6 +109,25 @@ static VhostUserMsg m __attribute__ ((unused));
 /* The version of the protocol we support */
 #define VHOST_USER_VERSION    (0x1)
 
+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 ioeventfd_enabled(void)
 {
     return kvm_enabled() && kvm_eventfds_enabled();
@@ -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 v3 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 899f354..a3a114d 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 v3 0/2] vhost-user: Extend protocol to receive replies on any command.

[Prerna]

On Thu, Jul 7, 2016 at 12:04 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 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:
>  [..snip..]
>
> 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
>
>
> 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(-)
>
>
Ping !
Michael, MarcAndre, Did you have a chance to look at this patch series?

Regards,
Prerna

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

[Marc-André Lureau]

Hi

On Mon, Jul 25, 2016 at 10:41 AM, Prerna <saxenap.ltc@gmail.com> wrote:
>
>
> On Thu, Jul 7, 2016 at 12:04 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 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:
>>  [..snip..]
>>
>> 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
>>
>>
>> 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(-)
>>
>
> Ping !
> Michael, MarcAndre, Did you have a chance to look at this patch series?
>

That's not going to make it in 2.7 I am afraid. Beside the second
patch that I think is somewhat superflous or worse, as I said in
previous review (so I won't ack it, but Michael liked it and he is the
maintainer)

It fails to compile, easy to fix by moving process_message_reply after
vhost_user_read:

/home/elmarco/src/qemu/hw/virtio/vhost-user.c: In function
‘process_message_reply’:
/home/elmarco/src/qemu/hw/virtio/vhost-user.c:117:9: warning: implicit
declaration of function ‘vhost_user_read’
[-Wimplicit-function-declaration]
     if (vhost_user_read(dev, &msg) < 0) {
         ^~~~~~~~~~~~~~~
/home/elmarco/src/qemu/hw/virtio/vhost-user.c:117:5: warning: nested
extern declaration of ‘vhost_user_read’ [-Wnested-externs]
     if (vhost_user_read(dev, &msg) < 0) {
     ^~
/home/elmarco/src/qemu/hw/virtio/vhost-user.c: At top level:
/home/elmarco/src/qemu/hw/virtio/vhost-user.c:136:12: error: static
declaration of ‘vhost_user_read’ follows non-static declaration
 static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
            ^~~~~~~~~~~~~~~
/home/elmarco/src/qemu/hw/virtio/vhost-user.c:117:9: note: previous
implicit declaration of ‘vhost_user_read’ was here
     if (vhost_user_read(dev, &msg) < 0) {
         ^~~~~~~~~~~~~~~

Secondly, make check just hangs in /x86_64/vhost-user/read-guest-mem
(a sign that backward compatibility is broken).

There is still many "response" wording, where "reply" should be used
for more consistency (VHOST_USER_NEED_RESPONSE_MASK and in the doc)

Regarding the doc, I would simplify it a bit:

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 newly
introduced "need_reply" [Bit 3] flag to any command. This indicates that
the client MUST reply 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, reply message 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 for messages that already require distinct replies, the presence of
need_reply bit being set brings no behavioural change.

-- 
Marc-André Lureau

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

[Prerna Saxena]

Hi Marc,
Thank you for taking a look.




On 25/07/16 3:57 pm, "Marc-André Lureau" <marcandre.lureau@gmail.com> wrote:

>Hi
>
>On Mon, Jul 25, 2016 at 10:41 AM, Prerna <saxenap.ltc@gmail.com> wrote:
>>
>>
>> On Thu, Jul 7, 2016 at 12:04 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 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:
>>>  [..snip..]
>>>
>>> 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
>>>
>>>
>>> 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(-)
>>>
>>
>> Ping !
>> Michael, MarcAndre, Did you have a chance to look at this patch series?
>>
>
>That's not going to make it in 2.7 I am afraid. Beside the second
>patch that I think is somewhat superflous or worse, as I said in
>previous review (so I won't ack it, but Michael liked it and he is the
>maintainer)
>
>It fails to compile, easy to fix by moving process_message_reply after
>vhost_user_read:
>
>/home/elmarco/src/qemu/hw/virtio/vhost-user.c: In function
>‘process_message_reply’:
>/home/elmarco/src/qemu/hw/virtio/vhost-user.c:117:9: warning: implicit
>declaration of function ‘vhost_user_read’
>[-Wimplicit-function-declaration]
>     if (vhost_user_read(dev, &msg) < 0) {
>         ^~~~~~~~~~~~~~~
>/home/elmarco/src/qemu/hw/virtio/vhost-user.c:117:5: warning: nested
>extern declaration of ‘vhost_user_read’ [-Wnested-externs]
>     if (vhost_user_read(dev, &msg) < 0) {
>     ^~
>/home/elmarco/src/qemu/hw/virtio/vhost-user.c: At top level:
>/home/elmarco/src/qemu/hw/virtio/vhost-user.c:136:12: error: static
>declaration of ‘vhost_user_read’ follows non-static declaration
> static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
>            ^~~~~~~~~~~~~~~
>/home/elmarco/src/qemu/hw/virtio/vhost-user.c:117:9: note: previous
>implicit declaration of ‘vhost_user_read’ was here
>     if (vhost_user_read(dev, &msg) < 0) {
>         ^~~~~~~~~~~~~~~

I really need to check on this. I am pretty positive I had verified this before posting, but its been a while since these patches were posted.


>
>Secondly, make check just hangs in /x86_64/vhost-user/read-guest-mem
>(a sign that backward compatibility is broken).
>
>There is still many "response" wording, where "reply" should be used
>for more consistency (VHOST_USER_NEED_RESPONSE_MASK and in the doc)

Right. There is a reason I havent reworded it here. We already have a VHOST_USER_REPLY_MASK 
flag that assumes that the incoming message is a reply to an already-sent vhost command.
Use of the word ‘REPLY’ in this context would have caused some confusion.

>
>Regarding the doc, I would simplify it a bit:
>
>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 newly
>introduced "need_reply" [Bit 3] flag to any command. This indicates that
>the client MUST reply 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, reply message 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 for messages that already require distinct replies, the presence of
>need_reply bit being set brings no behavioural change.
>
>-- 
>Marc-André Lureau

Regards,
Prerna

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

[Michael S. Tsirkin]

On Mon, Jul 25, 2016 at 02:27:18PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Jul 25, 2016 at 10:41 AM, Prerna <saxenap.ltc@gmail.com> wrote:
> >
> >
> > On Thu, Jul 7, 2016 at 12:04 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 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:
> >>  [..snip..]
> >>
> >> 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
> >>
> >>
> >> 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(-)
> >>
> >
> > Ping !
> > Michael, MarcAndre, Did you have a chance to look at this patch series?
> >
> 
> That's not going to make it in 2.7 I am afraid.

It's a bugfix so - depends on how quickly can comments be addressed.

-- 
MST

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

[Prerna Saxena]

On 27/07/16 9:51 am, "Michael S. Tsirkin" <mst@redhat.com> wrote:

>On Mon, Jul 25, 2016 at 02:27:18PM +0400, Marc-André Lureau wrote:
>> Hi
>> 
>> On Mon, Jul 25, 2016 at 10:41 AM, Prerna <saxenap.ltc@gmail.com> wrote:
>> >
>> >
>> > On Thu, Jul 7, 2016 at 12:04 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 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:
>> >>  [..snip..]
>> >>
>> >> 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
>> >>
>> >>
>> >> 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(-)
>> >>
>> >
>> > Ping !
>> > Michael, MarcAndre, Did you have a chance to look at this patch series?
>> >
>> 
>> That's not going to make it in 2.7 I am afraid.
>
>It's a bugfix so - depends on how quickly can comments be addressed.
>
>-- 
>MST

Thanks Michael, Marc, 
I just posted a v4 addressing the review comments. Both make-check and compilation run to completion.

Marc,
I addressed part of your suggestion on documentation. However, I have been reminded in the past about being more verbose while describing the change : <https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07428.html>

Hope this patch series is in time for 2.7 :-)

Regards,
Prerna