DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/3] net/virtio: add Vhost-user protocol features
@ 2020-05-28  7:46 Maxime Coquelin
  2020-05-28  7:46 ` [dpdk-dev] [PATCH 1/3] net/virtio: add vhost-user protocol features support Maxime Coquelin
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Maxime Coquelin @ 2020-05-28  7:46 UTC (permalink / raw)
  To: dev, amorenoz, xiaolong.ye, shahafs, matan; +Cc: Maxime Coquelin

This series adds Vhost-user protocol features support
to Virtio-user PMD's Vhost-user backend.

The first patch introduces protocol features 
negotiation, and the second one reply-ack feature.

The third patch adds support to the SET_STATUS
request, which is not merged upstream yet. Neither
in DPDK Vhost-user backend nor in Qemu.

So the first two patches can be applied as-is, while
for the last one, we have at least to wait the Qemu bits,
which includes the specification update, is merged
upstream.

Maxime Coquelin (3):
  net/virtio: add vhost-user protocol features support
  net/virtio: add reply-ack support to Virtio-user
  net/virtio: add Virtio status support to Virtio-user

 drivers/net/virtio/virtio_user/vhost.h        | 18 ++++++
 drivers/net/virtio/virtio_user/vhost_user.c   | 35 ++++++++++-
 .../net/virtio/virtio_user/virtio_user_dev.c  | 58 ++++++++++++++++++-
 .../net/virtio/virtio_user/virtio_user_dev.h  |  4 ++
 drivers/net/virtio/virtio_user_ethdev.c       | 20 +++++++
 5 files changed, 130 insertions(+), 5 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH 1/3] net/virtio: add vhost-user protocol features support
  2020-05-28  7:46 [dpdk-dev] [PATCH 0/3] net/virtio: add Vhost-user protocol features Maxime Coquelin
@ 2020-05-28  7:46 ` Maxime Coquelin
  2020-06-17 11:57   ` Xia, Chenbo
  2020-05-28  7:46 ` [dpdk-dev] [PATCH 2/3] net/virtio: add reply-ack support to Virtio-user Maxime Coquelin
  2020-05-28  7:46 ` [dpdk-dev] [PATCH 3/3] net/virtio: add Virtio status " Maxime Coquelin
  2 siblings, 1 reply; 8+ messages in thread
From: Maxime Coquelin @ 2020-05-28  7:46 UTC (permalink / raw)
  To: dev, amorenoz, xiaolong.ye, shahafs, matan; +Cc: Maxime Coquelin

This patch adds support for Vhost-user protocol features.
It is required to support protocol features that were not in
initial Vhost-user specification, such as reply-ack, MTU...

Also, this patch prevents Virtio multiqueue feature negotiation
if the slave does not support MQ protocol feature as stated
in Vhost-user specification:
"The multiple queues feature is supported only when the protocol
feature ``VHOST_USER_PROTOCOL_F_MQ`` (bit 0) is set."

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_user/vhost.h        |  9 +++++
 drivers/net/virtio/virtio_user/vhost_user.c   |  3 ++
 .../net/virtio/virtio_user/virtio_user_dev.c  | 39 ++++++++++++++++++-
 .../net/virtio/virtio_user/virtio_user_dev.h  |  3 ++
 drivers/net/virtio/virtio_user_ethdev.c       | 19 +++++++++
 5 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio/virtio_user/vhost.h b/drivers/net/virtio/virtio_user/vhost.h
index 1e784e58ef..9ace1a90c3 100644
--- a/drivers/net/virtio/virtio_user/vhost.h
+++ b/drivers/net/virtio/virtio_user/vhost.h
@@ -44,6 +44,15 @@ struct vhost_vring_addr {
 	uint64_t log_guest_addr;
 };
 
+#ifndef VHOST_USER_F_PROTOCOL_FEATURES
+#define VHOST_USER_F_PROTOCOL_FEATURES 30
+#endif
+
+/** Protocol features. */
+#ifndef VHOST_USER_PROTOCOL_F_MQ
+#define VHOST_USER_PROTOCOL_F_MQ	0
+#endif
+
 enum vhost_user_request {
 	VHOST_USER_NONE = 0,
 	VHOST_USER_GET_FEATURES = 1,
diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
index 74b82e56e4..b687665042 100644
--- a/drivers/net/virtio/virtio_user/vhost_user.c
+++ b/drivers/net/virtio/virtio_user/vhost_user.c
@@ -269,10 +269,12 @@ vhost_user_sock(struct virtio_user_dev *dev,
 
 	switch (req) {
 	case VHOST_USER_GET_FEATURES:
+	case VHOST_USER_GET_PROTOCOL_FEATURES:
 		need_reply = 1;
 		break;
 
 	case VHOST_USER_SET_FEATURES:
+	case VHOST_USER_SET_PROTOCOL_FEATURES:
 	case VHOST_USER_SET_LOG_BASE:
 		msg.payload.u64 = *((__u64 *)arg);
 		msg.size = sizeof(m.payload.u64);
@@ -351,6 +353,7 @@ vhost_user_sock(struct virtio_user_dev *dev,
 
 		switch (req) {
 		case VHOST_USER_GET_FEATURES:
+		case VHOST_USER_GET_PROTOCOL_FEATURES:
 			if (msg.size != sizeof(m.payload.u64)) {
 				PMD_DRV_LOG(ERR, "Received bad msg size");
 				return -1;
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 7fb135f49a..3afb09df2d 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -151,8 +151,10 @@ virtio_user_start_device(struct virtio_user_dev *dev)
 	if (virtio_user_queue_setup(dev, virtio_user_create_queue) < 0)
 		goto error;
 
-	/* Step 1: set features */
+	/* Step 1: negotiate protocol features & set features */
 	features = dev->features;
+
+
 	/* Strip VIRTIO_NET_F_MAC, as MAC address is handled in vdev init */
 	features &= ~(1ull << VIRTIO_NET_F_MAC);
 	/* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to know */
@@ -417,13 +419,19 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
 	 1ULL << VIRTIO_NET_F_GUEST_TSO6	|	\
 	 1ULL << VIRTIO_F_IN_ORDER		|	\
 	 1ULL << VIRTIO_F_VERSION_1		|	\
-	 1ULL << VIRTIO_F_RING_PACKED)
+	 1ULL << VIRTIO_F_RING_PACKED		|	\
+	 1ULL << VHOST_USER_F_PROTOCOL_FEATURES)
+
+#define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES		\
+	(1ULL << VHOST_USER_PROTOCOL_F_MQ)
 
 int
 virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 		     int cq, int queue_size, const char *mac, char **ifname,
 		     int server, int mrg_rxbuf, int in_order, int packed_vq)
 {
+	uint64_t protocol_features = 0;
+
 	pthread_mutex_init(&dev->mutex, NULL);
 	strlcpy(dev->path, path, PATH_MAX);
 	dev->started = 0;
@@ -434,6 +442,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 	dev->mac_specified = 0;
 	dev->frontend_features = 0;
 	dev->unsupported_features = ~VIRTIO_USER_SUPPORTED_FEATURES;
+	dev->protocol_features = VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES;
 	parse_mac(dev, mac);
 
 	if (*ifname) {
@@ -446,6 +455,10 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 		return -1;
 	}
 
+	if (!is_vhost_user_by_type(dev->path))
+		dev->unsupported_features |=
+			(1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
+
 	if (!dev->is_server) {
 		if (dev->ops->send_request(dev, VHOST_USER_SET_OWNER,
 					   NULL) < 0) {
@@ -460,6 +473,26 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 				     strerror(errno));
 			return -1;
 		}
+
+
+		if (dev->device_features &
+				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) {
+			if (dev->ops->send_request(dev,
+					VHOST_USER_GET_PROTOCOL_FEATURES,
+					&protocol_features))
+				return -1;
+		}
+
+		dev->protocol_features &= protocol_features;
+
+		if (dev->ops->send_request(dev,
+					VHOST_USER_SET_PROTOCOL_FEATURES,
+					&dev->protocol_features))
+			return -1;
+
+		if (!(dev->protocol_features &
+					(1ULL << VHOST_USER_PROTOCOL_F_MQ)))
+			dev->unsupported_features |= (1ull << VIRTIO_NET_F_MQ);
 	} else {
 		/* We just pretend vhost-user can support all these features.
 		 * Note that this could be problematic that if some feature is
@@ -469,6 +502,8 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 		dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES;
 	}
 
+
+
 	if (!mrg_rxbuf)
 		dev->unsupported_features |= (1ull << VIRTIO_NET_F_MRG_RXBUF);
 
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
index 3b6b6065a5..56e638f8a6 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -40,6 +40,9 @@ struct virtio_user_dev {
 	uint64_t	device_features; /* supported features by device */
 	uint64_t	frontend_features; /* enabled frontend features */
 	uint64_t	unsupported_features; /* unsupported features mask */
+	uint64_t	protocol_features; /* negotiated protocol features
+					    * (Vhost-user only)
+					    */
 	uint8_t		status;
 	uint16_t	port_id;
 	uint8_t		mac_addr[RTE_ETHER_ADDR_LEN];
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 798f191c32..ccb5a18e25 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -68,6 +68,7 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
 	int connectfd;
 	struct rte_eth_dev *eth_dev = &rte_eth_devices[dev->port_id];
 	struct virtio_hw *hw = eth_dev->data->dev_private;
+	uint64_t protocol_features;
 
 	connectfd = accept(dev->listenfd, NULL, NULL);
 	if (connectfd < 0)
@@ -81,6 +82,24 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
 		return -1;
 	}
 
+	if (dev->device_features &
+			(1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) {
+		if (dev->ops->send_request(dev,
+					VHOST_USER_GET_PROTOCOL_FEATURES,
+					&protocol_features))
+			return -1;
+
+		dev->protocol_features &= protocol_features;
+
+		if (dev->ops->send_request(dev,
+					VHOST_USER_SET_PROTOCOL_FEATURES,
+					&dev->protocol_features))
+			return -1;
+	}
+
+	if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ)))
+		dev->unsupported_features |= (1ull << VIRTIO_NET_F_MQ);
+
 	dev->device_features |= dev->frontend_features;
 
 	/* umask vhost-user unsupported features */
-- 
2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH 2/3] net/virtio: add reply-ack support to Virtio-user
  2020-05-28  7:46 [dpdk-dev] [PATCH 0/3] net/virtio: add Vhost-user protocol features Maxime Coquelin
  2020-05-28  7:46 ` [dpdk-dev] [PATCH 1/3] net/virtio: add vhost-user protocol features support Maxime Coquelin
@ 2020-05-28  7:46 ` Maxime Coquelin
  2020-05-28  7:46 ` [dpdk-dev] [PATCH 3/3] net/virtio: add Virtio status " Maxime Coquelin
  2 siblings, 0 replies; 8+ messages in thread
From: Maxime Coquelin @ 2020-05-28  7:46 UTC (permalink / raw)
  To: dev, amorenoz, xiaolong.ye, shahafs, matan; +Cc: Maxime Coquelin

This patch adds support reply-ack vhost-user protocol
feature, which is for now only used to ensure
VHOST_USER_SET_MEM_TABLE requests are handled by the
slave, but later will be used for VHOST_USER_SET_STATUS.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_user/vhost.h        |  6 ++++-
 drivers/net/virtio/virtio_user/vhost_user.c   | 24 ++++++++++++++++---
 .../net/virtio/virtio_user/virtio_user_dev.c  |  3 ++-
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/net/virtio/virtio_user/vhost.h b/drivers/net/virtio/virtio_user/vhost.h
index 9ace1a90c3..260e1c3081 100644
--- a/drivers/net/virtio/virtio_user/vhost.h
+++ b/drivers/net/virtio/virtio_user/vhost.h
@@ -50,7 +50,11 @@ struct vhost_vring_addr {
 
 /** Protocol features. */
 #ifndef VHOST_USER_PROTOCOL_F_MQ
-#define VHOST_USER_PROTOCOL_F_MQ	0
+#define VHOST_USER_PROTOCOL_F_MQ 0
+#endif
+
+#ifndef VHOST_USER_PROTOCOL_F_REPLY_ACK
+#define VHOST_USER_PROTOCOL_F_REPLY_ACK 3
 #endif
 
 enum vhost_user_request {
diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
index b687665042..f8d751c98e 100644
--- a/drivers/net/virtio/virtio_user/vhost_user.c
+++ b/drivers/net/virtio/virtio_user/vhost_user.c
@@ -32,6 +32,7 @@ struct vhost_user_msg {
 
 #define VHOST_USER_VERSION_MASK     0x3
 #define VHOST_USER_REPLY_MASK       (0x1 << 2)
+#define VHOST_USER_NEED_REPLY_MASK  (0x1 << 3)
 	uint32_t flags;
 	uint32_t size; /* the following payload size */
 	union {
@@ -251,6 +252,7 @@ vhost_user_sock(struct virtio_user_dev *dev,
 	struct vhost_user_msg msg;
 	struct vhost_vring_file *file = 0;
 	int need_reply = 0;
+	int has_reply_ack;
 	int fds[VHOST_MEMORY_MAX_NREGIONS];
 	int fd_num = 0;
 	int len;
@@ -263,6 +265,9 @@ vhost_user_sock(struct virtio_user_dev *dev,
 	if (dev->is_server && vhostfd < 0)
 		return -1;
 
+	if (dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK))
+		has_reply_ack = 1;
+
 	msg.request = req;
 	msg.flags = VHOST_USER_VERSION;
 	msg.size = 0;
@@ -291,6 +296,9 @@ vhost_user_sock(struct virtio_user_dev *dev,
 		msg.size = sizeof(m.payload.memory.nregions);
 		msg.size += sizeof(m.payload.memory.padding);
 		msg.size += fd_num * sizeof(struct vhost_memory_region);
+
+		if (has_reply_ack)
+			msg.flags |= VHOST_USER_NEED_REPLY_MASK;
 		break;
 
 	case VHOST_USER_SET_LOG_FD:
@@ -339,7 +347,7 @@ vhost_user_sock(struct virtio_user_dev *dev,
 		return -1;
 	}
 
-	if (need_reply) {
+	if (need_reply || msg.flags & VHOST_USER_NEED_REPLY_MASK) {
 		if (vhost_user_read(vhostfd, &msg) < 0) {
 			PMD_DRV_LOG(ERR, "Received msg failed: %s",
 				    strerror(errno));
@@ -369,8 +377,18 @@ vhost_user_sock(struct virtio_user_dev *dev,
 			       sizeof(struct vhost_vring_state));
 			break;
 		default:
-			PMD_DRV_LOG(ERR, "Received unexpected msg type");
-			return -1;
+			/* Reply-ack handling */
+			if (msg.size != sizeof(m.payload.u64)) {
+				PMD_DRV_LOG(ERR, "Received bad msg size");
+				return -1;
+			}
+
+			if (msg.payload.u64 != 0) {
+				PMD_DRV_LOG(ERR, "Slave replied NACK");
+				return -1;
+			}
+
+			break;
 		}
 	}
 
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 3afb09df2d..ea22af5dc4 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -423,7 +423,8 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
 	 1ULL << VHOST_USER_F_PROTOCOL_FEATURES)
 
 #define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES		\
-	(1ULL << VHOST_USER_PROTOCOL_F_MQ)
+	(1ULL << VHOST_USER_PROTOCOL_F_MQ |		\
+	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK)
 
 int
 virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
-- 
2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH 3/3] net/virtio: add Virtio status support to Virtio-user
  2020-05-28  7:46 [dpdk-dev] [PATCH 0/3] net/virtio: add Vhost-user protocol features Maxime Coquelin
  2020-05-28  7:46 ` [dpdk-dev] [PATCH 1/3] net/virtio: add vhost-user protocol features support Maxime Coquelin
  2020-05-28  7:46 ` [dpdk-dev] [PATCH 2/3] net/virtio: add reply-ack support to Virtio-user Maxime Coquelin
@ 2020-05-28  7:46 ` Maxime Coquelin
  2 siblings, 0 replies; 8+ messages in thread
From: Maxime Coquelin @ 2020-05-28  7:46 UTC (permalink / raw)
  To: dev, amorenoz, xiaolong.ye, shahafs, matan; +Cc: Maxime Coquelin

This patch adds support for VHOST_USER_SET_STATUS
request. It is used to make the backend aware of
Virtio devices status update.

It is useful for the backend to know when the Virtio
driver is done with the Virtio device configuration.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_user/vhost.h        |  5 +++++
 drivers/net/virtio/virtio_user/vhost_user.c   |  8 ++++++++
 .../net/virtio/virtio_user/virtio_user_dev.c  | 20 ++++++++++++++++++-
 .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
 drivers/net/virtio/virtio_user_ethdev.c       |  1 +
 5 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_user/vhost.h b/drivers/net/virtio/virtio_user/vhost.h
index 260e1c3081..fc360564eb 100644
--- a/drivers/net/virtio/virtio_user/vhost.h
+++ b/drivers/net/virtio/virtio_user/vhost.h
@@ -57,6 +57,10 @@ struct vhost_vring_addr {
 #define VHOST_USER_PROTOCOL_F_REPLY_ACK 3
 #endif
 
+#ifndef VHOST_USER_PROTOCOL_F_STATUS
+#define VHOST_USER_PROTOCOL_F_STATUS 15
+#endif
+
 enum vhost_user_request {
 	VHOST_USER_NONE = 0,
 	VHOST_USER_GET_FEATURES = 1,
@@ -77,6 +81,7 @@ enum vhost_user_request {
 	VHOST_USER_SET_PROTOCOL_FEATURES = 16,
 	VHOST_USER_GET_QUEUE_NUM = 17,
 	VHOST_USER_SET_VRING_ENABLE = 18,
+	VHOST_USER_SET_STATUS = 36,
 	VHOST_USER_MAX
 };
 
diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
index f8d751c98e..68232d75d7 100644
--- a/drivers/net/virtio/virtio_user/vhost_user.c
+++ b/drivers/net/virtio/virtio_user/vhost_user.c
@@ -278,6 +278,14 @@ vhost_user_sock(struct virtio_user_dev *dev,
 		need_reply = 1;
 		break;
 
+	case VHOST_USER_SET_STATUS:
+		if (!(dev->protocol_features &
+				(1ULL << VHOST_USER_PROTOCOL_F_STATUS)))
+			return 0;
+
+		if (has_reply_ack)
+			msg.flags |= VHOST_USER_NEED_REPLY_MASK;
+		/* Fallthrough */
 	case VHOST_USER_SET_FEATURES:
 	case VHOST_USER_SET_PROTOCOL_FEATURES:
 	case VHOST_USER_SET_LOG_BASE:
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index ea22af5dc4..cf29ea0b88 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -424,7 +424,8 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
 
 #define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES		\
 	(1ULL << VHOST_USER_PROTOCOL_F_MQ |		\
-	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK)
+	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK |	\
+	 1ULL << VHOST_USER_PROTOCOL_F_STATUS)
 
 int
 virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
@@ -782,3 +783,20 @@ virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t queue_idx)
 		__atomic_add_fetch(&vring->used->idx, 1, __ATOMIC_RELAXED);
 	}
 }
+
+int
+virtio_user_update_status(struct virtio_user_dev *dev, uint8_t status)
+{
+	int ret;
+	uint64_t arg = status;
+
+	/* Vhost-user only for now */
+	if (!is_vhost_user_by_type(dev->path))
+		return 0;
+
+	ret = dev->ops->send_request(dev, VHOST_USER_SET_STATUS, &arg);
+	if (ret)
+		return -1;
+
+	return 0;
+}
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
index 56e638f8a6..c49a896a88 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -71,4 +71,5 @@ void virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t queue_idx);
 void virtio_user_handle_cq_packed(struct virtio_user_dev *dev,
 				  uint16_t queue_idx);
 uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t q_pairs);
+int virtio_user_update_status(struct virtio_user_dev *dev, uint8_t status);
 #endif
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index ccb5a18e25..bed161ba07 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -271,6 +271,7 @@ virtio_user_set_status(struct virtio_hw *hw, uint8_t status)
 	else if (status == VIRTIO_CONFIG_STATUS_RESET)
 		virtio_user_reset(hw);
 	dev->status = status;
+	virtio_user_update_status(dev, status);
 }
 
 static uint8_t
-- 
2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3] net/virtio: add vhost-user protocol features support
  2020-05-28  7:46 ` [dpdk-dev] [PATCH 1/3] net/virtio: add vhost-user protocol features support Maxime Coquelin
@ 2020-06-17 11:57   ` Xia, Chenbo
  2020-06-17 12:22     ` Maxime Coquelin
  0 siblings, 1 reply; 8+ messages in thread
From: Xia, Chenbo @ 2020-06-17 11:57 UTC (permalink / raw)
  To: Maxime Coquelin, dev, amorenoz, Ye, Xiaolong, shahafs, matan

Hi Maxime,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Maxime Coquelin
> Sent: Thursday, May 28, 2020 3:46 PM
> To: dev@dpdk.org; amorenoz@redhat.com; Ye, Xiaolong
> <xiaolong.ye@intel.com>; shahafs@mellanox.com; matan@mellanox.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [dpdk-dev] [PATCH 1/3] net/virtio: add vhost-user protocol features
> support
> 
> This patch adds support for Vhost-user protocol features.
> It is required to support protocol features that were not in initial Vhost-user
> specification, such as reply-ack, MTU...
> 
> Also, this patch prevents Virtio multiqueue feature negotiation if the slave does
> not support MQ protocol feature as stated in Vhost-user specification:
> "The multiple queues feature is supported only when the protocol feature
> ``VHOST_USER_PROTOCOL_F_MQ`` (bit 0) is set."
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/virtio/virtio_user/vhost.h        |  9 +++++
>  drivers/net/virtio/virtio_user/vhost_user.c   |  3 ++
>  .../net/virtio/virtio_user/virtio_user_dev.c  | 39 ++++++++++++++++++-
>   .../net/virtio/virtio_user/virtio_user_dev.h  |  3 ++
>  drivers/net/virtio/virtio_user_ethdev.c       | 19 +++++++++
>  5 files changed, 71 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/vhost.h
> b/drivers/net/virtio/virtio_user/vhost.h
> index 1e784e58ef..9ace1a90c3 100644
> --- a/drivers/net/virtio/virtio_user/vhost.h
> +++ b/drivers/net/virtio/virtio_user/vhost.h
> @@ -44,6 +44,15 @@ struct vhost_vring_addr {
>  	uint64_t log_guest_addr;
>  };
> 
> +#ifndef VHOST_USER_F_PROTOCOL_FEATURES
> +#define VHOST_USER_F_PROTOCOL_FEATURES 30 #endif
> +
> +/** Protocol features. */
> +#ifndef VHOST_USER_PROTOCOL_F_MQ
> +#define VHOST_USER_PROTOCOL_F_MQ	0
> +#endif
> +
>  enum vhost_user_request {
>  	VHOST_USER_NONE = 0,
>  	VHOST_USER_GET_FEATURES = 1,
> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c
> b/drivers/net/virtio/virtio_user/vhost_user.c
> index 74b82e56e4..b687665042 100644
> --- a/drivers/net/virtio/virtio_user/vhost_user.c
> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
> @@ -269,10 +269,12 @@ vhost_user_sock(struct virtio_user_dev *dev,
> 
>  	switch (req) {
>  	case VHOST_USER_GET_FEATURES:
> +	case VHOST_USER_GET_PROTOCOL_FEATURES:
>  		need_reply = 1;
>  		break;
> 
>  	case VHOST_USER_SET_FEATURES:
> +	case VHOST_USER_SET_PROTOCOL_FEATURES:
>  	case VHOST_USER_SET_LOG_BASE:
>  		msg.payload.u64 = *((__u64 *)arg);
>  		msg.size = sizeof(m.payload.u64);
> @@ -351,6 +353,7 @@ vhost_user_sock(struct virtio_user_dev *dev,
> 
>  		switch (req) {
>  		case VHOST_USER_GET_FEATURES:
> +		case VHOST_USER_GET_PROTOCOL_FEATURES:
>  			if (msg.size != sizeof(m.payload.u64)) {
>  				PMD_DRV_LOG(ERR, "Received bad msg size");
>  				return -1;
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index 7fb135f49a..3afb09df2d 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -151,8 +151,10 @@ virtio_user_start_device(struct virtio_user_dev *dev)
>  	if (virtio_user_queue_setup(dev, virtio_user_create_queue) < 0)
>  		goto error;
> 
> -	/* Step 1: set features */
> +	/* Step 1: negotiate protocol features & set features */
>  	features = dev->features;
> +
> +
>  	/* Strip VIRTIO_NET_F_MAC, as MAC address is handled in vdev init */
>  	features &= ~(1ull << VIRTIO_NET_F_MAC);
>  	/* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to know
> */ @@ -417,13 +419,19 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
>  	 1ULL << VIRTIO_NET_F_GUEST_TSO6	|	\
>  	 1ULL << VIRTIO_F_IN_ORDER		|	\
>  	 1ULL << VIRTIO_F_VERSION_1		|	\
> -	 1ULL << VIRTIO_F_RING_PACKED)
> +	 1ULL << VIRTIO_F_RING_PACKED		|	\
> +	 1ULL << VHOST_USER_F_PROTOCOL_FEATURES)
> +
> +#define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES		\
> +	(1ULL << VHOST_USER_PROTOCOL_F_MQ)
> 
>  int
>  virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
>  		     int cq, int queue_size, const char *mac, char **ifname,
>  		     int server, int mrg_rxbuf, int in_order, int packed_vq)  {
> +	uint64_t protocol_features = 0;
> +
>  	pthread_mutex_init(&dev->mutex, NULL);
>  	strlcpy(dev->path, path, PATH_MAX);
>  	dev->started = 0;
> @@ -434,6 +442,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
> *path, int queues,
>  	dev->mac_specified = 0;
>  	dev->frontend_features = 0;
>  	dev->unsupported_features = ~VIRTIO_USER_SUPPORTED_FEATURES;
> +	dev->protocol_features =
> VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES;
>  	parse_mac(dev, mac);
> 
>  	if (*ifname) {
> @@ -446,6 +455,10 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
> *path, int queues,
>  		return -1;
>  	}
> 
> +	if (!is_vhost_user_by_type(dev->path))
> +		dev->unsupported_features |=
> +			(1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
> +
>  	if (!dev->is_server) {
>  		if (dev->ops->send_request(dev, VHOST_USER_SET_OWNER,
>  					   NULL) < 0) {
> @@ -460,6 +473,26 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
> *path, int queues,
>  				     strerror(errno));
>  			return -1;
>  		}
> +
> +
> +		if (dev->device_features &
> +				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) {
> +			if (dev->ops->send_request(dev,
> +					VHOST_USER_GET_PROTOCOL_FEATURES,
> +					&protocol_features))
> +				return -1;
> +		}

Should we put '}' after sending VHOST_USER_SET_PROTOCOL_FEATURES like in virtio_user_server_reconnect?
 
> +
> +		dev->protocol_features &= protocol_features;
> +
> +		if (dev->ops->send_request(dev,
> +					VHOST_USER_SET_PROTOCOL_FEATURES,
> +					&dev->protocol_features))
> +			return -1;
> +
> +		if (!(dev->protocol_features &
> +					(1ULL <<
> VHOST_USER_PROTOCOL_F_MQ)))
> +			dev->unsupported_features |= (1ull <<
> VIRTIO_NET_F_MQ);
>  	} else {
>  		/* We just pretend vhost-user can support all these features.
>  		 * Note that this could be problematic that if some feature is
> @@ -469,6 +502,8 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
> *path, int queues,
>  		dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES;
>  	}
> 
> +
> +
>  	if (!mrg_rxbuf)
>  		dev->unsupported_features |= (1ull <<
> VIRTIO_NET_F_MRG_RXBUF);
> 
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> index 3b6b6065a5..56e638f8a6 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> @@ -40,6 +40,9 @@ struct virtio_user_dev {
>  	uint64_t	device_features; /* supported features by device */
>  	uint64_t	frontend_features; /* enabled frontend features */
>  	uint64_t	unsupported_features; /* unsupported features mask
> */
> +	uint64_t	protocol_features; /* negotiated protocol features
> +					    * (Vhost-user only)
> +					    */
>  	uint8_t		status;
>  	uint16_t	port_id;
>  	uint8_t		mac_addr[RTE_ETHER_ADDR_LEN];
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index 798f191c32..ccb5a18e25 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -68,6 +68,7 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>  	int connectfd;
>  	struct rte_eth_dev *eth_dev = &rte_eth_devices[dev->port_id];
>  	struct virtio_hw *hw = eth_dev->data->dev_private;
> +	uint64_t protocol_features;
> 
>  	connectfd = accept(dev->listenfd, NULL, NULL);
>  	if (connectfd < 0)
> @@ -81,6 +82,24 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>  		return -1;
>  	}
> 
> +	if (dev->device_features &
> +			(1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) {
> +		if (dev->ops->send_request(dev,
> +
> 	VHOST_USER_GET_PROTOCOL_FEATURES,
> +					&protocol_features))
> +			return -1;
> +
> +		dev->protocol_features &= protocol_features;
> +
> +		if (dev->ops->send_request(dev,
> +
> 	VHOST_USER_SET_PROTOCOL_FEATURES,
> +					&dev->protocol_features))
> +			return -1;
> +	}
> +
> +	if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ)))
> +		dev->unsupported_features |= (1ull << VIRTIO_NET_F_MQ);

Should we consider the case that vhost-user does not support VHOST_USER_F_PROTOCOL_FEATURES but support
VIRTIO_NET_F_MQ? Because if the device negotiated feature does not include that, we should not use above logic to decide
whether MQ is supported. If the case should be considered, the above two lines should be moved into last '{}' and
same thing should be done in virtio_user_dev_init.

Thanks!
Chenbo

> +
>  	dev->device_features |= dev->frontend_features;
> 
>  	/* umask vhost-user unsupported features */
> --
> 2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3] net/virtio: add vhost-user protocol features support
  2020-06-17 11:57   ` Xia, Chenbo
@ 2020-06-17 12:22     ` Maxime Coquelin
  0 siblings, 0 replies; 8+ messages in thread
From: Maxime Coquelin @ 2020-06-17 12:22 UTC (permalink / raw)
  To: Xia, Chenbo, dev, amorenoz, Ye, Xiaolong, shahafs, matan



On 6/17/20 1:57 PM, Xia, Chenbo wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Maxime Coquelin
>> Sent: Thursday, May 28, 2020 3:46 PM
>> To: dev@dpdk.org; amorenoz@redhat.com; Ye, Xiaolong
>> <xiaolong.ye@intel.com>; shahafs@mellanox.com; matan@mellanox.com
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: [dpdk-dev] [PATCH 1/3] net/virtio: add vhost-user protocol features
>> support
>>
>> This patch adds support for Vhost-user protocol features.
>> It is required to support protocol features that were not in initial Vhost-user
>> specification, such as reply-ack, MTU...
>>
>> Also, this patch prevents Virtio multiqueue feature negotiation if the slave does
>> not support MQ protocol feature as stated in Vhost-user specification:
>> "The multiple queues feature is supported only when the protocol feature
>> ``VHOST_USER_PROTOCOL_F_MQ`` (bit 0) is set."
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  drivers/net/virtio/virtio_user/vhost.h        |  9 +++++
>>  drivers/net/virtio/virtio_user/vhost_user.c   |  3 ++
>>  .../net/virtio/virtio_user/virtio_user_dev.c  | 39 ++++++++++++++++++-
>>   .../net/virtio/virtio_user/virtio_user_dev.h  |  3 ++
>>  drivers/net/virtio/virtio_user_ethdev.c       | 19 +++++++++
>>  5 files changed, 71 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_user/vhost.h
>> b/drivers/net/virtio/virtio_user/vhost.h
>> index 1e784e58ef..9ace1a90c3 100644
>> --- a/drivers/net/virtio/virtio_user/vhost.h
>> +++ b/drivers/net/virtio/virtio_user/vhost.h
>> @@ -44,6 +44,15 @@ struct vhost_vring_addr {
>>  	uint64_t log_guest_addr;
>>  };
>>
>> +#ifndef VHOST_USER_F_PROTOCOL_FEATURES
>> +#define VHOST_USER_F_PROTOCOL_FEATURES 30 #endif
>> +
>> +/** Protocol features. */
>> +#ifndef VHOST_USER_PROTOCOL_F_MQ
>> +#define VHOST_USER_PROTOCOL_F_MQ	0
>> +#endif
>> +
>>  enum vhost_user_request {
>>  	VHOST_USER_NONE = 0,
>>  	VHOST_USER_GET_FEATURES = 1,
>> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c
>> b/drivers/net/virtio/virtio_user/vhost_user.c
>> index 74b82e56e4..b687665042 100644
>> --- a/drivers/net/virtio/virtio_user/vhost_user.c
>> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
>> @@ -269,10 +269,12 @@ vhost_user_sock(struct virtio_user_dev *dev,
>>
>>  	switch (req) {
>>  	case VHOST_USER_GET_FEATURES:
>> +	case VHOST_USER_GET_PROTOCOL_FEATURES:
>>  		need_reply = 1;
>>  		break;
>>
>>  	case VHOST_USER_SET_FEATURES:
>> +	case VHOST_USER_SET_PROTOCOL_FEATURES:
>>  	case VHOST_USER_SET_LOG_BASE:
>>  		msg.payload.u64 = *((__u64 *)arg);
>>  		msg.size = sizeof(m.payload.u64);
>> @@ -351,6 +353,7 @@ vhost_user_sock(struct virtio_user_dev *dev,
>>
>>  		switch (req) {
>>  		case VHOST_USER_GET_FEATURES:
>> +		case VHOST_USER_GET_PROTOCOL_FEATURES:
>>  			if (msg.size != sizeof(m.payload.u64)) {
>>  				PMD_DRV_LOG(ERR, "Received bad msg size");
>>  				return -1;
>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> index 7fb135f49a..3afb09df2d 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> @@ -151,8 +151,10 @@ virtio_user_start_device(struct virtio_user_dev *dev)
>>  	if (virtio_user_queue_setup(dev, virtio_user_create_queue) < 0)
>>  		goto error;
>>
>> -	/* Step 1: set features */
>> +	/* Step 1: negotiate protocol features & set features */
>>  	features = dev->features;
>> +
>> +
>>  	/* Strip VIRTIO_NET_F_MAC, as MAC address is handled in vdev init */
>>  	features &= ~(1ull << VIRTIO_NET_F_MAC);
>>  	/* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to know
>> */ @@ -417,13 +419,19 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
>>  	 1ULL << VIRTIO_NET_F_GUEST_TSO6	|	\
>>  	 1ULL << VIRTIO_F_IN_ORDER		|	\
>>  	 1ULL << VIRTIO_F_VERSION_1		|	\
>> -	 1ULL << VIRTIO_F_RING_PACKED)
>> +	 1ULL << VIRTIO_F_RING_PACKED		|	\
>> +	 1ULL << VHOST_USER_F_PROTOCOL_FEATURES)
>> +
>> +#define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES		\
>> +	(1ULL << VHOST_USER_PROTOCOL_F_MQ)
>>
>>  int
>>  virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
>>  		     int cq, int queue_size, const char *mac, char **ifname,
>>  		     int server, int mrg_rxbuf, int in_order, int packed_vq)  {
>> +	uint64_t protocol_features = 0;
>> +
>>  	pthread_mutex_init(&dev->mutex, NULL);
>>  	strlcpy(dev->path, path, PATH_MAX);
>>  	dev->started = 0;
>> @@ -434,6 +442,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
>> *path, int queues,
>>  	dev->mac_specified = 0;
>>  	dev->frontend_features = 0;
>>  	dev->unsupported_features = ~VIRTIO_USER_SUPPORTED_FEATURES;
>> +	dev->protocol_features =
>> VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES;
>>  	parse_mac(dev, mac);
>>
>>  	if (*ifname) {
>> @@ -446,6 +455,10 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
>> *path, int queues,
>>  		return -1;
>>  	}
>>
>> +	if (!is_vhost_user_by_type(dev->path))
>> +		dev->unsupported_features |=
>> +			(1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
>> +
>>  	if (!dev->is_server) {
>>  		if (dev->ops->send_request(dev, VHOST_USER_SET_OWNER,
>>  					   NULL) < 0) {
>> @@ -460,6 +473,26 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
>> *path, int queues,
>>  				     strerror(errno));
>>  			return -1;
>>  		}
>> +
>> +
>> +		if (dev->device_features &
>> +				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) {
>> +			if (dev->ops->send_request(dev,
>> +					VHOST_USER_GET_PROTOCOL_FEATURES,
>> +					&protocol_features))
>> +				return -1;
>> +		}
> 
> Should we put '}' after sending VHOST_USER_SET_PROTOCOL_FEATURES like in virtio_user_server_reconnect?

Good catch, we indeed don't want to send a Vhost-user request if it is
not supported by the Vhost-user backend.

>> +
>> +		dev->protocol_features &= protocol_features;
>> +
>> +		if (dev->ops->send_request(dev,
>> +					VHOST_USER_SET_PROTOCOL_FEATURES,
>> +					&dev->protocol_features))
>> +			return -1;
>> +
>> +		if (!(dev->protocol_features &
>> +					(1ULL <<
>> VHOST_USER_PROTOCOL_F_MQ)))
>> +			dev->unsupported_features |= (1ull <<
>> VIRTIO_NET_F_MQ);
>>  	} else {
>>  		/* We just pretend vhost-user can support all these features.
>>  		 * Note that this could be problematic that if some feature is
>> @@ -469,6 +502,8 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
>> *path, int queues,
>>  		dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES;
>>  	}
>>
>> +
>> +
>>  	if (!mrg_rxbuf)
>>  		dev->unsupported_features |= (1ull <<
>> VIRTIO_NET_F_MRG_RXBUF);
>>
>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> index 3b6b6065a5..56e638f8a6 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> @@ -40,6 +40,9 @@ struct virtio_user_dev {
>>  	uint64_t	device_features; /* supported features by device */
>>  	uint64_t	frontend_features; /* enabled frontend features */
>>  	uint64_t	unsupported_features; /* unsupported features mask
>> */
>> +	uint64_t	protocol_features; /* negotiated protocol features
>> +					    * (Vhost-user only)
>> +					    */
>>  	uint8_t		status;
>>  	uint16_t	port_id;
>>  	uint8_t		mac_addr[RTE_ETHER_ADDR_LEN];
>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>> b/drivers/net/virtio/virtio_user_ethdev.c
>> index 798f191c32..ccb5a18e25 100644
>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>> @@ -68,6 +68,7 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>>  	int connectfd;
>>  	struct rte_eth_dev *eth_dev = &rte_eth_devices[dev->port_id];
>>  	struct virtio_hw *hw = eth_dev->data->dev_private;
>> +	uint64_t protocol_features;
>>
>>  	connectfd = accept(dev->listenfd, NULL, NULL);
>>  	if (connectfd < 0)
>> @@ -81,6 +82,24 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>>  		return -1;
>>  	}
>>
>> +	if (dev->device_features &
>> +			(1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) {
>> +		if (dev->ops->send_request(dev,
>> +
>> 	VHOST_USER_GET_PROTOCOL_FEATURES,
>> +					&protocol_features))
>> +			return -1;
>> +
>> +		dev->protocol_features &= protocol_features;
>> +
>> +		if (dev->ops->send_request(dev,
>> +
>> 	VHOST_USER_SET_PROTOCOL_FEATURES,
>> +					&dev->protocol_features))
>> +			return -1;
>> +	}
>> +
>> +	if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ)))
>> +		dev->unsupported_features |= (1ull << VIRTIO_NET_F_MQ);
> 
> Should we consider the case that vhost-user does not support VHOST_USER_F_PROTOCOL_FEATURES but support
> VIRTIO_NET_F_MQ? Because if the device negotiated feature does not include that, we should not use above logic to decide
> whether MQ is supported. If the case should be considered, the above two lines should be moved into last '{}' and
> same thing should be done in virtio_user_dev_init.

The Vhost-user specification says:
"
Masters must not rely on the ``VHOST_USER_PROTOCOL_F_MQ`` protocol
feature for
devices with a fixed number of virtqueues.  Only true multiqueue devices
require this protocol feature.
"

Virtio-net device being a true multiqueue device it should require this.
But for now Virtio-user PMD does not use VHOST_USER_GET_QUEUE_NUM, so
it might not be mandatory.

I can drop it for now, but we should consider adding support for sending
VHOST_USER_GET_QUEUE_NUM, as the container may want to know how many
queues are supported by the Vhost-user backend or the vDPA device.

Thanks!
Maxime

> Thanks!
> Chenbo
> 
>> +
>>  	dev->device_features |= dev->frontend_features;
>>
>>  	/* umask vhost-user unsupported features */
>> --
>> 2.26.2
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH 3/3] net/virtio: add Virtio status support to Virtio-user
  2020-06-17 12:07 Xia, Chenbo
@ 2020-06-17 12:44 ` Maxime Coquelin
  0 siblings, 0 replies; 8+ messages in thread
From: Maxime Coquelin @ 2020-06-17 12:44 UTC (permalink / raw)
  To: Xia, Chenbo, dev, amorenoz, Ye, Xiaolong, shahafs, matan



On 6/17/20 2:07 PM, Xia, Chenbo wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Maxime Coquelin
>> Sent: Thursday, May 28, 2020 3:46 PM
>> To: dev@dpdk.org; amorenoz@redhat.com; Ye, Xiaolong
>> <xiaolong.ye@intel.com>; shahafs@mellanox.com; matan@mellanox.com
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: [dpdk-dev] [PATCH 3/3] net/virtio: add Virtio status support to Virtio-
>> user
>>
>> This patch adds support for VHOST_USER_SET_STATUS request. It is used to
>> make the backend aware of Virtio devices status update.
>>
>> It is useful for the backend to know when the Virtio driver is done with the Virtio
>> device configuration.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  drivers/net/virtio/virtio_user/vhost.h        |  5 +++++
>>  drivers/net/virtio/virtio_user/vhost_user.c   |  8 ++++++++
>>  .../net/virtio/virtio_user/virtio_user_dev.c  | 20 ++++++++++++++++++-
>>   .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
>>  drivers/net/virtio/virtio_user_ethdev.c       |  1 +
>>  5 files changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio/virtio_user/vhost.h
>> b/drivers/net/virtio/virtio_user/vhost.h
>> index 260e1c3081..fc360564eb 100644
>> --- a/drivers/net/virtio/virtio_user/vhost.h
>> +++ b/drivers/net/virtio/virtio_user/vhost.h
>> @@ -57,6 +57,10 @@ struct vhost_vring_addr {  #define
>> VHOST_USER_PROTOCOL_F_REPLY_ACK 3  #endif
>>
>> +#ifndef VHOST_USER_PROTOCOL_F_STATUS
>> +#define VHOST_USER_PROTOCOL_F_STATUS 15 #endif
>> +
>>  enum vhost_user_request {
>>  	VHOST_USER_NONE = 0,
>>  	VHOST_USER_GET_FEATURES = 1,
>> @@ -77,6 +81,7 @@ enum vhost_user_request {
>>  	VHOST_USER_SET_PROTOCOL_FEATURES = 16,
>>  	VHOST_USER_GET_QUEUE_NUM = 17,
>>  	VHOST_USER_SET_VRING_ENABLE = 18,
>> +	VHOST_USER_SET_STATUS = 36,
> 
> I see in qemu's vhost-user spec (https://www.qemu.org/docs/master/interop/vhost-user.html#protocol-features)
> that '36' is used by VHOST_USER_GET_MAX_MEM_SLOTS. And I don't see VHOST_USER_PROTOCOL_F_STATUS.
> Do I miss something here?

Partly, yes :)

In the cover letter, I mention that this patch should be applied only
once the spec update part of the Qemu series it depends on is merged:
"
So the first two patches can be applied as-is, while
for the last one, we have at least to wait the Qemu bits,
which includes the specification update, is merged
upstream.
"

This is a link to the Qemu patch:
https://patchwork.ozlabs.org/project/qemu-devel/patch/20200514073332.1434576-1-maxime.coquelin@redhat.com/

> Also, I see in drivers/net/virtio/virtio_pci.h, the status definition is too old and does not align with the spec now.
> Should we update that one?

Yes, that could be done, feel free to send a patch for this.

Thanks,
Maxime

> Thanks!
> Chenbo
> 
>>  	VHOST_USER_MAX
>>  };
>>
>> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c
>> b/drivers/net/virtio/virtio_user/vhost_user.c
>> index f8d751c98e..68232d75d7 100644
>> --- a/drivers/net/virtio/virtio_user/vhost_user.c
>> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
>> @@ -278,6 +278,14 @@ vhost_user_sock(struct virtio_user_dev *dev,
>>  		need_reply = 1;
>>  		break;
>>
>> +	case VHOST_USER_SET_STATUS:
>> +		if (!(dev->protocol_features &
>> +				(1ULL <<
>> VHOST_USER_PROTOCOL_F_STATUS)))
>> +			return 0;
>> +
>> +		if (has_reply_ack)
>> +			msg.flags |= VHOST_USER_NEED_REPLY_MASK;
>> +		/* Fallthrough */
>>  	case VHOST_USER_SET_FEATURES:
>>  	case VHOST_USER_SET_PROTOCOL_FEATURES:
>>  	case VHOST_USER_SET_LOG_BASE:
>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> index ea22af5dc4..cf29ea0b88 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> @@ -424,7 +424,8 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
>>
>>  #define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES		\
>>  	(1ULL << VHOST_USER_PROTOCOL_F_MQ |		\
>> -	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK)
>> +	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK |	\
>> +	 1ULL << VHOST_USER_PROTOCOL_F_STATUS)
>>
>>  int
>>  virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, @@ -
>> 782,3 +783,20 @@ virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t
>> queue_idx)
>>  		__atomic_add_fetch(&vring->used->idx, 1,
>> __ATOMIC_RELAXED);
>>  	}
>>  }
>> +
>> +int
>> +virtio_user_update_status(struct virtio_user_dev *dev, uint8_t status)
>> +{
>> +	int ret;
>> +	uint64_t arg = status;
>> +
>> +	/* Vhost-user only for now */
>> +	if (!is_vhost_user_by_type(dev->path))
>> +		return 0;
>> +
>> +	ret = dev->ops->send_request(dev, VHOST_USER_SET_STATUS, &arg);
>> +	if (ret)
>> +		return -1;
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> index 56e638f8a6..c49a896a88 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> @@ -71,4 +71,5 @@ void virtio_user_handle_cq(struct virtio_user_dev *dev,
>> uint16_t queue_idx);  void virtio_user_handle_cq_packed(struct virtio_user_dev
>> *dev,
>>  				  uint16_t queue_idx);
>>  uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t q_pairs);
>> +int virtio_user_update_status(struct virtio_user_dev *dev, uint8_t
>> +status);
>>  #endif
>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>> b/drivers/net/virtio/virtio_user_ethdev.c
>> index ccb5a18e25..bed161ba07 100644
>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>> @@ -271,6 +271,7 @@ virtio_user_set_status(struct virtio_hw *hw, uint8_t
>> status)
>>  	else if (status == VIRTIO_CONFIG_STATUS_RESET)
>>  		virtio_user_reset(hw);
>>  	dev->status = status;
>> +	virtio_user_update_status(dev, status);
>>  }
>>
>>  static uint8_t
>> --
>> 2.26.2
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH 3/3] net/virtio: add Virtio status support to Virtio-user
@ 2020-06-17 12:07 Xia, Chenbo
  2020-06-17 12:44 ` Maxime Coquelin
  0 siblings, 1 reply; 8+ messages in thread
From: Xia, Chenbo @ 2020-06-17 12:07 UTC (permalink / raw)
  To: Maxime Coquelin, dev, amorenoz, Ye, Xiaolong, shahafs, matan

Hi Maxime,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Maxime Coquelin
> Sent: Thursday, May 28, 2020 3:46 PM
> To: dev@dpdk.org; amorenoz@redhat.com; Ye, Xiaolong
> <xiaolong.ye@intel.com>; shahafs@mellanox.com; matan@mellanox.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [dpdk-dev] [PATCH 3/3] net/virtio: add Virtio status support to Virtio-
> user
> 
> This patch adds support for VHOST_USER_SET_STATUS request. It is used to
> make the backend aware of Virtio devices status update.
> 
> It is useful for the backend to know when the Virtio driver is done with the Virtio
> device configuration.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/virtio/virtio_user/vhost.h        |  5 +++++
>  drivers/net/virtio/virtio_user/vhost_user.c   |  8 ++++++++
>  .../net/virtio/virtio_user/virtio_user_dev.c  | 20 ++++++++++++++++++-
>   .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
>  drivers/net/virtio/virtio_user_ethdev.c       |  1 +
>  5 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/vhost.h
> b/drivers/net/virtio/virtio_user/vhost.h
> index 260e1c3081..fc360564eb 100644
> --- a/drivers/net/virtio/virtio_user/vhost.h
> +++ b/drivers/net/virtio/virtio_user/vhost.h
> @@ -57,6 +57,10 @@ struct vhost_vring_addr {  #define
> VHOST_USER_PROTOCOL_F_REPLY_ACK 3  #endif
> 
> +#ifndef VHOST_USER_PROTOCOL_F_STATUS
> +#define VHOST_USER_PROTOCOL_F_STATUS 15 #endif
> +
>  enum vhost_user_request {
>  	VHOST_USER_NONE = 0,
>  	VHOST_USER_GET_FEATURES = 1,
> @@ -77,6 +81,7 @@ enum vhost_user_request {
>  	VHOST_USER_SET_PROTOCOL_FEATURES = 16,
>  	VHOST_USER_GET_QUEUE_NUM = 17,
>  	VHOST_USER_SET_VRING_ENABLE = 18,
> +	VHOST_USER_SET_STATUS = 36,

I see in qemu's vhost-user spec (https://www.qemu.org/docs/master/interop/vhost-user.html#protocol-features)
that '36' is used by VHOST_USER_GET_MAX_MEM_SLOTS. And I don't see VHOST_USER_PROTOCOL_F_STATUS.
Do I miss something here?

Also, I see in drivers/net/virtio/virtio_pci.h, the status definition is too old and does not align with the spec now.
Should we update that one?

Thanks!
Chenbo

>  	VHOST_USER_MAX
>  };
> 
> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c
> b/drivers/net/virtio/virtio_user/vhost_user.c
> index f8d751c98e..68232d75d7 100644
> --- a/drivers/net/virtio/virtio_user/vhost_user.c
> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
> @@ -278,6 +278,14 @@ vhost_user_sock(struct virtio_user_dev *dev,
>  		need_reply = 1;
>  		break;
> 
> +	case VHOST_USER_SET_STATUS:
> +		if (!(dev->protocol_features &
> +				(1ULL <<
> VHOST_USER_PROTOCOL_F_STATUS)))
> +			return 0;
> +
> +		if (has_reply_ack)
> +			msg.flags |= VHOST_USER_NEED_REPLY_MASK;
> +		/* Fallthrough */
>  	case VHOST_USER_SET_FEATURES:
>  	case VHOST_USER_SET_PROTOCOL_FEATURES:
>  	case VHOST_USER_SET_LOG_BASE:
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index ea22af5dc4..cf29ea0b88 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -424,7 +424,8 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
> 
>  #define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES		\
>  	(1ULL << VHOST_USER_PROTOCOL_F_MQ |		\
> -	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK)
> +	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK |	\
> +	 1ULL << VHOST_USER_PROTOCOL_F_STATUS)
> 
>  int
>  virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, @@ -
> 782,3 +783,20 @@ virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t
> queue_idx)
>  		__atomic_add_fetch(&vring->used->idx, 1,
> __ATOMIC_RELAXED);
>  	}
>  }
> +
> +int
> +virtio_user_update_status(struct virtio_user_dev *dev, uint8_t status)
> +{
> +	int ret;
> +	uint64_t arg = status;
> +
> +	/* Vhost-user only for now */
> +	if (!is_vhost_user_by_type(dev->path))
> +		return 0;
> +
> +	ret = dev->ops->send_request(dev, VHOST_USER_SET_STATUS, &arg);
> +	if (ret)
> +		return -1;
> +
> +	return 0;
> +}
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> index 56e638f8a6..c49a896a88 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> @@ -71,4 +71,5 @@ void virtio_user_handle_cq(struct virtio_user_dev *dev,
> uint16_t queue_idx);  void virtio_user_handle_cq_packed(struct virtio_user_dev
> *dev,
>  				  uint16_t queue_idx);
>  uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t q_pairs);
> +int virtio_user_update_status(struct virtio_user_dev *dev, uint8_t
> +status);
>  #endif
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index ccb5a18e25..bed161ba07 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -271,6 +271,7 @@ virtio_user_set_status(struct virtio_hw *hw, uint8_t
> status)
>  	else if (status == VIRTIO_CONFIG_STATUS_RESET)
>  		virtio_user_reset(hw);
>  	dev->status = status;
> +	virtio_user_update_status(dev, status);
>  }
> 
>  static uint8_t
> --
> 2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-06-17 12:44 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-28  7:46 [dpdk-dev] [PATCH 0/3] net/virtio: add Vhost-user protocol features Maxime Coquelin
2020-05-28  7:46 ` [dpdk-dev] [PATCH 1/3] net/virtio: add vhost-user protocol features support Maxime Coquelin
2020-06-17 11:57   ` Xia, Chenbo
2020-06-17 12:22     ` Maxime Coquelin
2020-05-28  7:46 ` [dpdk-dev] [PATCH 2/3] net/virtio: add reply-ack support to Virtio-user Maxime Coquelin
2020-05-28  7:46 ` [dpdk-dev] [PATCH 3/3] net/virtio: add Virtio status " Maxime Coquelin
2020-06-17 12:07 Xia, Chenbo
2020-06-17 12:44 ` Maxime Coquelin

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git