diff mbox series

[1/2] net/i40e: fix incorrect FDIR flex mask

Message ID 20201104082959.63800-2-chenxux.di@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers show
Series net/i40e: fix incorrect FDIR flex configuration | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Chenxu Di Nov. 4, 2020, 8:29 a.m. UTC
The register of FDIR flex mask should not be set during flow validate.
It should be set when flow create.

Fixes: 6ced3dd72f5f ("net/i40e: support flexible payload parsing for FDIR")
Cc: stable@dpdk.org

Signed-off-by: Chenxu Di <chenxux.di@intel.com>
---
 drivers/net/i40e/i40e_ethdev.h |  1 +
 drivers/net/i40e/i40e_fdir.c   | 94 ++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_flow.c   | 97 +---------------------------------
 3 files changed, 97 insertions(+), 95 deletions(-)

Comments

Guo, Jia Nov. 4, 2020, 9:40 a.m. UTC | #1
Hi, chenxu

> -----Original Message-----
> From: Chenxu Di <chenxux.di@intel.com>
> Sent: Wednesday, November 4, 2020 4:30 PM
> To: dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>; Wang,
> Haiyue <haiyue.wang@intel.com>; Di, ChenxuX <chenxux.di@intel.com>;
> stable@dpdk.org
> Subject: [PATCH 1/2] net/i40e: fix incorrect FDIR flex mask
> 
> The register of FDIR flex mask should not be set during flow validate.
> It should be set when flow create.
> 
> Fixes: 6ced3dd72f5f ("net/i40e: support flexible payload parsing for FDIR")

Seems that you move the place of flexible payload parsing from flow validate to create, you delete something but miss some useless parameter deleting,
such as " off_arr/len_arr" etc in this [PATCH 1/2] or [PATCH 2/3]. Please double check if anything related remove and if you want you could merge into 1 remove patch if it could be.

> Cc: stable@dpdk.org
> 
> Signed-off-by: Chenxu Di <chenxux.di@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.h |  1 +
>  drivers/net/i40e/i40e_fdir.c   | 94 ++++++++++++++++++++++++++++++++
>  drivers/net/i40e/i40e_flow.c   | 97 +---------------------------------
>  3 files changed, 97 insertions(+), 95 deletions(-)
> 

<...>
Chenxu Di Nov. 4, 2020, 9:49 a.m. UTC | #2
Hi, Jia

> -----Original Message-----
> From: Guo, Jia <jia.guo@intel.com>
> Sent: Wednesday, November 4, 2020 5:41 PM
> To: Di, ChenxuX <chenxux.di@intel.com>; dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>; Wang, Haiyue
> <haiyue.wang@intel.com>; Di, ChenxuX <chenxux.di@intel.com>;
> stable@dpdk.org
> Subject: RE: [PATCH 1/2] net/i40e: fix incorrect FDIR flex mask
> 
> Hi, chenxu
> 
> > -----Original Message-----
> > From: Chenxu Di <chenxux.di@intel.com>
> > Sent: Wednesday, November 4, 2020 4:30 PM
> > To: dev@dpdk.org
> > Cc: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia
> > <jia.guo@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>; Di, ChenxuX
> > <chenxux.di@intel.com>; stable@dpdk.org
> > Subject: [PATCH 1/2] net/i40e: fix incorrect FDIR flex mask
> >
> > The register of FDIR flex mask should not be set during flow validate.
> > It should be set when flow create.
> >
> > Fixes: 6ced3dd72f5f ("net/i40e: support flexible payload parsing for
> > FDIR")
> 
> Seems that you move the place of flexible payload parsing from flow validate to
> create, you delete something but miss some useless parameter deleting, such as
> " off_arr/len_arr" etc in this [PATCH 1/2] or [PATCH 2/3]. Please double check if
> anything related remove and if you want you could merge into 1 remove patch if
> it could be.
> 

Got it, I will double check it.

> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Chenxu Di <chenxux.di@intel.com>
> > ---
> >  drivers/net/i40e/i40e_ethdev.h |  1 +
> >  drivers/net/i40e/i40e_fdir.c   | 94 ++++++++++++++++++++++++++++++++
> >  drivers/net/i40e/i40e_flow.c   | 97 +---------------------------------
> >  3 files changed, 97 insertions(+), 95 deletions(-)
> >
> 
> <...>
Chenxu Di Nov. 5, 2020, 8:40 a.m. UTC | #3
Hi, jia

> -----Original Message-----
> From: Di, ChenxuX
> Sent: Wednesday, November 4, 2020 5:49 PM
> To: Guo, Jia <jia.guo@intel.com>; dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>; Wang, Haiyue
> <haiyue.wang@intel.com>
> Subject: RE: [PATCH 1/2] net/i40e: fix incorrect FDIR flex mask
> 
> Hi, Jia
> 
> > -----Original Message-----
> > From: Guo, Jia <jia.guo@intel.com>
> > Sent: Wednesday, November 4, 2020 5:41 PM
> > To: Di, ChenxuX <chenxux.di@intel.com>; dev@dpdk.org
> > Cc: Xing, Beilei <beilei.xing@intel.com>; Wang, Haiyue
> > <haiyue.wang@intel.com>; Di, ChenxuX <chenxux.di@intel.com>;
> > stable@dpdk.org
> > Subject: RE: [PATCH 1/2] net/i40e: fix incorrect FDIR flex mask
> >
> > Hi, chenxu
> >
> > > -----Original Message-----
> > > From: Chenxu Di <chenxux.di@intel.com>
> > > Sent: Wednesday, November 4, 2020 4:30 PM
> > > To: dev@dpdk.org
> > > Cc: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia
> > > <jia.guo@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>; Di,
> > > ChenxuX <chenxux.di@intel.com>; stable@dpdk.org
> > > Subject: [PATCH 1/2] net/i40e: fix incorrect FDIR flex mask
> > >
> > > The register of FDIR flex mask should not be set during flow validate.
> > > It should be set when flow create.
> > >
> > > Fixes: 6ced3dd72f5f ("net/i40e: support flexible payload parsing for
> > > FDIR")
> >
> > Seems that you move the place of flexible payload parsing from flow
> > validate to create, you delete something but miss some useless
> > parameter deleting, such as " off_arr/len_arr" etc in this [PATCH 1/2]
> > or [PATCH 2/3]. Please double check if anything related remove and if
> > you want you could merge into 1 remove patch if it could be.
> >
> 
> Got it, I will double check it.
> 

I have double checked the code and I think the other code and parameter in parse function are useful and necessary.
The array  off_arr and len_arr is used to store the info of RAW if there are two or more RAW in pattern.

And I will merge into one patch in next version.


> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Chenxu Di <chenxux.di@intel.com>
> > > ---
> > >  drivers/net/i40e/i40e_ethdev.h |  1 +
> > >  drivers/net/i40e/i40e_fdir.c   | 94 ++++++++++++++++++++++++++++++++
> > >  drivers/net/i40e/i40e_flow.c   | 97 +---------------------------------
> > >  3 files changed, 97 insertions(+), 95 deletions(-)
> > >
> >
> > <...>
diff mbox series

Patch

diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 1466998aa..6be929f65 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -604,6 +604,7 @@  struct i40e_fdir_flow_ext {
 	uint16_t vlan_tci;
 	uint8_t flexbytes[RTE_ETH_FDIR_MAX_FLEXLEN];
 	/* It is filled by the flexible payload to match. */
+	uint8_t flex_mask[I40E_FDIR_MAX_FLEX_LEN];
 	uint8_t is_vf;   /* 1 for VF, 0 for port dev */
 	uint16_t dst_id; /* VF ID, available when is_vf is 1*/
 	bool inner_ip;   /* If there is inner ip */
diff --git a/drivers/net/i40e/i40e_fdir.c b/drivers/net/i40e/i40e_fdir.c
index aa8e72949..e31464eb7 100644
--- a/drivers/net/i40e/i40e_fdir.c
+++ b/drivers/net/i40e/i40e_fdir.c
@@ -1765,6 +1765,78 @@  i40e_add_del_fdir_filter(struct rte_eth_dev *dev,
 	return ret;
 }
 
+static int
+i40e_flow_store_flex_mask(struct i40e_pf *pf,
+			  enum i40e_filter_pctype pctype,
+			  uint8_t *mask)
+{
+	struct i40e_fdir_flex_mask flex_mask;
+	uint8_t nb_bitmask = 0;
+	uint16_t mask_tmp;
+	uint8_t i;
+
+	memset(&flex_mask, 0, sizeof(struct i40e_fdir_flex_mask));
+	for (i = 0; i < I40E_FDIR_MAX_FLEX_LEN; i += sizeof(uint16_t)) {
+		mask_tmp = I40E_WORD(mask[i], mask[i + 1]);
+		if (mask_tmp) {
+			flex_mask.word_mask |=
+				I40E_FLEX_WORD_MASK(i / sizeof(uint16_t));
+			if (mask_tmp != UINT16_MAX) {
+				flex_mask.bitmask[nb_bitmask].mask = ~mask_tmp;
+				flex_mask.bitmask[nb_bitmask].offset =
+					i / sizeof(uint16_t);
+				nb_bitmask++;
+				if (nb_bitmask > I40E_FDIR_BITMASK_NUM_WORD)
+					return -1;
+			}
+		}
+	}
+	flex_mask.nb_bitmask = nb_bitmask;
+
+	if (pf->fdir.flex_mask_flag[pctype] &&
+	    (memcmp(&flex_mask, &pf->fdir.flex_mask[pctype],
+		    sizeof(struct i40e_fdir_flex_mask))))
+		return -2;
+	else if (pf->fdir.flex_mask_flag[pctype] &&
+		 !(memcmp(&flex_mask, &pf->fdir.flex_mask[pctype],
+			  sizeof(struct i40e_fdir_flex_mask))))
+		return 1;
+
+	memcpy(&pf->fdir.flex_mask[pctype], &flex_mask,
+	       sizeof(struct i40e_fdir_flex_mask));
+	return 0;
+}
+
+static void
+i40e_flow_set_fdir_flex_msk(struct i40e_pf *pf,
+			    enum i40e_filter_pctype pctype)
+{
+	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
+	struct i40e_fdir_flex_mask *flex_mask;
+	uint32_t flxinset, fd_mask;
+	uint8_t i;
+
+	/* Set flex mask */
+	flex_mask = &pf->fdir.flex_mask[pctype];
+	flxinset = (flex_mask->word_mask <<
+		    I40E_PRTQF_FD_FLXINSET_INSET_SHIFT) &
+		I40E_PRTQF_FD_FLXINSET_INSET_MASK;
+	i40e_write_rx_ctl(hw, I40E_PRTQF_FD_FLXINSET(pctype), flxinset);
+
+	for (i = 0; i < flex_mask->nb_bitmask; i++) {
+		fd_mask = (flex_mask->bitmask[i].mask <<
+			   I40E_PRTQF_FD_MSK_MASK_SHIFT) &
+			   I40E_PRTQF_FD_MSK_MASK_MASK;
+		fd_mask |= ((flex_mask->bitmask[i].offset +
+			     I40E_FLX_OFFSET_IN_FIELD_VECTOR) <<
+			    I40E_PRTQF_FD_MSK_OFFSET_SHIFT) &
+				I40E_PRTQF_FD_MSK_OFFSET_MASK;
+		i40e_write_rx_ctl(hw, I40E_PRTQF_FD_MSK(pctype, i), fd_mask);
+	}
+
+	pf->fdir.flex_mask_flag[pctype] = 1;
+}
+
 static inline unsigned char *
 i40e_find_available_buffer(struct rte_eth_dev *dev)
 {
@@ -1820,10 +1892,12 @@  i40e_flow_add_del_fdir_filter(struct rte_eth_dev *dev,
 	unsigned char *pkt = NULL;
 	enum i40e_filter_pctype pctype;
 	struct i40e_fdir_info *fdir_info = &pf->fdir;
+	uint8_t flex_mask[I40E_FDIR_MAX_FLEX_LEN];
 	struct i40e_fdir_filter *node;
 	struct i40e_fdir_filter check_filter; /* Check if the filter exists */
 	bool wait_status = true;
 	int ret = 0;
+	int i;
 
 	if (pf->fdir.fdir_vsi == NULL) {
 		PMD_DRV_LOG(ERR, "FDIR is not enabled");
@@ -1856,6 +1930,26 @@  i40e_flow_add_del_fdir_filter(struct rte_eth_dev *dev,
 	i40e_fdir_filter_convert(filter, &check_filter);
 
 	if (add) {
+		if (!filter->input.flow_ext.customized_pctype) {
+			/* Store flex mask to SW */
+			for (i = 0; i < I40E_FDIR_MAX_FLEX_LEN; i++)
+				flex_mask[i] =
+					filter->input.flow_ext.flex_mask[i];
+
+			ret = i40e_flow_store_flex_mask(pf, pctype, flex_mask);
+			if (ret == -1) {
+				PMD_DRV_LOG(ERR, "Exceed maximal"
+					    " number of bitmasks");
+				return -EINVAL;
+			} else if (ret == -2) {
+				PMD_DRV_LOG(ERR, "Conflict with the"
+					    " first flexible rule");
+				return -EINVAL;
+			} else if (ret == 0) {
+				i40e_flow_set_fdir_flex_msk(pf, pctype);
+			}
+		}
+
 		ret = i40e_sw_fdir_filter_insert(pf, &check_filter);
 		if (ret < 0) {
 			PMD_DRV_LOG(ERR,
diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c
index adc5da1c5..4f916494e 100644
--- a/drivers/net/i40e/i40e_flow.c
+++ b/drivers/net/i40e/i40e_flow.c
@@ -2273,47 +2273,6 @@  i40e_flow_store_flex_pit(struct i40e_pf *pf,
 	return 0;
 }
 
-static int
-i40e_flow_store_flex_mask(struct i40e_pf *pf,
-			  enum i40e_filter_pctype pctype,
-			  uint8_t *mask)
-{
-	struct i40e_fdir_flex_mask flex_mask;
-	uint16_t mask_tmp;
-	uint8_t i, nb_bitmask = 0;
-
-	memset(&flex_mask, 0, sizeof(struct i40e_fdir_flex_mask));
-	for (i = 0; i < I40E_FDIR_MAX_FLEX_LEN; i += sizeof(uint16_t)) {
-		mask_tmp = I40E_WORD(mask[i], mask[i + 1]);
-		if (mask_tmp) {
-			flex_mask.word_mask |=
-				I40E_FLEX_WORD_MASK(i / sizeof(uint16_t));
-			if (mask_tmp != UINT16_MAX) {
-				flex_mask.bitmask[nb_bitmask].mask = ~mask_tmp;
-				flex_mask.bitmask[nb_bitmask].offset =
-					i / sizeof(uint16_t);
-				nb_bitmask++;
-				if (nb_bitmask > I40E_FDIR_BITMASK_NUM_WORD)
-					return -1;
-			}
-		}
-	}
-	flex_mask.nb_bitmask = nb_bitmask;
-
-	if (pf->fdir.flex_mask_flag[pctype] &&
-	    (memcmp(&flex_mask, &pf->fdir.flex_mask[pctype],
-		    sizeof(struct i40e_fdir_flex_mask))))
-		return -2;
-	else if (pf->fdir.flex_mask_flag[pctype] &&
-		 !(memcmp(&flex_mask, &pf->fdir.flex_mask[pctype],
-			  sizeof(struct i40e_fdir_flex_mask))))
-		return 1;
-
-	memcpy(&pf->fdir.flex_mask[pctype], &flex_mask,
-	       sizeof(struct i40e_fdir_flex_mask));
-	return 0;
-}
-
 static void
 i40e_flow_set_fdir_flex_pit(struct i40e_pf *pf,
 			    enum i40e_flxpld_layer_idx layer_idx,
@@ -2356,36 +2315,6 @@  i40e_flow_set_fdir_flex_pit(struct i40e_pf *pf,
 	pf->fdir.flex_pit_flag[layer_idx] = 1;
 }
 
-static void
-i40e_flow_set_fdir_flex_msk(struct i40e_pf *pf,
-			    enum i40e_filter_pctype pctype)
-{
-	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
-	struct i40e_fdir_flex_mask *flex_mask;
-	uint32_t flxinset, fd_mask;
-	uint8_t i;
-
-	/* Set flex mask */
-	flex_mask = &pf->fdir.flex_mask[pctype];
-	flxinset = (flex_mask->word_mask <<
-		    I40E_PRTQF_FD_FLXINSET_INSET_SHIFT) &
-		I40E_PRTQF_FD_FLXINSET_INSET_MASK;
-	i40e_write_rx_ctl(hw, I40E_PRTQF_FD_FLXINSET(pctype), flxinset);
-
-	for (i = 0; i < flex_mask->nb_bitmask; i++) {
-		fd_mask = (flex_mask->bitmask[i].mask <<
-			   I40E_PRTQF_FD_MSK_MASK_SHIFT) &
-			I40E_PRTQF_FD_MSK_MASK_MASK;
-		fd_mask |= ((flex_mask->bitmask[i].offset +
-			     I40E_FLX_OFFSET_IN_FIELD_VECTOR) <<
-			    I40E_PRTQF_FD_MSK_OFFSET_SHIFT) &
-			I40E_PRTQF_FD_MSK_OFFSET_MASK;
-		i40e_write_rx_ctl(hw, I40E_PRTQF_FD_MSK(pctype, i), fd_mask);
-	}
-
-	pf->fdir.flex_mask_flag[pctype] = 1;
-}
-
 static int
 i40e_flow_set_fdir_inset(struct i40e_pf *pf,
 			 enum i40e_filter_pctype pctype,
@@ -2604,10 +2533,8 @@  i40e_flow_parse_fdir_pattern(struct rte_eth_dev *dev,
 	uint16_t len_arr[I40E_MAX_FLXPLD_FIED];
 	struct i40e_fdir_flex_pit flex_pit;
 	uint8_t next_dst_off = 0;
-	uint8_t flex_mask[I40E_FDIR_MAX_FLEX_LEN];
 	uint16_t flex_size;
 	bool cfg_flex_pit = true;
-	bool cfg_flex_msk = true;
 	uint16_t ether_type;
 	uint32_t vtc_flow_cpu;
 	bool outer_ip = true;
@@ -2615,7 +2542,6 @@  i40e_flow_parse_fdir_pattern(struct rte_eth_dev *dev,
 
 	memset(off_arr, 0, sizeof(off_arr));
 	memset(len_arr, 0, sizeof(len_arr));
-	memset(flex_mask, 0, I40E_FDIR_MAX_FLEX_LEN);
 	filter->input.flow_ext.customized_pctype = false;
 	for (; item->type != RTE_FLOW_ITEM_TYPE_END; item++) {
 		if (item->last) {
@@ -3205,7 +3131,8 @@  i40e_flow_parse_fdir_pattern(struct rte_eth_dev *dev,
 				j = i + next_dst_off;
 				filter->input.flow_ext.flexbytes[j] =
 					raw_spec->pattern[i];
-				flex_mask[j] = raw_mask->pattern[i];
+				filter->input.flow_ext.flex_mask[j] =
+					raw_mask->pattern[i];
 			}
 
 			next_dst_off += raw_spec->length;
@@ -3296,28 +3223,8 @@  i40e_flow_parse_fdir_pattern(struct rte_eth_dev *dev,
 			return -rte_errno;
 		}
 
-		/* Store flex mask to SW */
-		ret = i40e_flow_store_flex_mask(pf, pctype, flex_mask);
-		if (ret == -1) {
-			rte_flow_error_set(error, EINVAL,
-					   RTE_FLOW_ERROR_TYPE_ITEM,
-					   item,
-					   "Exceed maximal number of bitmasks");
-			return -rte_errno;
-		} else if (ret == -2) {
-			rte_flow_error_set(error, EINVAL,
-					   RTE_FLOW_ERROR_TYPE_ITEM,
-					   item,
-					   "Conflict with the first flexible rule");
-			return -rte_errno;
-		} else if (ret > 0)
-			cfg_flex_msk = false;
-
 		if (cfg_flex_pit)
 			i40e_flow_set_fdir_flex_pit(pf, layer_idx, raw_id);
-
-		if (cfg_flex_msk)
-			i40e_flow_set_fdir_flex_msk(pf, pctype);
 	}
 
 	filter->input.pctype = pctype;