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

Message ID 20200520013328.98838-1-xiangxia.m.yue@gmail.com
State New
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

Viacheslav 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
Tonghao Zhang May 27, 2020, 7:43 a.m. UTC | #3
On Sun, May 24, 2020 at 6:47 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
>
> On Sat, May 23, 2020 at 1:17 AM Slava Ovsiienko
> <viacheslavo@mellanox.com> wrote:
> >
> > Hi, Tonghao
Hi maintainers,
one question, as doc said:
dv_xmeta_en
2, this engages extensive metadata mode, the MARK and META related
actions and items operate within all supported steering domains,
including FDB, MARK and META information may cross the domain
boundaries. The META item is 32 bits wide, the MARK item width depends
on kernel and firmware configurations and might be 0, 16 or 24 bits.
The actual supported width can be retrieved in runtime by series of
rte_flow_validate() trials.

I set the metadata in fdb, and then output to vf port. In the vf port
nic table, I try to match the metadata which set in fdb table,
but I can't match the metadata. That is a bug ? the doc said: "META
information may cross the domain boundaries."

https://doc.dpdk.org/guides/nics/mlx5.html

> > 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