[v4,1/3] app/testpmd: add GENEVE parsing

Message ID 20200915131717.18252-2-ophirmu@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series Add GENEVE protocol parsing to testpmd |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Ophir Munk Sept. 15, 2020, 1:17 p.m. UTC
  From: Ophir Munk <ophirmu@mellanox.com>

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 <ophirmu@mellanox.com>
---
 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
  

Comments

Ophir Munk Sept. 15, 2020, 1:56 p.m. UTC | #1
Adding Olivier who is the maintainer of the Network headers (lib/librte_net/rte_geneve.h).

> -----Original Message-----
> From: Ophir Munk <ophirmu@nvidia.com>
> Sent: Tuesday, September 15, 2020 4:17 PM
> To: dev@dpdk.org; Wenzhuo Lu <wenzhuo.lu@intel.com>; Beilei Xing
> <beilei.xing@intel.com>; Bernard Iremonger
> <bernard.iremonger@intel.com>; Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: Ophir Munk <ophirmu@nvidia.com>; Ophir Munk
> <ophirmu@mellanox.com>
> Subject: [PATCH v4 1/3] app/testpmd: add GENEVE parsing
> 
> From: Ophir Munk <ophirmu@mellanox.com>
> 
> 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 <ophirmu@mellanox.com>
> ---
>  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
> 
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c index
> 7ece398..eb87b9b 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -43,6 +43,7 @@
>  #include <rte_flow.h>
>  #include <rte_gro.h>
>  #include <rte_gso.h>
> +#include <rte_geneve.h>
> 
>  #include "testpmd.h"
> 
> @@ -63,6 +64,7 @@
>  #endif
> 
>  uint16_t vxlan_gpe_udp_port = 4790;
> +uint16_t geneve_udp_port = RTE_GENEVE_DEFAULT_PORT;
> 
>  /* structure that caches offload info for the current packet */  struct
> testpmd_offload_info { @@ -333,6 +335,64 @@ parse_vxlan_gpe(struct
> rte_udp_hdr *udp_hdr,
>  	info->l2_len += RTE_ETHER_VXLAN_GPE_HLEN;  }
> 
> +/* Fill in outer layers length */
> +static void
> +update_tunnel_outer(struct testpmd_offload_info *info) {
> +	info->is_tunnel = 1;
> +	info->outer_ethertype = info->ethertype;
> +	info->outer_l2_len = info->l2_len;
> +	info->outer_l3_len = info->l3_len;
> +	info->outer_l4_proto = info->l4_proto; }
> +
> +/* Parse a geneve header */
> +static void
> +parse_geneve(struct rte_udp_hdr *udp_hdr,
> +	    struct testpmd_offload_info *info) {
> +	struct rte_ether_hdr *eth_hdr;
> +	struct rte_ipv4_hdr *ipv4_hdr;
> +	struct rte_ipv6_hdr *ipv6_hdr;
> +	struct rte_geneve_hdr *geneve_hdr;
> +	uint16_t geneve_len;
> +
> +	/* Check udp destination port. */
> +	if (udp_hdr->dst_port != _htons(geneve_udp_port))
> +		return;
> +
> +	geneve_hdr = (struct rte_geneve_hdr *)((char *)udp_hdr +
> +				sizeof(struct rte_udp_hdr));
> +	geneve_len = sizeof(struct rte_geneve_hdr) + geneve_hdr->opt_len *
> 4;
> +	if (!geneve_hdr->proto || geneve_hdr->proto ==
> +	    _htons(RTE_GENEVE_TYPE_IPV4)) {
> +		update_tunnel_outer(info);
> +		ipv4_hdr = (struct rte_ipv4_hdr *)((char *)geneve_hdr +
> +			   geneve_len);
> +		parse_ipv4(ipv4_hdr, info);
> +		info->ethertype = _htons(RTE_ETHER_TYPE_IPV4);
> +		info->l2_len = 0;
> +	} else if (geneve_hdr->proto == _htons(RTE_GENEVE_TYPE_IPV6)) {
> +		update_tunnel_outer(info);
> +		ipv6_hdr = (struct rte_ipv6_hdr *)((char *)geneve_hdr +
> +			   geneve_len);
> +		info->ethertype = _htons(RTE_ETHER_TYPE_IPV6);
> +		parse_ipv6(ipv6_hdr, info);
> +		info->l2_len = 0;
> +
> +	} else if (geneve_hdr->proto == _htons(RTE_GENEVE_TYPE_ETH)) {
> +		update_tunnel_outer(info);
> +		eth_hdr = (struct rte_ether_hdr *)((char *)geneve_hdr +
> +			  geneve_len);
> +		parse_ethernet(eth_hdr, info);
> +	} else
> +		return;
> +
> +	info->l2_len +=
> +		(sizeof(struct rte_udp_hdr) + sizeof(struct rte_geneve_hdr) +
> +		((struct rte_geneve_hdr *)geneve_hdr)->opt_len * 4); }
> +
>  /* Parse a gre header */
>  static void
>  parse_gre(struct simple_gre_hdr *gre_hdr, struct testpmd_offload_info
> *info) @@ -865,9 +925,17 @@ pkt_burst_checksum_forward(struct
> fwd_stream *fs)
>  				}
>  				parse_vxlan(udp_hdr, &info,
>  					    m->packet_type);
> -				if (info.is_tunnel)
> +				if (info.is_tunnel) {
>  					tx_ol_flags |=
>  						PKT_TX_TUNNEL_VXLAN;
> +					goto tunnel_update;
> +				}
> +				parse_geneve(udp_hdr, &info);
> +				if (info.is_tunnel) {
> +					tx_ol_flags |=
> +						PKT_TX_TUNNEL_GENEVE;
> +					goto tunnel_update;
> +				}
>  			} else if (info.l4_proto == IPPROTO_GRE) {
>  				struct simple_gre_hdr *gre_hdr;
> 
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
> a8ae5cc..f405d4e 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -452,6 +452,7 @@ extern struct fwd_lcore  **fwd_lcores;  extern struct
> fwd_stream **fwd_streams;
> 
>  extern uint16_t vxlan_gpe_udp_port; /**< UDP port of tunnel VXLAN-GPE.
> */
> +extern uint16_t geneve_udp_port; /**< UDP port of tunnel GENEVE. */
> 
>  extern portid_t nb_peer_eth_addrs; /**< Number of peer ethernet
> addresses. */  extern struct rte_ether_addr
> peer_eth_addrs[RTE_MAX_ETHPORTS]; diff --git
> a/lib/librte_net/meson.build b/lib/librte_net/meson.build index
> 24ed825..52d3a97 100644
> --- a/lib/librte_net/meson.build
> +++ b/lib/librte_net/meson.build
> @@ -16,7 +16,8 @@ headers = files('rte_ip.h',
>  	'rte_net_crc.h',
>  	'rte_mpls.h',
>  	'rte_higig.h',
> -	'rte_ecpri.h')
> +	'rte_ecpri.h',
> +	'rte_geneve.h')
> 
>  sources = files('rte_arp.c', 'rte_ether.c', 'rte_net.c', 'rte_net_crc.c')  deps +=
> ['mbuf'] diff --git a/lib/librte_net/rte_geneve.h b/lib/librte_net/rte_geneve.h
> new file mode 100644 index 0000000..d50f5b3
> --- /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 <stdint.h>
> +
> +#include <rte_udp.h>
> +
> +
> +#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).  */
> +	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). */
> +#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). */
> +	uint8_t opts[];		/**<Variable length options. */
> +} __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. */
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* RTE_GENEVE_H_ */
> --
> 2.8.4
  
Olivier Matz Sept. 17, 2020, 12:23 p.m. UTC | #2
Hi Ophir,

Please find some comments below.

On Tue, Sep 15, 2020 at 01:17:15PM +0000, Ophir Munk wrote:
> From: Ophir Munk <ophirmu@mellanox.com>
> 
> 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 <ophirmu@mellanox.com>


>  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 <stdint.h>
> +
> +#include <rte_udp.h>

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[];		/**<Variable length options. */

Since the option length is a multiple of four-bytes, would uint32_t[]
make more sense here?

Missing a space after "<".

> +} __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
  
Ophir Munk Sept. 18, 2020, 2:21 p.m. UTC | #3
Hi Olivier,
Please find comments inline.
I sent v5 based on your review.
http://patches.dpdk.org/project/dpdk/list/?series=12354

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Thursday, September 17, 2020 3:23 PM
> To: Ophir Munk <ophirmu@nvidia.com>
> Cc: dev@dpdk.org; Wenzhuo Lu <wenzhuo.lu@intel.com>; Beilei Xing
> <beilei.xing@intel.com>; Bernard Iremonger
> <bernard.iremonger@intel.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
> Ophir Munk <ophirmu@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/3] app/testpmd: add GENEVE parsing
> 
> Hi Ophir,
> 
> Please find some comments below.
> 
> On Tue, Sep 15, 2020 at 01:17:15PM +0000, Ophir Munk wrote:
> > From: Ophir Munk <ophirmu@mellanox.com>
> >
> > 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 <ophirmu@mellanox.com>
> 
> 
> >  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.
> 

Thanks for sending a patch. I would appreciate it if it includes Geneve as well.

> > --- /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 <stdint.h>
> > +
> > +#include <rte_udp.h>
> 
> Is this include needed? Maybe it comes from a copy/paste of VXLAN?
> 

The include was dropped in v5 (not needed).

> > +
> > +
> > +#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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdoc.
> dpdk.org%2Fapi%2Fstructrte__flow__attr.html%23ae4d19341d5298a2bc61f
> 9eb941b1179c&amp;data=02%7C01%7Cophirmu%40nvidia.com%7C95918f4
> 2491c4832d8a308d85b047a22%7C43083d15727340c1b7db39efd9ccc17a%7
> C0%7C0%7C637359422086641153&amp;sdata=LJjYkeHy9eHGc9xu42E%2F8
> h54dzxrmiO7V0NCeLXndpU%3D&amp;reserved=0
> 
> 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)".
> 

The number of bits is removed from the comments (v5).

> By the way, there are 2 spaces at the end of the comment.
> 

Extra spaces removed.

> > +	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"?
> 

In v5 all "rsvd" fields are renamed as "reserved".

> 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
> 

I suggest leaving the vni as is (array of unsigned chars) since I do not see a gain by changing it to an integer of 24 bits.
If we did change - it will not turn VNI from network order to host order. So in case you wanted to print the VNI (host order) you will have to manipulate the bytes order anyway based on endianness. 
What do you think?

> > +	uint8_t opts[];		/**<Variable length options. */
> 
> Since the option length is a multiple of four-bytes, would uint32_t[] make
> more sense here?
> 

opts[] type changed to uint32_t  in v5.

> Missing a space after "<".
> 

Space added.

> > +} __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.
> 
> 

Definitions RTE_GENEVE_TYPE_IPV4 and RTE_GENEVE_TYPE_IPV6 dropped (remaining with RTE_GENEVE_TYPE_ETH only).

> Regards,
> Olivier

Regards,
Ophir
  

Patch

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 7ece398..eb87b9b 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -43,6 +43,7 @@ 
 #include <rte_flow.h>
 #include <rte_gro.h>
 #include <rte_gso.h>
+#include <rte_geneve.h>
 
 #include "testpmd.h"
 
@@ -63,6 +64,7 @@ 
 #endif
 
 uint16_t vxlan_gpe_udp_port = 4790;
+uint16_t geneve_udp_port = RTE_GENEVE_DEFAULT_PORT;
 
 /* structure that caches offload info for the current packet */
 struct testpmd_offload_info {
@@ -333,6 +335,64 @@  parse_vxlan_gpe(struct rte_udp_hdr *udp_hdr,
 	info->l2_len += RTE_ETHER_VXLAN_GPE_HLEN;
 }
 
+/* Fill in outer layers length */
+static void
+update_tunnel_outer(struct testpmd_offload_info *info)
+{
+	info->is_tunnel = 1;
+	info->outer_ethertype = info->ethertype;
+	info->outer_l2_len = info->l2_len;
+	info->outer_l3_len = info->l3_len;
+	info->outer_l4_proto = info->l4_proto;
+}
+
+/* Parse a geneve header */
+static void
+parse_geneve(struct rte_udp_hdr *udp_hdr,
+	    struct testpmd_offload_info *info)
+{
+	struct rte_ether_hdr *eth_hdr;
+	struct rte_ipv4_hdr *ipv4_hdr;
+	struct rte_ipv6_hdr *ipv6_hdr;
+	struct rte_geneve_hdr *geneve_hdr;
+	uint16_t geneve_len;
+
+	/* Check udp destination port. */
+	if (udp_hdr->dst_port != _htons(geneve_udp_port))
+		return;
+
+	geneve_hdr = (struct rte_geneve_hdr *)((char *)udp_hdr +
+				sizeof(struct rte_udp_hdr));
+	geneve_len = sizeof(struct rte_geneve_hdr) + geneve_hdr->opt_len * 4;
+	if (!geneve_hdr->proto || geneve_hdr->proto ==
+	    _htons(RTE_GENEVE_TYPE_IPV4)) {
+		update_tunnel_outer(info);
+		ipv4_hdr = (struct rte_ipv4_hdr *)((char *)geneve_hdr +
+			   geneve_len);
+		parse_ipv4(ipv4_hdr, info);
+		info->ethertype = _htons(RTE_ETHER_TYPE_IPV4);
+		info->l2_len = 0;
+	} else if (geneve_hdr->proto == _htons(RTE_GENEVE_TYPE_IPV6)) {
+		update_tunnel_outer(info);
+		ipv6_hdr = (struct rte_ipv6_hdr *)((char *)geneve_hdr +
+			   geneve_len);
+		info->ethertype = _htons(RTE_ETHER_TYPE_IPV6);
+		parse_ipv6(ipv6_hdr, info);
+		info->l2_len = 0;
+
+	} else if (geneve_hdr->proto == _htons(RTE_GENEVE_TYPE_ETH)) {
+		update_tunnel_outer(info);
+		eth_hdr = (struct rte_ether_hdr *)((char *)geneve_hdr +
+			  geneve_len);
+		parse_ethernet(eth_hdr, info);
+	} else
+		return;
+
+	info->l2_len +=
+		(sizeof(struct rte_udp_hdr) + sizeof(struct rte_geneve_hdr) +
+		((struct rte_geneve_hdr *)geneve_hdr)->opt_len * 4);
+}
+
 /* Parse a gre header */
 static void
 parse_gre(struct simple_gre_hdr *gre_hdr, struct testpmd_offload_info *info)
@@ -865,9 +925,17 @@  pkt_burst_checksum_forward(struct fwd_stream *fs)
 				}
 				parse_vxlan(udp_hdr, &info,
 					    m->packet_type);
-				if (info.is_tunnel)
+				if (info.is_tunnel) {
 					tx_ol_flags |=
 						PKT_TX_TUNNEL_VXLAN;
+					goto tunnel_update;
+				}
+				parse_geneve(udp_hdr, &info);
+				if (info.is_tunnel) {
+					tx_ol_flags |=
+						PKT_TX_TUNNEL_GENEVE;
+					goto tunnel_update;
+				}
 			} else if (info.l4_proto == IPPROTO_GRE) {
 				struct simple_gre_hdr *gre_hdr;
 
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index a8ae5cc..f405d4e 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -452,6 +452,7 @@  extern struct fwd_lcore  **fwd_lcores;
 extern struct fwd_stream **fwd_streams;
 
 extern uint16_t vxlan_gpe_udp_port; /**< UDP port of tunnel VXLAN-GPE. */
+extern uint16_t geneve_udp_port; /**< UDP port of tunnel GENEVE. */
 
 extern portid_t nb_peer_eth_addrs; /**< Number of peer ethernet addresses. */
 extern struct rte_ether_addr peer_eth_addrs[RTE_MAX_ETHPORTS];
diff --git a/lib/librte_net/meson.build b/lib/librte_net/meson.build
index 24ed825..52d3a97 100644
--- a/lib/librte_net/meson.build
+++ b/lib/librte_net/meson.build
@@ -16,7 +16,8 @@  headers = files('rte_ip.h',
 	'rte_net_crc.h',
 	'rte_mpls.h',
 	'rte_higig.h',
-	'rte_ecpri.h')
+	'rte_ecpri.h',
+	'rte_geneve.h')
 
 sources = files('rte_arp.c', 'rte_ether.c', 'rte_net.c', 'rte_net_crc.c')
 deps += ['mbuf']
diff --git a/lib/librte_net/rte_geneve.h b/lib/librte_net/rte_geneve.h
new file mode 100644
index 0000000..d50f5b3
--- /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 <stdint.h>
+
+#include <rte_udp.h>
+
+
+#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).  */
+	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). */
+#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). */
+	uint8_t opts[];		/**<Variable length options. */
+} __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. */
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* RTE_GENEVE_H_ */