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....@gmail.com <xiangxia.m....@gmail.com> > Sent: Sunday, May 24, 2020 14:07 > To: Slava Ovsiienko <viachesl...@mellanox.com> > Cc: dev@dpdk.org; Tonghao Zhang <xiangxia.m....@gmail.com>; > sta...@dpdk.org > Subject: [PATCH dpdk-dev v2] net/mlx5: check the reg available for metadata > action > > From: Tonghao Zhang <xiangxia.m....@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&data=02%7C01%7Cviacheslavo%40mellan > ox.com%7Ce55c7bccccd14490af4208d7ffd29c78%7Ca652971c7d2e4d9ba6a > 4d149256f461b%7C0%7C0%7C637259152350270223&sdata=PiDsa8dD > 4bJrGL2FGHWUOybIAMD5pxq8p5vxmtlS9lc%3D&reserved=0 > Fixes: fcc8d2f716fd ("net/mlx5: extend flow metadata support") > Cc: sta...@dpdk.org > > Signed-off-by: Tonghao Zhang <xiangxia.m....@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
