[Qemu-devel] [PATCH 0/4] vhost-user: protocol updates

[Qemu-devel] [PATCH 0/4] vhost-user: protocol updates

[Michael S. Tsirkin]

This patchset sets the stage for extending the vhost user
protocol, with full backwards compatibility.

The approach is to negotiate feature bits queried and
acknowledged during device setup.

For now, there's no new functionality: two new messages
are added that will allow negotiating new messages
required for functionality such as MQ and migration.

For now, I used the feature bit 30 to signal support for these new messages,
and we now have 64 more bits to play.

The patches can be found in my tree, branch vhost-user.

Only patch 1 is intended for 2.4.

Posting early so people working on extensions such as
migration can review this - but please note
the protocol is not set in stone yet.

Michael S. Tsirkin (4):
  Revert "vhost-user: add multi queue support"
  vhost-user: refactor ioctl translation
  vhost-user: add protocol feature negotiation
  vhost-user: unit test for new messages

 qapi-schema.json          |   6 +-
 include/hw/virtio/vhost.h |   1 +
 hw/net/vhost_net.c        |   5 +-
 hw/virtio/vhost-user.c    | 150 ++++++++++++++++++++++++++++++----------------
 net/vhost-user.c          |  37 ++++--------
 tests/vhost-user-test.c   |  19 ++++++
 docs/specs/vhost-user.txt |  40 +++++++++++--
 qemu-options.hx           |   5 +-
 8 files changed, 174 insertions(+), 89 deletions(-)

-- 
MST

[Qemu-devel] [PATCH 1/4] Revert "vhost-user: add multi queue support"

[Michael S. Tsirkin]

This reverts commit 830d70db692e374b55555f4407f96a1ceefdcc97.

The interface as merged isn't fully backwards-compatible with existing
clients. Revert it.  Let's redo this after 2.4, based on protocol
extensions in follow-up patches.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 qapi-schema.json          |  6 +-----
 hw/net/vhost_net.c        |  3 +--
 hw/virtio/vhost-user.c    | 11 +----------
 net/vhost-user.c          | 37 +++++++++++++------------------------
 docs/specs/vhost-user.txt |  5 -----
 qemu-options.hx           |  5 ++---
 6 files changed, 18 insertions(+), 49 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 106008c..88a3883 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2463,16 +2463,12 @@
 #
 # @vhostforce: #optional vhost on for non-MSIX virtio guests (default: false).
 #
-# @queues: #optional number of queues to be created for multiqueue vhost-user
-#          (default: 1) (Since 2.4)
-#
 # Since 2.1
 ##
 { 'struct': 'NetdevVhostUserOptions',
   'data': {
     'chardev':        'str',
-    '*vhostforce':    'bool',
-    '*queues':        'uint32' } }
+    '*vhostforce':    'bool' } }
 
 ##
 # @NetClientOptions
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 9bd360b..5c1d11f 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -160,7 +160,6 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
 
     net->dev.nvqs = 2;
     net->dev.vqs = net->vqs;
-    net->dev.vq_index = net->nc->queue_index;
 
     r = vhost_dev_init(&net->dev, options->opaque,
                        options->backend_type);
@@ -287,7 +286,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
         for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
             const VhostOps *vhost_ops = net->dev.vhost_ops;
             int r = vhost_ops->vhost_call(&net->dev, VHOST_RESET_OWNER,
-                                          &file);
+                                          NULL);
             assert(r >= 0);
         }
     }
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index d6f2163..e7ab829 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -210,12 +210,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
         break;
 
     case VHOST_SET_OWNER:
-        break;
-
     case VHOST_RESET_OWNER:
-        memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
-        msg.state.index += dev->vq_index;
-        msg.size = sizeof(m.state);
         break;
 
     case VHOST_SET_MEM_TABLE:
@@ -258,20 +253,17 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
     case VHOST_SET_VRING_NUM:
     case VHOST_SET_VRING_BASE:
         memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
-        msg.state.index += dev->vq_index;
         msg.size = sizeof(m.state);
         break;
 
     case VHOST_GET_VRING_BASE:
         memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
-        msg.state.index += dev->vq_index;
         msg.size = sizeof(m.state);
         need_reply = 1;
         break;
 
     case VHOST_SET_VRING_ADDR:
         memcpy(&msg.addr, arg, sizeof(struct vhost_vring_addr));
-        msg.addr.index += dev->vq_index;
         msg.size = sizeof(m.addr);
         break;
 
@@ -279,7 +271,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
     case VHOST_SET_VRING_CALL:
     case VHOST_SET_VRING_ERR:
         file = arg;
-        msg.u64 = (file->index + dev->vq_index) & VHOST_USER_VRING_IDX_MASK;
+        msg.u64 = file->index & VHOST_USER_VRING_IDX_MASK;
         msg.size = sizeof(m.u64);
         if (ioeventfd_enabled() && file->fd > 0) {
             fds[fd_num++] = file->fd;
@@ -321,7 +313,6 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
                 error_report("Received bad msg size.");
                 return -1;
             }
-            msg.state.index -= dev->vq_index;
             memcpy(arg, &msg.state, sizeof(struct vhost_vring_state));
             break;
         default:
diff --git a/net/vhost-user.c b/net/vhost-user.c
index b51bc04..93dcecd 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -120,39 +120,35 @@ static void net_vhost_user_event(void *opaque, int event)
     case CHR_EVENT_OPENED:
         vhost_user_start(s);
         net_vhost_link_down(s, false);
-        error_report("chardev \"%s\" went up", s->nc.info_str);
+        error_report("chardev \"%s\" went up", s->chr->label);
         break;
     case CHR_EVENT_CLOSED:
         net_vhost_link_down(s, true);
         vhost_user_stop(s);
-        error_report("chardev \"%s\" went down", s->nc.info_str);
+        error_report("chardev \"%s\" went down", s->chr->label);
         break;
     }
 }
 
 static int net_vhost_user_init(NetClientState *peer, const char *device,
-                               const char *name, CharDriverState *chr,
-                               uint32_t queues)
+                               const char *name, CharDriverState *chr)
 {
     NetClientState *nc;
     VhostUserState *s;
-    int i;
 
-    for (i = 0; i < queues; i++) {
-        nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
+    nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
 
-        snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user%d to %s",
-                 i, chr->label);
+    snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user to %s",
+             chr->label);
 
-        s = DO_UPCAST(VhostUserState, nc, nc);
+    s = DO_UPCAST(VhostUserState, nc, nc);
 
-        /* We don't provide a receive callback */
-        s->nc.receive_disabled = 1;
-        s->chr = chr;
-        s->nc.queue_index = i;
+    /* We don't provide a receive callback */
+    s->nc.receive_disabled = 1;
+    s->chr = chr;
+
+    qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s);
 
-        qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s);
-    }
     return 0;
 }
 
@@ -230,7 +226,6 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
 int net_init_vhost_user(const NetClientOptions *opts, const char *name,
                         NetClientState *peer, Error **errp)
 {
-    uint32_t queues;
     const NetdevVhostUserOptions *vhost_user_opts;
     CharDriverState *chr;
 
@@ -248,12 +243,6 @@ int net_init_vhost_user(const NetClientOptions *opts, const char *name,
         return -1;
     }
 
-    /* number of queues for multiqueue */
-    if (vhost_user_opts->has_queues) {
-        queues = vhost_user_opts->queues;
-    } else {
-        queues = 1;
-    }
 
-    return net_vhost_user_init(peer, "vhost_user", name, chr, queues);
+    return net_vhost_user_init(peer, "vhost_user", name, chr);
 }
diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 2c8e934..650bb18 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -127,11 +127,6 @@ in the ancillary data:
 If Master is unable to send the full message or receives a wrong reply it will
 close the connection. An optional reconnection mechanism can be implemented.
 
-Multi queue support
--------------------
-The protocol supports multiple queues by setting all index fields in the sent
-messages to a properly calculated value.
-
 Message types
 -------------
 
diff --git a/qemu-options.hx b/qemu-options.hx
index 7b8efbf..8c9add9 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1963,14 +1963,13 @@ The hubport netdev lets you connect a NIC to a QEMU "vlan" instead of a single
 netdev.  @code{-net} and @code{-device} with parameter @option{vlan} create the
 required hub automatically.
 
-@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off][,queues=n]
+@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off]
 
 Establish a vhost-user netdev, backed by a chardev @var{id}. The chardev should
 be a unix domain socket backed one. The vhost-user uses a specifically defined
 protocol to pass vhost ioctl replacement messages to an application on the other
 end of the socket. On non-MSIX guests, the feature can be forced with
-@var{vhostforce}. Use 'queues=@var{n}' to specify the number of queues to
-be created for multiqueue vhost-user.
+@var{vhostforce}.
 
 Example:
 @example
-- 
MST

[Qemu-devel] [PATCH 2/4] vhost-user: refactor ioctl translation

[Michael S. Tsirkin]

translate at the outset, have rest of code use vhost-user features
exclusively.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/vhost-user.c | 87 ++++++++++++++++++++++++++------------------------
 1 file changed, 46 insertions(+), 41 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index e7ab829..5a83d00 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -89,35 +89,40 @@ static bool ioeventfd_enabled(void)
     return kvm_enabled() && kvm_eventfds_enabled();
 }
 
-static unsigned long int ioctl_to_vhost_user_request[VHOST_USER_MAX] = {
-    -1,                     /* VHOST_USER_NONE */
-    VHOST_GET_FEATURES,     /* VHOST_USER_GET_FEATURES */
-    VHOST_SET_FEATURES,     /* VHOST_USER_SET_FEATURES */
-    VHOST_SET_OWNER,        /* VHOST_USER_SET_OWNER */
-    VHOST_RESET_OWNER,      /* VHOST_USER_RESET_OWNER */
-    VHOST_SET_MEM_TABLE,    /* VHOST_USER_SET_MEM_TABLE */
-    VHOST_SET_LOG_BASE,     /* VHOST_USER_SET_LOG_BASE */
-    VHOST_SET_LOG_FD,       /* VHOST_USER_SET_LOG_FD */
-    VHOST_SET_VRING_NUM,    /* VHOST_USER_SET_VRING_NUM */
-    VHOST_SET_VRING_ADDR,   /* VHOST_USER_SET_VRING_ADDR */
-    VHOST_SET_VRING_BASE,   /* VHOST_USER_SET_VRING_BASE */
-    VHOST_GET_VRING_BASE,   /* VHOST_USER_GET_VRING_BASE */
-    VHOST_SET_VRING_KICK,   /* VHOST_USER_SET_VRING_KICK */
-    VHOST_SET_VRING_CALL,   /* VHOST_USER_SET_VRING_CALL */
-    VHOST_SET_VRING_ERR     /* VHOST_USER_SET_VRING_ERR */
-};
-
 static VhostUserRequest vhost_user_request_translate(unsigned long int request)
 {
-    VhostUserRequest idx;
-
-    for (idx = 0; idx < VHOST_USER_MAX; idx++) {
-        if (ioctl_to_vhost_user_request[idx] == request) {
-            break;
-        }
+    switch (request) {
+    case VHOST_GET_FEATURES:
+        return VHOST_USER_GET_FEATURES;
+    case VHOST_SET_FEATURES:
+        return VHOST_USER_SET_FEATURES;
+    case VHOST_SET_OWNER:
+        return VHOST_USER_SET_OWNER;
+    case VHOST_RESET_OWNER:
+        return VHOST_USER_RESET_OWNER;
+    case VHOST_SET_MEM_TABLE:
+        return VHOST_USER_SET_MEM_TABLE;
+    case VHOST_SET_LOG_BASE:
+        return VHOST_USER_SET_LOG_BASE;
+    case VHOST_SET_LOG_FD:
+        return VHOST_USER_SET_LOG_FD;
+    case VHOST_SET_VRING_NUM:
+        return VHOST_USER_SET_VRING_NUM;
+    case VHOST_SET_VRING_ADDR:
+        return VHOST_USER_SET_VRING_ADDR;
+    case VHOST_SET_VRING_BASE:
+        return VHOST_USER_SET_VRING_BASE;
+    case VHOST_GET_VRING_BASE:
+        return VHOST_USER_GET_VRING_BASE;
+    case VHOST_SET_VRING_KICK:
+        return VHOST_USER_SET_VRING_KICK;
+    case VHOST_SET_VRING_CALL:
+        return VHOST_USER_SET_VRING_CALL;
+    case VHOST_SET_VRING_ERR:
+        return VHOST_USER_SET_VRING_ERR;
+    default:
+        return VHOST_USER_MAX;
     }
-
-    return (idx == VHOST_USER_MAX) ? VHOST_USER_NONE : idx;
 }
 
 static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
@@ -198,22 +203,22 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
     msg.flags = VHOST_USER_VERSION;
     msg.size = 0;
 
-    switch (request) {
-    case VHOST_GET_FEATURES:
+    switch (msg_request) {
+    case VHOST_USER_GET_FEATURES:
         need_reply = 1;
         break;
 
-    case VHOST_SET_FEATURES:
-    case VHOST_SET_LOG_BASE:
+    case VHOST_USER_SET_FEATURES:
+    case VHOST_USER_SET_LOG_BASE:
         msg.u64 = *((__u64 *) arg);
         msg.size = sizeof(m.u64);
         break;
 
-    case VHOST_SET_OWNER:
-    case VHOST_RESET_OWNER:
+    case VHOST_USER_SET_OWNER:
+    case VHOST_USER_RESET_OWNER:
         break;
 
-    case VHOST_SET_MEM_TABLE:
+    case VHOST_USER_SET_MEM_TABLE:
         for (i = 0; i < dev->mem->nregions; ++i) {
             struct vhost_memory_region *reg = dev->mem->regions + i;
             ram_addr_t ram_addr;
@@ -246,30 +251,30 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
 
         break;
 
-    case VHOST_SET_LOG_FD:
+    case VHOST_USER_SET_LOG_FD:
         fds[fd_num++] = *((int *) arg);
         break;
 
-    case VHOST_SET_VRING_NUM:
-    case VHOST_SET_VRING_BASE:
+    case VHOST_USER_SET_VRING_NUM:
+    case VHOST_USER_SET_VRING_BASE:
         memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
         msg.size = sizeof(m.state);
         break;
 
-    case VHOST_GET_VRING_BASE:
+    case VHOST_USER_GET_VRING_BASE:
         memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
         msg.size = sizeof(m.state);
         need_reply = 1;
         break;
 
-    case VHOST_SET_VRING_ADDR:
+    case VHOST_USER_SET_VRING_ADDR:
         memcpy(&msg.addr, arg, sizeof(struct vhost_vring_addr));
         msg.size = sizeof(m.addr);
         break;
 
-    case VHOST_SET_VRING_KICK:
-    case VHOST_SET_VRING_CALL:
-    case VHOST_SET_VRING_ERR:
+    case VHOST_USER_SET_VRING_KICK:
+    case VHOST_USER_SET_VRING_CALL:
+    case VHOST_USER_SET_VRING_ERR:
         file = arg;
         msg.u64 = file->index & VHOST_USER_VRING_IDX_MASK;
         msg.size = sizeof(m.u64);
-- 
MST

[Qemu-devel] [PATCH 3/4] vhost-user: add protocol feature negotiation

[Michael S. Tsirkin]

Support a separate bitmask for vhost-user protocol features,
and messages to get/set protocol features.

Invoke them at init.

No features are defined yet.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/virtio/vhost.h |  1 +
 hw/net/vhost_net.c        |  2 ++
 hw/virtio/vhost-user.c    | 52 +++++++++++++++++++++++++++++++++++++++++++++++
 docs/specs/vhost-user.txt | 37 +++++++++++++++++++++++++++++++++
 4 files changed, 92 insertions(+)

diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index dd51050..6467c73 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -47,6 +47,7 @@ struct vhost_dev {
     unsigned long long features;
     unsigned long long acked_features;
     unsigned long long backend_features;
+    unsigned long long protocol_features;
     bool started;
     bool log_enabled;
     unsigned long long log_size;
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 5c1d11f..c864237 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -152,8 +152,10 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
         net->dev.backend_features = qemu_has_vnet_hdr(options->net_backend)
             ? 0 : (1ULL << VHOST_NET_F_VIRTIO_NET_HDR);
         net->backend = r;
+        net->dev.protocol_features = 0;
     } else {
         net->dev.backend_features = 0;
+        net->dev.protocol_features = 0;
         net->backend = -1;
     }
     net->nc = options->net_backend;
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 5a83d00..c4428a1 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -25,6 +25,9 @@
 
 #define VHOST_MEMORY_MAX_NREGIONS    8
 
+#define VHOST_USER_F_PROTOCOL_FEATURES 30
+#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x0ULL
+
 typedef enum VhostUserRequest {
     VHOST_USER_NONE = 0,
     VHOST_USER_GET_FEATURES = 1,
@@ -41,6 +44,8 @@ typedef enum VhostUserRequest {
     VHOST_USER_SET_VRING_KICK = 12,
     VHOST_USER_SET_VRING_CALL = 13,
     VHOST_USER_SET_VRING_ERR = 14,
+    VHOST_USER_GET_PROTOCOL_FEATURES = 15,
+    VHOST_USER_SET_PROTOCOL_FEATURES = 26,
     VHOST_USER_MAX
 } VhostUserRequest;
 
@@ -332,10 +337,57 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
 
 static int vhost_user_init(struct vhost_dev *dev, void *opaque)
 {
+    VhostUserMsg msg = { 0 };
+    int err;
+
     assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
 
     dev->opaque = opaque;
 
+    msg.request = VHOST_USER_GET_FEATURES;
+    msg.flags = VHOST_USER_VERSION;
+    msg.size = 0;
+
+    err = vhost_user_write(dev, &msg, NULL, 0);
+    if (err < 0) {
+        return err;
+    }
+
+    err = vhost_user_read(dev, &msg);
+    if (err < 0) {
+        return err;
+    }
+
+    if (__virtio_has_feature(msg.u64, VHOST_USER_F_PROTOCOL_FEATURES)) {
+        dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
+
+        msg.request = VHOST_USER_GET_PROTOCOL_FEATURES;
+        msg.flags = VHOST_USER_VERSION;
+        msg.size = 0;
+
+        err = vhost_user_write(dev, &msg, NULL, 0);
+        if (err < 0) {
+            return err;
+        }
+
+        err = vhost_user_read(dev, &msg);
+        if (err < 0) {
+            return err;
+        }
+
+        dev->protocol_features = msg.u64 & VHOST_USER_PROTOCOL_FEATURE_MASK;
+
+        msg.request = VHOST_USER_SET_PROTOCOL_FEATURES;
+        msg.flags = VHOST_USER_VERSION;
+        msg.u64 = dev->protocol_features;
+        msg.size = sizeof msg.u64;
+
+        err = vhost_user_write(dev, &msg, NULL, 0);
+        if (err < 0) {
+            return err;
+        }
+    }
+
     return 0;
 }
 
diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 650bb18..0062baa 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -113,6 +113,7 @@ message replies. Most of the requests don't require replies. Here is a list of
 the ones that do:
 
  * VHOST_GET_FEATURES
+ * VHOST_GET_PROTOCOL_FEATURES
  * VHOST_GET_VRING_BASE
 
 There are several messages that the master sends with file descriptors passed
@@ -127,6 +128,13 @@ in the ancillary data:
 If Master is unable to send the full message or receives a wrong reply it will
 close the connection. An optional reconnection mechanism can be implemented.
 
+Any protocol extensions are gated by protocol feature bits,
+which allows full backwards compatibility on both master
+and slave.
+As older slaves don't support negotiating protocol features,
+a feature bit was dedicated for this purpose:
+#define VHOST_USER_F_PROTOCOL_FEATURES 30
+
 Message types
 -------------
 
@@ -138,6 +146,8 @@ Message types
       Slave payload: u64
 
       Get from the underlying vhost implementation the features bitmask.
+      Feature bit VHOST_USER_F_PROTOCOL_FEATURES signals slave support for
+      VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES.
 
  * VHOST_USER_SET_FEATURES
 
@@ -146,6 +156,33 @@ Message types
       Master payload: u64
 
       Enable features in the underlying vhost implementation using a bitmask.
+      Feature bit VHOST_USER_F_PROTOCOL_FEATURES signals master support for
+      VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES.
+
+ * VHOST_USER_GET_PROTOCOL_FEATURES
+
+      Id: 15
+      Equivalent ioctl: VHOST_GET_FEATURES
+      Master payload: N/A
+      Slave payload: u64
+
+      Get the protocol feature bitmask from the underlying vhost implementation.
+      Only legal if feature bit VHOST_USER_F_PROTOCOL_FEATURES is present in
+      VHOST_USER_GET_FEATURES.
+      Note: slave that reported VHOST_USER_F_PROTOCOL_FEATURES must support
+      this message even before VHOST_USER_SET_FEATURES was called.
+
+ * VHOST_USER_SET_PROTOCOL_FEATURES
+
+      Id: 16
+      Ioctl: VHOST_SET_FEATURES
+      Master payload: u64
+
+      Enable protocol features in the underlying vhost implementation.
+      Only legal if feature bit VHOST_USER_F_PROTOCOL_FEATURES is present in
+      VHOST_USER_GET_FEATURES.
+      Note: slave that reported VHOST_USER_F_PROTOCOL_FEATURES must support
+      this message even before VHOST_USER_SET_FEATURES was called.
 
  * VHOST_USER_SET_OWNER
 
-- 
MST

[Qemu-devel] [PATCH 4/4] vhost-user: unit test for new messages

[Michael S. Tsirkin]

Data is empty for now, but do make sure master
sets the new feature bit flag.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tests/vhost-user-test.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 75fedf0..228acb6 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -53,6 +53,8 @@
 
 #define VHOST_MEMORY_MAX_NREGIONS    8
 
+#define VHOST_USER_F_PROTOCOL_FEATURES 30
+
 typedef enum VhostUserRequest {
     VHOST_USER_NONE = 0,
     VHOST_USER_GET_FEATURES = 1,
@@ -69,6 +71,8 @@ typedef enum VhostUserRequest {
     VHOST_USER_SET_VRING_KICK = 12,
     VHOST_USER_SET_VRING_CALL = 13,
     VHOST_USER_SET_VRING_ERR = 14,
+    VHOST_USER_GET_PROTOCOL_FEATURES = 15,
+    VHOST_USER_SET_PROTOCOL_FEATURES = 16,
     VHOST_USER_MAX
 } VhostUserRequest;
 
@@ -293,11 +297,26 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         /* send back features to qemu */
         msg.flags |= VHOST_USER_REPLY_MASK;
         msg.size = sizeof(m.u64);
+        msg.u64 = 0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
+        p = (uint8_t *) &msg;
+        qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
+        break;
+
+    case VHOST_USER_SET_FEATURES:
+	g_assert_cmpint(msg.u64 & (0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES),
+			!=, 0ULL);
+        break;
+
+    case VHOST_USER_GET_PROTOCOL_FEATURES:
+        /* send back features to qemu */
+        msg.flags |= VHOST_USER_REPLY_MASK;
+        msg.size = sizeof(m.u64);
         msg.u64 = 0;
         p = (uint8_t *) &msg;
         qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
         break;
 
+
     case VHOST_USER_GET_VRING_BASE:
         /* send back vring base to qemu */
         msg.flags |= VHOST_USER_REPLY_MASK;
-- 
MST

Re: [Qemu-devel] [PATCH for-2.4 1/4] Revert "vhost-user: add multi queue support"

[Eric Blake]

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

On 07/17/2015 08:09 AM, Michael S. Tsirkin wrote:
> This reverts commit 830d70db692e374b55555f4407f96a1ceefdcc97.
> 
> The interface as merged isn't fully backwards-compatible with existing
> clients. Revert it.  Let's redo this after 2.4, based on protocol
> extensions in follow-up patches.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  qapi-schema.json          |  6 +-----
>  hw/net/vhost_net.c        |  3 +--
>  hw/virtio/vhost-user.c    | 11 +----------
>  net/vhost-user.c          | 37 +++++++++++++------------------------
>  docs/specs/vhost-user.txt |  5 -----
>  qemu-options.hx           |  5 ++---
>  6 files changed, 18 insertions(+), 49 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 0/4] vhost-user: protocol updates

[Maxime Leroy]

Hi Michael,

On Fri, Jul 17, 2015 at 4:09 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> This patchset sets the stage for extending the vhost user
> protocol, with full backwards compatibility.
>
> The approach is to negotiate feature bits queried and
> acknowledged during device setup.
>
> For now, there's no new functionality: two new messages
> are added that will allow negotiating new messages
> required for functionality such as MQ and migration.
>
> For now, I used the feature bit 30 to signal support for these new messages,
> and we now have 64 more bits to play.
>
> The patches can be found in my tree, branch vhost-user.

Where can I find your qemu tree ? I would like to check the multi-queue case.

Thanks,

Maxime

Re: [Qemu-devel] [PATCH 3/4] vhost-user: add protocol feature negotiation

[Flavio Leitner]

On Fri, Jul 17, 2015 at 05:09:38PM +0300, Michael S. Tsirkin wrote:
> Support a separate bitmask for vhost-user protocol features,
> and messages to get/set protocol features.
> 
> Invoke them at init.
> 
> No features are defined yet.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/virtio/vhost.h |  1 +
>  hw/net/vhost_net.c        |  2 ++
>  hw/virtio/vhost-user.c    | 52 +++++++++++++++++++++++++++++++++++++++++++++++
>  docs/specs/vhost-user.txt | 37 +++++++++++++++++++++++++++++++++
>  4 files changed, 92 insertions(+)
> 
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index dd51050..6467c73 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -47,6 +47,7 @@ struct vhost_dev {
>      unsigned long long features;
>      unsigned long long acked_features;
>      unsigned long long backend_features;
> +    unsigned long long protocol_features;
>      bool started;
>      bool log_enabled;
>      unsigned long long log_size;
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 5c1d11f..c864237 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -152,8 +152,10 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
>          net->dev.backend_features = qemu_has_vnet_hdr(options->net_backend)
>              ? 0 : (1ULL << VHOST_NET_F_VIRTIO_NET_HDR);
>          net->backend = r;
> +        net->dev.protocol_features = 0;
>      } else {
>          net->dev.backend_features = 0;
> +        net->dev.protocol_features = 0;
>          net->backend = -1;
>      }
>      net->nc = options->net_backend;
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 5a83d00..c4428a1 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -25,6 +25,9 @@
>  
>  #define VHOST_MEMORY_MAX_NREGIONS    8
>  
> +#define VHOST_USER_F_PROTOCOL_FEATURES 30
> +#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x0ULL
> +
>  typedef enum VhostUserRequest {
>      VHOST_USER_NONE = 0,
>      VHOST_USER_GET_FEATURES = 1,
> @@ -41,6 +44,8 @@ typedef enum VhostUserRequest {
>      VHOST_USER_SET_VRING_KICK = 12,
>      VHOST_USER_SET_VRING_CALL = 13,
>      VHOST_USER_SET_VRING_ERR = 14,
> +    VHOST_USER_GET_PROTOCOL_FEATURES = 15,
> +    VHOST_USER_SET_PROTOCOL_FEATURES = 26,

I think you meant 16 here.
fbl


>      VHOST_USER_MAX
>  } VhostUserRequest;
>  
> @@ -332,10 +337,57 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
>  
>  static int vhost_user_init(struct vhost_dev *dev, void *opaque)
>  {
> +    VhostUserMsg msg = { 0 };
> +    int err;
> +
>      assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
>  
>      dev->opaque = opaque;
>  
> +    msg.request = VHOST_USER_GET_FEATURES;
> +    msg.flags = VHOST_USER_VERSION;
> +    msg.size = 0;
> +
> +    err = vhost_user_write(dev, &msg, NULL, 0);
> +    if (err < 0) {
> +        return err;
> +    }
> +
> +    err = vhost_user_read(dev, &msg);
> +    if (err < 0) {
> +        return err;
> +    }
> +
> +    if (__virtio_has_feature(msg.u64, VHOST_USER_F_PROTOCOL_FEATURES)) {
> +        dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> +
> +        msg.request = VHOST_USER_GET_PROTOCOL_FEATURES;
> +        msg.flags = VHOST_USER_VERSION;
> +        msg.size = 0;
> +
> +        err = vhost_user_write(dev, &msg, NULL, 0);
> +        if (err < 0) {
> +            return err;
> +        }
> +
> +        err = vhost_user_read(dev, &msg);
> +        if (err < 0) {
> +            return err;
> +        }
> +
> +        dev->protocol_features = msg.u64 & VHOST_USER_PROTOCOL_FEATURE_MASK;
> +
> +        msg.request = VHOST_USER_SET_PROTOCOL_FEATURES;
> +        msg.flags = VHOST_USER_VERSION;
> +        msg.u64 = dev->protocol_features;
> +        msg.size = sizeof msg.u64;
> +
> +        err = vhost_user_write(dev, &msg, NULL, 0);
> +        if (err < 0) {
> +            return err;
> +        }
> +    }
> +
>      return 0;
>  }
>  
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 650bb18..0062baa 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -113,6 +113,7 @@ message replies. Most of the requests don't require replies. Here is a list of
>  the ones that do:
>  
>   * VHOST_GET_FEATURES
> + * VHOST_GET_PROTOCOL_FEATURES
>   * VHOST_GET_VRING_BASE
>  
>  There are several messages that the master sends with file descriptors passed
> @@ -127,6 +128,13 @@ in the ancillary data:
>  If Master is unable to send the full message or receives a wrong reply it will
>  close the connection. An optional reconnection mechanism can be implemented.
>  
> +Any protocol extensions are gated by protocol feature bits,
> +which allows full backwards compatibility on both master
> +and slave.
> +As older slaves don't support negotiating protocol features,
> +a feature bit was dedicated for this purpose:
> +#define VHOST_USER_F_PROTOCOL_FEATURES 30
> +
>  Message types
>  -------------
>  
> @@ -138,6 +146,8 @@ Message types
>        Slave payload: u64
>  
>        Get from the underlying vhost implementation the features bitmask.
> +      Feature bit VHOST_USER_F_PROTOCOL_FEATURES signals slave support for
> +      VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES.
>  
>   * VHOST_USER_SET_FEATURES
>  
> @@ -146,6 +156,33 @@ Message types
>        Master payload: u64
>  
>        Enable features in the underlying vhost implementation using a bitmask.
> +      Feature bit VHOST_USER_F_PROTOCOL_FEATURES signals master support for
> +      VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES.
> +
> + * VHOST_USER_GET_PROTOCOL_FEATURES
> +
> +      Id: 15
> +      Equivalent ioctl: VHOST_GET_FEATURES
> +      Master payload: N/A
> +      Slave payload: u64
> +
> +      Get the protocol feature bitmask from the underlying vhost implementation.
> +      Only legal if feature bit VHOST_USER_F_PROTOCOL_FEATURES is present in
> +      VHOST_USER_GET_FEATURES.
> +      Note: slave that reported VHOST_USER_F_PROTOCOL_FEATURES must support
> +      this message even before VHOST_USER_SET_FEATURES was called.
> +
> + * VHOST_USER_SET_PROTOCOL_FEATURES
> +
> +      Id: 16
> +      Ioctl: VHOST_SET_FEATURES
> +      Master payload: u64
> +
> +      Enable protocol features in the underlying vhost implementation.
> +      Only legal if feature bit VHOST_USER_F_PROTOCOL_FEATURES is present in
> +      VHOST_USER_GET_FEATURES.
> +      Note: slave that reported VHOST_USER_F_PROTOCOL_FEATURES must support
> +      this message even before VHOST_USER_SET_FEATURES was called.
>  
>   * VHOST_USER_SET_OWNER
>  
> -- 
> MST
> 

Re: [Qemu-devel] [PATCH 0/4] vhost-user: protocol updates

[Flavio Leitner]

On Fri, Jul 17, 2015 at 05:09:26PM +0300, Michael S. Tsirkin wrote:
> This patchset sets the stage for extending the vhost user
> protocol, with full backwards compatibility.
> 
> The approach is to negotiate feature bits queried and
> acknowledged during device setup.
> 
> For now, there's no new functionality: two new messages
> are added that will allow negotiating new messages
> required for functionality such as MQ and migration.
> 
> For now, I used the feature bit 30 to signal support for these new messages,
> and we now have 64 more bits to play.
> 
> The patches can be found in my tree, branch vhost-user.
> 
> Only patch 1 is intended for 2.4.
> 
> Posting early so people working on extensions such as
> migration can review this - but please note
> the protocol is not set in stone yet.
> 
> Michael S. Tsirkin (4):
>   Revert "vhost-user: add multi queue support"
>   vhost-user: refactor ioctl translation
>   vhost-user: add protocol feature negotiation
>   vhost-user: unit test for new messages
> 
>  qapi-schema.json          |   6 +-
>  include/hw/virtio/vhost.h |   1 +
>  hw/net/vhost_net.c        |   5 +-
>  hw/virtio/vhost-user.c    | 150 ++++++++++++++++++++++++++++++----------------
>  net/vhost-user.c          |  37 ++++--------
>  tests/vhost-user-test.c   |  19 ++++++
>  docs/specs/vhost-user.txt |  40 +++++++++++--
>  qemu-options.hx           |   5 +-
>  8 files changed, 174 insertions(+), 89 deletions(-)
> 
> -- 
> MST
> 

The libvirt.org seems to be out so I can't check if the first
patch in the series reverting the mq support can be done.

The refactoring looks good.

Regarding to the protocol feature bits, it is nice because gives
flexibility so that master and slave can negotiate a common set
of features.  But it creates a matrix of possibilities that it
might be hard to maintain/test in the long term (64 possible
features and probably the last bit indicating another set of
64 bits, etc..)

Perhaps when this becomes a real problem one option could be that
we move some extensions to have mandatory support in the next 
protocol version. Doing so, would keep the matrix of extensions
under control.

Thanks,
fbl

Re: [Qemu-devel] [PATCH 3/4] vhost-user: add protocol feature negotiation

[Marc-André Lureau]

Hi

On Fri, Jul 17, 2015 at 4:09 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> +#define VHOST_USER_F_PROTOCOL_FEATURES 30
> +#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x0ULL

Since this flag is among the VHOST_GET_FEATURES, shouldn't it be part
of linux-headers/linux/vhost.h ? (next to VHOST_F_LOG_ALL and
VHOST_NET_F_VIRTIO_NET_HDR)

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 4/4] vhost-user: unit test for new messages

[Ouyang, Changchun]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Friday, July 17, 2015 10:10 PM
> To: qemu-devel@nongnu.org
> Cc: marcandre.lureau@gmail.com; haifeng.lin@huawei.com;
> thibaut.collet@6wind.com; Ouyang, Changchun; fbl@redhat.com; Peter
> Maydell; Nikolay Nikolaev; Gonglei; Stefan Hajnoczi
> Subject: [PATCH 4/4] vhost-user: unit test for new messages
> 
> Data is empty for now, but do make sure master sets the new feature bit flag.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  tests/vhost-user-test.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c index
> 75fedf0..228acb6 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -53,6 +53,8 @@
> 
>  #define VHOST_MEMORY_MAX_NREGIONS    8
> 
> +#define VHOST_USER_F_PROTOCOL_FEATURES 30
> +
>  typedef enum VhostUserRequest {
>      VHOST_USER_NONE = 0,
>      VHOST_USER_GET_FEATURES = 1,
> @@ -69,6 +71,8 @@ typedef enum VhostUserRequest {
>      VHOST_USER_SET_VRING_KICK = 12,
>      VHOST_USER_SET_VRING_CALL = 13,
>      VHOST_USER_SET_VRING_ERR = 14,
> +    VHOST_USER_GET_PROTOCOL_FEATURES = 15,
> +    VHOST_USER_SET_PROTOCOL_FEATURES = 16,
>      VHOST_USER_MAX
>  } VhostUserRequest;
> 
> @@ -293,11 +297,26 @@ static void chr_read(void *opaque, const uint8_t
> *buf, int size)
>          /* send back features to qemu */
>          msg.flags |= VHOST_USER_REPLY_MASK;
>          msg.size = sizeof(m.u64);
> +        msg.u64 = 0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> +        p = (uint8_t *) &msg;
> +        qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
> +        break;
> +
> +    case VHOST_USER_SET_FEATURES:
> +	g_assert_cmpint(msg.u64 & (0x1ULL <<
> VHOST_USER_F_PROTOCOL_FEATURES),
> +			!=, 0ULL);
> +        break;
> +
> +    case VHOST_USER_GET_PROTOCOL_FEATURES:

Do we also need add test codes for the case: VHOST_USER_SET_PROTOCOL_FEATURES?

> +        /* send back features to qemu */
> +        msg.flags |= VHOST_USER_REPLY_MASK;
> +        msg.size = sizeof(m.u64);
>          msg.u64 = 0;
>          p = (uint8_t *) &msg;
>          qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
>          break;
> 
> +
>      case VHOST_USER_GET_VRING_BASE:
>          /* send back vring base to qemu */
>          msg.flags |= VHOST_USER_REPLY_MASK;
> --
> MST

Re: [Qemu-devel] [PATCH 0/4] vhost-user: protocol updates

[Thibaut Collet]

On Fri, Jul 17, 2015 at 4:09 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> This patchset sets the stage for extending the vhost user
> protocol, with full backwards compatibility.
>
> The approach is to negotiate feature bits queried and
> acknowledged during device setup.
>
> For now, there's no new functionality: two new messages
> are added that will allow negotiating new messages
> required for functionality such as MQ and migration.
>
> For now, I used the feature bit 30 to signal support for these new messages,
> and we now have 64 more bits to play.
>
> The patches can be found in my tree, branch vhost-user.
>
> Only patch 1 is intended for 2.4.
>
> Posting early so people working on extensions such as
> migration can review this - but please note
> the protocol is not set in stone yet.
>
> Michael S. Tsirkin (4):
>   Revert "vhost-user: add multi queue support"
>   vhost-user: refactor ioctl translation
>   vhost-user: add protocol feature negotiation
>   vhost-user: unit test for new messages
>
>  qapi-schema.json          |   6 +-
>  include/hw/virtio/vhost.h |   1 +
>  hw/net/vhost_net.c        |   5 +-
>  hw/virtio/vhost-user.c    | 150 ++++++++++++++++++++++++++++++----------------
>  net/vhost-user.c          |  37 ++++--------
>  tests/vhost-user-test.c   |  19 ++++++
>  docs/specs/vhost-user.txt |  40 +++++++++++--
>  qemu-options.hx           |   5 +-
>  8 files changed, 174 insertions(+), 89 deletions(-)
>
> --
> MST
>

Great job.
I will reuse the protocol extension to rewrite my patch for live
migration with vhost user and legacy guest that does not support
GUEST_ANNOUNCE.
I will rebase my work on top of this patch and Marc André one about
"add migration log support".
I hope to finalize my patch next week and provide a complete live
migration with vapp as backend.

Thanks.

Thibaut.