net/virtio-user: check negotiated features before set

  From: eric zhang <eric.zhang@windriver.com>

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>
Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 drivers/net/virtio/virtio_user/vhost_kernel.c     |  2 +-
 drivers/net/virtio/virtio_user/vhost_kernel_tap.c | 56 +++++++++++++++++------
 drivers/net/virtio/virtio_user/vhost_kernel_tap.h |  2 +-
 3 files changed, 44 insertions(+), 16 deletions(-)
  

On Thu, Aug 09, 2018 at 01:34:55PM -0500, Allain Legacy wrote:
>From: eric zhang <eric.zhang@windriver.com>
>
>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>
>Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
>---
> drivers/net/virtio/virtio_user/vhost_kernel.c     |  2 +-
> drivers/net/virtio/virtio_user/vhost_kernel_tap.c | 56 +++++++++++++++++------
> drivers/net/virtio/virtio_user/vhost_kernel_tap.h |  2 +-
> 3 files changed, 44 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c b/drivers/net/virtio/virtio_user/vhost_kernel.c
>index b2444096c..66f2eaca9 100644
>--- a/drivers/net/virtio/virtio_user/vhost_kernel.c
>+++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
>@@ -339,7 +339,7 @@ vhost_kernel_enable_queue_pair(struct virtio_user_dev *dev,
> 		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 9ea7ade74..eda9fb78c 100644
>--- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
>+++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
>@@ -16,21 +16,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)
>+{
>+	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;
>+
>+	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
>@@ -90,13 +123,8 @@ 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));
>+	if (vhost_kernel_tap_set_offload(tapfd, features) != 0)

Since we only check for != 0 we could just have it return a bool.
But it is just a nit.

Reviewed-by: Jens Freimann <jfreimann@redhat.com> 

regards,
Jens
  

On Thu, Aug 09, 2018 at 01:34:55PM -0500, Allain Legacy wrote:
> From: eric zhang <eric.zhang@windriver.com>
> 
> 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>
> Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
> ---
>  drivers/net/virtio/virtio_user/vhost_kernel.c     |  2 +-
>  drivers/net/virtio/virtio_user/vhost_kernel_tap.c | 56 +++++++++++++++++------
>  drivers/net/virtio/virtio_user/vhost_kernel_tap.h |  2 +-
>  3 files changed, 44 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c b/drivers/net/virtio/virtio_user/vhost_kernel.c
> index b2444096c..66f2eaca9 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel.c
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
> @@ -339,7 +339,7 @@ vhost_kernel_enable_queue_pair(struct virtio_user_dev *dev,
>  		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 9ea7ade74..eda9fb78c 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> @@ -16,21 +16,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)
> +{
> +	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;
> +
> +	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;
> +			}
> +		}

I'm not quite sure whether it's a good idea to
return failure when failed to set offloads to
tap. And QEMU also doesn't do this:

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

We should also check whether offloads available
when handling VHOST_GET_FEATURES.

Thanks

> +	}
> +
> +	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
> @@ -90,13 +123,8 @@ 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));
> +	if (vhost_kernel_tap_set_offload(tapfd, features) != 0)
> +		goto error;
>  
>  	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 01a026f50..e0e95b4f5 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> @@ -36,4 +36,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);
> -- 
> 2.12.1
>
  

> -----Original Message-----
> From: Tiwei Bie [mailto:tiwei.bie@intel.com]
> Sent: Tuesday, August 14, 2018 11:40 PM
> To: Legacy, Allain
> Cc: maxime.coquelin@redhat.com; zhihong.wang@intel.com;
> dev@dpdk.org; Peters, Matt; Zhang, Qing Long (Eric)
> Subject: Re: [PATCH] net/virtio-user: check negotiated features before set
> 
> On Thu, Aug 09, 2018 at 01:34:55PM -0500, Allain Legacy wrote:
> > From: eric zhang <eric.zhang@windriver.com>
> >
> > 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>
> > Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
> > ---
> >  drivers/net/virtio/virtio_user/vhost_kernel.c     |  2 +-
> >  drivers/net/virtio/virtio_user/vhost_kernel_tap.c | 56
> > +++++++++++++++++------
> > drivers/net/virtio/virtio_user/vhost_kernel_tap.h |  2 +-
> >  3 files changed, 44 insertions(+), 16 deletions(-)
> >
<...>
> 
> I'm not quite sure whether it's a good idea to return failure when failed to set
> offloads to tap. And QEMU also doesn't do this:
> 
> https://github.com/qemu/qemu/blob/6ad90805383e/net/tap-linux.c#L261

Regardless of what has already been done in qemu my preference is that if a requested feature cannot be set then a failure should be propagated up to the application.  Otherwise, the application is left to debug a silent failure which, depending on what that is, may be difficult.   But, as the maintainer, that is your call so if you want it changed to a silent failure I will do that.   Just so that I am clear you want both failures to be ignored?  That is, if a requested offload is not supported we should silently ignore the -ENOTSUP, and also if the ioctl fails for some other reason we should ignore that too?


> 
> We should also check whether offloads available when handling
> VHOST_GET_FEATURES.

That will have to be a different patch as it is outside of the scope of what was being fixed by this one.   For this issue we were fixing the fact that offload features were being enabled even when they were not requested by the application.


Allain
  

On Wed, Aug 15, 2018 at 12:28:05PM +0000, Legacy, Allain wrote:
> > -----Original Message-----
> > From: Tiwei Bie [mailto:tiwei.bie@intel.com]
> > Sent: Tuesday, August 14, 2018 11:40 PM
> > To: Legacy, Allain
> > Cc: maxime.coquelin@redhat.com; zhihong.wang@intel.com;
> > dev@dpdk.org; Peters, Matt; Zhang, Qing Long (Eric)
> > Subject: Re: [PATCH] net/virtio-user: check negotiated features before set
> > 
> > On Thu, Aug 09, 2018 at 01:34:55PM -0500, Allain Legacy wrote:
> > > From: eric zhang <eric.zhang@windriver.com>
> > >
> > > 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>
> > > Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
> > > ---
> > >  drivers/net/virtio/virtio_user/vhost_kernel.c     |  2 +-
> > >  drivers/net/virtio/virtio_user/vhost_kernel_tap.c | 56
> > > +++++++++++++++++------
> > > drivers/net/virtio/virtio_user/vhost_kernel_tap.h |  2 +-
> > >  3 files changed, 44 insertions(+), 16 deletions(-)
> > >
> <...>
> > 
> > I'm not quite sure whether it's a good idea to return failure when failed to set
> > offloads to tap. And QEMU also doesn't do this:
> > 
> > https://github.com/qemu/qemu/blob/6ad90805383e/net/tap-linux.c#L261
> 
> Regardless of what has already been done in qemu my preference is that if a requested feature cannot be set then a failure should be propagated up to the application.  Otherwise, the application is left to debug a silent failure which, depending on what that is, may be difficult.

It's still logged as an error, isn't it?

Currently, it's just related to the guest offloads. It seems
that failed to set offloads to tap just means that users of
tap won't be able to leverage the offload capabilities provided
by guest (i.e. virtio-user) as tap won't announce them. And
it's not really necessary to fail the virtio-user's device
start in this case. Thoughts?

> But, as the maintainer, that is your call so if you want it changed to a silent failure I will do that.   Just so that I am clear you want both failures to be ignored?  That is, if a requested offload is not supported we should silently ignore the -ENOTSUP, and also if the ioctl fails for some other reason we should ignore that too?
> 
> 
> > 
> > We should also check whether offloads available when handling
> > VHOST_GET_FEATURES.
> 
> That will have to be a different patch as it is outside of the scope of what was being fixed by this one.   For this issue we were fixing the fact that offload features were being enabled even when they were not requested by the application.

It's better to fix it in the same patch set if the
error handling is changed.

Thanks

> 
> 
> Allain
> 
>