Re: [dpdk-dev] [PATCH dpdk-dev] net/mlx5: check the reg available for metadata action

[Slava Ovsiienko <viacheslavo@mellanox.com>]

Hi, Tonghao

Thank you for the patch.
I suppose, the patch should be extended to encompass the routines:
- flow_dv_convert_action_mark
- flow_dv_convert_action_set_meta (done in the patch)
- flow_dv_validate_item_mark
- flow_dv_validate_item_mark
- flow_dv_validate_action_flag
- flow_dv_validate_action_mark

In action converting routines we should add MLX5_ASSERT()
(returning REG_NONE must not happen there - the wrong
conditions must be filtered out on validation stage)

One more point - it would be good to add cc:stable@dpdk.org

Would you like to extend the patch or let us do it?

With  best regards,
Slava

> -----Original Message-----
> From: xiangxia.m.yue@gmail.com <xiangxia.m.yue@gmail.com>
> Sent: Wednesday, May 20, 2020 4:33
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> Cc: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> Subject: [PATCH dpdk-dev] net/mlx5: check the reg available for metadata
> action
> 
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> If user don't set the dv_xmeta_en to 1 or 2, in the
> flow_dv_convert_action_set_meta function:
> * flow_dv_get_metadata_reg may return the REG_NONE,
>   when MLX5_METADATA_FDB enabled for metadata set
>   action.
> * reg_to_field(REG_NONE) return MLX5_MODI_OUT_NONE
>   which is invalid.
> 
> The rdma-core calltrace:
> dr_action_create_modify_action
> dr_actions_convert_modify_header
> dr_action_modify_sw_to_hw
> dr_action_modify_sw_to_hw_set
> dr_ste_get_modify_hdr_hw_field
> 
> sw_field [MLX5_MODI_OUT_NONE 4095]
> should not > ste_ctx->modify_hdr_field_arr_sz [92]
> 
> As doc[1] says:
> | dv_xmeta_en 0, this is default value, defines the legacy mode, the
> | MARK and META related actions and items operate only within NIC Tx and
> | NIC Rx steering domains, no MARK and META information crosses the
> | domain boundaries.
> 
> This patch add check for that case to warn that not supported.
> 
> [1] -
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdoc.dp
> dk.org%2Fguides-
> 20.02%2Fnics%2Fmlx5.html&amp;data=02%7C01%7Cviacheslavo%40mellan
> ox.com%7C49a86a92f7884fb4d45808d7fc5de31a%7Ca652971c7d2e4d9ba6
> a4d149256f461b%7C0%7C0%7C637255352498582078&amp;sdata=cDfb%2F
> oJAPNvGhMjUrjmjAE3R%2FH4wUpOI7WcZ5miTLvA%3D&amp;reserved=0
> 
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>  drivers/net/mlx5/mlx5_flow_dv.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> b/drivers/net/mlx5/mlx5_flow_dv.c index e4818319507c..dfcaf90eda11
> 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -1251,6 +1251,12 @@ flow_dv_convert_action_set_meta
> 
>  	if (reg < 0)
>  		return reg;
> +
> +	if (reg == REG_NONE)
> +		return rte_flow_error_set(error, ENOTSUP,
> +					  RTE_FLOW_ERROR_TYPE_ITEM,
> +					  NULL, "unavailable "
> +					  "metadata register");
>  	/*
>  	 * In datapath code there is no endianness
>  	 * coversions for perfromance reasons, all
> --
> 2.26.1