[dpdk-dev] [PATCH] net/bnxt: modify mark manager validity checks

[Mike Baucom <michael.baucom@broadcom.com>]

From: Kishore Padmanabha <kishore.padmanabha@broadcom.com>

The ulp mark manager originally assumed that zero was an invalid
mark and used it for invalidation and deletion.  The mark manager
now supports adding zero as a mark, flags for validity and type,
and adds explicit bounds checking instead of relying on mask.

Signed-off-by: Kishore Padmanabha <kishore.padmanabha@broadcom.com>
Reviewed-by: Mike Baucom <michael.baucom@broadcom.com>
---
 drivers/net/bnxt/tf_ulp/ulp_mapper.c   |  22 ++---
 drivers/net/bnxt/tf_ulp/ulp_mark_mgr.c | 176 ++++++++++++++++++++-------------
 drivers/net/bnxt/tf_ulp/ulp_mark_mgr.h |  25 ++---
 3 files changed, 127 insertions(+), 96 deletions(-)

diff --git a/drivers/net/bnxt/tf_ulp/ulp_mapper.c b/drivers/net/bnxt/tf_ulp/ulp_mapper.c
index 9ea6fdb..938b88e 100644
--- a/drivers/net/bnxt/tf_ulp/ulp_mapper.c
+++ b/drivers/net/bnxt/tf_ulp/ulp_mapper.c
@@ -459,18 +459,9 @@
 ulp_mapper_mark_free(struct bnxt_ulp_context *ulp,
 		     struct ulp_flow_db_res_params *res)
 {
-	uint32_t flag;
-	uint32_t fid;
-	uint32_t gfid;
-
-	fid	  = (uint32_t)res->resource_hndl;
-	TF_GET_FLAG_FROM_FLOW_ID(fid, flag);
-	TF_GET_GFID_FROM_FLOW_ID(fid, gfid);
-
 	return ulp_mark_db_mark_del(ulp,
-				    (flag == TF_GFID_TABLE_EXTERNAL),
-				    gfid,
-				    0);
+				    res->resource_type,
+				    res->resource_hndl);
 }
 
 /*
@@ -1318,10 +1309,9 @@
 		mark = tfp_be_to_cpu_32(val);
 
 		TF_GET_GFID_FROM_FLOW_ID(iparms.flow_id, gfid);
-		TF_GET_FLAG_FROM_FLOW_ID(iparms.flow_id, flag);
-
+		flag = BNXT_ULP_MARK_GLOBAL_HW_FID;
 		rc = ulp_mark_db_mark_add(parms->ulp_ctx,
-					  (flag == TF_GFID_TABLE_EXTERNAL),
+					  flag,
 					  gfid,
 					  mark);
 		if (rc) {
@@ -1336,8 +1326,8 @@
 		memset(&fid_parms, 0, sizeof(fid_parms));
 		fid_parms.direction	= tbl->direction;
 		fid_parms.resource_func	= BNXT_ULP_RESOURCE_FUNC_HW_FID;
-		fid_parms.resource_type	= tbl->table_type;
-		fid_parms.resource_hndl	= iparms.flow_id;
+		fid_parms.resource_type	= flag;
+		fid_parms.resource_hndl	= gfid;
 		fid_parms.critical_resource = 0;
 
 		rc = ulp_flow_db_resource_add(parms->ulp_ctx,
diff --git a/drivers/net/bnxt/tf_ulp/ulp_mark_mgr.c b/drivers/net/bnxt/tf_ulp/ulp_mark_mgr.c
index ad83531..9e8b81e 100644
--- a/drivers/net/bnxt/tf_ulp/ulp_mark_mgr.c
+++ b/drivers/net/bnxt/tf_ulp/ulp_mark_mgr.c
@@ -14,6 +14,13 @@
 #include "ulp_template_db.h"
 #include "ulp_template_struct.h"
 
+#define ULP_MARK_DB_ENTRY_SET_VALID(mark_info) ((mark_info)->flags |=\
+						BNXT_ULP_MARK_VALID)
+#define ULP_MARK_DB_ENTRY_IS_INVALID(mark_info) (!((mark_info)->flags &\
+						   BNXT_ULP_MARK_VALID))
+#define ULP_MARK_DB_ENTRY_IS_GLOBAL_HW_FID(mark_info) ((mark_info)->flags &\
+						BNXT_ULP_MARK_GLOBAL_HW_FID)
+
 static inline uint32_t
 ulp_mark_db_idx_get(bool is_gfid, uint32_t fid, struct bnxt_ulp_mark_tbl *mtbl)
 {
@@ -25,52 +32,14 @@
 
 		/* Need to truncate anything beyond supported flows */
 		idx &= mtbl->gfid_mask;
-
 		if (hashtype)
 			idx |= mtbl->gfid_type_bit;
 	} else {
 		idx = fid;
 	}
-
 	return idx;
 }
 
-static int32_t
-ulp_mark_db_mark_set(struct bnxt_ulp_context *ctxt,
-		     bool is_gfid,
-		     uint32_t fid,
-		     uint32_t mark)
-{
-	struct		bnxt_ulp_mark_tbl *mtbl;
-	uint32_t	idx = 0;
-
-	if (!ctxt) {
-		BNXT_TF_DBG(ERR, "Invalid ulp context\n");
-		return -EINVAL;
-	}
-
-	mtbl = bnxt_ulp_cntxt_ptr2_mark_db_get(ctxt);
-	if (!mtbl) {
-		BNXT_TF_DBG(ERR, "Unable to get Mark DB\n");
-		return -EINVAL;
-	}
-
-	idx = ulp_mark_db_idx_get(is_gfid, fid, mtbl);
-
-	if (is_gfid) {
-		BNXT_TF_DBG(DEBUG, "Set GFID[0x%0x] = 0x%0x\n", idx, mark);
-
-		mtbl->gfid_tbl[idx].mark_id = mark;
-		mtbl->gfid_tbl[idx].valid = true;
-	} else {
-		/* For the LFID, the FID is used as the index */
-		mtbl->lfid_tbl[fid].mark_id = mark;
-		mtbl->lfid_tbl[fid].valid = true;
-	}
-
-	return 0;
-}
-
 /*
  * Allocate and Initialize all Mark Manager resources for this ulp context.
  *
@@ -105,48 +74,48 @@
 	if (!mark_tbl)
 		goto mem_error;
 
-	/* Need to allocate 2 * Num flows to account for hash type bit. */
+	/* Need to allocate 2 * Num flows to account for hash type bit.*/
+	mark_tbl->lfid_num_entries = dparms->lfid_entries;
 	mark_tbl->lfid_tbl = rte_zmalloc("ulp_rx_em_flow_mark_table",
-					 dparms->lfid_entries *
-					    sizeof(struct bnxt_lfid_mark_info),
+					 mark_tbl->lfid_num_entries *
+					 sizeof(struct bnxt_lfid_mark_info),
 					 0);
-
 	if (!mark_tbl->lfid_tbl)
 		goto mem_error;
 
-	/* Need to allocate 2 * Num flows to account for hash type bit. */
+	/* Need to allocate 2 * Num flows to account for hash type bit */
+	mark_tbl->gfid_num_entries = dparms->gfid_entries;
 	mark_tbl->gfid_tbl = rte_zmalloc("ulp_rx_eem_flow_mark_table",
-					 2 * dparms->num_flows *
-					    sizeof(struct bnxt_gfid_mark_info),
+					 mark_tbl->gfid_num_entries *
+					 sizeof(struct bnxt_gfid_mark_info),
 					 0);
 	if (!mark_tbl->gfid_tbl)
 		goto mem_error;
 
 	/*
-	 * TBD: This needs to be generalized for better mark handling
 	 * These values are used to compress the FID to the allowable index
-	 * space.  The FID from hw may be the full hash.
+	 * space.  The FID from hw may be the full hash which may be a big
+	 * value to allocate and so allocate only needed hash values.
+	 * gfid mask is the number of flow entries for the each left/right
+	 * hash  The gfid type bit is used to get to the higher or lower hash
+	 * entries.
 	 */
-	mark_tbl->gfid_max	= dparms->gfid_entries - 1;
-	mark_tbl->gfid_mask	= (dparms->gfid_entries / 2) - 1;
-	mark_tbl->gfid_type_bit = (dparms->gfid_entries / 2);
+	mark_tbl->gfid_mask	= (mark_tbl->gfid_num_entries / 2) - 1;
+	mark_tbl->gfid_type_bit = (mark_tbl->gfid_num_entries / 2);
 
 	BNXT_TF_DBG(DEBUG, "GFID Max = 0x%08x\nGFID MASK = 0x%08x\n",
-		    mark_tbl->gfid_max,
+		    mark_tbl->gfid_num_entries - 1,
 		    mark_tbl->gfid_mask);
 
 	/* Add the mark tbl to the ulp context. */
 	bnxt_ulp_cntxt_ptr2_mark_db_set(ctxt, mark_tbl);
-
 	return 0;
 
 mem_error:
 	rte_free(mark_tbl->gfid_tbl);
 	rte_free(mark_tbl->lfid_tbl);
 	rte_free(mark_tbl);
-	BNXT_TF_DBG(DEBUG,
-		    "Failed to allocate memory for mark mgr\n");
-
+	BNXT_TF_DBG(DEBUG, "Failed to allocate memory for mark mgr\n");
 	return -ENOMEM;
 }
 
@@ -208,7 +177,8 @@
 	idx = ulp_mark_db_idx_get(is_gfid, fid, mtbl);
 
 	if (is_gfid) {
-		if (!mtbl->gfid_tbl[idx].valid)
+		if (idx >= mtbl->gfid_num_entries ||
+		    ULP_MARK_DB_ENTRY_IS_INVALID(&mtbl->gfid_tbl[idx]))
 			return -EINVAL;
 
 		BNXT_TF_DBG(DEBUG, "Get GFID[0x%0x] = 0x%0x\n",
@@ -216,7 +186,8 @@
 
 		*mark = mtbl->gfid_tbl[idx].mark_id;
 	} else {
-		if (!mtbl->gfid_tbl[idx].valid)
+		if (idx >= mtbl->lfid_num_entries ||
+		    ULP_MARK_DB_ENTRY_IS_INVALID(&mtbl->lfid_tbl[idx]))
 			return -EINVAL;
 
 		BNXT_TF_DBG(DEBUG, "Get LFID[0x%0x] = 0x%0x\n",
@@ -233,7 +204,7 @@
  *
  * ctxt [in] The ulp context for the mark manager
  *
- * is_gfid [in] The type of fid (GFID or LFID)
+ * mark_flag [in] mark flags.
  *
  * fid [in] The flow id that is returned by HW in BD
  *
@@ -242,11 +213,47 @@
  */
 int32_t
 ulp_mark_db_mark_add(struct bnxt_ulp_context *ctxt,
-		     bool is_gfid,
-		     uint32_t gfid,
+		     uint32_t mark_flag,
+		     uint32_t fid,
 		     uint32_t mark)
 {
-	return ulp_mark_db_mark_set(ctxt, is_gfid, gfid, mark);
+	struct bnxt_ulp_mark_tbl *mtbl;
+	uint32_t idx = 0;
+	bool is_gfid;
+
+	if (!ctxt) {
+		BNXT_TF_DBG(ERR, "Invalid ulp context\n");
+		return -EINVAL;
+	}
+
+	mtbl = bnxt_ulp_cntxt_ptr2_mark_db_get(ctxt);
+	if (!mtbl) {
+		BNXT_TF_DBG(ERR, "Unable to get Mark DB\n");
+		return -EINVAL;
+	}
+
+	is_gfid = (mark_flag & BNXT_ULP_MARK_GLOBAL_HW_FID);
+	if (is_gfid) {
+		idx = ulp_mark_db_idx_get(is_gfid, fid, mtbl);
+		if (idx >= mtbl->gfid_num_entries) {
+			BNXT_TF_DBG(ERR, "Mark index greater than allocated\n");
+			return -EINVAL;
+		}
+		BNXT_TF_DBG(DEBUG, "Set GFID[0x%0x] = 0x%0x\n", idx, mark);
+		mtbl->gfid_tbl[idx].mark_id = mark;
+		ULP_MARK_DB_ENTRY_SET_VALID(&mtbl->gfid_tbl[idx]);
+
+	} else {
+		/* For the LFID, the FID is used as the index */
+		if (fid >= mtbl->lfid_num_entries) {
+			BNXT_TF_DBG(ERR, "Mark index greater than allocated\n");
+			return -EINVAL;
+		}
+		mtbl->lfid_tbl[fid].mark_id = mark;
+		ULP_MARK_DB_ENTRY_SET_VALID(&mtbl->lfid_tbl[fid]);
+	}
+
+	return 0;
 }
 
 /*
@@ -254,18 +261,51 @@
  *
  * ctxt [in] The ulp context for the mark manager
  *
- * is_gfid [in] The type of fid (GFID or LFID)
+ * mark_flag [in] mark flags.
  *
  * fid [in] The flow id that is returned by HW in BD
  *
- * mark [in] The mark to be associated with the FID
- *
  */
 int32_t
 ulp_mark_db_mark_del(struct bnxt_ulp_context *ctxt,
-		     bool is_gfid,
-		     uint32_t gfid,
-		     uint32_t mark  __rte_unused)
+		     uint32_t mark_flag,
+		     uint32_t fid)
 {
-	return ulp_mark_db_mark_set(ctxt, is_gfid, gfid, ULP_MARK_INVALID);
+	struct bnxt_ulp_mark_tbl *mtbl;
+	uint32_t idx = 0;
+	bool is_gfid;
+
+	if (!ctxt) {
+		BNXT_TF_DBG(ERR, "Invalid ulp context\n");
+		return -EINVAL;
+	}
+
+	mtbl = bnxt_ulp_cntxt_ptr2_mark_db_get(ctxt);
+	if (!mtbl) {
+		BNXT_TF_DBG(ERR, "Unable to get Mark DB\n");
+		return -EINVAL;
+	}
+
+	is_gfid = (mark_flag & BNXT_ULP_MARK_GLOBAL_HW_FID);
+	if (is_gfid) {
+		idx = ulp_mark_db_idx_get(is_gfid, fid, mtbl);
+		if (idx >= mtbl->gfid_num_entries) {
+			BNXT_TF_DBG(ERR, "Mark index greater than allocated\n");
+			return -EINVAL;
+		}
+		BNXT_TF_DBG(DEBUG, "Reset GFID[0x%0x]\n", idx);
+		memset(&mtbl->gfid_tbl[idx], 0,
+		       sizeof(struct bnxt_gfid_mark_info));
+
+	} else {
+		/* For the LFID, the FID is used as the index */
+		if (fid >= mtbl->lfid_num_entries) {
+			BNXT_TF_DBG(ERR, "Mark index greater than allocated\n");
+			return -EINVAL;
+		}
+		memset(&mtbl->lfid_tbl[fid], 0,
+		       sizeof(struct bnxt_lfid_mark_info));
+	}
+
+	return 0;
 }
diff --git a/drivers/net/bnxt/tf_ulp/ulp_mark_mgr.h b/drivers/net/bnxt/tf_ulp/ulp_mark_mgr.h
index 0f8a5e5..fd0d840 100644
--- a/drivers/net/bnxt/tf_ulp/ulp_mark_mgr.h
+++ b/drivers/net/bnxt/tf_ulp/ulp_mark_mgr.h
@@ -8,23 +8,27 @@
 
 #include "bnxt_ulp.h"
 
-#define ULP_MARK_INVALID (0)
+#define BNXT_ULP_MARK_VALID   0x1
+#define BNXT_ULP_MARK_GLOBAL_HW_FID 0x4
+#define BNXT_ULP_MARK_LOCAL_HW_FID 0x8
+
 struct bnxt_lfid_mark_info {
 	uint16_t	mark_id;
-	bool		valid;
+	uint16_t	flags;
 };
 
 struct bnxt_gfid_mark_info {
 	uint32_t	mark_id;
-	bool		valid;
+	uint16_t	flags;
 };
 
 struct bnxt_ulp_mark_tbl {
 	struct bnxt_lfid_mark_info	*lfid_tbl;
 	struct bnxt_gfid_mark_info	*gfid_tbl;
+	uint32_t			lfid_num_entries;
+	uint32_t			gfid_num_entries;
 	uint32_t			gfid_mask;
 	uint32_t			gfid_type_bit;
-	uint32_t			gfid_max;
 };
 
 /*
@@ -77,7 +81,7 @@ struct bnxt_ulp_mark_tbl {
  *
  * ctxt [in] The ulp context for the mark manager
  *
- * is_gfid [in] The type of fid (GFID or LFID)
+ * mark_flag [in] mark flags.
  *
  * fid [in] The flow id that is returned by HW in BD
  *
@@ -86,7 +90,7 @@ struct bnxt_ulp_mark_tbl {
  */
 int32_t
 ulp_mark_db_mark_add(struct bnxt_ulp_context *ctxt,
-		     bool is_gfid,
+		     uint32_t mark_flag,
 		     uint32_t gfid,
 		     uint32_t mark);
 
@@ -95,17 +99,14 @@ struct bnxt_ulp_mark_tbl {
  *
  * ctxt [in] The ulp context for the mark manager
  *
- * is_gfid [in] The type of fid (GFID or LFID)
+ * mark_flag [in] mark flags
  *
  * fid [in] The flow id that is returned by HW in BD
  *
- * mark [in] The mark to be associated with the FID
- *
  */
 int32_t
 ulp_mark_db_mark_del(struct bnxt_ulp_context *ctxt,
-		     bool is_gfid,
-		     uint32_t gfid,
-		     uint32_t mark);
+		     uint32_t mark_flag,
+		     uint32_t gfid);
 
 #endif /* _ULP_MARK_MGR_H_ */
-- 
1.9.1