[dpdk-dev] [PATCH 13/20] testpmd: support gre tunnels in csum fwd engine

[Liu, Jijiang]

Hi Olivier,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz at 6wind.com]
> Sent: Monday, February 2, 2015 8:55 PM
> To: Liu, Jijiang
> Cc: Ananyev, Konstantin; Zhang, Helin; dev at dpdk.org
> Subject: Re: [PATCH 13/20] testpmd: support gre tunnels in csum fwd engine
> 
> Hi Jijiang,
> 
> On 02/02/2015 04:04 AM, Liu, Jijiang wrote:
> >> +/* simplified GRE header (flags must be 0) */ struct simple_gre_hdr {
> >> +	uint16_t flags;
> >> +	uint16_t proto;
> >> +};
> > I suggest you to remove the comment ' flags must be 0'，the reason is that we
> just use the header structure to check what the protocol is.
> > It is not necessary to require the flag must be 0.
> 
> The signification of this comment is that the only supported header in the
> parse_gre() function is the header without optional fields (checksum, offset,
> key, ...). These fields are not present when flags is set to 0.
> 
> >
> >>  static uint16_t
> >>  get_psd_sum(void *l3_hdr, uint16_t ethertype, uint64_t ol_flags)  {
> >> @@ -
> >> 218,6 +224,60 @@ parse_vxlan(struct udp_hdr *udp_hdr, struct
> >> testpmd_offload_info *info,
> >>  	info->l2_len += ETHER_VXLAN_HLEN; /* add udp + vxlan */  }
> >>
> >> +/* Parse a gre header */
> >> +static void
> >> +parse_gre(struct simple_gre_hdr *gre_hdr, struct
> >> +testpmd_offload_info
> >> +*info) {
> >> +	struct ether_hdr *eth_hdr;
> >> +	struct ipv4_hdr *ipv4_hdr;
> >> +	struct ipv6_hdr *ipv6_hdr;
> >> +
> >> +	/* if flags != 0; it's not supported */
> >> +	if (gre_hdr->flags != 0)
> >> +		return;
> > I suggest you remove the check here, you can add some comments in front of
> this function to explain that actual NVGRE and IP over GRE is not supported now.
> >
> > For example, when I want to support NVGRE TX checksum offload, I will do the
> following change.
> >
> > Or you still keep it here, but anyway, I will change it later.
> >
> >
> >> +
> >> +	if (gre_hdr->proto == _htons(ETHER_TYPE_IPv4)) {
> >> +		info->is_tunnel = 1;
> >> +		info->outer_ethertype = info->ethertype;
> >> +		info->outer_l2_len = info->l2_len;
> >> +		info->outer_l3_len = info->l3_len;
> >> +
> >> +		ipv4_hdr = (struct ipv4_hdr *)((char *)gre_hdr +
> >> +			sizeof(struct simple_gre_hdr));
> >> +
> >> +		parse_ipv4(ipv4_hdr, info);
> >> +		info->ethertype = _htons(ETHER_TYPE_IPv4);
> >> +		info->l2_len = 0;
> >> +
> >> +	} else if (gre_hdr->proto == _htons(ETHER_TYPE_IPv6)) {
> >> +		info->is_tunnel = 1;
> >> +		info->outer_ethertype = info->ethertype;
> >> +		info->outer_l2_len = info->l2_len;
> >> +		info->outer_l3_len = info->l3_len;
> >> +
> >> +		ipv6_hdr = (struct ipv6_hdr *)((char *)gre_hdr +
> >> +			sizeof(struct simple_gre_hdr));
> >> +
> >> +		info->ethertype = _htons(ETHER_TYPE_IPv6);
> >> +		parse_ipv6(ipv6_hdr, info);
> >> +		info->l2_len = 0;
> >> +
> >> +	} else if (gre_hdr->proto == _htons(0x6558)) { /* ETH_P_TEB in
> >> +linux
> >> */
> >> +		info->is_tunnel = 1;
> >> +		info->outer_ethertype = info->ethertype;
> >> +		info->outer_l2_len = info->l2_len;
> >> +		info->outer_l3_len = info->l3_len;
> >> +
> >> +		eth_hdr = (struct ether_hdr *)((char *)gre_hdr +
> >> +			sizeof(struct simple_gre_hdr));
> >
> > For NVGRE:
> > I will do some change here.
> > eth_hdr = (struct ether_hdr *)((char *)gre_hdr +
> > 		sizeof(struct nvgre_hdr)); // replace  simple_gre_hdr with
> nvgre_hdr.
> >
> 
> I think replacing gre_hdr by nvgre_hdr here would break the gre.
> If we want to support NVGRE, I think we should check the value of gre_hdr-
> >flags to determine the length of the gre header.
> 
> I'm not a NVGRE expert, but from what I've seen NVGRE is just a GRE header
> with the key flag present.

 NVGRE frame format in section 3.2
http://datatracker.ietf.org/doc/draft-sridharan-virtualization-nvgre/?include_text=1

When gre_hdr->proto = htons (0x6558),  which means this is NVGRE protocol( MAC over GRE), the key flag must be present, so gre_hdr->flags != 0.

Anyway,  the following check should be removed when we support NVGRE TX checksum offload later, or it will failed.

	/* if flags != 0; it's not supported */
              if (gre_hdr->flags != 0)
		return;
> Regards,
> Olivier