From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id BAA91A04BB; Thu, 17 Sep 2020 14:23:19 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 9D8711D62D; Thu, 17 Sep 2020 14:23:18 +0200 (CEST) Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by dpdk.org (Postfix) with ESMTP id 82EC91D621 for ; Thu, 17 Sep 2020 14:23:16 +0200 (CEST) Received: by mail-wm1-f67.google.com with SMTP id w2so1772487wmi.1 for ; Thu, 17 Sep 2020 05:23:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=Yj9ixopYkEyFUuEkYS2pAzecBUQEMgfqZ42cEjhZUSY=; b=FGeDYxu7zxVHvjseJnbqtBHsjS7FpVwlsVcBZp42SQaJl7xEaocX+IVLlsJk/khNx2 0jfqVfNA9NhFrGDcwOXawzSr3weDWx536UaVpllKfNsPqbvCW/uJYVw5NH56cfYwudcT CweOOPo4R4ydgL0m/2iUmc9m5RrFVNVn43aRvAONKo+DX+heoMpwr0D1ibMvxhfC+SfD sYH22gFbdt4DPS+/9ypfP8V5QtMx4KSlVlb5stnF2GkpQoFaFua7avtxfA2dJ4eAUmzX OVBTMoV7VDeOOJte51iFrAHGkPU85s67QjbkbGaVDobOesQ2jXkqGC1ocMdnJdgLSAeB kRLg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Yj9ixopYkEyFUuEkYS2pAzecBUQEMgfqZ42cEjhZUSY=; b=Hpf7BZwrJO0Ul7BsGT3gjuHKkGOTzUgEsLL1eCGlE5ALdZ+7vFGoSf8RoIvZsvgLri dmjkt5+dP+UeK2EgH0BOG9OzH/00QbbStRciC+7wWxHoRL3WLlwH51ljtvI6JQSyW4Tt Xv4VaWmUxcbS73vHV/7ynCoS69dP1qvD7rAIWPWHycsvnQHrPDE+zsYh94/YoBYtfuKp lZmBvhSCKTTvZXpvQ0D3VNjbq/wi9WUporO02ctb/1HppBrxhDaIukzn9HH8rVSIV8mi vRjZ/bI22M6ezx//fkFvaVrI8cVPBryEPNpbs8LuiKnBHI4tPAG9/4WolkNgPyPdJUqS Qa4Q== X-Gm-Message-State: AOAM532ArZhq3aOUPGvYME4F1Yps3wqyTaoqhSjVANUftCdd+GWavdw6 wbEDSwYhqZcbpj9HEevhlwzVRg== X-Google-Smtp-Source: ABdhPJzcufcwAzYAPQJwSG+EOSOBsT8FRIm8ZBWcUXS/U+WuzVNasCq/JoFrioCQjxxKTjHqGGbhMQ== X-Received: by 2002:a1c:a593:: with SMTP id o141mr9869449wme.88.1600345396099; Thu, 17 Sep 2020 05:23:16 -0700 (PDT) Received: from 6wind.com (2a01cb0c0005a600345636f7e65ed1a0.ipv6.abo.wanadoo.fr. [2a01:cb0c:5:a600:3456:36f7:e65e:d1a0]) by smtp.gmail.com with ESMTPSA id t1sm10144154wmi.16.2020.09.17.05.23.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 17 Sep 2020 05:23:15 -0700 (PDT) Date: Thu, 17 Sep 2020 14:23:14 +0200 From: Olivier Matz To: Ophir Munk Cc: dev@dpdk.org, Wenzhuo Lu , Beilei Xing , Bernard Iremonger , Ferruh Yigit , Ophir Munk Message-ID: <20200917122314.GQ21395@platinum> References: <20200915125319.16568-2-ophirmu@nvidia.com> <20200915131717.18252-1-ophirmu@nvidia.com> <20200915131717.18252-2-ophirmu@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200915131717.18252-2-ophirmu@nvidia.com> User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-dev] [PATCH v4 1/3] app/testpmd: add GENEVE parsing 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Ophir, Please find some comments below. On Tue, Sep 15, 2020 at 01:17:15PM +0000, Ophir Munk wrote: > From: Ophir Munk > > GENEVE is a widely used tunneling protocol in modern Virtualized > Networks. testpmd already supports parsing of several tunneling > protocols including VXLAN, VXLAN-GPE, GRE. This commit adds GENEVE > parsing of inner protocols (IPv4-0x0800, IPv6-0x86dd, Ethernet-0x6558) > based on IETF draft-ietf-nvo3-geneve-09. GENEVE is considered more > flexible than the other protocols. In terms of protocol format GENEVE > header has a variable length options as opposed to other tunneling > protocols which have a fixed header size. > > Signed-off-by: Ophir Munk > app/test-pmd/csumonly.c | 70 ++++++++++++++++++++++++++++++++++++++++++- > app/test-pmd/testpmd.h | 1 + > lib/librte_net/meson.build | 3 +- > lib/librte_net/rte_geneve.h | 72 +++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 144 insertions(+), 2 deletions(-) > create mode 100644 lib/librte_net/rte_geneve.h An entry could be added in doc/api/doxy-api-index.md. Some more protocols are missing, I'll send a patch to add them. > --- /dev/null > +++ b/lib/librte_net/rte_geneve.h > @@ -0,0 +1,72 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright 2020 Mellanox Technologies, Ltd > + */ > + > +#ifndef _RTE_GENEVE_H_ > +#define _RTE_GENEVE_H_ > + > +/** > + * @file > + * > + * GENEVE-related definitions > + */ > + > +#include > + > +#include Is this include needed? Maybe it comes from a copy/paste of VXLAN? > + > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +/** GENEVE default port. */ > +#define RTE_GENEVE_DEFAULT_PORT 6081 > + > +/** > + * GENEVE protocol header. (draft-ietf-nvo3-geneve-09) > + * Contains: > + * 2-bits version (must be 0). > + * 6-bits option length in four byte multiples, not including the eight > + * bytes of the fixed tunnel header. > + * 1-bit control packet. > + * 1-bit critical options in packet. > + * 6-bits reserved > + * 16-bits Protocol Type. The protocol data unit after the Geneve header > + * following the EtherType convention. Ethernet itself is represented by > + * the value 0x6558. > + * 24-bits Virtual Network Identifier (VNI). Virtual network unique identified. > + * 8-bits reserved bits (must be 0 on transmission and ignored on receipt). > + * More-bits (optional) variable length options. > + */ > +__extension__ > +struct rte_geneve_hdr { > +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN > + uint8_t ver:2; /**< Version (2). */ Isn't the (2) in the comment redundant with the :2 in the type? Here is how bitfield look like in doxygen documentation: https://doc.dpdk.org/api/structrte__flow__attr.html#ae4d19341d5298a2bc61f9eb941b1179c It's true that the field documentation miss the number of bits. So if you feel it's needed, I prefer something more explicit like "(2 bits)" instead of just "(2)". By the way, there are 2 spaces at the end of the comment. > + uint8_t opt_len:6; /**< Options length (6). */ > + uint8_t oam:1; /**< Control packet (1). */ > + uint8_t critical:1; /**< Critical packet (1). */ > + uint8_t rsvd1:6; /**< Reserved (6). */ "reserved" instead of "rsvd"? The Internet-Draft says "Rsvd" for this one, but "Reserved" for the other. > +#else > + uint8_t opt_len:6; /**< Options length (6). */ > + uint8_t ver:2; /**< Version (2). */ > + uint8_t rsvd1:6; /**< Reserved (6). */ > + uint8_t critical:1; /**< Critical packet (1). */ > + uint8_t oam:1; /**< Control packet (1). */ > +#endif > + rte_be16_t proto; /**< Protocol type (16). */ > + uint8_t vni[3]; /**< Virtual network identifier (24). */ > + uint8_t rsvd2; /**< Reserved (8). */ vni is an identifier, so I wonder if it would make sense to have it as an integer instead of an array of uint8. Something like this: #if RTE_BYTE_ORDER == RTE_BIG_ENDIAN uint32_t vni:24; uint32_t reserved2:8; #else uint32_t vni:24; uint32_t reserved2:8; #endif > + uint8_t opts[]; /** +} __rte_packed; > + > +/* GENEVE next protocol types */ > +#define RTE_GENEVE_TYPE_IPV4 0x0800 /**< IPv4 Protocol. */ > +#define RTE_GENEVE_TYPE_IPV6 0x86dd /**< IPv6 Protocol. */ > +#define RTE_GENEVE_TYPE_ETH 0x6558 /**< Ethernet Protocol. */ >From what I understand in the draft, I think only RTE_GENEVE_TYPE_ETH is needed. Protocol Type (16 bits): The type of the protocol data unit appearing after the Geneve header. This follows the EtherType [ETYPES] convention; with Ethernet itself being represented by the value 0x6558. Shouldn't we use RTE_ETHER_TYPE_* instead of redefining them here? 0x6558 is RTE_ETHER_TYPE_TEB (Transparent Ethernet Bridging) and is also used in case of NVGRE. Regards, Olivier