From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; 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" <jia.guo@intel.com>
To: "Li, Xiaoyun" <xiaoyun.li@intel.com>, "orika@nvidia.com"
<orika@nvidia.com>, "Zhang, Qi Z" <qi.z.zhang@intel.com>, "Xing, Beilei"
<beilei.xing@intel.com>, "Wu, Jingjing" <jingjing.wu@intel.com>
CC: "Zhang, Yuying" <yuying.zhang@intel.com>, "Xu, Ting" <ting.xu@intel.com>,
"dev@dpdk.org" <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: <dabc778d1a104a9ab708d7e933f9a97c@intel.com>
References: <20210317031211.18145-1-jia.guo@intel.com>
<20210317031211.18145-5-jia.guo@intel.com>
<DM4PR11MB55345C5CF290C172BF64972599689@DM4PR11MB5534.namprd11.prod.outlook.com>
In-Reply-To: <DM4PR11MB55345C5CF290C172BF64972599689@DM4PR11MB5534.namprd11.prod.outlook.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
Hi, xiaoyun
> -----Original Message-----
> From: Li, Xiaoyun <xiaoyun.li@intel.com>
> Sent: Friday, March 19, 2021 3:57 PM
> To: Guo, Jia <jia.guo@intel.com>; orika@nvidia.com; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjin=
g
> <jingjing.wu@intel.com>
> Cc: Zhang, Yuying <yuying.zhang@intel.com>; Xu, Ting <ting.xu@intel.com>;
> 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 <jia.guo@intel.com>
> > Sent: Wednesday, March 17, 2021 11:12
> > To: orika@nvidia.com; Zhang, Qi Z <qi.z.zhang@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>; Wu,
> > Jingjing <jingjing.wu@intel.com>
> > Cc: Zhang, Yuying <yuying.zhang@intel.com>; Xu, Ting
> > <ting.xu@intel.com>; dev@dpdk.org; Guo, Jia <jia.guo@intel.com>
> > 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 <ting.xu@intel.com>
> > Signed-off-by: Jeff Guo <jia.guo@intel.com>
> > ---
> > 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 @@
> <snip>
> >
> > 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;
> >
> <snip>
> > +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.
> <snip>
> > +
> > +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
> <snip>
> > --
> > 2.20.1
>=20