[Qemu-devel] [PATCH v2 0/3] vhost-user: support safe protocol

[Qemu-devel] [PATCH v2 0/3] vhost-user: support safe protocol

[linhaifeng]

From: Linhaifeng <haifeng.lin@huawei.com>

Mostly the same as ioctl master need the return value to
decided going on or not.So we add these patches for more
safe communication.

change log:
v1->v2: modify the annotate about slave's version.

Linhaifeng (3):
  vhost-user: update the protocol.
  vhost-user:update the version to 0x6
  vhost-user:add reply for other messages

 docs/specs/vhost-user.txt | 17 +++++++++++--
 hw/virtio/vhost-user.c    | 64 ++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 67 insertions(+), 14 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v2 1/3] vhost-user: update the protocol.

[linhaifeng]

From: Linhaifeng <haifeng.lin@huawei.com>

Every messages need reply.
This path just update the vhost-user.txt to version 0x6.

Signed-off-by: Linhaifeng <haifeng.lin@huawei.com>
---
 docs/specs/vhost-user.txt | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 650bb18..448babc 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -23,6 +23,10 @@ be a software Ethernet switch running in user space, such as Snabbswitch.
 Master and slave can be either a client (i.e. connecting) or server (listening)
 in the socket communication.
 
+version 0x1: Supply base communication between master and slave.
+version 0x6: Add reply for more robust.
+
+
 Message Specification
 ---------------------
 
@@ -35,7 +39,7 @@ consists of 3 header fields and a payload:
 
  * Request: 32-bit type of the request
  * Flags: 32-bit bit field:
-   - Lower 2 bits are the version (currently 0x01)
+   - Lower 2 bits are the version (currently 0x06)
    - Bit 2 is the reply flag - needs to be sent on each reply from the slave
  * Size - 32-bit size of the payload
 
@@ -144,6 +148,7 @@ Message types
       Id: 2
       Ioctl: VHOST_SET_FEATURES
       Master payload: u64
+      Slave payload: u64 0:success else:fail
 
       Enable features in the underlying vhost implementation using a bitmask.
 
@@ -171,6 +176,7 @@ Message types
       Id: 5
       Equivalent ioctl: VHOST_SET_MEM_TABLE
       Master payload: memory regions description
+      Slave payload: u64 0:success else:fail
 
       Sets the memory map regions on the slave so it can translate the vring
       addresses. In the ancillary data there is an array of file descriptors
@@ -182,6 +188,7 @@ Message types
       Id: 6
       Equivalent ioctl: VHOST_SET_LOG_BASE
       Master payload: u64
+      Slave payload: u64 0:success else:fail
 
       Sets the logging base address.
 
@@ -190,6 +197,7 @@ Message types
       Id: 7
       Equivalent ioctl: VHOST_SET_LOG_FD
       Master payload: N/A
+      Slave payload: u64 0:success else:fail
 
       Sets the logging file descriptor, which is passed as ancillary data.
 
@@ -198,6 +206,7 @@ Message types
       Id: 8
       Equivalent ioctl: VHOST_SET_VRING_NUM
       Master payload: vring state description
+      Slave payload: u64 0:success else:fail
 
       Sets the number of vrings for this owner.
 
@@ -206,7 +215,7 @@ Message types
       Id: 9
       Equivalent ioctl: VHOST_SET_VRING_ADDR
       Master payload: vring address description
-      Slave payload: N/A
+      Slave payload: u64 0:success else:fail
 
       Sets the addresses of the different aspects of the vring.
 
@@ -215,6 +224,7 @@ Message types
       Id: 10
       Equivalent ioctl: VHOST_SET_VRING_BASE
       Master payload: vring state description
+      Slave payload: u64 0:success else:fail
 
       Sets the base offset in the available vring.
 
@@ -232,6 +242,7 @@ Message types
       Id: 12
       Equivalent ioctl: VHOST_SET_VRING_KICK
       Master payload: u64
+      Slave payload: u64 0:success else:fail
 
       Set the event file descriptor for adding buffers to the vring. It
       is passed in the ancillary data.
@@ -245,6 +256,7 @@ Message types
       Id: 13
       Equivalent ioctl: VHOST_SET_VRING_CALL
       Master payload: u64
+      Slave payload: u64 0:success else:fail
 
       Set the event file descriptor to signal when buffers are used. It
       is passed in the ancillary data.
@@ -258,6 +270,7 @@ Message types
       Id: 14
       Equivalent ioctl: VHOST_SET_VRING_ERR
       Master payload: u64
+      Slave payload: u64 0:success else:fail
 
       Set the event file descriptor to signal when error occurs. It
       is passed in the ancillary data.
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v2 1/3] vhost-user: update the protocol.

[Michael S. Tsirkin]

On Fri, Feb 13, 2015 at 09:45:37PM +0800, linhaifeng wrote:
> @@ -35,7 +39,7 @@ consists of 3 header fields and a payload:
>  
>   * Request: 32-bit type of the request
>   * Flags: 32-bit bit field:
> -   - Lower 2 bits are the version (currently 0x01)
> +   - Lower 2 bits are the version (currently 0x06)
>     - Bit 2 is the reply flag - needs to be sent on each reply from the slave
>   * Size - 32-bit size of the payload
>  

How do you encode 0x6 in a 2 bit field?

-- 
MST

Re: [Qemu-devel] [PATCH v2 1/3] vhost-user: update the protocol.

[Linhaifeng]

On 2015/2/15 5:03, Michael S. Tsirkin wrote:
> On Fri, Feb 13, 2015 at 09:45:37PM +0800, linhaifeng wrote:
>> @@ -35,7 +39,7 @@ consists of 3 header fields and a payload:
>>  
>>   * Request: 32-bit type of the request
>>   * Flags: 32-bit bit field:
>> -   - Lower 2 bits are the version (currently 0x01)
>> +   - Lower 2 bits are the version (currently 0x06)
>>     - Bit 2 is the reply flag - needs to be sent on each reply from the slave
>>   * Size - 32-bit size of the payload
>>  
> 
> How do you encode 0x6 in a 2 bit field?
> 

I think the reply flag is unnecessary,so i remove it and let all the field as version.
The existing version is 0x5 now.

Sorry, i need to update the vhost-user.txt to describe it.


-- 
Regards,
Haifeng

[Qemu-devel] [PATCH v2 2/3] vhost-user:update the version to 0x6

[linhaifeng]

From: Linhaifeng <haifeng.lin@huawei.com>

We not need the VHOST_USER_REPLY_MASK so the base version now is 0x5.
  - update the version to 0x6.
  - change the name form flag to version.

Signed-off-by: Linhaifeng <haifeng.lin@huawei.com>
---
 hw/virtio/vhost-user.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index aefe0bb..d56115a 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -59,10 +59,7 @@ typedef struct VhostUserMemory {
 
 typedef struct VhostUserMsg {
     VhostUserRequest request;
-
-#define VHOST_USER_VERSION_MASK     (0x3)
-#define VHOST_USER_REPLY_MASK       (0x1<<2)
-    uint32_t flags;
+    uint32_t version;
     uint32_t size; /* the following payload size */
     union {
 #define VHOST_USER_VRING_IDX_MASK   (0xff)
@@ -74,15 +71,18 @@ typedef struct VhostUserMsg {
     };
 } QEMU_PACKED VhostUserMsg;
 
+static uint32_t slave_version;
 static VhostUserMsg m __attribute__ ((unused));
 #define VHOST_USER_HDR_SIZE (sizeof(m.request) \
-                            + sizeof(m.flags) \
+                            + sizeof(m.version) \
                             + sizeof(m.size))
 
 #define VHOST_USER_PAYLOAD_SIZE (sizeof(m) - VHOST_USER_HDR_SIZE)
 
-/* The version of the protocol we support */
-#define VHOST_USER_VERSION    (0x1)
+/* The version of the protocol we support.
+ * Slaves' version should maller than  VHOST_USER_VERSION.
+ */
+#define VHOST_USER_VERSION    (0x6)
 
 static bool ioeventfd_enabled(void)
 {
@@ -134,12 +134,12 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
     }
 
     /* validate received flags */
-    if (msg->flags != (VHOST_USER_REPLY_MASK | VHOST_USER_VERSION)) {
-        error_report("Failed to read msg header."
-                " Flags 0x%x instead of 0x%x.\n", msg->flags,
-                VHOST_USER_REPLY_MASK | VHOST_USER_VERSION);
+    if (msg->version > VHOST_USER_VERSION) {
+        error_report("Invalid version 0x%x.\n"
+                "Vhost user version is 0x%x", msg->version, VHOST_USER_VERSION);
         goto fail;
     }
+    slave_version = msg->version;
 
     /* validate message size is sane */
     if (msg->size > VHOST_USER_PAYLOAD_SIZE) {
@@ -195,7 +195,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
 
     msg_request = vhost_user_request_translate(request);
     msg.request = msg_request;
-    msg.flags = VHOST_USER_VERSION;
+    msg.version = VHOST_USER_VERSION;
     msg.size = 0;
 
     switch (request) {
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 3/3] vhost-user:add reply for other messages

[linhaifeng]

From: Linhaifeng <haifeng.lin@huawei.com>

If slave's version bigger than 0x5 we will wait for reply.

Signed-off-by: Linhaifeng <haifeng.lin@huawei.com>
---
 hw/virtio/vhost-user.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index d56115a..ae684b6 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -80,10 +80,17 @@ static VhostUserMsg m __attribute__ ((unused));
 #define VHOST_USER_PAYLOAD_SIZE (sizeof(m) - VHOST_USER_HDR_SIZE)
 
 /* The version of the protocol we support.
- * Slaves' version should maller than  VHOST_USER_VERSION.
+ * Slaves' version must not bigger than  VHOST_USER_VERSION.
  */
+#define VHOST_USER_BASE       (0x5)
 #define VHOST_USER_VERSION    (0x6)
 
+#define VHOST_NEED_REPLY \
+{    \
+    if (slave_version > VHOST_USER_BASE)     \
+        need_reply = 1;    \
+}
+
 static bool ioeventfd_enabled(void)
 {
     return kvm_enabled() && kvm_eventfds_enabled();
@@ -207,6 +214,8 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
     case VHOST_SET_LOG_BASE:
         msg.u64 = *((__u64 *) arg);
         msg.size = sizeof(m.u64);
+
+        VHOST_NEED_REPLY;
         break;
 
     case VHOST_SET_OWNER:
@@ -244,16 +253,21 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
         msg.size += sizeof(m.memory.padding);
         msg.size += fd_num * sizeof(VhostUserMemoryRegion);
 
+        VHOST_NEED_REPLY;
         break;
 
     case VHOST_SET_LOG_FD:
         fds[fd_num++] = *((int *) arg);
+
+        VHOST_NEED_REPLY;
         break;
 
     case VHOST_SET_VRING_NUM:
     case VHOST_SET_VRING_BASE:
         memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
         msg.size = sizeof(m.state);
+
+        VHOST_NEED_REPLY;
         break;
 
     case VHOST_GET_VRING_BASE:
@@ -265,6 +279,8 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
     case VHOST_SET_VRING_ADDR:
         memcpy(&msg.addr, arg, sizeof(struct vhost_vring_addr));
         msg.size = sizeof(m.addr);
+
+        VHOST_NEED_REPLY;
         break;
 
     case VHOST_SET_VRING_KICK:
@@ -278,6 +294,8 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
         } else {
             msg.u64 |= VHOST_USER_VRING_NOFD_MASK;
         }
+
+        VHOST_NEED_REPLY;
         break;
     default:
         error_report("vhost-user trying to send unhandled ioctl\n");
@@ -315,6 +333,28 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
             }
             memcpy(arg, &msg.state, sizeof(struct vhost_vring_state));
             break;
+        case VHOST_USER_SET_FEATURES:
+        case VHOST_USER_SET_LOG_BASE:
+        case VHOST_USER_SET_OWNER:
+        case VHOST_USER_RESET_OWNER:
+        case VHOST_USER_SET_MEM_TABLE:
+        case VHOST_USER_SET_LOG_FD:
+        case VHOST_USER_SET_VRING_NUM:
+        case VHOST_USER_SET_VRING_BASE:
+        case VHOST_USER_SET_VRING_ADDR:
+        case VHOST_USER_SET_VRING_KICK:
+        case VHOST_USER_SET_VRING_CALL:
+        case VHOST_USER_SET_VRING_ERR:
+            if (msg.size != sizeof(m.u64)) {
+                error_report("Received bad msg size.");
+                return -1;
+            } else {
+                if (m.u64) {
+                    error_report("Failed to handle request %d.", msg_request);
+                    return -1;
+                }
+            }
+            break;
         default:
             error_report("Received unexpected msg type.\n");
             return -1;
-- 
1.7.12.4