Re: [net PATCH V3 1/2] octeontx2-af: Fix hash extraction mbox message

[Simon Horman]

On Mon, Jul 17, 2023 at 01:00:48AM +0530, Suman Ghosh wrote:
> As of today, hash extraction mbox message response supports only the
> secret key which is not a complete solution. This patch fixes that and
> adds support to extract both hash mask and hash control along with the
> secret key. These are needed to use hash reduction of 128 bit IPv6
> address to 32 bit. Hash mask decides on the bits from the 128 bit IPv6
> address which should be used for 32 bit hash calculation. After
> generating the 32 bit hash, hash control decides how many bits from the
> 32 bit hash can be taken into consideration.
> 
> Fixes: a95ab93550d3 ("octeontx2-af: Use hashed field in MCAM key")
> Signed-off-by: Suman Ghosh <sumang@xxxxxxxxxxx>
> ---
>  .../net/ethernet/marvell/octeontx2/af/mbox.h  |  6 ++---
>  .../marvell/octeontx2/af/rvu_npc_hash.c       | 27 ++++++++++---------
>  .../marvell/octeontx2/af/rvu_npc_hash.h       |  9 ++++---
>  3 files changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> index eba307eee2b2..5a5c23a02261 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> @@ -246,9 +246,9 @@ M(NPC_MCAM_READ_BASE_RULE, 0x6011, npc_read_base_steer_rule,            \
>  M(NPC_MCAM_GET_STATS, 0x6012, npc_mcam_entry_stats,                     \
>  				   npc_mcam_get_stats_req,              \
>  				   npc_mcam_get_stats_rsp)              \
> -M(NPC_GET_FIELD_HASH_INFO, 0x6013, npc_get_field_hash_info,                     \
> -				   npc_get_field_hash_info_req,              \
> -				   npc_get_field_hash_info_rsp)              \
> +M(NPC_GET_FIELD_HASH_INFO, 0x6013, npc_get_field_hash_info,             \
> +				   npc_get_field_hash_info_req,         \
> +				   npc_get_field_hash_info_rsp)         \
>  M(NPC_GET_FIELD_STATUS, 0x6014, npc_get_field_status,                     \
>  				   npc_get_field_status_req,              \
>  				   npc_get_field_status_rsp)              \

This hunk is a white-space only change that doesn't seem
related to the patch description.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_hash.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_hash.c
> index 6fe67f3a7f6f..a3bc53d22dc0 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_hash.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_hash.c
> @@ -96,7 +96,7 @@ u32 npc_field_hash_calc(u64 *ldata, struct npc_get_field_hash_info_rsp rsp,
>  	field_hash = rvu_npc_toeplitz_hash(data_padded, hash_key, 128, 159);
>  
>  	field_hash &= FIELD_GET(GENMASK(63, 32), rsp.hash_ctrl[intf][hash_idx]);
> -	field_hash += FIELD_GET(GENMASK(31, 0), rsp.hash_ctrl[intf][hash_idx]);
> +	field_hash |= FIELD_GET(GENMASK(31, 0), rsp.hash_ctrl[intf][hash_idx]);
>  	return field_hash;
>  }
>  
> @@ -253,7 +253,8 @@ void npc_update_field_hash(struct rvu *rvu, u8 intf,
>  	}
>  
>  	req.intf = intf;
> -	rvu_mbox_handler_npc_get_field_hash_info(rvu, &req, &rsp);
> +	if (rvu_mbox_handler_npc_get_field_hash_info(rvu, &req, &rsp))
> +		return;
>  
>  	for (hash_idx = 0; hash_idx < NPC_MAX_HASH; hash_idx++) {
>  		cfg = rvu_read64(rvu, blkaddr, NPC_AF_INTFX_HASHX_CFG(intf, hash_idx));
> @@ -319,9 +320,9 @@ int rvu_mbox_handler_npc_get_field_hash_info(struct rvu *rvu,
>  					     struct npc_get_field_hash_info_req *req,
>  					     struct npc_get_field_hash_info_rsp *rsp)
>  {
> +	int hash_idx, hash_mask_idx, blkaddr;
>  	u64 *secret_key = rsp->secret_key;
>  	u8 intf = req->intf;
> -	int i, j, blkaddr;
>  
>  	blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NPC, 0);
>  	if (blkaddr < 0) {
> @@ -333,18 +334,18 @@ int rvu_mbox_handler_npc_get_field_hash_info(struct rvu *rvu,
>  	secret_key[1] = rvu_read64(rvu, blkaddr, NPC_AF_INTFX_SECRET_KEY1(intf));
>  	secret_key[2] = rvu_read64(rvu, blkaddr, NPC_AF_INTFX_SECRET_KEY2(intf));
>  
> -	for (i = 0; i < NPC_MAX_HASH; i++) {
> -		for (j = 0; j < NPC_MAX_HASH_MASK; j++) {
> -			rsp->hash_mask[NIX_INTF_RX][i][j] =
> -				GET_KEX_LD_HASH_MASK(NIX_INTF_RX, i, j);
> -			rsp->hash_mask[NIX_INTF_TX][i][j] =
> -				GET_KEX_LD_HASH_MASK(NIX_INTF_TX, i, j);
> +	for (hash_idx = 0; hash_idx < NPC_MAX_HASH; hash_idx++)
> +		for (hash_mask_idx = 0; hash_mask_idx < NPC_MAX_HASH_MASK; hash_mask_idx++) {
> +			rsp->hash_mask[NIX_INTF_RX][hash_idx][hash_mask_idx] =
> +				GET_KEX_LD_HASH_MASK(NIX_INTF_RX, hash_idx, hash_mask_idx);
> +			rsp->hash_mask[NIX_INTF_TX][hash_idx][hash_mask_idx] =
> +				GET_KEX_LD_HASH_MASK(NIX_INTF_TX, hash_idx, hash_mask_idx);
>  		}
> -	}
>  
> -	for (i = 0; i < NPC_MAX_INTF; i++)
> -		for (j = 0; j < NPC_MAX_HASH; j++)
> -			rsp->hash_ctrl[i][j] = GET_KEX_LD_HASH_CTRL(i, j);
> +	for (hash_idx = 0; hash_idx < NPC_MAX_INTF; hash_idx++)
> +		for (hash_mask_idx = 0; hash_mask_idx < NPC_MAX_HASH; hash_mask_idx++)
> +			rsp->hash_ctrl[hash_idx][hash_mask_idx] =
> +				GET_KEX_LD_HASH_CTRL(hash_idx, hash_mask_idx);
>  
>  	return 0;
>  }

The three hunks above appear to change the iterator variables for the loops
without changing functionality. This doesn't seem to match the patch
description.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_hash.h b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_hash.h
> index a1c3d987b804..eb9cb311b934 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_hash.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_hash.h
> @@ -12,9 +12,6 @@
>  #define RVU_NPC_HASH_SECRET_KEY1 0xa9d5af4c9fbc87b4
>  #define RVU_NPC_HASH_SECRET_KEY2 0x5954c9e7
>  
> -#define NPC_MAX_HASH 2
> -#define NPC_MAX_HASH_MASK 2
> -

This seems to remove duplicated #defines.
This doesn't seem to match the patch description.

>  #define KEX_LD_CFG_USE_HASH(use_hash, bytesm1, hdr_ofs, ena, flags_ena, key_ofs) \
>  			    ((use_hash) << 20 | ((bytesm1) << 16) | ((hdr_ofs) << 8) | \
>  			     ((ena) << 7) | ((flags_ena) << 6) | ((key_ofs) & 0x3F))
> @@ -41,6 +38,12 @@
>  	rvu_write64(rvu, blkaddr,	\
>  		    NPC_AF_INTFX_HASHX_RESULT_CTRL(intf, ld), cfg)
>  
> +#define GET_KEX_LD_HASH_CTRL(intf, ld)  \
> +	rvu_read64(rvu, blkaddr, NPC_AF_INTFX_HASHX_RESULT_CTRL(intf, ld))
> +
> +#define GET_KEX_LD_HASH_MASK(intf, ld, mask_idx)	\
> +	rvu_read64(rvu, blkaddr, NPC_AF_INTFX_HASHX_MASKX(intf, ld, mask_idx))
> +

This seems to duplicate existing MACROS,
which appear a few lines further above in this file.

>  struct npc_mcam_kex_hash {
>  	/* NPC_AF_INTF(0..1)_LID(0..7)_LT(0..15)_LD(0..1)_CFG */
>  	bool lid_lt_ld_hash_en[NPC_MAX_INTF][NPC_MAX_LID][NPC_MAX_LT][NPC_MAX_LD];
> -- 
> 2.25.1
> 
>