From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 972CC1CE3F for ; Fri, 6 Apr 2018 06:59:34 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Apr 2018 21:59:33 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,414,1517904000"; d="scan'208";a="45386868" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga001.jf.intel.com with ESMTP; 05 Apr 2018 21:59:32 -0700 Received: from fmsmsx122.amr.corp.intel.com (10.18.125.37) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 5 Apr 2018 21:59:31 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx122.amr.corp.intel.com (10.18.125.37) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 5 Apr 2018 21:59:31 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.241]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.88]) with mapi id 14.03.0319.002; Fri, 6 Apr 2018 12:59:28 +0800 From: "Zhang, Qi Z" To: Adrien Mazarguil CC: Thomas Monjalon , "Yigit, Ferruh" , "dev@dpdk.org" , "Lu, Wenzhuo" , "Wu, Jingjing" , Ajit Khaparde , Somnath Kotur , John Daley , Hyong Youb Kim , "Xing, Beilei" , "Ananyev, Konstantin" , Nelio Laranjeiro , Yongseok Koh , Jacek Siuda , Tomasz Duszynski , Dmitri Epshtein , Natalie Samsonov , Jianbo Liu , Andrew Rybchenko , "Pascal Mazon" Thread-Topic: [PATCH v1 11/16] ethdev: refine TPID handling in flow API Thread-Index: AQHTzC2oRN3WVY5R1UaFN1W2wZgCR6PyM9Dg//+FewCAAXSPgA== Date: Fri, 6 Apr 2018 04:59:06 +0000 Message-ID: <039ED4275CED7440929022BC67E7061153183322@SHSMSX103.ccr.corp.intel.com> References: <20180404150312.12304-1-adrien.mazarguil@6wind.com> <20180404150312.12304-12-adrien.mazarguil@6wind.com> <039ED4275CED7440929022BC67E7061153182FE4@SHSMSX103.ccr.corp.intel.com> <20180405143920.GM4957@6wind.com> In-Reply-To: <20180405143920.GM4957@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v1 11/16] ethdev: refine TPID handling in flow API 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: , X-List-Received-Date: Fri, 06 Apr 2018 04:59:35 -0000 > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > Sent: Thursday, April 5, 2018 10:39 PM > To: Zhang, Qi Z > Cc: Thomas Monjalon ; Yigit, Ferruh > ; dev@dpdk.org; Lu, Wenzhuo > ; Wu, Jingjing ; Ajit Khapar= de > ; Somnath Kotur > ; John Daley ; Hyong > Youb Kim ; Xing, Beilei ; Anany= ev, > Konstantin ; Nelio Laranjeiro > ; Yongseok Koh ; Jacek > Siuda ; Tomasz Duszynski ; Dmitri > Epshtein ; Natalie Samsonov ; > Jianbo Liu ; Andrew Rybchenko > ; Pascal Mazon > Subject: Re: [PATCH v1 11/16] ethdev: refine TPID handling in flow API >=20 > On Thu, Apr 05, 2018 at 02:20:34PM +0000, Zhang, Qi Z wrote: > > Hi Adrien: > > > > > Hi PMD maintainers, while I'm pretty confident in these changes, I > > > could not validate them with all devices. > > > > > > It would be great if you could apply this patch, run testpmd, create > > > VLAN flow rules with/without inner EtherType as described and send > > > matching traffic while making sure nothing was broken in the process. > > > > > > Thanks! > > > --- > > > diff --git a/drivers/net/i40e/i40e_flow.c > > > b/drivers/net/i40e/i40e_flow.c index 1b336df74..c6dd889ad 100644 > > > --- a/drivers/net/i40e/i40e_flow.c > > > +++ b/drivers/net/i40e/i40e_flow.c > > > @@ -2490,24 +2490,36 @@ i40e_flow_parse_fdir_pattern(struct > > > rte_eth_dev *dev, > > > "Invalid MAC_addr mask."); > > > return -rte_errno; > > > } > > > + } > > > + if (eth_spec && eth_mask && eth_mask->type) { > > > + enum rte_flow_item_type next =3D (item + 1)->type; > > > > > > - if ((eth_mask->type & UINT16_MAX) =3D=3D > > > - UINT16_MAX) { > > > - input_set |=3D I40E_INSET_LAST_ETHER_TYPE; > > > - filter->input.flow.l2_flow.ether_type =3D > > > - eth_spec->type; > > > + if (eth_mask->type !=3D RTE_BE16(0xffff)) { > > > + rte_flow_error_set(error, EINVAL, > > > + RTE_FLOW_ERROR_TYPE_ITEM, > > > + item, > > > + "Invalid type mask."); > > > + return -rte_errno; > > > } > > > > > > ether_type =3D rte_be_to_cpu_16(eth_spec->type); > > > - if (ether_type =3D=3D ETHER_TYPE_IPv4 || > > > - ether_type =3D=3D ETHER_TYPE_IPv6 || > > > - ether_type =3D=3D ETHER_TYPE_ARP || > > > - ether_type =3D=3D outer_tpid) { > > > + > > > + if ((next =3D=3D RTE_FLOW_ITEM_TYPE_VLAN && > > > + ether_type !=3D outer_tpid) || > > > + (next !=3D RTE_FLOW_ITEM_TYPE_VLAN && > > > + (ether_type =3D=3D ETHER_TYPE_IPv4 || > > > + ether_type =3D=3D ETHER_TYPE_IPv6 || > > > + ether_type =3D=3D ETHER_TYPE_ARP || > > > + ether_type =3D=3D outer_tpid))) { > > > rte_flow_error_set(error, EINVAL, > > > RTE_FLOW_ERROR_TYPE_ITEM, > > > item, > > > "Unsupported ether_type."); > > > return -rte_errno; > > > + } else if (next !=3D RTE_FLOW_ITEM_TYPE_VLAN) { > > > + input_set |=3D I40E_INSET_LAST_ETHER_TYPE; > > > + filter->input.flow.l2_flow.ether_type =3D > > > + eth_spec->type; > > > } > > > } > > > > > > @@ -2518,6 +2530,14 @@ i40e_flow_parse_fdir_pattern(struct > > > rte_eth_dev *dev, > > > case RTE_FLOW_ITEM_TYPE_VLAN: > > > vlan_spec =3D item->spec; > > > vlan_mask =3D item->mask; > > > + > > > + if (input_set & I40E_INSET_LAST_ETHER_TYPE) { > > > + rte_flow_error_set(error, EINVAL, > > > + RTE_FLOW_ERROR_TYPE_ITEM, > > > + item, > > > + "Unsupported outer TPID."); > > > + return -rte_errno; > > > + } > > > > Please correct me I'm wrong, now I40E_INSET_LAST_ETHER_TYPE will only > > be set when next !=3D RTE_FLOW_ITEM_TYPE_VLAN while, > RTE_FLOW_ITEM_TYPE_VLAN will only be the next to > RTE_FLOW_ITEM_TYPE_ETH for fdir, so above check seems unnecessary ? >=20 > You're right, it's unnecessary. I can remove this change or leave it as a= safety > measure for future contributions, since when parsing a VLAN item one is n= ot > necessarily aware of what happened in previous iterations. >=20 > How about an assertion check for debugging purposes instead? >=20 > RTE_ASSERT(!(input_set & I40E_INSET_LAST_ETHER_TYPE)); Agree to use assert. >=20 > -- > Adrien Mazarguil > 6WIND