[1/3] net/virtio: add vhost-user protocol features support
diff mbox series

Message ID 20200528074627.439192-2-maxime.coquelin@redhat.com
State Superseded
Delegated to: Maxime Coquelin
Headers show
Series
  • net/virtio: add Vhost-user protocol features
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/iol-testing warning Testing issues
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/checkpatch success coding style OK

Commit Message

Maxime Coquelin May 28, 2020, 7:46 a.m. UTC
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(-)

Comments

Xia, Chenbo June 17, 2020, 11:57 a.m. UTC | #1
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
Maxime Coquelin June 17, 2020, 12:22 p.m. UTC | #2
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
>

Patch
diff mbox series

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 */