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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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
>