Re: [dpdk-dev] [PATCH 1/3] net/virtio: add vhost-user protocol features support

["Xia, Chenbo" <chenbo.xia@intel.com>]

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