From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 6133E2C5 for ; Wed, 15 Aug 2018 05:42:19 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Aug 2018 20:42:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,241,1531810800"; d="scan'208";a="81456076" Received: from debian.sh.intel.com ([10.67.104.166]) by fmsmga001.fm.intel.com with ESMTP; 14 Aug 2018 20:42:12 -0700 Date: Wed, 15 Aug 2018 11:40:01 +0800 From: Tiwei Bie To: Allain Legacy Cc: maxime.coquelin@redhat.com, zhihong.wang@intel.com, dev@dpdk.org, matt.peters@windriver.com, eric.zhang@windriver.com Message-ID: <20180815034001.GA21177@debian.sh.intel.com> References: <20180809183455.32442-1-allain.legacy@windriver.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20180809183455.32442-1-allain.legacy@windriver.com> User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-dev] [PATCH] net/virtio-user: check negotiated features before set X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 15 Aug 2018 03:42:20 -0000 On Thu, Aug 09, 2018 at 01:34:55PM -0500, Allain Legacy wrote: > From: eric zhang > > 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 > Signed-off-by: Allain Legacy > --- > 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 >