From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 959E01396 for ; Fri, 30 Sep 2016 08:06:23 +0200 (CEST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga103.jf.intel.com with ESMTP; 29 Sep 2016 23:05:51 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,271,1473145200"; d="scan'208";a="174846994" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga004.fm.intel.com with ESMTP; 29 Sep 2016 23:05:51 -0700 Received: from fmsmsx101.amr.corp.intel.com (10.18.124.199) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 29 Sep 2016 23:05:51 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx101.amr.corp.intel.com (10.18.124.199) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 29 Sep 2016 23:05:51 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.234]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.118]) with mapi id 14.03.0248.002; Fri, 30 Sep 2016 14:05:49 +0800 From: "Wu, Jingjing" To: "Yigit, Ferruh" , "Guo, Jia" , "Zhang, Helin" CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] drivers/i40e: fix the hash filter invalid calculation in X722 Thread-Index: AQHSF+NrAc2Z9HtTAkqF3lsqEeEZ5aCQRRiAgAFK14A= Date: Fri, 30 Sep 2016 06:05:48 +0000 Message-ID: <9BB6961774997848B5B42BEC655768F80E274D11@SHSMSX103.ccr.corp.intel.com> References: <1474887098-115474-1-git-send-email-jia.guo@intel.com> In-Reply-To: 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] drivers/i40e: fix the hash filter invalid calculation in X722 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 Sep 2016 06:06:24 -0000 > -----Original Message----- > From: Yigit, Ferruh > Sent: Friday, September 30, 2016 2:16 AM > To: Guo, Jia; Zhang, Helin; Wu, Jingjing > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] drivers/i40e: fix the hash filter invalid > calculation in X722 >=20 > On 9/26/2016 11:51 AM, Jeff Guo wrote: > > As X722 extracts IPv4 header to Field Vector different with > > XL710/X710, need to corresponding to modify the fields of IPv4 header > > in input set to map different default Field Vector Table of different n= ics. > > > > Signed-off-by: Jeff Guo > > --- > > drivers/net/i40e/i40e_ethdev.c | 77 > > +++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 73 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c > > b/drivers/net/i40e/i40e_ethdev.c index b04c833..9b4c71f 100644 > > --- a/drivers/net/i40e/i40e_ethdev.c > > +++ b/drivers/net/i40e/i40e_ethdev.c > > @@ -211,6 +211,18 @@ > > #define I40E_REG_INSET_L3_SRC_IP4 0x0001800000000000ULL > > /* Destination IPv4 address */ > > #define I40E_REG_INSET_L3_DST_IP4 0x0000001800000000ULL > > +#ifdef X722_SUPPORT > > +/* Source IPv4 address for X722 */ > > +#define I40E_X722_REG_INSET_L3_SRC_IP4 0x0006000000000000ULL >=20 > These macros defined here within "#ifdef X722_SUPPORT" and later used > unconditionally, this will cause a compile error when "X722_SUPPORT" not > defined. These macros defined are used in condition later. Maybe the compile error Already exist in current driver. We need to check that and fix this.=20 >=20 > > +/* Destination IPv4 address for X722 */ > > +#define I40E_X722_REG_INSET_L3_DST_IP4 0x0000060000000000ULL > > +/* IPv4 Type of Service (TOS) */ > > +#define I40E_X722_REG_INSET_L3_IP4_TOS 0x0040000000000000ULL >=20 > This value seems same as I40E_REG_INSET_L3_IP4_TOS, why creating a X722 > version of this? >=20 > > +/* IPv4 Protocol */ > > +#define I40E_X722_REG_INSET_L3_IP4_PROTO > 0x0010000000000000ULL > > +/* IPv4 Time to Live */ > > +#define I40E_X722_REG_INSET_L3_IP4_TTL 0x0010000000000000ULL > > +#endif > > /* IPv4 Type of Service (TOS) */ > > #define I40E_REG_INSET_L3_IP4_TOS 0x0040000000000000ULL > > /* IPv4 Protocol */ > > @@ -7372,7 +7384,7 @@ i40e_parse_input_set(uint64_t *inset, > > * and vice versa > > */ > > static uint64_t > > -i40e_translate_input_set_reg(uint64_t input) > > +i40e_translate_input_set_reg(enum i40e_mac_type type, uint64_t input) > > { > > uint64_t val =3D 0; > > uint16_t i; > > @@ -7419,14 +7431,70 @@ i40e_translate_input_set_reg(uint64_t input) > > {I40E_INSET_FLEX_PAYLOAD_W8, > I40E_REG_INSET_FLEX_PAYLOAD_WORD8}, > > }; > > > > + static const struct { > > + uint64_t inset; > > + uint64_t inset_reg; >=20 > Since creating second instance of this struct, why not extract strcut > declaration? >=20 > > + } inset_map1[] =3D { >=20 > Is it possible to use more descriptive variable name, hard to distinguish= diff > between inset_map and inset_map1. >=20 > > + {I40E_INSET_DMAC, I40E_REG_INSET_L2_DMAC}, > > + {I40E_INSET_SMAC, I40E_REG_INSET_L2_SMAC}, > > + {I40E_INSET_VLAN_OUTER, > I40E_REG_INSET_L2_OUTER_VLAN}, > > + {I40E_INSET_VLAN_INNER, > I40E_REG_INSET_L2_INNER_VLAN}, > > + {I40E_INSET_LAST_ETHER_TYPE, > I40E_REG_INSET_LAST_ETHER_TYPE}, >=20 > ----> > > + {I40E_INSET_IPV4_SRC, I40E_X722_REG_INSET_L3_SRC_IP4}, > > + {I40E_INSET_IPV4_DST, I40E_X722_REG_INSET_L3_DST_IP4}, > > + {I40E_INSET_IPV4_TOS, I40E_X722_REG_INSET_L3_IP4_TOS}, > > + {I40E_INSET_IPV4_PROTO, > I40E_X722_REG_INSET_L3_IP4_PROTO}, > > + {I40E_INSET_IPV4_TTL, I40E_X722_REG_INSET_L3_IP4_TTL}, > <---- > Since limited number of values are different from inset_map[], and most o= f > them are duplication, is it possible to prevent duplication? Didn't find and proposal on that. Because we need to support I40E_X722_REG_INSET_XX and I40E_REG_INSET_XX at the same time. So it Cannot be achieved by #ifdef and #endif. >=20 > > + {I40E_INSET_IPV6_SRC, I40E_REG_INSET_L3_SRC_IP6}, > > + {I40E_INSET_IPV6_DST, I40E_REG_INSET_L3_DST_IP6}, > > + {I40E_INSET_IPV6_TC, I40E_REG_INSET_L3_IP6_TC}, > > + {I40E_INSET_IPV6_NEXT_HDR, > I40E_REG_INSET_L3_IP6_NEXT_HDR}, > > + {I40E_INSET_IPV6_HOP_LIMIT, > I40E_REG_INSET_L3_IP6_HOP_LIMIT}, > > + {I40E_INSET_SRC_PORT, I40E_REG_INSET_L4_SRC_PORT}, > > + {I40E_INSET_DST_PORT, I40E_REG_INSET_L4_DST_PORT}, > > + {I40E_INSET_SCTP_VT, > I40E_REG_INSET_L4_SCTP_VERIFICATION_TAG}, > > + {I40E_INSET_TUNNEL_ID, I40E_REG_INSET_TUNNEL_ID}, > > + {I40E_INSET_TUNNEL_DMAC, > > + I40E_REG_INSET_TUNNEL_L2_INNER_DST_MAC}, > > + {I40E_INSET_TUNNEL_IPV4_DST, > I40E_REG_INSET_TUNNEL_L3_DST_IP4}, > > + {I40E_INSET_TUNNEL_IPV6_DST, > I40E_REG_INSET_TUNNEL_L3_DST_IP6}, > > + {I40E_INSET_TUNNEL_SRC_PORT, > > + I40E_REG_INSET_TUNNEL_L4_UDP_SRC_PORT}, > > + {I40E_INSET_TUNNEL_DST_PORT, > > + I40E_REG_INSET_TUNNEL_L4_UDP_DST_PORT}, > > + {I40E_INSET_VLAN_TUNNEL, > I40E_REG_INSET_TUNNEL_VLAN}, > > + {I40E_INSET_FLEX_PAYLOAD_W1, > I40E_REG_INSET_FLEX_PAYLOAD_WORD1}, > > + {I40E_INSET_FLEX_PAYLOAD_W2, > I40E_REG_INSET_FLEX_PAYLOAD_WORD2}, > > + {I40E_INSET_FLEX_PAYLOAD_W3, > I40E_REG_INSET_FLEX_PAYLOAD_WORD3}, > > + {I40E_INSET_FLEX_PAYLOAD_W4, > I40E_REG_INSET_FLEX_PAYLOAD_WORD4}, > > + {I40E_INSET_FLEX_PAYLOAD_W5, > I40E_REG_INSET_FLEX_PAYLOAD_WORD5}, > > + {I40E_INSET_FLEX_PAYLOAD_W6, > I40E_REG_INSET_FLEX_PAYLOAD_WORD6}, > > + {I40E_INSET_FLEX_PAYLOAD_W7, > I40E_REG_INSET_FLEX_PAYLOAD_WORD7}, > > + {I40E_INSET_FLEX_PAYLOAD_W8, > I40E_REG_INSET_FLEX_PAYLOAD_WORD8}, > > + }; > > + > > if (input =3D=3D 0) > > return val; > > > > /* Translate input set to register aware inset */ > > +#ifdef X722_SUPPORT > > + if (type =3D=3D I40E_MAC_X722) { > > + for (i =3D 0; i < RTE_DIM(inset_map1); i++) { > > + if (input & inset_map1[i].inset) > > + val |=3D inset_map1[i].inset_reg; > > + } > > + } else { > > + for (i =3D 0; i < RTE_DIM(inset_map); i++) { > > + if (input & inset_map[i].inset) > > + val |=3D inset_map[i].inset_reg; > > + } > > + } > > +#else > > for (i =3D 0; i < RTE_DIM(inset_map); i++) { > > if (input & inset_map[i].inset) > > val |=3D inset_map[i].inset_reg; > > } > > +#endif >=20 > What about something like this, to prevent duplication: >=20 > inset_map_x =3D inset_map; >=20 > #ifdef X722_SUPPORT > if (type =3D=3D I40E_MAC_X722) > inset_map_x =3D inset_map1; > #endif >=20 > for (i =3D 0; i < RTE_DIM(inset_map_x); i++) { > if (input & inset_map_x[i].inset) > val |=3D inset_map_x[i].inset_reg; > } >=20 Also thought about that way, but if X722_SUPPORT is not set Compile error will report because of unused parameter. Thanks Jingjing