DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH dpdk-dev] net/mlx5: check the reg available for metadata action
@ 2020-05-20  1:33 xiangxia.m.yue
  2020-05-22 17:17 ` Slava Ovsiienko
  2020-05-24 11:06 ` [dpdk-dev] [PATCH dpdk-dev v2] " xiangxia.m.yue
  0 siblings, 2 replies; 8+ messages in thread
From: xiangxia.m.yue @ 2020-05-20  1:33 UTC (permalink / raw)
  To: viacheslavo, dev; +Cc: Tonghao Zhang

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(+)

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH dpdk-dev] net/mlx5: check the reg available for metadata action
  2020-05-20  1:33 [dpdk-dev] [PATCH dpdk-dev] net/mlx5: check the reg available for metadata action xiangxia.m.yue
@ 2020-05-22 17:17 ` Slava Ovsiienko
  2020-05-24 10:47   ` Tonghao Zhang
  2020-05-24 11:06 ` [dpdk-dev] [PATCH dpdk-dev v2] " xiangxia.m.yue
  1 sibling, 1 reply; 8+ messages in thread
From: Slava Ovsiienko @ 2020-05-22 17:17 UTC (permalink / raw)
  To: xiangxia.m.yue, dev

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH dpdk-dev] net/mlx5: check the reg available for metadata action
  2020-05-22 17:17 ` Slava Ovsiienko
@ 2020-05-24 10:47   ` Tonghao Zhang
  2020-05-27  7:43     ` Tonghao Zhang
  0 siblings, 1 reply; 8+ messages in thread
From: Tonghao Zhang @ 2020-05-24 10:47 UTC (permalink / raw)
  To: Slava Ovsiienko; +Cc: dev

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH dpdk-dev v2] net/mlx5: check the reg available for metadata action
  2020-05-20  1:33 [dpdk-dev] [PATCH dpdk-dev] net/mlx5: check the reg available for metadata action xiangxia.m.yue
  2020-05-22 17:17 ` Slava Ovsiienko
@ 2020-05-24 11:06 ` xiangxia.m.yue
  2020-08-05 17:09   ` Slava Ovsiienko
  2020-11-20 14:48   ` [dpdk-dev] [PATCH v3] " Viacheslav Ovsiienko
  1 sibling, 2 replies; 8+ messages in thread
From: xiangxia.m.yue @ 2020-05-24 11:06 UTC (permalink / raw)
  To: viacheslavo; +Cc: dev, Tonghao Zhang, stable

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
Fixes: fcc8d2f716fd ("net/mlx5: extend flow metadata support")
Cc: stable@dpdk.org

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

diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index e4818319507c..c77835270d60 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -1251,6 +1251,7 @@ flow_dv_convert_action_set_meta
 
 	if (reg < 0)
 		return reg;
+	MLX5_ASSERT(reg != REG_NONE);
 	/*
 	 * In datapath code there is no endianness
 	 * coversions for perfromance reasons, all
@@ -1449,7 +1450,6 @@ flow_dv_validate_item_meta(struct rte_eth_dev *dev __rte_unused,
 			   struct rte_flow_error *error)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
-	struct mlx5_dev_config *config = &priv->config;
 	const struct rte_flow_item_meta *spec = item->spec;
 	const struct rte_flow_item_meta *mask = item->mask;
 	struct rte_flow_item_meta nic_mask = {
@@ -1463,23 +1463,25 @@ flow_dv_validate_item_meta(struct rte_eth_dev *dev __rte_unused,
 					  RTE_FLOW_ERROR_TYPE_ITEM_SPEC,
 					  item->spec,
 					  "data cannot be empty");
-	if (config->dv_xmeta_en != MLX5_XMETA_MODE_LEGACY) {
-		if (!mlx5_flow_ext_mreg_supported(dev))
-			return rte_flow_error_set(error, ENOTSUP,
+	if (!mlx5_flow_ext_mreg_supported(dev))
+		return rte_flow_error_set(error, ENOTSUP,
 					  RTE_FLOW_ERROR_TYPE_ITEM, item,
 					  "extended metadata register"
 					  " isn't supported");
-		reg = flow_dv_get_metadata_reg(dev, attr, error);
-		if (reg < 0)
-			return reg;
-		if (reg == REG_B)
-			return rte_flow_error_set(error, ENOTSUP,
+	reg = flow_dv_get_metadata_reg(dev, attr, error);
+	if (reg < 0)
+		return reg;
+	if (reg == REG_NONE)
+		return rte_flow_error_set(error, ENOTSUP,
+					  RTE_FLOW_ERROR_TYPE_ITEM, item,
+					  "unavailable metadata register");
+	if (reg == REG_B)
+		return rte_flow_error_set(error, ENOTSUP,
 					  RTE_FLOW_ERROR_TYPE_ITEM, item,
 					  "match on reg_b "
 					  "isn't supported");
-		if (reg != REG_A)
-			nic_mask.data = priv->sh->dv_meta_mask;
-	}
+	if (reg != REG_A)
+		nic_mask.data = priv->sh->dv_meta_mask;
 	if (!mask)
 		mask = &rte_flow_item_meta_mask;
 	if (!mask->data)
@@ -2244,6 +2246,11 @@ flow_dv_validate_action_set_meta(struct rte_eth_dev *dev,
 	reg = flow_dv_get_metadata_reg(dev, attr, error);
 	if (reg < 0)
 		return reg;
+	if (reg == REG_NONE)
+		return rte_flow_error_set(error, ENOTSUP,
+					  RTE_FLOW_ERROR_TYPE_ACTION,
+					  action, "unavailable "
+					  "metadata register");
 	if (reg != REG_A && reg != REG_B) {
 		struct mlx5_priv *priv = dev->data->dev_private;
 
-- 
2.26.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH dpdk-dev] net/mlx5: check the reg available for metadata action
  2020-05-24 10:47   ` Tonghao Zhang
@ 2020-05-27  7:43     ` Tonghao Zhang
  0 siblings, 0 replies; 8+ messages in thread
From: Tonghao Zhang @ 2020-05-27  7:43 UTC (permalink / raw)
  To: Slava Ovsiienko, Matan Azrad; +Cc: dev

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



-- 
Best regards, Tonghao

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH dpdk-dev v2] net/mlx5: check the reg available for metadata action
  2020-05-24 11:06 ` [dpdk-dev] [PATCH dpdk-dev v2] " xiangxia.m.yue
@ 2020-08-05 17:09   ` Slava Ovsiienko
  2020-11-20 14:48   ` [dpdk-dev] [PATCH v3] " Viacheslav Ovsiienko
  1 sibling, 0 replies; 8+ messages in thread
From: Slava Ovsiienko @ 2020-08-05 17:09 UTC (permalink / raw)
  To: xiangxia.m.yue; +Cc: dev, stable

Hi, Tonghao

Thank you for the patch,  and a lot of my apologizes - I mixed up your patch with other
fix and thought we did not need it. Now I reviwed it again - we fixed flow_dv_validate_item_meta()
(in other way - with explicit register check) but flow_dv_validate_action_set_meta() still needs the fix.
Would you like to rebase your patch and send v3 or prefer to let us do it?

With best regards, Slava

> -----Original Message-----
> From: xiangxia.m.yue@gmail.com <xiangxia.m.yue@gmail.com>
> Sent: Sunday, May 24, 2020 14:07
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; Tonghao Zhang <xiangxia.m.yue@gmail.com>;
> stable@dpdk.org
> Subject: [PATCH dpdk-dev v2] 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%7Ce55c7bccccd14490af4208d7ffd29c78%7Ca652971c7d2e4d9ba6a
> 4d149256f461b%7C0%7C0%7C637259152350270223&amp;sdata=PiDsa8dD
> 4bJrGL2FGHWUOybIAMD5pxq8p5vxmtlS9lc%3D&amp;reserved=0
> Fixes: fcc8d2f716fd ("net/mlx5: extend flow metadata support")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>  drivers/net/mlx5/mlx5_flow_dv.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> b/drivers/net/mlx5/mlx5_flow_dv.c index e4818319507c..c77835270d60
> 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -1251,6 +1251,7 @@ flow_dv_convert_action_set_meta
> 
>  	if (reg < 0)
>  		return reg;
> +	MLX5_ASSERT(reg != REG_NONE);
>  	/*
>  	 * In datapath code there is no endianness
>  	 * coversions for perfromance reasons, all @@ -1449,7 +1450,6 @@
> flow_dv_validate_item_meta(struct rte_eth_dev *dev __rte_unused,
>  			   struct rte_flow_error *error)
>  {
>  	struct mlx5_priv *priv = dev->data->dev_private;
> -	struct mlx5_dev_config *config = &priv->config;
>  	const struct rte_flow_item_meta *spec = item->spec;
>  	const struct rte_flow_item_meta *mask = item->mask;
>  	struct rte_flow_item_meta nic_mask = { @@ -1463,23 +1463,25
> @@ flow_dv_validate_item_meta(struct rte_eth_dev *dev __rte_unused,
> 
> RTE_FLOW_ERROR_TYPE_ITEM_SPEC,
>  					  item->spec,
>  					  "data cannot be empty");
> -	if (config->dv_xmeta_en != MLX5_XMETA_MODE_LEGACY) {
> -		if (!mlx5_flow_ext_mreg_supported(dev))
> -			return rte_flow_error_set(error, ENOTSUP,
> +	if (!mlx5_flow_ext_mreg_supported(dev))
> +		return rte_flow_error_set(error, ENOTSUP,
>  					  RTE_FLOW_ERROR_TYPE_ITEM,
> item,
>  					  "extended metadata register"
>  					  " isn't supported");
> -		reg = flow_dv_get_metadata_reg(dev, attr, error);
> -		if (reg < 0)
> -			return reg;
> -		if (reg == REG_B)
> -			return rte_flow_error_set(error, ENOTSUP,
> +	reg = flow_dv_get_metadata_reg(dev, attr, error);
> +	if (reg < 0)
> +		return reg;
> +	if (reg == REG_NONE)
> +		return rte_flow_error_set(error, ENOTSUP,
> +					  RTE_FLOW_ERROR_TYPE_ITEM,
> item,
> +					  "unavailable metadata register");
> +	if (reg == REG_B)
> +		return rte_flow_error_set(error, ENOTSUP,
>  					  RTE_FLOW_ERROR_TYPE_ITEM,
> item,
>  					  "match on reg_b "
>  					  "isn't supported");
> -		if (reg != REG_A)
> -			nic_mask.data = priv->sh->dv_meta_mask;
> -	}
> +	if (reg != REG_A)
> +		nic_mask.data = priv->sh->dv_meta_mask;
>  	if (!mask)
>  		mask = &rte_flow_item_meta_mask;
>  	if (!mask->data)
> @@ -2244,6 +2246,11 @@ flow_dv_validate_action_set_meta(struct
> rte_eth_dev *dev,
>  	reg = flow_dv_get_metadata_reg(dev, attr, error);
>  	if (reg < 0)
>  		return reg;
> +	if (reg == REG_NONE)
> +		return rte_flow_error_set(error, ENOTSUP,
> +					  RTE_FLOW_ERROR_TYPE_ACTION,
> +					  action, "unavailable "
> +					  "metadata register");
>  	if (reg != REG_A && reg != REG_B) {
>  		struct mlx5_priv *priv = dev->data->dev_private;
> 
> --
> 2.26.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH v3] net/mlx5: check the reg available for metadata action
  2020-05-24 11:06 ` [dpdk-dev] [PATCH dpdk-dev v2] " xiangxia.m.yue
  2020-08-05 17:09   ` Slava Ovsiienko
@ 2020-11-20 14:48   ` Viacheslav Ovsiienko
  2020-11-22 13:00     ` Raslan Darawsheh
  1 sibling, 1 reply; 8+ messages in thread
From: Viacheslav Ovsiienko @ 2020-11-20 14:48 UTC (permalink / raw)
  To: dev; +Cc: rasland, matan, orika, thomas, xiangxia.m.yue, stable

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) returns MLX5_MODI_OUT_NONE,
  that is invalid and rdma-core fails.

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

Fixes: fcc8d2f716fd ("net/mlx5: extend flow metadata support")
Cc: stable@dpdk.org

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

diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index fa2e8f2..bee22f5 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -1217,6 +1217,7 @@ struct field_modify_info modify_tcp[] = {
 
 	if (reg < 0)
 		return reg;
+	MLX5_ASSERT(reg != REG_NON);
 	/*
 	 * In datapath code there is no endianness
 	 * coversions for perfromance reasons, all
@@ -1438,6 +1439,10 @@ struct field_modify_info modify_tcp[] = {
 		reg = flow_dv_get_metadata_reg(dev, attr, error);
 		if (reg < 0)
 			return reg;
+		if (reg == REG_NON)
+			return rte_flow_error_set(error, ENOTSUP,
+					RTE_FLOW_ERROR_TYPE_ITEM, item,
+					"unavalable extended metadata register");
 		if (reg == REG_B)
 			return rte_flow_error_set(error, ENOTSUP,
 					  RTE_FLOW_ERROR_TYPE_ITEM, item,
@@ -2446,6 +2451,10 @@ struct field_modify_info modify_tcp[] = {
 	reg = flow_dv_get_metadata_reg(dev, attr, error);
 	if (reg < 0)
 		return reg;
+	if (reg == REG_NON)
+		return rte_flow_error_set(error, ENOTSUP,
+					  RTE_FLOW_ERROR_TYPE_ACTION, action,
+					  "unavalable extended metadata register");
 	if (reg != REG_A && reg != REG_B) {
 		struct mlx5_priv *priv = dev->data->dev_private;
 
@@ -7471,6 +7480,7 @@ struct mlx5_hlist_entry *
 		reg = flow_dv_get_metadata_reg(dev, attr, NULL);
 		if (reg < 0)
 			return;
+		MLX5_ASSERT(reg != REG_NON);
 		/*
 		 * In datapath code there is no endianness
 		 * coversions for perfromance reasons, all
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v3] net/mlx5: check the reg available for metadata action
  2020-11-20 14:48   ` [dpdk-dev] [PATCH v3] " Viacheslav Ovsiienko
@ 2020-11-22 13:00     ` Raslan Darawsheh
  0 siblings, 0 replies; 8+ messages in thread
From: Raslan Darawsheh @ 2020-11-22 13:00 UTC (permalink / raw)
  To: Slava Ovsiienko, dev
  Cc: Matan Azrad, Ori Kam, NBU-Contact-Thomas Monjalon,
	xiangxia.m.yue, stable

Hi,

> -----Original Message-----
> From: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> Sent: Friday, November 20, 2020 4:48 PM
> To: dev@dpdk.org
> Cc: Raslan Darawsheh <rasland@nvidia.com>; Matan Azrad
> <matan@nvidia.com>; Ori Kam <orika@nvidia.com>; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>; xiangxia.m.yue@gmail.com;
> stable@dpdk.org
> Subject: [PATCH v3] 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) returns MLX5_MODI_OUT_NONE,
>   that is invalid and rdma-core fails.
> 
> 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
> 
> Fixes: fcc8d2f716fd ("net/mlx5: extend flow metadata support")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
Patch applied to next-net-mlx,

Kindest regards,
Raslan Darawsheh

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-11-22 13:00 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20  1:33 [dpdk-dev] [PATCH dpdk-dev] net/mlx5: check the reg available for metadata action xiangxia.m.yue
2020-05-22 17:17 ` Slava Ovsiienko
2020-05-24 10:47   ` Tonghao Zhang
2020-05-27  7:43     ` Tonghao Zhang
2020-05-24 11:06 ` [dpdk-dev] [PATCH dpdk-dev v2] " xiangxia.m.yue
2020-08-05 17:09   ` Slava Ovsiienko
2020-11-20 14:48   ` [dpdk-dev] [PATCH v3] " Viacheslav Ovsiienko
2020-11-22 13:00     ` Raslan Darawsheh

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).