From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4C04BA034F; Tue, 23 Mar 2021 01:58:26 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id ADE224067E; Tue, 23 Mar 2021 01:58:25 +0100 (CET) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mails.dpdk.org (Postfix) with ESMTP id 7B7804014D for ; Tue, 23 Mar 2021 01:58:23 +0100 (CET) IronPort-SDR: xoYMhEBww/SvaIq0TrYUNWTMnxnrup2c0gxhPbfSusnFP8DfisX4wpT2sKXzabWqZUIOUtW+gm RPjared5f3Xg== X-IronPort-AV: E=McAfee;i="6000,8403,9931"; a="177947311" X-IronPort-AV: E=Sophos;i="5.81,269,1610438400"; d="scan'208";a="177947311" Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Mar 2021 17:58:22 -0700 IronPort-SDR: 2XIoJlT+mnxtqN4RsAccdJ0J3SzNPWcZEOpuGXQuE3H4PZ4H3kTAJlWUmJPtWN2wYHUPczmFuo ZMeDKculjT9g== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.81,269,1610438400"; d="scan'208";a="375871785" Received: from fmsmsx604.amr.corp.intel.com ([10.18.126.84]) by orsmga006.jf.intel.com with ESMTP; 22 Mar 2021 17:58:22 -0700 Received: from shsmsx603.ccr.corp.intel.com (10.109.6.143) by fmsmsx604.amr.corp.intel.com (10.18.126.84) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2106.2; Mon, 22 Mar 2021 17:58:21 -0700 Received: from shsmsx601.ccr.corp.intel.com (10.109.6.141) by SHSMSX603.ccr.corp.intel.com (10.109.6.143) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2106.2; Tue, 23 Mar 2021 08:58:19 +0800 Received: from shsmsx601.ccr.corp.intel.com ([10.109.6.141]) by SHSMSX601.ccr.corp.intel.com ([10.109.6.141]) with mapi id 15.01.2106.013; Tue, 23 Mar 2021 08:58:19 +0800 From: "Guo, Jia" To: "Li, Xiaoyun" , "orika@nvidia.com" , "Zhang, Qi Z" , "Xing, Beilei" , "Wu, Jingjing" CC: "Zhang, Yuying" , "Xu, Ting" , "dev@dpdk.org" Thread-Topic: [PATCH v1 4/4] net/iavf: support FDIR for IP fragment packet Thread-Index: AQHXGtxBCljNI/JQb0SK8qUvioq926qKbyIAgAZUwmA= Date: Tue, 23 Mar 2021 00:58:19 +0000 Message-ID: References: <20210317031211.18145-1-jia.guo@intel.com> <20210317031211.18145-5-jia.guo@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.5.1.3 x-originating-ip: [10.239.127.36] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v1 4/4] net/iavf: support FDIR for IP fragment packet X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi, xiaoyun > -----Original Message----- > From: Li, Xiaoyun > Sent: Friday, March 19, 2021 3:57 PM > To: Guo, Jia ; orika@nvidia.com; Zhang, Qi Z > ; Xing, Beilei ; Wu, Jingjin= g > > Cc: Zhang, Yuying ; Xu, Ting ; > dev@dpdk.org > Subject: RE: [PATCH v1 4/4] net/iavf: support FDIR for IP fragment packet >=20 > Hi >=20 > > -----Original Message----- > > From: Guo, Jia > > Sent: Wednesday, March 17, 2021 11:12 > > To: orika@nvidia.com; Zhang, Qi Z ; Xing, Beilei > > ; Li, Xiaoyun ; Wu, > > Jingjing > > Cc: Zhang, Yuying ; Xu, Ting > > ; dev@dpdk.org; Guo, Jia > > Subject: [PATCH v1 4/4] net/iavf: support FDIR for IP fragment packet > > > > New FDIR parsing are added to handle the fragmented IPv4/IPv6 packet. > > > > Signed-off-by: Ting Xu > > Signed-off-by: Jeff Guo > > --- > > drivers/net/iavf/iavf_fdir.c | 278 ++++++++++++++++++--------- > > drivers/net/iavf/iavf_generic_flow.h | 5 + > > 2 files changed, 190 insertions(+), 93 deletions(-) > > > > diff --git a/drivers/net/iavf/iavf_fdir.c > > b/drivers/net/iavf/iavf_fdir.c index > > e3f3b5f22a..348d423081 100644 > > --- a/drivers/net/iavf/iavf_fdir.c > > +++ b/drivers/net/iavf/iavf_fdir.c > > @@ -34,7 +34,7 @@ > > > > > uint8_t ipv6_addr_mask[16] =3D { > > 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, @@ -528,12 > > +534,6 @@ iavf_fdir_parse_pattern(__rte_unused struct iavf_adapter > > +*ad, > > }; > > > > for (item =3D pattern; item->type !=3D RTE_FLOW_ITEM_TYPE_END; item++)= { > > -if (item->last) { >=20 > You can't directly remove the check. If users use "last" for items like > RTE_FLOW_ITEM_TYPE_ETH. The driver doesn't support that. >=20 Oh, that is like as you said. Specific enabling should be addressed. > > -rte_flow_error_set(error, EINVAL, > > -RTE_FLOW_ERROR_TYPE_ITEM, item, > > -"Not support range"); > > -} > > - > > item_type =3D item->type; > > > > > +if (ipv4_mask->hdr.version_ihl || > > + ipv4_mask->hdr.total_length || > > + ipv4_mask->hdr.hdr_checksum) { > > +rte_flow_error_set(error, EINVAL, > > + > > RTE_FLOW_ERROR_TYPE_ITEM, > > + item, "Invalid IPv4 mask."); > > +return -rte_errno; > > +} > > > > -rte_memcpy(hdr->buffer, > > -&ipv4_spec->hdr, > > -sizeof(ipv4_spec->hdr)); >=20 > The ipv4_mask is checked. But what about ipv4_last? > What if users set a rule which includes "last" for hdr.version_ihl? The c= ode > doesn't process that and not err return. > You should block all situations in ipv4_last except for ipv4_last- > >hdr.fragment_offset. >=20 Ok. > > > + > > +if (ipv4_mask->hdr.packet_id =3D=3D UINT16_MAX || > > + ipv4_mask->hdr.fragment_offset =3D=3D UINT16_MAX) { >=20 > And I don't understand your logic here. > Only if ipv4_mask->hdr.fragment_offset and ipv4_mask->hdr.packet_id Are > UINT16_MAX, you process this case. But what about other cases? Shouldn't > those cases return err like not supported? > And the mask for fragment_offset shouldn't be 0xffff (UINT16_MAX), it > should be 0x3fff (RTE_IPV4_HDR_OFFSET_MASK | > RTE_IPV4_HDR_MF_FLAG). Because only the last 14 bits matters. The other 2 > bits are reserved bit and DF bit, it doesn't matter if it's fragment pack= ets or > not. >=20 Em, there are definitely something not cover. The negotiate should be speci= fic at device but not at avf. > > +if (ipv4_last && > > + ipv4_spec->hdr.packet_id =3D=3D 0 && > > + ipv4_last->hdr.packet_id =3D=3D 0xffff) >=20 > And I don't understand this part too. I thought it's a fragment rule and = non- > fragment rule. Why is this related to packet_id? And what about > fragment_offset spec and last? > In ovs usercase, the rule for fragment packets is like flow create 0 ingr= ess > pattern eth / ipv4 src is xxx dst is xxx fragment_offset spec 0x8 > fragment_offset last 0x2000 fragment_offset mask 0x3fff / end actions > queue index 1 / end >=20 > And the rule for non-fragment rule is like: > flow create 0 ingress pattern eth / ipv4 src is xxx dst is xxx fragment_o= ffset > spec 0 fragment_offset mask 0x3fff / end actions queue index 1 / end or > flow create 0 ingress pattern eth / ipv4 src is xxx dst is xxx fragment_o= ffset > mask 0x3fff / end actions queue index 1 / end >=20 > How can your codes here make sure the rules behave correctly? >=20 What i want is that use a dummy input set for the fragment packet. At lease= , we have handle 5tuple/specific id/any packet id for Fragment packet, and = for one-fragment packet, seems that no need to use fragment_offset to identify it seem default is for ip other. =20 > > +spec_all_pid =3D true; > > + > > +/* All IPv4 fragment packet has the same > > + * ethertype, if the spec is for all invalid > > + * packet id, set the ethertype into input set. > > + */ > > +input_set |=3D spec_all_pid ? > > +IAVF_INSET_ETHERTYPE : > > +IAVF_INSET_IPV4_ID; > > + > > +if (spec_all_pid) > > + > > VIRTCHNL_ADD_PROTO_HDR_FIELD_BIT(hdr1, > > +ETH, ETHERTYPE); > > +else > > + > > VIRTCHNL_ADD_PROTO_HDR_FIELD_BIT(hdr, > > +IPV4, PKID); > > } > > > > +rte_memcpy(hdr->buffer, &ipv4_spec->hdr, > > + sizeof(ipv4_spec->hdr)); > > + > > hdrs.count =3D ++layer; > > break; > > >=20 > I'm not familiar with IPv6 fragment rule. So not review that part. >=20 > > > -- > > 2.20.1 >=20