From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier MATZ Subject: Re: [PATCH 13/20] testpmd: support gre tunnels in csum fwd engine Date: Mon, 02 Feb 2015 13:55:19 +0100 Message-ID: <54CF73B7.1020104@6wind.com> References: <1421883395-27235-1-git-send-email-olivier.matz@6wind.com> <1422623775-8050-1-git-send-email-olivier.matz@6wind.com> <1422623775-8050-14-git-send-email-olivier.matz@6wind.com> <1ED644BD7E0A5F4091CF203DAFB8E4CC01DC9728@SHSMSX101.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-2022-jp Content-Transfer-Encoding: 7bit Cc: "dev-VfR2kkLFssw@public.gmane.org" To: "Liu, Jijiang" Return-path: In-Reply-To: <1ED644BD7E0A5F4091CF203DAFB8E4CC01DC9728-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" 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