Hi Maxime,
> -----Original Message-----
> From: dev <dev-boun...@dpdk.org> On Behalf Of Maxime Coquelin
> Sent: Thursday, May 28, 2020 3:46 PM
> To: dev@dpdk.org; amore...@redhat.com; Ye, Xiaolong
> <xiaolong...@intel.com>; shah...@mellanox.com; ma...@mellanox.com
> Cc: Maxime Coquelin <maxime.coque...@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.coque...@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
