Re: [dpdk-dev] [PATCH v2] net/virtio-user: check negotiated features before set

[Tiwei Bie <tiwei.bie@intel.com>]

On Mon, Aug 27, 2018 at 02:37:24PM -0400, eric zhang wrote:
> 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.
> 
> Signed-off-by: eric zhang <eric.zhang@windriver.com>
> 
> ---
> v2:
> * don't return failure when failed to set offload to tap
> * check if offloads available when handling VHOST_GET_FEATURES
> ---
>  drivers/net/virtio/virtio_user/vhost_kernel.c     |  8 ++--
>  drivers/net/virtio/virtio_user/vhost_kernel_tap.c | 55 +++++++++++++++++------
>  drivers/net/virtio/virtio_user/vhost_kernel_tap.h |  2 +-
>  3 files changed, 47 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c b/drivers/net/virtio/virtio_user/vhost_kernel.c
> index dd24b6b..5c39f26 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel.c
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
> @@ -278,9 +278,11 @@ struct vhost_memory_kernel {
>  	if (!ret && req_kernel == VHOST_GET_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.
> +		 * reporting to upper layer. For guest offloads we check if
> +		 * they are available in the negotiated features.
>  		 */
> -		*((uint64_t *)arg) |= VHOST_KERNEL_GUEST_OFFLOADS_MASK;
> +		*((uint64_t *)arg) |=
> +			(dev->features & VHOST_KERNEL_GUEST_OFFLOADS_MASK);

VHOST_GET_FEATURES returns the features supported
by backend instead of the negotiated features. We
need to check whether tap support vnet_hdr etc to
know whether these features are available.

>  		*((uint64_t *)arg) |= VHOST_KERNEL_HOST_OFFLOADS_MASK;
>  
>  		/* vhost_kernel will not declare this feature, but it does
> @@ -381,7 +383,7 @@ struct vhost_memory_kernel {
>  		hdr_size = sizeof(struct virtio_net_hdr);
>  
>  	tapfd = vhost_kernel_open_tap(&dev->ifname, hdr_size, req_mq,
> -			 (char *)dev->mac_addr);
> +			 (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 d036428..5e86404 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> @@ -45,21 +45,54 @@
>  
>  #include "vhost_kernel_tap.h"
>  #include "../virtio_logs.h"
> +#include "../virtio_pci.h"
> +
> +static int
> +vhost_kernel_tap_set_offload(int fd, uint64_t feature)

s/feature/features/

> +{
> +	unsigned int offload = 0;
> +
> +	if (feature & (1ULL << VIRTIO_NET_F_GUEST_CSUM))
> +		offload |= TUN_F_CSUM;
> +	if (feature & (1ULL << VIRTIO_NET_F_GUEST_TSO4))
> +		offload |= TUN_F_TSO4;
> +	if (feature & (1ULL << VIRTIO_NET_F_GUEST_TSO6))
> +		offload |= TUN_F_TSO6;
> +	if (feature & ((1ULL << VIRTIO_NET_F_GUEST_TSO4) |
> +		(1ULL << VIRTIO_NET_F_GUEST_TSO6)) &&
> +		(feature & (1ULL << VIRTIO_NET_F_GUEST_ECN)))
> +		offload |= TUN_F_TSO_ECN;
> +	if (feature & (1ULL << VIRTIO_NET_F_GUEST_UFO))
> +		offload |= TUN_F_UFO;

The features other than CSUM feature depends on the
CSUM feature. Maybe it's better to do what QEMU does,
i.e. announce these offloads in tap only when the
CSUM feature is available:

https://github.com/qemu/qemu/blob/6ad90805383e/net/tap-linux.c#L247-L257

Thanks

> +
> +	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,
> -			 const char *mac)
> +			 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
> @@ -119,13 +152,7 @@
>  		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;
> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> index 402f964..ea7a6c9 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> @@ -65,4 +65,4 @@
>  #define PATH_NET_TUN	"/dev/net/tun"
>  
>  int vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq,
> -			 const char *mac);
> +			 const char *mac, uint64_t features);
> -- 
> 1.8.3.1
>