[dpdk-stable] [PATCH] net/virtio-user: check negotiated features before set

[Yongseok Koh]

> On Jan 23, 2019, at 1:40 AM, eric zhang <eric.zhang at windriver.com> wrote:
> 
> From: Eric Zhang <eric.zhang at windriver.com>
> 
> [ backported from upstream commit 1db4d2330bc849a19b9f18479ae7a5a75bc01df2 ]
> 
> This patch checks negotiated features to see if necessary to offload
> before set the tap device offload capabilities. It also checks if kernel
> support the TUNSETOFFLOAD operation.
> 
> Fixes: 5e97e4202563 ("net/virtio-user: enable offloading")
> Cc: stable at dpdk.org
> 
> Signed-off-by: Eric Zhang <eric.zhang at windriver.com>
> Reviewed-by: Tiwei Bie <tiwei.bie at intel.com>
> ---

Applied to stable/17.11

thanks,
Yongseok


> drivers/net/virtio/virtio_user/vhost_kernel.c     | 19 ++++---
> drivers/net/virtio/virtio_user/vhost_kernel_tap.c | 68 ++++++++++++++++++-----
> drivers/net/virtio/virtio_user/vhost_kernel_tap.h |  3 +-
> 3 files changed, 68 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c b/drivers/net/virtio/virtio_user/vhost_kernel.c
> index 68d28b1..3502001 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel.c
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
> @@ -189,8 +189,8 @@ prepare_vhost_memory_kernel(void)
> 	 (1ULL << VIRTIO_NET_F_HOST_TSO6) |	\
> 	 (1ULL << VIRTIO_NET_F_CSUM))
> 
> -static int
> -tap_supporte_mq(void)
> +static unsigned int
> +tap_support_features(void)
> {
> 	int tapfd;
> 	unsigned int tap_features;
> @@ -209,7 +209,7 @@ tap_supporte_mq(void)
> 	}
> 
> 	close(tapfd);
> -	return tap_features & IFF_MULTI_QUEUE;
> +	return tap_features;
> }
> 
> static int
> @@ -223,6 +223,7 @@ vhost_kernel_ioctl(struct virtio_user_dev *dev,
> 	struct vhost_memory_kernel *vm = NULL;
> 	int vhostfd;
> 	unsigned int queue_sel;
> +	unsigned int features;
> 
> 	PMD_DRV_LOG(INFO, "%s", vhost_msg_strings[req]);
> 
> @@ -276,17 +277,20 @@ vhost_kernel_ioctl(struct virtio_user_dev *dev,
> 	}
> 
> 	if (!ret && req_kernel == VHOST_GET_FEATURES) {
> +		features = tap_support_features();
> 		/* with tap as the backend, all these features are supported
> 		 * but not claimed by vhost-net, so we add them back when
> 		 * reporting to upper layer.
> 		 */
> -		*((uint64_t *)arg) |= VHOST_KERNEL_GUEST_OFFLOADS_MASK;
> -		*((uint64_t *)arg) |= VHOST_KERNEL_HOST_OFFLOADS_MASK;
> +		if (features & IFF_VNET_HDR) {
> +			*((uint64_t *)arg) |= VHOST_KERNEL_GUEST_OFFLOADS_MASK;
> +			*((uint64_t *)arg) |= VHOST_KERNEL_HOST_OFFLOADS_MASK;
> +		}
> 
> 		/* vhost_kernel will not declare this feature, but it does
> 		 * support multi-queue.
> 		 */
> -		if (tap_supporte_mq())
> +		if (features & IFF_MULTI_QUEUE)
> 			*(uint64_t *)arg |= (1ull << VIRTIO_NET_F_MQ);
> 	}
> 
> @@ -380,7 +384,8 @@ vhost_kernel_enable_queue_pair(struct virtio_user_dev *dev,
> 	else
> 		hdr_size = sizeof(struct virtio_net_hdr);
> 
> -	tapfd = vhost_kernel_open_tap(&dev->ifname, hdr_size, req_mq);
> +	tapfd = vhost_kernel_open_tap(&dev->ifname, hdr_size, req_mq,
> +			 (char *)dev->mac_addr, dev->features);
> 	if (tapfd < 0) {
> 		PMD_DRV_LOG(ERR, "fail to open tap for vhost kernel");
> 		return -1;
> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> index 689a5cf..e9ee774 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> @@ -36,26 +36,64 @@
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <net/if.h>
> +#include <net/if_arp.h>
> #include <errno.h>
> #include <string.h>
> #include <limits.h>
> 
> +#include <rte_ether.h>
> +
> #include "vhost_kernel_tap.h"
> #include "../virtio_logs.h"
> +#include "../virtio_pci.h"
> +
> +static int
> +vhost_kernel_tap_set_offload(int fd, uint64_t features)
> +{
> +	unsigned int offload = 0;
> +
> +	if (features & (1ULL << VIRTIO_NET_F_GUEST_CSUM)) {
> +		offload |= TUN_F_CSUM;
> +		if (features & (1ULL << VIRTIO_NET_F_GUEST_TSO4))
> +			offload |= TUN_F_TSO4;
> +		if (features & (1ULL << VIRTIO_NET_F_GUEST_TSO6))
> +			offload |= TUN_F_TSO6;
> +		if (features & ((1ULL << VIRTIO_NET_F_GUEST_TSO4) |
> +			(1ULL << VIRTIO_NET_F_GUEST_TSO6)) &&
> +			(features & (1ULL << VIRTIO_NET_F_GUEST_ECN)))
> +			offload |= TUN_F_TSO_ECN;
> +		if (features & (1ULL << VIRTIO_NET_F_GUEST_UFO))
> +			offload |= TUN_F_UFO;
> +	}
> +
> +	if (offload != 0) {
> +		/* Check if our kernel supports TUNSETOFFLOAD */
> +		if (ioctl(fd, TUNSETOFFLOAD, 0) != 0 && errno == EINVAL) {
> +			PMD_DRV_LOG(ERR, "Kernel does't support TUNSETOFFLOAD\n");
> +			return -ENOTSUP;
> +		}
> +
> +		if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) {
> +			offload &= ~TUN_F_UFO;
> +			if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) {
> +				PMD_DRV_LOG(ERR, "TUNSETOFFLOAD ioctl() failed: %s\n",
> +					strerror(errno));
> +				return -1;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> 
> int
> -vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq)
> +vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq,
> +			 const char *mac, uint64_t features)
> {
> 	unsigned int tap_features;
> 	int sndbuf = INT_MAX;
> 	struct ifreq ifr;
> 	int tapfd;
> -	unsigned int offload =
> -			TUN_F_CSUM |
> -			TUN_F_TSO4 |
> -			TUN_F_TSO6 |
> -			TUN_F_TSO_ECN |
> -			TUN_F_UFO;
> 
> 	/* TODO:
> 	 * 1. verify we can get/set vnet_hdr_len, tap_probe_vnet_hdr_len
> @@ -115,13 +153,15 @@ vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq)
> 		goto error;
> 	}
> 
> -	/* TODO: before set the offload capabilities, we'd better (1) check
> -	 * negotiated features to see if necessary to offload; (2) query tap
> -	 * to see if it supports the offload capabilities.
> -	 */
> -	if (ioctl(tapfd, TUNSETOFFLOAD, offload) != 0)
> -		PMD_DRV_LOG(ERR, "TUNSETOFFLOAD ioctl() failed: %s",
> -			   strerror(errno));
> +	vhost_kernel_tap_set_offload(tapfd, features);
> +
> +	memset(&ifr, 0, sizeof(ifr));
> +	ifr.ifr_hwaddr.sa_family = ARPHRD_ETHER;
> +	memcpy(ifr.ifr_hwaddr.sa_data, mac, ETHER_ADDR_LEN);
> +	if (ioctl(tapfd, SIOCSIFHWADDR, (void *)&ifr) == -1) {
> +		PMD_DRV_LOG(ERR, "SIOCSIFHWADDR failed: %s", strerror(errno));
> +		goto error;
> +	}
> 
> 	if (!(*p_ifname))
> 		*p_ifname = strdup(ifr.ifr_name);
> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> index eae340c..ea7a6c9 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> @@ -64,4 +64,5 @@
> /* Constants */
> #define PATH_NET_TUN	"/dev/net/tun"
> 
> -int vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq);
> +int vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq,
> +			 const char *mac, uint64_t features);
> -- 
> 2.7.4
>