[for-2.7,v5.1,1/2] vhost-user: Introduce a new protocol feature REPLY_ACK.

Commit Message

Felipe Franciosi July 29, 2016, 3:31 p.m. UTC

Heya,

On 29 Jul 2016, at 13:47, Marc-André Lureau <[e-mail]<mailto:[e-mail]>> wrote:

Hi

On Thu, Jul 28, 2016 at 11:07 AM, Prerna Saxena <[e-mail]<mailto:[e-mail]>> wrote:
From: Prerna Saxena <[e-mail]<mailto:[e-mail]>>


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 <[e-mail]<mailto:[e-mail]>>

---
docs/specs/vhost-user.txt | 26 ++++++++++++++++++++++++++
hw/virtio/vhost-user.c    | 32 ++++++++++++++++++++++++++++++++
2 files changed, 58 insertions(+)

    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 <[e-mail]<mailto:[e-mail]>>




--
Marc-André Lureau

Comments

Eric Blake July 29, 2016, 8:42 p.m. UTC | #1

On 07/29/2016 09:31 AM, Felipe Franciosi wrote:
> Heya,
> 
> On 29 Jul 2016, at 13:47, Marc-André Lureau <[e-mail]<mailto:[e-mail]>> wrote:
> 
> Hi
> 
> On Thu, Jul 28, 2016 at 11:07 AM, Prerna Saxena <[e-mail]<mailto:[e-mail]>> wrote:
> From: Prerna Saxena <[e-mail]<mailto:[e-mail]>>

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.

Felipe Franciosi July 30, 2016, 9:38 a.m. UTC | #2

(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 <[e-mail]> wrote:

> 

> On 07/29/2016 09:31 AM, Felipe Franciosi wrote:

>> Heya,

>> 

>> On 29 Jul 2016, at 13:47, Marc-André Lureau <[e-mail]<mailto:[e-mail]>> wrote:

>> 

>> Hi

>> 

>> On Thu, Jul 28, 2016 at 11:07 AM, Prerna Saxena <[e-mail]<mailto:[e-mail]>> wrote:

>> From: Prerna Saxena <[e-mail]<mailto:[e-mail]>>

> 

> 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    [phone]

> Libvirt virtualization library http://libvirt.org

>

Patch

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