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

2015-07-22 Thread Marc-André Lureau 
Hi

On Fri, Jul 17, 2015 at 4:09 PM, Michael S. Tsirkin m...@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

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

2015-07-17 Thread 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 m...@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_NREGIONS8
 
+#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

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

2015-07-17 Thread 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 m...@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_NREGIONS8
  
 +#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