Re: [PATCH 13/20] testpmd: support gre tunnels in csum fwd engine

[Olivier MATZ <olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

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.

Regards,
Olivier