Thanks, PSB. > -----Original Message----- > From: Shahaf Shuler > Sent: Monday, October 29, 2018 12:03 PM > To: Dekel Peled <dek...@mellanox.com>; Yongseok Koh > <ys...@mellanox.com> > Cc: dev@dpdk.org; Ori Kam <or...@mellanox.com> > Subject: RE: [dpdk-dev] [PATCH v6 6/6] net/mlx5: add raw data encap decap > to Direct Verbs > > Thursday, October 25, 2018 11:08 PM, Dekel Peled: > > Subject: [dpdk-dev] [PATCH v6 6/6] net/mlx5: add raw data encap decap > > to Direct Verbs > > > > This patch implements the encap and decap actions, using raw data, in > > DV flow for MLX5 PMD. > > > > Signed-off-by: Dekel Peled <dek...@mellanox.com> > > --- > > drivers/net/mlx5/mlx5_flow.h | 12 ++- > > drivers/net/mlx5/mlx5_flow_dv.c | 227 > > ++++++++++++++++++++++++++++++++++++++-- > > 2 files changed, 224 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/net/mlx5/mlx5_flow.h > > b/drivers/net/mlx5/mlx5_flow.h index 2d73a05..92ba111 100644 > > --- a/drivers/net/mlx5/mlx5_flow.h > > +++ b/drivers/net/mlx5/mlx5_flow.h > > @@ -96,15 +96,19 @@ > > #define MLX5_FLOW_ACTION_VXLAN_DECAP (1u << 23) #define > > MLX5_FLOW_ACTION_NVGRE_ENCAP (1u << 24) #define > > MLX5_FLOW_ACTION_NVGRE_DECAP (1u << 25) > > +#define MLX5_FLOW_ACTION_RAW_ENCAP (1u << 26) #define > > +MLX5_FLOW_ACTION_RAW_DECAP (1u << 27) > > > > #define MLX5_FLOW_FATE_ACTIONS \ > > (MLX5_FLOW_ACTION_DROP | MLX5_FLOW_ACTION_QUEUE | > > MLX5_FLOW_ACTION_RSS) > > > > -#define MLX5_FLOW_ENCAP_ACTIONS \ > > - (MLX5_FLOW_ACTION_VXLAN_ENCAP | > > MLX5_FLOW_ACTION_NVGRE_ENCAP) > > +#define MLX5_FLOW_ENCAP_ACTIONS > > (MLX5_FLOW_ACTION_VXLAN_ENCAP | \ > > + MLX5_FLOW_ACTION_NVGRE_ENCAP | \ > > + MLX5_FLOW_ACTION_RAW_ENCAP) > > > > -#define MLX5_FLOW_DECAP_ACTIONS \ > > - (MLX5_FLOW_ACTION_VXLAN_DECAP | > > MLX5_FLOW_ACTION_NVGRE_DECAP) > > +#define MLX5_FLOW_DECAP_ACTIONS > > (MLX5_FLOW_ACTION_VXLAN_DECAP | \ > > + MLX5_FLOW_ACTION_NVGRE_DECAP | \ > > + MLX5_FLOW_ACTION_RAW_DECAP) > > > > #ifndef IPPROTO_MPLS > > #define IPPROTO_MPLS 137 > > diff --git a/drivers/net/mlx5/mlx5_flow_dv.c > > b/drivers/net/mlx5/mlx5_flow_dv.c index d7d0c6b..ca75645 100644 > > --- a/drivers/net/mlx5/mlx5_flow_dv.c > > +++ b/drivers/net/mlx5/mlx5_flow_dv.c > > @@ -186,6 +186,82 @@ > > } > > > > /** > > + * Validate the raw encap action. > > + * > > + * @param[in] action_flags > > + * Holds the actions detected until now. > > + * @param[in] action > > + * Pointer to the encap action. > > + * @param[in] attr > > + * Pointer to flow attributes > > + * @param[out] error > > + * Pointer to error structure. > > + * > > + * @return > > + * 0 on success, a negative errno value otherwise and rte_errno is set. > > + */ > > +static int > > +flow_dv_validate_action_raw_encap(uint64_t action_flags, > > + const struct rte_flow_action *action, > > + const struct rte_flow_attr *attr, > > + struct rte_flow_error *error) > > +{ > > + if (!(action->conf)) > > + return rte_flow_error_set(error, EINVAL, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > action, > > + "configuration cannot be null"); > > + if (action_flags & MLX5_FLOW_ACTION_DROP) > > + return rte_flow_error_set(error, EINVAL, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > NULL, > > + "can't drop and encap in same > > flow"); > > + if (action_flags & MLX5_FLOW_ENCAP_ACTIONS) > > + return rte_flow_error_set(error, EINVAL, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > NULL, > > + "can only have a single encap" > > + " action in a flow"); > > + /* encap without preceding decap is not supported for ingress */ > > + if (attr->ingress && !(action_flags & > > MLX5_FLOW_ACTION_RAW_DECAP)) > > + return rte_flow_error_set(error, ENOTSUP, > > + > > RTE_FLOW_ERROR_TYPE_ATTR_INGRESS, > > + NULL, > > + "encap action not supported for " > > + "ingress"); > > The error message doesn't seems informative enough.
This is the case when flow rule is created with ingress attribute, and only raw_encap action, without raw_decap. I used the same error message as in L2 encap case. > > > + return 0; > > +} > > + > > +/** > > + * Validate the raw decap action. > > + * > > + * @param[in] action_flags > > + * Holds the actions detected until now. > > + * @param[out] error > > + * Pointer to error structure. > > + * > > + * @return > > + * 0 on success, a negative errno value otherwise and rte_errno is set. > > + */ > > +static int > > +flow_dv_validate_action_raw_decap(uint64_t action_flags, > > + struct rte_flow_error *error) > > +{ > > + if (action_flags & MLX5_FLOW_ACTION_DROP) > > + return rte_flow_error_set(error, EINVAL, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > NULL, > > + "can't drop and decap in same > > flow"); > > + if (action_flags & MLX5_FLOW_ENCAP_ACTIONS) > > + return rte_flow_error_set(error, EINVAL, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > NULL, > > + "can't have encap action before" > > + " decap action"); > > + if (action_flags & MLX5_FLOW_DECAP_ACTIONS) > > + return rte_flow_error_set(error, EINVAL, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > NULL, > > + "can only have a single decap" > > + " action in a flow"); > > Just like you don't allow to do only encap w/o decap on ingress, on egress we > cannot do only decap w/o a subsequent encap. This validation was implemented during action creation, to save the checking of later actions. I will move it to here. > > > + return 0; > > +} > > + > > +/** > > * Get the size of specific rte_flow_item_type > > * > > * @param[in] item_type > > @@ -396,6 +472,7 @@ > > /** > > * Convert L2 encap action to DV specification. > > * Used for VXLAN and NVGRE encap actions. > > + * Also used for raw encap action without preceding decap. > > * > > * @param[in] dev > > * Pointer to rte_eth_dev structure. > > @@ -414,23 +491,34 @@ > > { > > struct ibv_flow_action *verbs_action = NULL; > > const struct rte_flow_item *encap_data; > > + const struct rte_flow_action_raw_encap *raw_encap_data; > > This one can be declared inside the if statement I prefer all variable declarations at start of function. > > > struct priv *priv = dev->data->dev_private; > > uint8_t buf[MLX5_ENCAP_MAX_LEN]; > > + uint8_t *buf_ptr = buf; > > Why you need this buf_ptr? Compilation issue with the fixed sized array of > buf? I needed it to refer to different buffers, raw buffer in raw_encap_data, or buffer converted from encap_data. It will be removed now when moving to use cache. > > > size_t size = 0; > > int convert_result = 0; > > > > - if (action->type == RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP) > > - encap_data = ((const struct rte_flow_action_vxlan_encap *) > > + if (action->type == RTE_FLOW_ACTION_TYPE_RAW_ENCAP) { > > + raw_encap_data = > > + (const struct rte_flow_action_raw_encap *)action- > > >conf; > > + buf_ptr = raw_encap_data->data; > > + size = raw_encap_data->size; > > + } else { > > + if (action->type == > > RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP) > > + encap_data = > > + ((const struct rte_flow_action_vxlan_encap > > *) > > action->conf)->definition; > > - else > > - encap_data = ((const struct rte_flow_action_nvgre_encap *) > > + else > > + encap_data = > > + ((const struct rte_flow_action_nvgre_encap > > *) > > action->conf)->definition; > > - convert_result = flow_dv_convert_encap_data(encap_data, buf, > > - &size, error); > > - if (convert_result) > > - return NULL; > > + convert_result = flow_dv_convert_encap_data(encap_data, > > buf, > > + &size, error); > > + if (convert_result) > > + return NULL; > > + } > > verbs_action = mlx5_glue- > > >dv_create_flow_action_packet_reformat > > - (priv->ctx, size, (size ? buf : NULL), > > + (priv->ctx, size, (size ? buf_ptr : NULL), > > > > MLX5DV_FLOW_ACTION_PACKET_REFORMAT_TYPE_L2_TO_L2_TUNNEL, > > MLX5DV_FLOW_TABLE_TYPE_NIC_TX); > > if (!verbs_action) > > @@ -472,6 +560,50 @@ > > } > > > > /** > > + * Convert raw decap/encap (L3 tunnel) action to DV specification. > > + * > > + * @param[in] dev > > + * Pointer to rte_eth_dev structure. > > + * @param[in] action > > + * Pointer to action structure. > > + * @param[in] attr > > + * Pointer to the flow attributes. > > + * @param[out] error > > + * Pointer to the error structure. > > + * > > + * @return > > + * Pointer to action on success, NULL otherwise and rte_errno is set. > > + */ > > +static struct ibv_flow_action * > > +flow_dv_create_action_raw_encap(struct rte_eth_dev *dev, > > I would call it flow_dv_create_action_packet_reformat, as this is the only > case this function is being called. > For raw_encap only you will use the l2_encap function. I prefer this name, since L2 and L3 encap and decap use mlx5dv packet reformat function. > > > + const struct rte_flow_action *action, > > + const struct rte_flow_attr *attr, > > + struct rte_flow_error *error) > > +{ > > + struct ibv_flow_action *verbs_action = NULL; > > + const struct rte_flow_action_raw_encap *encap_data; > > + struct priv *priv = dev->data->dev_private; > > + enum mlx5dv_flow_action_packet_reformat_type reformat_type; > > + enum mlx5dv_flow_table_type ft_type; > > + > > + encap_data = (const struct rte_flow_action_raw_encap *)action- > > >conf; > > + reformat_type = attr->egress ? > > + > > MLX5DV_FLOW_ACTION_PACKET_REFORMAT_TYPE_L2_TO_L3_TU > > NNEL : > > + > > MLX5DV_FLOW_ACTION_PACKET_REFORMAT_TYPE_L3_TUNNEL_T > > O_L2; > > + ft_type = attr->egress ? > > + MLX5DV_FLOW_TABLE_TYPE_NIC_TX : > > + MLX5DV_FLOW_TABLE_TYPE_NIC_RX; > > + verbs_action = mlx5_glue- > > >dv_create_flow_action_packet_reformat > > + (priv->ctx, encap_data->size, > > + (encap_data->size ? encap_data->data : > > NULL), > > + reformat_type, ft_type); > > + if (!verbs_action) > > + rte_flow_error_set(error, EINVAL, > > RTE_FLOW_ERROR_TYPE_ACTION, > > + NULL, "cannot create encap action"); > > + return verbs_action; > > +} > > + > > +/** > > * Verify the @p attributes will be correctly understood by the NIC and > store > > * them in the @p flow if everything is correct. > > * > > @@ -726,7 +858,6 @@ > > > > RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP ? > > > > MLX5_FLOW_ACTION_VXLAN_ENCAP : > > > > MLX5_FLOW_ACTION_NVGRE_ENCAP; > > - > > ++actions_n; > > break; > > case RTE_FLOW_ACTION_TYPE_VXLAN_DECAP: > > @@ -740,7 +871,23 @@ > > > > RTE_FLOW_ACTION_TYPE_VXLAN_DECAP ? > > > > MLX5_FLOW_ACTION_VXLAN_DECAP : > > > > MLX5_FLOW_ACTION_NVGRE_DECAP; > > - > > + ++actions_n; > > + break; > > + case RTE_FLOW_ACTION_TYPE_RAW_ENCAP: > > + ret = > > flow_dv_validate_action_raw_encap(action_flags, > > + actions, attr, > > + error); > > + if (ret < 0) > > + return ret; > > + action_flags |= MLX5_FLOW_ACTION_RAW_ENCAP; > > + ++actions_n; > > + break; > > + case RTE_FLOW_ACTION_TYPE_RAW_DECAP: > > + ret = > > flow_dv_validate_action_raw_decap(action_flags, > > + error); > > + if (ret < 0) > > + return ret; > > + action_flags |= MLX5_FLOW_ACTION_RAW_DECAP; > > ++actions_n; > > break; > > default: > > @@ -1550,6 +1697,64 @@ > > MLX5_FLOW_ACTION_NVGRE_DECAP; > > actions_n++; > > break; > > + case RTE_FLOW_ACTION_TYPE_RAW_ENCAP: > > + /* Handle encap action with preceding decap */ > > + if (flow->actions & MLX5_FLOW_ACTION_RAW_DECAP) { > > + dev_flow->dv.actions[actions_n].type = > > + > > MLX5DV_FLOW_ACTION_IBV_FLOW_ACTION; > > + dev_flow->dv.actions[actions_n].action = > > + flow_dv_create_action_raw_encap > > + (dev, action, > > + attr, error); > > + if (!(dev_flow->dv.actions[actions_n].action)) > > + return -rte_errno; > > + dev_flow->dv.verbs_action = > > + dev_flow->dv.actions[actions_n].action; > > + } else { > > + /* Handle encap action without preceding decap */ > > + dev_flow->dv.actions[actions_n].type = > > + > > MLX5DV_FLOW_ACTION_IBV_FLOW_ACTION; > > + dev_flow->dv.actions[actions_n].action = > > + flow_dv_create_action_l2_encap > > + (dev, action, error); > > + if (!(dev_flow->dv.actions[actions_n].action)) > > + return -rte_errno; > > + dev_flow->dv.verbs_action = > > + dev_flow->dv.actions[actions_n].action; > > + } > > + flow->actions |= MLX5_FLOW_ACTION_RAW_ENCAP; > > + actions_n++; > > + break; > > + case RTE_FLOW_ACTION_TYPE_RAW_DECAP: > > + /* Check if this decap action is followed by encap. */ > > + for (; action->type != RTE_FLOW_ACTION_TYPE_END && > > + action->type != RTE_FLOW_ACTION_TYPE_RAW_ENCAP; > > + action++) { > > + } > > + /* Handle decap action only if it isn't followed by encap */ > > + if (action->type != RTE_FLOW_ACTION_TYPE_RAW_ENCAP) { > > + /* on egress, decap without following encap is error. > > */ > > + if (attr->egress) > > + return rte_flow_error_set > > + (error, ENOTSUP, > > + > > RTE_FLOW_ERROR_TYPE_ATTR_EGRESS, > > + NULL, > > + "decap action not supported for " > > + "egress"); > > This check should have been performed on the validate stage. I will move it. > > > + dev_flow->dv.actions[actions_n].type = > > + > > MLX5DV_FLOW_ACTION_IBV_FLOW_ACTION; > > + dev_flow->dv.actions[actions_n].action = > > + flow_dv_create_action_l2_decap > > + (dev, action, error); > > + if (!(dev_flow->dv.actions[actions_n].action)) > > + return -rte_errno; > > + dev_flow->dv.verbs_action = > > + dev_flow->dv.actions[actions_n].action; > > + actions_n++; > > + } > > + /* If decap is followed by encap, handle it at encap case. */ > > + flow->actions |= MLX5_FLOW_ACTION_RAW_DECAP; > > + break; > > default: > > break; > > } > > -- > > 1.8.3.1