[dpdk-stable] patch 'net/virtio-user: check negotiated features before set' has been queued to stable release 18.08.1

[Kevin Traynor]

Hi,

FYI, your patch has been queued to stable release 18.08.1

Note it hasn't been pushed to http://dpdk.org/browse/dpdk-stable yet.
It will be pushed if I get no objections before 11/23/18. So please
shout if anyone has objections.

Also note that after the patch there's a diff of the upstream commit vs the patch applied
to the branch. If the code is different (ie: not only metadata diffs), due for example to
a change in context or macro names, please double check it.

Thanks.

Kevin Traynor

---
>From 96906d293ca53a338201f5cecdc40b5148472f8c Mon Sep 17 00:00:00 2001
From: Eric Zhang <eric.zhang at windriver.com>
Date: Wed, 29 Aug 2018 11:55:21 -0400
Subject: [PATCH] net/virtio-user: check negotiated features before set

[ 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")

Signed-off-by: Eric Zhang <eric.zhang at windriver.com>
Reviewed-by: Tiwei Bie <tiwei.bie at intel.com>
---
 drivers/net/virtio/virtio_user/vhost_kernel.c | 18 +++---
 .../net/virtio/virtio_user/vhost_kernel_tap.c | 56 ++++++++++++++-----
 .../net/virtio/virtio_user/vhost_kernel_tap.h |  2 +-
 3 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c b/drivers/net/virtio/virtio_user/vhost_kernel.c
index b2444096c..d1be82162 100644
--- a/drivers/net/virtio/virtio_user/vhost_kernel.c
+++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
@@ -148,6 +148,6 @@ prepare_vhost_memory_kernel(void)
 	 (1ULL << VIRTIO_NET_F_CSUM))
 
-static int
-tap_supporte_mq(void)
+static unsigned int
+tap_support_features(void)
 {
 	int tapfd;
@@ -168,5 +168,5 @@ tap_supporte_mq(void)
 
 	close(tapfd);
-	return tap_features & IFF_MULTI_QUEUE;
+	return tap_features;
 }
 
@@ -182,4 +182,5 @@ vhost_kernel_ioctl(struct virtio_user_dev *dev,
 	int vhostfd;
 	unsigned int queue_sel;
+	unsigned int features;
 
 	PMD_DRV_LOG(INFO, "%s", vhost_msg_strings[req]);
@@ -235,15 +236,18 @@ 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);
 	}
@@ -340,5 +344,5 @@ vhost_kernel_enable_queue_pair(struct virtio_user_dev *dev,
 
 	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");
diff --git a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
index 9ea7ade74..a3faf1d0c 100644
--- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
+++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
@@ -17,8 +17,48 @@
 #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,
-			 const char *mac)
+			 const char *mac, uint64_t features)
 {
 	unsigned int tap_features;
@@ -26,10 +66,4 @@ vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq,
 	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:
@@ -91,11 +125,5 @@ vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq,
 	}
 
-	/* 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));
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
@@ -37,3 +37,3 @@
 
 int vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq,
-			 const char *mac);
+			 const char *mac, uint64_t features);
-- 
2.19.0

---
  Diff of the applied patch vs upstream commit (please double-check if non-empty:
---
--- -	2018-11-20 17:53:08.142112656 +0000
+++ 0028-net-virtio-user-check-negotiated-features-before-set.patch	2018-11-20 17:53:07.000000000 +0000
@@ -1,14 +1,15 @@
-From 1db4d2330bc849a19b9f18479ae7a5a75bc01df2 Mon Sep 17 00:00:00 2001
+From 96906d293ca53a338201f5cecdc40b5148472f8c Mon Sep 17 00:00:00 2001
 From: Eric Zhang <eric.zhang at windriver.com>
 Date: Wed, 29 Aug 2018 11:55:21 -0400
 Subject: [PATCH] net/virtio-user: check negotiated features before set
 
+[ 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>