net/mlx5: check the reg available for metadata action
diff mbox series

Message ID 20200520013328.98838-1-xiangxia.m.yue@gmail.com
State Deferred
Delegated to: Raslan Darawsheh
Headers show
Series
  • net/mlx5: check the reg available for metadata action
Related show

Checks

Context Check Description
ci/iol-mellanox-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/checkpatch success coding style OK

Commit Message

Tonghao Zhang May 20, 2020, 1:33 a.m. UTC
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] - http://doc.dpdk.org/guides-20.02/nics/mlx5.html

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 drivers/net/mlx5/mlx5_flow_dv.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Slava Ovsiienko May 22, 2020, 5:17 p.m. UTC | #1
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
Tonghao Zhang May 24, 2020, 10:47 a.m. UTC | #2
On Sat, May 23, 2020 at 1:17 AM Slava Ovsiienko
<viacheslavo@mellanox.com> wrote:
>
> 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?
Hi Slava, thanks for your reviews. I haven't done some research for
tag/flag/mark
so you can send a patch to fix it. and I will sent v2 for metadata
action. Thanks.
> 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
>


--
Best regards, Tonghao

Patch
diff mbox series

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