|SUCCESS| net/mlx5: fix the eCPRI match on HWS root table

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164963

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-04

commit 93c7d4c22628
Author: Bing Zhao <bingz at nvidia.com>

net/mlx5: fix the eCPRI match on HWS root table

This patch fixes eCPRI matching behavior on HWS root tables by
introducing per-table flags to track whether eCPRI type and message body
fields are masked. The patch modifies both matcher creation and rule
translation paths.

> diff --git a/drivers/net/mlx5/hws/mlx5dr_matcher.c b/drivers/net/mlx5/hws/mlx5dr_matcher.c
> index 8c07ea8882..511f394448 100644
> --- a/drivers/net/mlx5/hws/mlx5dr_matcher.c
> +++ b/drivers/net/mlx5/hws/mlx5dr_matcher.c
> @@ -1381,6 +1381,7 @@ static int mlx5dr_matcher_init_root(struct mlx5dr_matcher *matcher)
>  	LIST_INSERT_HEAD(&matcher->tbl->head, matcher, next);
>  	pthread_spin_unlock(&ctx->ctrl_lock);
>  
> +	matcher->hws_root_match_flags = flow_attr.hws_root_match_flags;

Does this code correctly initialize hws_root_match_flags? The
flow_attr.hws_root_match_flags field is read here but I don't see where
it's being set before this point in mlx5dr_matcher_init_root().

[ ... ]

> diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
> index c9e72a33d6..39c85eb9a4 100644
> --- a/drivers/net/mlx5/mlx5_flow.h
> +++ b/drivers/net/mlx5/mlx5_flow.h
> @@ -117,6 +117,9 @@ enum mlx5_indirect_type {
>  	((struct rte_flow_action_handle *)(uintptr_t) \
>  	 ((MLX5_INDIRECT_ACTION_TYPE_CT << MLX5_INDIRECT_ACTION_TYPE_OFFSET) | (index)))
>  
> +#define MLX5_EMPTY_ECPRI_TYPE_MASK RTE_BIT32(0)
> +#define MLX5_EMPTY_ECPRI_BODY_MASK RTE_BIT32(1)
> +
>  enum mlx5_indirect_list_type {
>  	MLX5_INDIRECT_ACTION_LIST_TYPE_ERR = 0,
>  	MLX5_INDIRECT_ACTION_LIST_TYPE_LEGACY = 1,
> @@ -1264,6 +1267,7 @@ struct mlx5_flow_attr {
>  	/* Action flags, used by priority adjustment. */
>  	uint32_t act_flags;
>  	uint32_t tbl_type; /* Flow table type. */
> +	uint32_t hws_root_match_flags;
>  };

[ ... ]

> @@ -1985,6 +1989,8 @@ struct mlx5_flow_workspace {
>  	uint32_t skip_matcher_reg:1;
>  	/* Indicates if need to skip matcher register in translate. */
>  	uint32_t mark:1; /* Indicates if flow contains mark action. */
> +	uint32_t empty_ecpri_type_mask:1; /* Indicates if eCPRI type was not masked. */
> +	uint32_t empty_ecpri_body_mask:1; /* Indicates if 1st DW of eCPRI payload was not masked. */

These comments exceed 79 characters. The lines should be wrapped or the
comments shortened.

[ ... ]

> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
> index c2a2874913..307354c886 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c

[ ... ]

> @@ -11401,6 +11401,20 @@ flow_dv_translate_item_gtp_psc(void *key, const struct rte_flow_item *item,
>  	return 0;
>  }
>  
> +
> +static inline void
> +flow_dv_set_ecpri_sample1(void *misc4_v,
> +			  const struct rte_flow_item_ecpri *ecpri_m,
> +			  const struct rte_flow_item_ecpri *ecpri_v,
> +			  uint32_t sample1)
> +{
> +	void *dw_v;
> +	dw_v = MLX5_ADDR_OF(fte_match_set_misc4, misc4_v, prog_sample_field_value_1);
> +	*(uint32_t *)dw_v = ecpri_v->hdr.dummy[0] & ecpri_m->hdr.dummy[0];

This code violates the DPDK C11 style forbidden token policy. Direct
casts to modify pointers like *(uint32_t *)dw_v should be avoided. Can
this use MLX5_SET or similar macros instead?

> +	/* Sample#1, to match message body, offset 4. */
> +	MLX5_SET(fte_match_set_misc4, misc4_v, prog_sample_field_id_1, sample1);
> +}
> +
>  /**
>   * Add eCPRI item to matcher and to the value.
>   *

[ ... ]

> @@ -11418,6 +11432,7 @@ flow_dv_translate_item_gtp_psc(void *key, const struct rte_flow_item *item,
>  static void
>  flow_dv_translate_item_ecpri(struct rte_eth_dev *dev, void *key,
>  			     const struct rte_flow_item *item,
> +			     struct mlx5_dv_matcher_workspace *wks,
>  			     uint64_t last_item, uint32_t key_type)
>  {
>  	struct mlx5_priv *priv = dev->data->dev_private;

[ ... ]

> +	/* HWS matcher does need a non-empty mask. */

Does this comment have a typo? Should it say "does NOT need"?

>  	if (MLX5_ITEM_VALID(item, key_type))
>  		return;
> -	MLX5_ITEM_UPDATE(item, key_type, ecpri_v, ecpri_m,
> -		&rte_flow_item_ecpri_mask);
> +	MLX5_ITEM_UPDATE(item, key_type, ecpri_v, ecpri_m, &rte_flow_item_ecpri_mask);

[ ... ]

> +	if (key_type == MLX5_SET_MATCHER_HS_M) {
> +		if (!ecpri_m->hdr.common.u32) {
> +			*wks->p_root_flags |= MLX5_EMPTY_ECPRI_TYPE_MASK;
> +			return;
> +		}
> +		/* Initialized with 0, this should not hit. */
> +		*wks->p_root_flags &= ~MLX5_EMPTY_ECPRI_TYPE_MASK;
> +	}
> +	if (key_type == MLX5_SET_MATCHER_HS_V && (*wks->p_root_flags & MLX5_EMPTY_ECPRI_TYPE_MASK))
> +		return;

Does this code have proper NULL pointer checks? Can wks or
wks->p_root_flags ever be NULL when key_type is MLX5_SET_MATCHER_HS_M
or MLX5_SET_MATCHER_HS_V?

> +	if ((key_type & MLX5_SET_MATCHER_SW) && !ecpri_m->hdr.common.u32)
>  		return;
>  	samples = priv->sh->ecpri_parser.ids;
>  	/* Need to take the whole DW as the mask to fill the entry. */

[ ... ]

> @@ -11473,27 +11502,40 @@ flow_dv_translate_item_ecpri(struct rte_eth_dev *dev, void *key,
>  	 * Checking if message body part needs to be matched.
>  	 * Some wildcard rules only matching type field should be supported.
>  	 */
> -	if (ecpri_m->hdr.dummy[0]) {
> -		if (key_type == MLX5_SET_MATCHER_SW_M)
> -			common.u32 = rte_be_to_cpu_32(ecpri_vv->hdr.common.u32);
> -		else
> -			common.u32 = rte_be_to_cpu_32(ecpri_v->hdr.common.u32);
> +	if (key_type == MLX5_SET_MATCHER_HS_M) {
> +		if (!ecpri_m->hdr.dummy[0]) {
> +			*wks->p_root_flags |= MLX5_EMPTY_ECPRI_BODY_MASK;
> +			return;
> +		}
> +		*wks->p_root_flags &= ~MLX5_EMPTY_ECPRI_BODY_MASK;
> +	}
> +	if (key_type == MLX5_SET_MATCHER_HS_V && (*wks->p_root_flags & MLX5_EMPTY_ECPRI_BODY_MASK))
> +		return;
> +	if (key_type & MLX5_SET_MATCHER_SW) {
> +		if (!ecpri_m->hdr.dummy[0])
> +			return;
> +		common.u32 = rte_be_to_cpu_32(ecpri_vv->hdr.common.u32);
> +	} else if (key_type == MLX5_SET_MATCHER_HS_V) {
> +		common.u32 = rte_be_to_cpu_32(ecpri_v->hdr.common.u32);
> +	}
> +	if (key_type != MLX5_SET_MATCHER_HS_M) {
>  		switch (common.type) {
>  		case RTE_ECPRI_MSG_TYPE_IQ_DATA:
>  		case RTE_ECPRI_MSG_TYPE_RTC_CTRL:
>  		case RTE_ECPRI_MSG_TYPE_DLY_MSR:
> -			dw_v = MLX5_ADDR_OF(fte_match_set_misc4, misc4_v,
> -					    prog_sample_field_value_1);
> -			*(uint32_t *)dw_v = ecpri_v->hdr.dummy[0] &
> -					    ecpri_m->hdr.dummy[0];
> -			/* Sample#1, to match message body, offset 4. */
> -			MLX5_SET(fte_match_set_misc4, misc4_v,
> -				 prog_sample_field_id_1, samples[1]);
> +			flow_dv_set_ecpri_sample1(misc4_v, ecpri_v, ecpri_m, samples[1]);

Does the function call flow_dv_set_ecpri_sample1() swap the order of
ecpri_v and ecpri_m parameters? The function signature shows ecpri_m
comes before ecpri_v, but this call passes ecpri_v first.

>  			break;
>  		default:
>  			/* Others, do not match any sample ID. */
>  			break;
>  		}
> +	} else {
> +		/* When MLX5_SET_MATCHER_HS_M on root table (default value),
> +		 * the sample ID and mask should be set, or else prog_sample_field_id_1 will
> +		 * be zero and a mismatch with the HWS rule.

These comment lines exceed 79 characters and should be wrapped.

> +		 * Empty body mask for a matcher returns directly in the previous lines.
> +		 */
> +		flow_dv_set_ecpri_sample1(misc4_v, ecpri_v, ecpri_m, samples[1]);
>  	}
>  }

[ ... ]

> @@ -14698,6 +14740,7 @@ mlx5_flow_dv_translate_items_hws_impl(const struct rte_flow_item *items,
>  		.attr = &rattr,
>  		.rss_desc = &rss_desc,
>  		.group = attr->group,
> +		.p_root_flags = &attr->hws_root_match_flags,
>  	};
>  	int ret = 0;
>