* [dpdk-dev] [PATCH] net/mlx5: fix the modify checking in flow validate
@ 2020-02-19 14:27 Bing Zhao
2020-04-13 14:37 ` [dpdk-dev] [PATCH v2] " Bing Zhao
0 siblings, 1 reply; 5+ messages in thread
From: Bing Zhao @ 2020-02-19 14:27 UTC (permalink / raw)
To: orika, viacheslavo; +Cc: rasland, matan, dev, stable
The header modify actions number supported now has some limitation,
and it is decided by both driver and hardware. If the configuration
is different or the table to insert the flow is different, the result
might be different if the flow contains header modify actions.
Currently, the actual action number could only be calculated in the
later stage called translate, from user specified value to the driver
format. And the action numbers checking is missed in the flow
validation. So PMD will return incorrect result to indicate the
flow actions are valid by rte_flow_validate but then it will fail
when calling rte_flow_create.
Adding some simple checking in the validation will help to get rid
of this incorrect checking. Most of the actions will only consume 1
SW action field except the MAC address and IPv6 address. And from
SW POV, the maximal action fields for these will be consumed even if
only part of such field will be modified because that there is no
mask in the flow actions and the mask will always be all ONEs.
Fixes: 9597330c6844 ("net/mlx5: update modify header action translator")
Cc: viacheslavo@mellanox.com
Cc: stable@dpdk.org
Signed-off-by: Bing Zhao <bingz@mellanox.com>
---
drivers/net/mlx5/mlx5_flow.h | 17 +++++++++++++++
drivers/net/mlx5/mlx5_flow_dv.c | 47 ++++++++++++++++++++++++++++++++++++-----
2 files changed, 59 insertions(+), 5 deletions(-)
diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
index 791f3bd..45db487 100644
--- a/drivers/net/mlx5/mlx5_flow.h
+++ b/drivers/net/mlx5/mlx5_flow.h
@@ -328,6 +328,23 @@ enum mlx5_feature_name {
#define MLX5_GENEVE_OPT_LEN_0 14
#define MLX5_GENEVE_OPT_LEN_1 63
+/* Software header modify action numbers of a flow. */
+#define MLX5_ACT_NUM_MDF_IPV4 1
+#define MLX5_ACT_NUM_MDF_IPV6 4
+#define MLX5_ACT_NUM_MDF_MAC 2
+#define MLX5_ACT_NUM_MDF_VID 1
+#define MLX5_ACT_NUM_MDF_PORT 2
+#define MLX5_ACT_NUM_MDF_TTL 1
+#define MLX5_ACT_NUM_DEC_TTL MLX5_ACT_NUM_MDF_TTL
+#define MLX5_ACT_NUM_MDF_TCPSEQ 1
+#define MLX5_ACT_NUM_MDF_TCPACK 1
+#define MLX5_ACT_NUM_SET_REG 1
+#define MLX5_ACT_NUM_SET_TAG 0
+#define MLX5_ACT_NUM_CPY_MREG MLX5_ACT_NUM_SET_TAG
+#define MLX5_ACT_NUM_SET_MARK 1
+#define MLX5_ACT_NUM_SET_META 1
+#define MLX5_ACT_NUM_SET_DSCP 1
+
enum mlx5_flow_drv_type {
MLX5_FLOW_TYPE_MIN,
MLX5_FLOW_TYPE_DV,
diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 467d1ce..f79203a 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -375,6 +375,7 @@ struct field_modify_info modify_tcp[] = {
/* Fetch variable byte size mask from the array. */
mask = flow_dv_fetch_field((const uint8_t *)item->mask +
field->offset, field->size);
+ MLX5_ASSERT(mask);
if (!mask) {
++field;
continue;
@@ -4302,7 +4303,9 @@ struct field_modify_info modify_tcp[] = {
* Pointer to error structure.
*
* @return
- * 0 on success, a negative errno value otherwise and rte_errno is set.
+ * - 0 on success and non root table.
+ * - 1 on success and root table.
+ * - a negative errno value otherwise and rte_errno is set.
*/
static int
flow_dv_validate_attributes(struct rte_eth_dev *dev,
@@ -4312,6 +4315,7 @@ struct field_modify_info modify_tcp[] = {
{
struct mlx5_priv *priv = dev->data->dev_private;
uint32_t priority_max = priv->config.flow_prio - 1;
+ int ret = 0;
#ifndef HAVE_MLX5DV_DR
if (attributes->group)
@@ -4321,13 +4325,14 @@ struct field_modify_info modify_tcp[] = {
"groups are not supported");
#else
uint32_t table;
- int ret;
ret = mlx5_flow_group_to_table(attributes, external,
attributes->group, !!priv->fdb_def_rule,
&table, error);
if (ret)
return ret;
+ if (!table)
+ ret = MLX5DV_DR_ACTION_FLAGS_ROOT_LEVEL;
#endif
if (attributes->priority != MLX5_FLOW_PRIO_RSVD &&
attributes->priority >= priority_max)
@@ -4357,7 +4362,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ERROR_TYPE_ATTR, NULL,
"must specify exactly one of "
"ingress or egress");
- return 0;
+ return ret;
}
/**
@@ -4407,12 +4412,15 @@ struct field_modify_info modify_tcp[] = {
struct mlx5_priv *priv = dev->data->dev_private;
struct mlx5_dev_config *dev_conf = &priv->config;
uint16_t queue_index = 0xFFFF;
+ int16_t rw_act_num = 0;
+ uint64_t is_root;
if (items == NULL)
return -1;
ret = flow_dv_validate_attributes(dev, attr, external, error);
if (ret < 0)
return ret;
+ is_root = (uint64_t)ret;
for (; items->type != RTE_FLOW_ITEM_TYPE_END; items++) {
int tunnel = !!(item_flags & MLX5_FLOW_LAYER_TUNNEL);
int type = items->type;
@@ -4684,6 +4692,7 @@ struct field_modify_info modify_tcp[] = {
action_flags |= MLX5_FLOW_ACTION_FLAG;
++actions_n;
}
+ rw_act_num += MLX5_ACT_NUM_SET_MARK;
break;
case RTE_FLOW_ACTION_TYPE_MARK:
ret = flow_dv_validate_action_mark(dev, actions,
@@ -4702,6 +4711,7 @@ struct field_modify_info modify_tcp[] = {
action_flags |= MLX5_FLOW_ACTION_MARK;
++actions_n;
}
+ rw_act_num += MLX5_ACT_NUM_SET_MARK;
break;
case RTE_FLOW_ACTION_TYPE_SET_META:
ret = flow_dv_validate_action_set_meta(dev, actions,
@@ -4713,6 +4723,7 @@ struct field_modify_info modify_tcp[] = {
if (!(action_flags & MLX5_FLOW_MODIFY_HDR_ACTIONS))
++actions_n;
action_flags |= MLX5_FLOW_ACTION_SET_META;
+ rw_act_num += MLX5_ACT_NUM_SET_META;
break;
case RTE_FLOW_ACTION_TYPE_SET_TAG:
ret = flow_dv_validate_action_set_tag(dev, actions,
@@ -4724,6 +4735,7 @@ struct field_modify_info modify_tcp[] = {
if (!(action_flags & MLX5_FLOW_MODIFY_HDR_ACTIONS))
++actions_n;
action_flags |= MLX5_FLOW_ACTION_SET_TAG;
+ rw_act_num += MLX5_ACT_NUM_SET_TAG;
break;
case RTE_FLOW_ACTION_TYPE_DROP:
ret = mlx5_flow_validate_action_drop(action_flags,
@@ -4800,6 +4812,7 @@ struct field_modify_info modify_tcp[] = {
return ret;
/* Count VID with push_vlan command. */
action_flags |= MLX5_FLOW_ACTION_OF_SET_VLAN_VID;
+ rw_act_num += MLX5_ACT_NUM_MDF_VID;
break;
case RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP:
case RTE_FLOW_ACTION_TYPE_NVGRE_ENCAP:
@@ -4858,8 +4871,15 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_MAC_SRC ?
MLX5_FLOW_ACTION_SET_MAC_SRC :
MLX5_FLOW_ACTION_SET_MAC_DST;
+ /*
+ * Even if the source and destination MAC addresses have
+ * overlap in the header with 4B alignment, the convert
+ * function will handle them separately and 4 SW actions
+ * will be created. And 2 actions will be added each
+ * time no matter how many bytes of address will be set.
+ */
+ rw_act_num += MLX5_ACT_NUM_MDF_MAC;
break;
-
case RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC:
case RTE_FLOW_ACTION_TYPE_SET_IPV4_DST:
ret = flow_dv_validate_action_modify_ipv4(action_flags,
@@ -4875,6 +4895,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC ?
MLX5_FLOW_ACTION_SET_IPV4_SRC :
MLX5_FLOW_ACTION_SET_IPV4_DST;
+ rw_act_num += MLX5_ACT_NUM_MDF_IPV4;
break;
case RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC:
case RTE_FLOW_ACTION_TYPE_SET_IPV6_DST:
@@ -4897,6 +4918,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC ?
MLX5_FLOW_ACTION_SET_IPV6_SRC :
MLX5_FLOW_ACTION_SET_IPV6_DST;
+ rw_act_num += MLX5_ACT_NUM_MDF_IPV6;
break;
case RTE_FLOW_ACTION_TYPE_SET_TP_SRC:
case RTE_FLOW_ACTION_TYPE_SET_TP_DST:
@@ -4913,6 +4935,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_TP_SRC ?
MLX5_FLOW_ACTION_SET_TP_SRC :
MLX5_FLOW_ACTION_SET_TP_DST;
+ rw_act_num += MLX5_ACT_NUM_MDF_PORT;
break;
case RTE_FLOW_ACTION_TYPE_DEC_TTL:
case RTE_FLOW_ACTION_TYPE_SET_TTL:
@@ -4929,6 +4952,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_TTL ?
MLX5_FLOW_ACTION_SET_TTL :
MLX5_FLOW_ACTION_DEC_TTL;
+ rw_act_num += MLX5_ACT_NUM_MDF_TTL;
break;
case RTE_FLOW_ACTION_TYPE_JUMP:
ret = flow_dv_validate_action_jump(actions,
@@ -4956,6 +4980,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ ?
MLX5_FLOW_ACTION_INC_TCP_SEQ :
MLX5_FLOW_ACTION_DEC_TCP_SEQ;
+ rw_act_num += MLX5_ACT_NUM_MDF_TCPSEQ;
break;
case RTE_FLOW_ACTION_TYPE_INC_TCP_ACK:
case RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK:
@@ -4973,10 +4998,13 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_INC_TCP_ACK ?
MLX5_FLOW_ACTION_INC_TCP_ACK :
MLX5_FLOW_ACTION_DEC_TCP_ACK;
+ rw_act_num += MLX5_ACT_NUM_MDF_TCPACK;
break;
- case MLX5_RTE_FLOW_ACTION_TYPE_TAG:
case MLX5_RTE_FLOW_ACTION_TYPE_MARK:
+ break;
+ case MLX5_RTE_FLOW_ACTION_TYPE_TAG:
case MLX5_RTE_FLOW_ACTION_TYPE_COPY_MREG:
+ rw_act_num += MLX5_ACT_NUM_SET_TAG;
break;
case RTE_FLOW_ACTION_TYPE_METER:
ret = mlx5_flow_validate_action_meter(dev,
@@ -5000,6 +5028,7 @@ struct field_modify_info modify_tcp[] = {
if (!(action_flags & MLX5_FLOW_MODIFY_HDR_ACTIONS))
++actions_n;
action_flags |= MLX5_FLOW_ACTION_SET_IPV4_DSCP;
+ rw_act_num += MLX5_ACT_NUM_SET_DSCP;
break;
case RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP:
ret = flow_dv_validate_action_modify_ipv6_dscp
@@ -5013,6 +5042,7 @@ struct field_modify_info modify_tcp[] = {
if (!(action_flags & MLX5_FLOW_MODIFY_HDR_ACTIONS))
++actions_n;
action_flags |= MLX5_FLOW_ACTION_SET_IPV6_DSCP;
+ rw_act_num += MLX5_ACT_NUM_SET_DSCP;
break;
default:
return rte_flow_error_set(error, ENOTSUP,
@@ -5085,6 +5115,13 @@ struct field_modify_info modify_tcp[] = {
NULL, "encap is not supported"
" for ingress traffic");
}
+ if ((uint32_t)rw_act_num >=
+ flow_dv_modify_hdr_action_max(dev, is_root)) {
+ return rte_flow_error_set(error, ENOTSUP,
+ RTE_FLOW_ERROR_TYPE_ACTION,
+ NULL, "too many header modify"
+ " actions to support");
+ }
return 0;
}
--
1.8.3.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [dpdk-dev] [PATCH v2] net/mlx5: fix the modify checking in flow validate
2020-02-19 14:27 [dpdk-dev] [PATCH] net/mlx5: fix the modify checking in flow validate Bing Zhao
@ 2020-04-13 14:37 ` Bing Zhao
2020-04-14 7:53 ` [dpdk-dev] [PATCH v3] net/mlx5: fix header modify action validation Bing Zhao
0 siblings, 1 reply; 5+ messages in thread
From: Bing Zhao @ 2020-04-13 14:37 UTC (permalink / raw)
To: viacheslavo, rasland; +Cc: orika, matan, dev, stable
The header modify actions number supported now has some limitation,
and it is decided by both driver and hardware. If the configuration
is different or the table to insert the flow is different, the result
might be different if the flow contains header modify actions.
Currently, the actual action number could only be calculated in the
later stage called translate, from user specified value to the driver
format. And the action numbers checking is missed in the flow
validation. So PMD will return incorrect result to indicate the
flow actions are valid by rte_flow_validate but then it will fail
when calling rte_flow_create.
Adding some simple checking in the validation will help to get rid
of this incorrect checking. Most of the actions will only consume 1
SW action field except the MAC address and IPv6 address. And from
SW POV, the maximal action fields for these will be consumed even if
only part of such field will be modified because that there is no
mask in the flow actions and the mask will always be all ONEs.
The metering or extra metadata supports will cost one more action.
Fixes: 9597330c6844 ("net/mlx5: update modify header action translator")
Cc: viacheslavo@mellanox.com
Cc: stable@dpdk.org
Signed-off-by: Bing Zhao <bingz@mellanox.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
v2: take hairpin, metadata and meter into consideration
---
drivers/net/mlx5/mlx5_flow.c | 58 +++++++++++++++++++----------------
drivers/net/mlx5/mlx5_flow.h | 18 +++++++++++
drivers/net/mlx5/mlx5_flow_dv.c | 63 +++++++++++++++++++++++++++++++++-----
drivers/net/mlx5/mlx5_flow_verbs.c | 3 ++
4 files changed, 108 insertions(+), 34 deletions(-)
diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index c44bc1f..954cf74 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -2326,6 +2326,7 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
const struct rte_flow_item items[] __rte_unused,
const struct rte_flow_action actions[] __rte_unused,
bool external __rte_unused,
+ int hairpin __rte_unused,
struct rte_flow_error *error)
{
return rte_flow_error_set(error, ENOTSUP,
@@ -2441,6 +2442,8 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
* Pointer to the list of actions.
* @param[in] external
* This flow rule is created by request external to PMD.
+ * @param[in] hairpin
+ * Number of hairpin TX actions, 0 means classic flow.
* @param[out] error
* Pointer to the error structure.
*
@@ -2452,13 +2455,14 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
const struct rte_flow_attr *attr,
const struct rte_flow_item items[],
const struct rte_flow_action actions[],
- bool external, struct rte_flow_error *error)
+ bool external, int hairpin, struct rte_flow_error *error)
{
const struct mlx5_flow_driver_ops *fops;
enum mlx5_flow_drv_type type = flow_get_drv_type(dev, attr);
fops = flow_get_drv_ops(type);
- return fops->validate(dev, attr, items, actions, external, error);
+ return fops->validate(dev, attr, items, actions, external,
+ hairpin, error);
}
/**
@@ -2620,27 +2624,6 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
}
/**
- * Validate a flow supported by the NIC.
- *
- * @see rte_flow_validate()
- * @see rte_flow_ops
- */
-int
-mlx5_flow_validate(struct rte_eth_dev *dev,
- const struct rte_flow_attr *attr,
- const struct rte_flow_item items[],
- const struct rte_flow_action actions[],
- struct rte_flow_error *error)
-{
- int ret;
-
- ret = flow_drv_validate(dev, attr, items, actions, true, error);
- if (ret < 0)
- return ret;
- return 0;
-}
-
-/**
* Get RSS action from the action list.
*
* @param[in] actions
@@ -4218,15 +4201,16 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
const struct rte_flow_action *p_actions_rx = actions;
uint32_t i;
uint32_t flow_size;
- int hairpin_flow = 0;
+ int hairpin_flow;
uint32_t hairpin_id = 0;
struct rte_flow_attr attr_tx = { .priority = 0 };
- int ret = flow_drv_validate(dev, attr, items, p_actions_rx, external,
- error);
+ int ret;
+ hairpin_flow = flow_check_hairpin_split(dev, attr, actions);
+ ret = flow_drv_validate(dev, attr, items, p_actions_rx,
+ external, hairpin_flow, error);
if (ret < 0)
return NULL;
- hairpin_flow = flow_check_hairpin_split(dev, attr, actions);
if (hairpin_flow > 0) {
if (hairpin_flow > MLX5_MAX_SPLIT_ACTIONS) {
rte_errno = EINVAL;
@@ -4421,6 +4405,26 @@ struct rte_flow *
}
/**
+ * Validate a flow supported by the NIC.
+ *
+ * @see rte_flow_validate()
+ * @see rte_flow_ops
+ */
+int
+mlx5_flow_validate(struct rte_eth_dev *dev,
+ const struct rte_flow_attr *attr,
+ const struct rte_flow_item items[],
+ const struct rte_flow_action actions[],
+ struct rte_flow_error *error)
+{
+ int hairpin_flow;
+
+ hairpin_flow = flow_check_hairpin_split(dev, attr, actions);
+ return flow_drv_validate(dev, attr, items, actions,
+ true, hairpin_flow, error);
+}
+
+/**
* Create a flow.
*
* @see rte_flow_create()
diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
index daa1f84..a8fc104 100644
--- a/drivers/net/mlx5/mlx5_flow.h
+++ b/drivers/net/mlx5/mlx5_flow.h
@@ -332,6 +332,23 @@ enum mlx5_feature_name {
#define MLX5_ENCAPSULATION_DECISION_SIZE (sizeof(struct rte_flow_item_eth) + \
sizeof(struct rte_flow_item_ipv4))
+/* Software header modify action numbers of a flow. */
+#define MLX5_ACT_NUM_MDF_IPV4 1
+#define MLX5_ACT_NUM_MDF_IPV6 4
+#define MLX5_ACT_NUM_MDF_MAC 2
+#define MLX5_ACT_NUM_MDF_VID 1
+#define MLX5_ACT_NUM_MDF_PORT 2
+#define MLX5_ACT_NUM_MDF_TTL 1
+#define MLX5_ACT_NUM_DEC_TTL MLX5_ACT_NUM_MDF_TTL
+#define MLX5_ACT_NUM_MDF_TCPSEQ 1
+#define MLX5_ACT_NUM_MDF_TCPACK 1
+#define MLX5_ACT_NUM_SET_REG 1
+#define MLX5_ACT_NUM_SET_TAG 1
+#define MLX5_ACT_NUM_CPY_MREG MLX5_ACT_NUM_SET_TAG
+#define MLX5_ACT_NUM_SET_MARK MLX5_ACT_NUM_SET_TAG
+#define MLX5_ACT_NUM_SET_META MLX5_ACT_NUM_SET_TAG
+#define MLX5_ACT_NUM_SET_DSCP 1
+
enum mlx5_flow_drv_type {
MLX5_FLOW_TYPE_MIN,
MLX5_FLOW_TYPE_DV,
@@ -745,6 +762,7 @@ typedef int (*mlx5_flow_validate_t)(struct rte_eth_dev *dev,
const struct rte_flow_item items[],
const struct rte_flow_action actions[],
bool external,
+ int hairpin,
struct rte_flow_error *error);
typedef struct mlx5_flow *(*mlx5_flow_prepare_t)
(struct rte_eth_dev *dev, const struct rte_flow_attr *attr,
diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 18ea577..51bdffa 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -430,6 +430,7 @@ struct field_modify_info modify_tcp[] = {
/* Fetch variable byte size mask from the array. */
mask = flow_dv_fetch_field((const uint8_t *)item->mask +
field->offset, field->size);
+ MLX5_ASSERT(mask);
if (!mask) {
++field;
continue;
@@ -4436,7 +4437,9 @@ struct field_modify_info modify_tcp[] = {
* Pointer to error structure.
*
* @return
- * 0 on success, a negative errno value otherwise and rte_errno is set.
+ * - 0 on success and non root table.
+ * - 1 on success and root table.
+ * - a negative errno value otherwise and rte_errno is set.
*/
static int
flow_dv_validate_attributes(struct rte_eth_dev *dev,
@@ -4446,6 +4449,7 @@ struct field_modify_info modify_tcp[] = {
{
struct mlx5_priv *priv = dev->data->dev_private;
uint32_t priority_max = priv->config.flow_prio - 1;
+ int ret = 0;
#ifndef HAVE_MLX5DV_DR
if (attributes->group)
@@ -4454,14 +4458,15 @@ struct field_modify_info modify_tcp[] = {
NULL,
"groups are not supported");
#else
- uint32_t table;
- int ret;
+ uint32_t table = 0;
ret = mlx5_flow_group_to_table(attributes, external,
attributes->group, !!priv->fdb_def_rule,
&table, error);
if (ret)
return ret;
+ if (!table)
+ ret = MLX5DV_DR_ACTION_FLAGS_ROOT_LEVEL;
#endif
if (attributes->priority != MLX5_FLOW_PRIO_RSVD &&
attributes->priority >= priority_max)
@@ -4491,7 +4496,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ERROR_TYPE_ATTR, NULL,
"must specify exactly one of "
"ingress or egress");
- return 0;
+ return ret;
}
/**
@@ -4507,6 +4512,8 @@ struct field_modify_info modify_tcp[] = {
* Pointer to the list of actions.
* @param[in] external
* This flow rule is created by request external to PMD.
+ * @param[in] hairpin
+ * Number of hairpin TX actions, 0 means classic flow.
* @param[out] error
* Pointer to the error structure.
*
@@ -4517,7 +4524,7 @@ struct field_modify_info modify_tcp[] = {
flow_dv_validate(struct rte_eth_dev *dev, const struct rte_flow_attr *attr,
const struct rte_flow_item items[],
const struct rte_flow_action actions[],
- bool external, struct rte_flow_error *error)
+ bool external, int hairpin, struct rte_flow_error *error)
{
int ret;
uint64_t action_flags = 0;
@@ -4563,12 +4570,15 @@ struct field_modify_info modify_tcp[] = {
struct mlx5_priv *priv = dev->data->dev_private;
struct mlx5_dev_config *dev_conf = &priv->config;
uint16_t queue_index = 0xFFFF;
+ int16_t rw_act_num = 0;
+ uint64_t is_root;
if (items == NULL)
return -1;
ret = flow_dv_validate_attributes(dev, attr, external, error);
if (ret < 0)
return ret;
+ is_root = (uint64_t)ret;
for (; items->type != RTE_FLOW_ITEM_TYPE_END; items++) {
int tunnel = !!(item_flags & MLX5_FLOW_LAYER_TUNNEL);
int type = items->type;
@@ -4842,6 +4852,7 @@ struct field_modify_info modify_tcp[] = {
action_flags |= MLX5_FLOW_ACTION_FLAG;
++actions_n;
}
+ rw_act_num += MLX5_ACT_NUM_SET_MARK;
break;
case RTE_FLOW_ACTION_TYPE_MARK:
ret = flow_dv_validate_action_mark(dev, actions,
@@ -4860,6 +4871,7 @@ struct field_modify_info modify_tcp[] = {
action_flags |= MLX5_FLOW_ACTION_MARK;
++actions_n;
}
+ rw_act_num += MLX5_ACT_NUM_SET_MARK;
break;
case RTE_FLOW_ACTION_TYPE_SET_META:
ret = flow_dv_validate_action_set_meta(dev, actions,
@@ -4871,6 +4883,7 @@ struct field_modify_info modify_tcp[] = {
if (!(action_flags & MLX5_FLOW_MODIFY_HDR_ACTIONS))
++actions_n;
action_flags |= MLX5_FLOW_ACTION_SET_META;
+ rw_act_num += MLX5_ACT_NUM_SET_META;
break;
case RTE_FLOW_ACTION_TYPE_SET_TAG:
ret = flow_dv_validate_action_set_tag(dev, actions,
@@ -4882,6 +4895,7 @@ struct field_modify_info modify_tcp[] = {
if (!(action_flags & MLX5_FLOW_MODIFY_HDR_ACTIONS))
++actions_n;
action_flags |= MLX5_FLOW_ACTION_SET_TAG;
+ rw_act_num += MLX5_ACT_NUM_SET_TAG;
break;
case RTE_FLOW_ACTION_TYPE_DROP:
ret = mlx5_flow_validate_action_drop(action_flags,
@@ -4959,6 +4973,7 @@ struct field_modify_info modify_tcp[] = {
return ret;
/* Count VID with push_vlan command. */
action_flags |= MLX5_FLOW_ACTION_OF_SET_VLAN_VID;
+ rw_act_num += MLX5_ACT_NUM_MDF_VID;
break;
case RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP:
case RTE_FLOW_ACTION_TYPE_NVGRE_ENCAP:
@@ -5020,8 +5035,15 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_MAC_SRC ?
MLX5_FLOW_ACTION_SET_MAC_SRC :
MLX5_FLOW_ACTION_SET_MAC_DST;
+ /*
+ * Even if the source and destination MAC addresses have
+ * overlap in the header with 4B alignment, the convert
+ * function will handle them separately and 4 SW actions
+ * will be created. And 2 actions will be added each
+ * time no matter how many bytes of address will be set.
+ */
+ rw_act_num += MLX5_ACT_NUM_MDF_MAC;
break;
-
case RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC:
case RTE_FLOW_ACTION_TYPE_SET_IPV4_DST:
ret = flow_dv_validate_action_modify_ipv4(action_flags,
@@ -5037,6 +5059,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC ?
MLX5_FLOW_ACTION_SET_IPV4_SRC :
MLX5_FLOW_ACTION_SET_IPV4_DST;
+ rw_act_num += MLX5_ACT_NUM_MDF_IPV4;
break;
case RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC:
case RTE_FLOW_ACTION_TYPE_SET_IPV6_DST:
@@ -5059,6 +5082,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC ?
MLX5_FLOW_ACTION_SET_IPV6_SRC :
MLX5_FLOW_ACTION_SET_IPV6_DST;
+ rw_act_num += MLX5_ACT_NUM_MDF_IPV6;
break;
case RTE_FLOW_ACTION_TYPE_SET_TP_SRC:
case RTE_FLOW_ACTION_TYPE_SET_TP_DST:
@@ -5075,6 +5099,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_TP_SRC ?
MLX5_FLOW_ACTION_SET_TP_SRC :
MLX5_FLOW_ACTION_SET_TP_DST;
+ rw_act_num += MLX5_ACT_NUM_MDF_PORT;
break;
case RTE_FLOW_ACTION_TYPE_DEC_TTL:
case RTE_FLOW_ACTION_TYPE_SET_TTL:
@@ -5091,6 +5116,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_TTL ?
MLX5_FLOW_ACTION_SET_TTL :
MLX5_FLOW_ACTION_DEC_TTL;
+ rw_act_num += MLX5_ACT_NUM_MDF_TTL;
break;
case RTE_FLOW_ACTION_TYPE_JUMP:
ret = flow_dv_validate_action_jump(actions,
@@ -5118,6 +5144,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ ?
MLX5_FLOW_ACTION_INC_TCP_SEQ :
MLX5_FLOW_ACTION_DEC_TCP_SEQ;
+ rw_act_num += MLX5_ACT_NUM_MDF_TCPSEQ;
break;
case RTE_FLOW_ACTION_TYPE_INC_TCP_ACK:
case RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK:
@@ -5135,10 +5162,13 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_INC_TCP_ACK ?
MLX5_FLOW_ACTION_INC_TCP_ACK :
MLX5_FLOW_ACTION_DEC_TCP_ACK;
+ rw_act_num += MLX5_ACT_NUM_MDF_TCPACK;
break;
- case MLX5_RTE_FLOW_ACTION_TYPE_TAG:
case MLX5_RTE_FLOW_ACTION_TYPE_MARK:
+ break;
+ case MLX5_RTE_FLOW_ACTION_TYPE_TAG:
case MLX5_RTE_FLOW_ACTION_TYPE_COPY_MREG:
+ rw_act_num += MLX5_ACT_NUM_SET_TAG;
break;
case RTE_FLOW_ACTION_TYPE_METER:
ret = mlx5_flow_validate_action_meter(dev,
@@ -5149,6 +5179,8 @@ struct field_modify_info modify_tcp[] = {
return ret;
action_flags |= MLX5_FLOW_ACTION_METER;
++actions_n;
+ /* Meter action will add one more TAG action. */
+ rw_act_num += MLX5_ACT_NUM_SET_TAG;
break;
case RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP:
ret = flow_dv_validate_action_modify_ipv4_dscp
@@ -5162,6 +5194,7 @@ struct field_modify_info modify_tcp[] = {
if (!(action_flags & MLX5_FLOW_MODIFY_HDR_ACTIONS))
++actions_n;
action_flags |= MLX5_FLOW_ACTION_SET_IPV4_DSCP;
+ rw_act_num += MLX5_ACT_NUM_SET_DSCP;
break;
case RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP:
ret = flow_dv_validate_action_modify_ipv6_dscp
@@ -5175,6 +5208,7 @@ struct field_modify_info modify_tcp[] = {
if (!(action_flags & MLX5_FLOW_MODIFY_HDR_ACTIONS))
++actions_n;
action_flags |= MLX5_FLOW_ACTION_SET_IPV6_DSCP;
+ rw_act_num += MLX5_ACT_NUM_SET_DSCP;
break;
default:
return rte_flow_error_set(error, ENOTSUP,
@@ -5247,6 +5281,21 @@ struct field_modify_info modify_tcp[] = {
NULL, "encap is not supported"
" for ingress traffic");
}
+ /* Hairpin flow will add one more TAG action. */
+ if (hairpin > 0)
+ rw_act_num += MLX5_ACT_NUM_SET_TAG;
+ /* extra metadata enabled: one more TAG action will be add. */
+ if (dev_conf->dv_flow_en &&
+ dev_conf->dv_xmeta_en != MLX5_XMETA_MODE_LEGACY &&
+ mlx5_flow_ext_mreg_supported(dev))
+ rw_act_num += MLX5_ACT_NUM_SET_TAG;
+ if ((uint32_t)rw_act_num >=
+ flow_dv_modify_hdr_action_max(dev, is_root)) {
+ return rte_flow_error_set(error, ENOTSUP,
+ RTE_FLOW_ERROR_TYPE_ACTION,
+ NULL, "too many header modify"
+ " actions to support");
+ }
return 0;
}
diff --git a/drivers/net/mlx5/mlx5_flow_verbs.c b/drivers/net/mlx5/mlx5_flow_verbs.c
index ef4d7a3..6595ecf 100644
--- a/drivers/net/mlx5/mlx5_flow_verbs.c
+++ b/drivers/net/mlx5/mlx5_flow_verbs.c
@@ -1115,6 +1115,8 @@
* Pointer to the list of actions.
* @param[in] external
* This flow rule is created by request external to PMD.
+ * @param[in] hairpin
+ * Number of hairpin TX actions, 0 means classic flow.
* @param[out] error
* Pointer to the error structure.
*
@@ -1127,6 +1129,7 @@
const struct rte_flow_item items[],
const struct rte_flow_action actions[],
bool external __rte_unused,
+ int hairpin __rte_unused,
struct rte_flow_error *error)
{
int ret;
--
1.8.3.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [dpdk-dev] [PATCH v3] net/mlx5: fix header modify action validation
2020-04-13 14:37 ` [dpdk-dev] [PATCH v2] " Bing Zhao
@ 2020-04-14 7:53 ` Bing Zhao
2020-04-21 14:03 ` [dpdk-dev] [PATCH v4] " Bing Zhao
0 siblings, 1 reply; 5+ messages in thread
From: Bing Zhao @ 2020-04-14 7:53 UTC (permalink / raw)
To: viacheslavo, rasland; +Cc: orika, matan, dev, stable
The header modify actions number supported now has some limitation,
and it is decided by both driver and hardware. If the configuration
is different or the table to insert the flow is different, the result
might be different if the flow contains header modify actions.
Currently, the actual action number could only be calculated in the
later stage called translate, from user specified value to the driver
format. And the action numbers checking is missed in the flow
validation. So PMD will return incorrect result to indicate the
flow actions are valid by rte_flow_validate but then it will fail
when calling rte_flow_create.
Adding some simple checking in the validation will help to get rid
of this incorrect checking. Most of the actions will only consume 1
SW action field except the MAC address and IPv6 address. And from
SW POV, the maximal action fields for these will be consumed even if
only part of such field will be modified because that there is no
mask in the flow actions and the mask will always be all ONEs.
The metering or extra metadata supports will cost one more action.
Fixes: 9597330c6844 ("net/mlx5: update modify header action translator")
Cc: viacheslavo@mellanox.com
Cc: stable@dpdk.org
Signed-off-by: Bing Zhao <bingz@mellanox.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
v3: change the title of commit message
v2: take hairpin, metadata and meter into consideration
---
drivers/net/mlx5/mlx5_flow.c | 58 +++++++++++++++++++----------------
drivers/net/mlx5/mlx5_flow.h | 18 +++++++++++
drivers/net/mlx5/mlx5_flow_dv.c | 63 +++++++++++++++++++++++++++++++++-----
drivers/net/mlx5/mlx5_flow_verbs.c | 3 ++
4 files changed, 108 insertions(+), 34 deletions(-)
diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index c44bc1f..954cf74 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -2326,6 +2326,7 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
const struct rte_flow_item items[] __rte_unused,
const struct rte_flow_action actions[] __rte_unused,
bool external __rte_unused,
+ int hairpin __rte_unused,
struct rte_flow_error *error)
{
return rte_flow_error_set(error, ENOTSUP,
@@ -2441,6 +2442,8 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
* Pointer to the list of actions.
* @param[in] external
* This flow rule is created by request external to PMD.
+ * @param[in] hairpin
+ * Number of hairpin TX actions, 0 means classic flow.
* @param[out] error
* Pointer to the error structure.
*
@@ -2452,13 +2455,14 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
const struct rte_flow_attr *attr,
const struct rte_flow_item items[],
const struct rte_flow_action actions[],
- bool external, struct rte_flow_error *error)
+ bool external, int hairpin, struct rte_flow_error *error)
{
const struct mlx5_flow_driver_ops *fops;
enum mlx5_flow_drv_type type = flow_get_drv_type(dev, attr);
fops = flow_get_drv_ops(type);
- return fops->validate(dev, attr, items, actions, external, error);
+ return fops->validate(dev, attr, items, actions, external,
+ hairpin, error);
}
/**
@@ -2620,27 +2624,6 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
}
/**
- * Validate a flow supported by the NIC.
- *
- * @see rte_flow_validate()
- * @see rte_flow_ops
- */
-int
-mlx5_flow_validate(struct rte_eth_dev *dev,
- const struct rte_flow_attr *attr,
- const struct rte_flow_item items[],
- const struct rte_flow_action actions[],
- struct rte_flow_error *error)
-{
- int ret;
-
- ret = flow_drv_validate(dev, attr, items, actions, true, error);
- if (ret < 0)
- return ret;
- return 0;
-}
-
-/**
* Get RSS action from the action list.
*
* @param[in] actions
@@ -4218,15 +4201,16 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
const struct rte_flow_action *p_actions_rx = actions;
uint32_t i;
uint32_t flow_size;
- int hairpin_flow = 0;
+ int hairpin_flow;
uint32_t hairpin_id = 0;
struct rte_flow_attr attr_tx = { .priority = 0 };
- int ret = flow_drv_validate(dev, attr, items, p_actions_rx, external,
- error);
+ int ret;
+ hairpin_flow = flow_check_hairpin_split(dev, attr, actions);
+ ret = flow_drv_validate(dev, attr, items, p_actions_rx,
+ external, hairpin_flow, error);
if (ret < 0)
return NULL;
- hairpin_flow = flow_check_hairpin_split(dev, attr, actions);
if (hairpin_flow > 0) {
if (hairpin_flow > MLX5_MAX_SPLIT_ACTIONS) {
rte_errno = EINVAL;
@@ -4421,6 +4405,26 @@ struct rte_flow *
}
/**
+ * Validate a flow supported by the NIC.
+ *
+ * @see rte_flow_validate()
+ * @see rte_flow_ops
+ */
+int
+mlx5_flow_validate(struct rte_eth_dev *dev,
+ const struct rte_flow_attr *attr,
+ const struct rte_flow_item items[],
+ const struct rte_flow_action actions[],
+ struct rte_flow_error *error)
+{
+ int hairpin_flow;
+
+ hairpin_flow = flow_check_hairpin_split(dev, attr, actions);
+ return flow_drv_validate(dev, attr, items, actions,
+ true, hairpin_flow, error);
+}
+
+/**
* Create a flow.
*
* @see rte_flow_create()
diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
index daa1f84..a8fc104 100644
--- a/drivers/net/mlx5/mlx5_flow.h
+++ b/drivers/net/mlx5/mlx5_flow.h
@@ -332,6 +332,23 @@ enum mlx5_feature_name {
#define MLX5_ENCAPSULATION_DECISION_SIZE (sizeof(struct rte_flow_item_eth) + \
sizeof(struct rte_flow_item_ipv4))
+/* Software header modify action numbers of a flow. */
+#define MLX5_ACT_NUM_MDF_IPV4 1
+#define MLX5_ACT_NUM_MDF_IPV6 4
+#define MLX5_ACT_NUM_MDF_MAC 2
+#define MLX5_ACT_NUM_MDF_VID 1
+#define MLX5_ACT_NUM_MDF_PORT 2
+#define MLX5_ACT_NUM_MDF_TTL 1
+#define MLX5_ACT_NUM_DEC_TTL MLX5_ACT_NUM_MDF_TTL
+#define MLX5_ACT_NUM_MDF_TCPSEQ 1
+#define MLX5_ACT_NUM_MDF_TCPACK 1
+#define MLX5_ACT_NUM_SET_REG 1
+#define MLX5_ACT_NUM_SET_TAG 1
+#define MLX5_ACT_NUM_CPY_MREG MLX5_ACT_NUM_SET_TAG
+#define MLX5_ACT_NUM_SET_MARK MLX5_ACT_NUM_SET_TAG
+#define MLX5_ACT_NUM_SET_META MLX5_ACT_NUM_SET_TAG
+#define MLX5_ACT_NUM_SET_DSCP 1
+
enum mlx5_flow_drv_type {
MLX5_FLOW_TYPE_MIN,
MLX5_FLOW_TYPE_DV,
@@ -745,6 +762,7 @@ typedef int (*mlx5_flow_validate_t)(struct rte_eth_dev *dev,
const struct rte_flow_item items[],
const struct rte_flow_action actions[],
bool external,
+ int hairpin,
struct rte_flow_error *error);
typedef struct mlx5_flow *(*mlx5_flow_prepare_t)
(struct rte_eth_dev *dev, const struct rte_flow_attr *attr,
diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 18ea577..51bdffa 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -430,6 +430,7 @@ struct field_modify_info modify_tcp[] = {
/* Fetch variable byte size mask from the array. */
mask = flow_dv_fetch_field((const uint8_t *)item->mask +
field->offset, field->size);
+ MLX5_ASSERT(mask);
if (!mask) {
++field;
continue;
@@ -4436,7 +4437,9 @@ struct field_modify_info modify_tcp[] = {
* Pointer to error structure.
*
* @return
- * 0 on success, a negative errno value otherwise and rte_errno is set.
+ * - 0 on success and non root table.
+ * - 1 on success and root table.
+ * - a negative errno value otherwise and rte_errno is set.
*/
static int
flow_dv_validate_attributes(struct rte_eth_dev *dev,
@@ -4446,6 +4449,7 @@ struct field_modify_info modify_tcp[] = {
{
struct mlx5_priv *priv = dev->data->dev_private;
uint32_t priority_max = priv->config.flow_prio - 1;
+ int ret = 0;
#ifndef HAVE_MLX5DV_DR
if (attributes->group)
@@ -4454,14 +4458,15 @@ struct field_modify_info modify_tcp[] = {
NULL,
"groups are not supported");
#else
- uint32_t table;
- int ret;
+ uint32_t table = 0;
ret = mlx5_flow_group_to_table(attributes, external,
attributes->group, !!priv->fdb_def_rule,
&table, error);
if (ret)
return ret;
+ if (!table)
+ ret = MLX5DV_DR_ACTION_FLAGS_ROOT_LEVEL;
#endif
if (attributes->priority != MLX5_FLOW_PRIO_RSVD &&
attributes->priority >= priority_max)
@@ -4491,7 +4496,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ERROR_TYPE_ATTR, NULL,
"must specify exactly one of "
"ingress or egress");
- return 0;
+ return ret;
}
/**
@@ -4507,6 +4512,8 @@ struct field_modify_info modify_tcp[] = {
* Pointer to the list of actions.
* @param[in] external
* This flow rule is created by request external to PMD.
+ * @param[in] hairpin
+ * Number of hairpin TX actions, 0 means classic flow.
* @param[out] error
* Pointer to the error structure.
*
@@ -4517,7 +4524,7 @@ struct field_modify_info modify_tcp[] = {
flow_dv_validate(struct rte_eth_dev *dev, const struct rte_flow_attr *attr,
const struct rte_flow_item items[],
const struct rte_flow_action actions[],
- bool external, struct rte_flow_error *error)
+ bool external, int hairpin, struct rte_flow_error *error)
{
int ret;
uint64_t action_flags = 0;
@@ -4563,12 +4570,15 @@ struct field_modify_info modify_tcp[] = {
struct mlx5_priv *priv = dev->data->dev_private;
struct mlx5_dev_config *dev_conf = &priv->config;
uint16_t queue_index = 0xFFFF;
+ int16_t rw_act_num = 0;
+ uint64_t is_root;
if (items == NULL)
return -1;
ret = flow_dv_validate_attributes(dev, attr, external, error);
if (ret < 0)
return ret;
+ is_root = (uint64_t)ret;
for (; items->type != RTE_FLOW_ITEM_TYPE_END; items++) {
int tunnel = !!(item_flags & MLX5_FLOW_LAYER_TUNNEL);
int type = items->type;
@@ -4842,6 +4852,7 @@ struct field_modify_info modify_tcp[] = {
action_flags |= MLX5_FLOW_ACTION_FLAG;
++actions_n;
}
+ rw_act_num += MLX5_ACT_NUM_SET_MARK;
break;
case RTE_FLOW_ACTION_TYPE_MARK:
ret = flow_dv_validate_action_mark(dev, actions,
@@ -4860,6 +4871,7 @@ struct field_modify_info modify_tcp[] = {
action_flags |= MLX5_FLOW_ACTION_MARK;
++actions_n;
}
+ rw_act_num += MLX5_ACT_NUM_SET_MARK;
break;
case RTE_FLOW_ACTION_TYPE_SET_META:
ret = flow_dv_validate_action_set_meta(dev, actions,
@@ -4871,6 +4883,7 @@ struct field_modify_info modify_tcp[] = {
if (!(action_flags & MLX5_FLOW_MODIFY_HDR_ACTIONS))
++actions_n;
action_flags |= MLX5_FLOW_ACTION_SET_META;
+ rw_act_num += MLX5_ACT_NUM_SET_META;
break;
case RTE_FLOW_ACTION_TYPE_SET_TAG:
ret = flow_dv_validate_action_set_tag(dev, actions,
@@ -4882,6 +4895,7 @@ struct field_modify_info modify_tcp[] = {
if (!(action_flags & MLX5_FLOW_MODIFY_HDR_ACTIONS))
++actions_n;
action_flags |= MLX5_FLOW_ACTION_SET_TAG;
+ rw_act_num += MLX5_ACT_NUM_SET_TAG;
break;
case RTE_FLOW_ACTION_TYPE_DROP:
ret = mlx5_flow_validate_action_drop(action_flags,
@@ -4959,6 +4973,7 @@ struct field_modify_info modify_tcp[] = {
return ret;
/* Count VID with push_vlan command. */
action_flags |= MLX5_FLOW_ACTION_OF_SET_VLAN_VID;
+ rw_act_num += MLX5_ACT_NUM_MDF_VID;
break;
case RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP:
case RTE_FLOW_ACTION_TYPE_NVGRE_ENCAP:
@@ -5020,8 +5035,15 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_MAC_SRC ?
MLX5_FLOW_ACTION_SET_MAC_SRC :
MLX5_FLOW_ACTION_SET_MAC_DST;
+ /*
+ * Even if the source and destination MAC addresses have
+ * overlap in the header with 4B alignment, the convert
+ * function will handle them separately and 4 SW actions
+ * will be created. And 2 actions will be added each
+ * time no matter how many bytes of address will be set.
+ */
+ rw_act_num += MLX5_ACT_NUM_MDF_MAC;
break;
-
case RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC:
case RTE_FLOW_ACTION_TYPE_SET_IPV4_DST:
ret = flow_dv_validate_action_modify_ipv4(action_flags,
@@ -5037,6 +5059,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC ?
MLX5_FLOW_ACTION_SET_IPV4_SRC :
MLX5_FLOW_ACTION_SET_IPV4_DST;
+ rw_act_num += MLX5_ACT_NUM_MDF_IPV4;
break;
case RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC:
case RTE_FLOW_ACTION_TYPE_SET_IPV6_DST:
@@ -5059,6 +5082,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC ?
MLX5_FLOW_ACTION_SET_IPV6_SRC :
MLX5_FLOW_ACTION_SET_IPV6_DST;
+ rw_act_num += MLX5_ACT_NUM_MDF_IPV6;
break;
case RTE_FLOW_ACTION_TYPE_SET_TP_SRC:
case RTE_FLOW_ACTION_TYPE_SET_TP_DST:
@@ -5075,6 +5099,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_TP_SRC ?
MLX5_FLOW_ACTION_SET_TP_SRC :
MLX5_FLOW_ACTION_SET_TP_DST;
+ rw_act_num += MLX5_ACT_NUM_MDF_PORT;
break;
case RTE_FLOW_ACTION_TYPE_DEC_TTL:
case RTE_FLOW_ACTION_TYPE_SET_TTL:
@@ -5091,6 +5116,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_TTL ?
MLX5_FLOW_ACTION_SET_TTL :
MLX5_FLOW_ACTION_DEC_TTL;
+ rw_act_num += MLX5_ACT_NUM_MDF_TTL;
break;
case RTE_FLOW_ACTION_TYPE_JUMP:
ret = flow_dv_validate_action_jump(actions,
@@ -5118,6 +5144,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ ?
MLX5_FLOW_ACTION_INC_TCP_SEQ :
MLX5_FLOW_ACTION_DEC_TCP_SEQ;
+ rw_act_num += MLX5_ACT_NUM_MDF_TCPSEQ;
break;
case RTE_FLOW_ACTION_TYPE_INC_TCP_ACK:
case RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK:
@@ -5135,10 +5162,13 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_INC_TCP_ACK ?
MLX5_FLOW_ACTION_INC_TCP_ACK :
MLX5_FLOW_ACTION_DEC_TCP_ACK;
+ rw_act_num += MLX5_ACT_NUM_MDF_TCPACK;
break;
- case MLX5_RTE_FLOW_ACTION_TYPE_TAG:
case MLX5_RTE_FLOW_ACTION_TYPE_MARK:
+ break;
+ case MLX5_RTE_FLOW_ACTION_TYPE_TAG:
case MLX5_RTE_FLOW_ACTION_TYPE_COPY_MREG:
+ rw_act_num += MLX5_ACT_NUM_SET_TAG;
break;
case RTE_FLOW_ACTION_TYPE_METER:
ret = mlx5_flow_validate_action_meter(dev,
@@ -5149,6 +5179,8 @@ struct field_modify_info modify_tcp[] = {
return ret;
action_flags |= MLX5_FLOW_ACTION_METER;
++actions_n;
+ /* Meter action will add one more TAG action. */
+ rw_act_num += MLX5_ACT_NUM_SET_TAG;
break;
case RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP:
ret = flow_dv_validate_action_modify_ipv4_dscp
@@ -5162,6 +5194,7 @@ struct field_modify_info modify_tcp[] = {
if (!(action_flags & MLX5_FLOW_MODIFY_HDR_ACTIONS))
++actions_n;
action_flags |= MLX5_FLOW_ACTION_SET_IPV4_DSCP;
+ rw_act_num += MLX5_ACT_NUM_SET_DSCP;
break;
case RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP:
ret = flow_dv_validate_action_modify_ipv6_dscp
@@ -5175,6 +5208,7 @@ struct field_modify_info modify_tcp[] = {
if (!(action_flags & MLX5_FLOW_MODIFY_HDR_ACTIONS))
++actions_n;
action_flags |= MLX5_FLOW_ACTION_SET_IPV6_DSCP;
+ rw_act_num += MLX5_ACT_NUM_SET_DSCP;
break;
default:
return rte_flow_error_set(error, ENOTSUP,
@@ -5247,6 +5281,21 @@ struct field_modify_info modify_tcp[] = {
NULL, "encap is not supported"
" for ingress traffic");
}
+ /* Hairpin flow will add one more TAG action. */
+ if (hairpin > 0)
+ rw_act_num += MLX5_ACT_NUM_SET_TAG;
+ /* extra metadata enabled: one more TAG action will be add. */
+ if (dev_conf->dv_flow_en &&
+ dev_conf->dv_xmeta_en != MLX5_XMETA_MODE_LEGACY &&
+ mlx5_flow_ext_mreg_supported(dev))
+ rw_act_num += MLX5_ACT_NUM_SET_TAG;
+ if ((uint32_t)rw_act_num >=
+ flow_dv_modify_hdr_action_max(dev, is_root)) {
+ return rte_flow_error_set(error, ENOTSUP,
+ RTE_FLOW_ERROR_TYPE_ACTION,
+ NULL, "too many header modify"
+ " actions to support");
+ }
return 0;
}
diff --git a/drivers/net/mlx5/mlx5_flow_verbs.c b/drivers/net/mlx5/mlx5_flow_verbs.c
index ef4d7a3..6595ecf 100644
--- a/drivers/net/mlx5/mlx5_flow_verbs.c
+++ b/drivers/net/mlx5/mlx5_flow_verbs.c
@@ -1115,6 +1115,8 @@
* Pointer to the list of actions.
* @param[in] external
* This flow rule is created by request external to PMD.
+ * @param[in] hairpin
+ * Number of hairpin TX actions, 0 means classic flow.
* @param[out] error
* Pointer to the error structure.
*
@@ -1127,6 +1129,7 @@
const struct rte_flow_item items[],
const struct rte_flow_action actions[],
bool external __rte_unused,
+ int hairpin __rte_unused,
struct rte_flow_error *error)
{
int ret;
--
1.8.3.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [dpdk-dev] [PATCH v4] net/mlx5: fix header modify action validation
2020-04-14 7:53 ` [dpdk-dev] [PATCH v3] net/mlx5: fix header modify action validation Bing Zhao
@ 2020-04-21 14:03 ` Bing Zhao
2020-04-21 14:59 ` Raslan Darawsheh
0 siblings, 1 reply; 5+ messages in thread
From: Bing Zhao @ 2020-04-21 14:03 UTC (permalink / raw)
To: viacheslavo, rasland; +Cc: orika, matan, dev, stable
The header modify actions number supported now has some limitation,
and it is decided by both driver and hardware. If the configuration
is different or the table to insert the flow is different, the result
might be different if the flow contains header modify actions.
Currently, the actual action number could only be calculated in the
later stage called translate, from user specified value to the driver
format. And the action numbers checking is missed in the flow
validation. So PMD will return incorrect result to indicate the
flow actions are valid by rte_flow_validate but then it will fail
when calling rte_flow_create.
Adding some simple checking in the validation will help to get rid
of this incorrect checking. Most of the actions will only consume 1
SW action field except the MAC address and IPv6 address. And from
SW POV, the maximal action fields for these will be consumed even if
only part of such field will be modified because that there is no
mask in the flow actions and the mask will always be all ONEs.
The metering or extra metadata supports will cost one more action.
Fixes: 9597330c6844 ("net/mlx5: update modify header action translator")
Cc: viacheslavo@mellanox.com
Cc: stable@dpdk.org
Signed-off-by: Bing Zhao <bingz@mellanox.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
v4: rebase to solve the conflict
v3: change the title of commit message
v2: take hairpin, metadata and meter into consideration
---
drivers/net/mlx5/mlx5_flow.c | 58 +++++++++++++++++++----------------
drivers/net/mlx5/mlx5_flow.h | 18 +++++++++++
drivers/net/mlx5/mlx5_flow_dv.c | 63 +++++++++++++++++++++++++++++++++-----
drivers/net/mlx5/mlx5_flow_verbs.c | 3 ++
4 files changed, 108 insertions(+), 34 deletions(-)
diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 109d71e..84aa069 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -2342,6 +2342,7 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
const struct rte_flow_item items[] __rte_unused,
const struct rte_flow_action actions[] __rte_unused,
bool external __rte_unused,
+ int hairpin __rte_unused,
struct rte_flow_error *error)
{
return rte_flow_error_set(error, ENOTSUP,
@@ -2457,6 +2458,8 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
* Pointer to the list of actions.
* @param[in] external
* This flow rule is created by request external to PMD.
+ * @param[in] hairpin
+ * Number of hairpin TX actions, 0 means classic flow.
* @param[out] error
* Pointer to the error structure.
*
@@ -2468,13 +2471,14 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
const struct rte_flow_attr *attr,
const struct rte_flow_item items[],
const struct rte_flow_action actions[],
- bool external, struct rte_flow_error *error)
+ bool external, int hairpin, struct rte_flow_error *error)
{
const struct mlx5_flow_driver_ops *fops;
enum mlx5_flow_drv_type type = flow_get_drv_type(dev, attr);
fops = flow_get_drv_ops(type);
- return fops->validate(dev, attr, items, actions, external, error);
+ return fops->validate(dev, attr, items, actions, external,
+ hairpin, error);
}
/**
@@ -2636,27 +2640,6 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
}
/**
- * Validate a flow supported by the NIC.
- *
- * @see rte_flow_validate()
- * @see rte_flow_ops
- */
-int
-mlx5_flow_validate(struct rte_eth_dev *dev,
- const struct rte_flow_attr *attr,
- const struct rte_flow_item items[],
- const struct rte_flow_action actions[],
- struct rte_flow_error *error)
-{
- int ret;
-
- ret = flow_drv_validate(dev, attr, items, actions, true, error);
- if (ret < 0)
- return ret;
- return 0;
-}
-
-/**
* Get RSS action from the action list.
*
* @param[in] actions
@@ -4271,15 +4254,16 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
const struct rte_flow_action *p_actions_rx = actions;
uint32_t i;
uint32_t idx = 0;
- int hairpin_flow = 0;
+ int hairpin_flow;
uint32_t hairpin_id = 0;
struct rte_flow_attr attr_tx = { .priority = 0 };
- int ret = flow_drv_validate(dev, attr, items, p_actions_rx, external,
- error);
+ int ret;
+ hairpin_flow = flow_check_hairpin_split(dev, attr, actions);
+ ret = flow_drv_validate(dev, attr, items, p_actions_rx,
+ external, hairpin_flow, error);
if (ret < 0)
return 0;
- hairpin_flow = flow_check_hairpin_split(dev, attr, actions);
if (hairpin_flow > 0) {
if (hairpin_flow > MLX5_MAX_SPLIT_ACTIONS) {
rte_errno = EINVAL;
@@ -4471,6 +4455,26 @@ struct rte_flow *
}
/**
+ * Validate a flow supported by the NIC.
+ *
+ * @see rte_flow_validate()
+ * @see rte_flow_ops
+ */
+int
+mlx5_flow_validate(struct rte_eth_dev *dev,
+ const struct rte_flow_attr *attr,
+ const struct rte_flow_item items[],
+ const struct rte_flow_action actions[],
+ struct rte_flow_error *error)
+{
+ int hairpin_flow;
+
+ hairpin_flow = flow_check_hairpin_split(dev, attr, actions);
+ return flow_drv_validate(dev, attr, items, actions,
+ true, hairpin_flow, error);
+}
+
+/**
* Create a flow.
*
* @see rte_flow_create()
diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
index 74017cf..e9ed793 100644
--- a/drivers/net/mlx5/mlx5_flow.h
+++ b/drivers/net/mlx5/mlx5_flow.h
@@ -332,6 +332,23 @@ enum mlx5_feature_name {
#define MLX5_ENCAPSULATION_DECISION_SIZE (sizeof(struct rte_flow_item_eth) + \
sizeof(struct rte_flow_item_ipv4))
+/* Software header modify action numbers of a flow. */
+#define MLX5_ACT_NUM_MDF_IPV4 1
+#define MLX5_ACT_NUM_MDF_IPV6 4
+#define MLX5_ACT_NUM_MDF_MAC 2
+#define MLX5_ACT_NUM_MDF_VID 1
+#define MLX5_ACT_NUM_MDF_PORT 2
+#define MLX5_ACT_NUM_MDF_TTL 1
+#define MLX5_ACT_NUM_DEC_TTL MLX5_ACT_NUM_MDF_TTL
+#define MLX5_ACT_NUM_MDF_TCPSEQ 1
+#define MLX5_ACT_NUM_MDF_TCPACK 1
+#define MLX5_ACT_NUM_SET_REG 1
+#define MLX5_ACT_NUM_SET_TAG 1
+#define MLX5_ACT_NUM_CPY_MREG MLX5_ACT_NUM_SET_TAG
+#define MLX5_ACT_NUM_SET_MARK MLX5_ACT_NUM_SET_TAG
+#define MLX5_ACT_NUM_SET_META MLX5_ACT_NUM_SET_TAG
+#define MLX5_ACT_NUM_SET_DSCP 1
+
enum mlx5_flow_drv_type {
MLX5_FLOW_TYPE_MIN,
MLX5_FLOW_TYPE_DV,
@@ -812,6 +829,7 @@ typedef int (*mlx5_flow_validate_t)(struct rte_eth_dev *dev,
const struct rte_flow_item items[],
const struct rte_flow_action actions[],
bool external,
+ int hairpin,
struct rte_flow_error *error);
typedef struct mlx5_flow *(*mlx5_flow_prepare_t)
(struct rte_eth_dev *dev, const struct rte_flow_attr *attr,
diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index e156c79..fe6072d 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -434,6 +434,7 @@ struct field_modify_info modify_tcp[] = {
/* Fetch variable byte size mask from the array. */
mask = flow_dv_fetch_field((const uint8_t *)item->mask +
field->offset, field->size);
+ MLX5_ASSERT(mask);
if (!mask) {
++field;
continue;
@@ -4490,7 +4491,9 @@ struct field_modify_info modify_tcp[] = {
* Pointer to error structure.
*
* @return
- * 0 on success, a negative errno value otherwise and rte_errno is set.
+ * - 0 on success and non root table.
+ * - 1 on success and root table.
+ * - a negative errno value otherwise and rte_errno is set.
*/
static int
flow_dv_validate_attributes(struct rte_eth_dev *dev,
@@ -4500,6 +4503,7 @@ struct field_modify_info modify_tcp[] = {
{
struct mlx5_priv *priv = dev->data->dev_private;
uint32_t priority_max = priv->config.flow_prio - 1;
+ int ret = 0;
#ifndef HAVE_MLX5DV_DR
if (attributes->group)
@@ -4508,14 +4512,15 @@ struct field_modify_info modify_tcp[] = {
NULL,
"groups are not supported");
#else
- uint32_t table;
- int ret;
+ uint32_t table = 0;
ret = mlx5_flow_group_to_table(attributes, external,
attributes->group, !!priv->fdb_def_rule,
&table, error);
if (ret)
return ret;
+ if (!table)
+ ret = MLX5DV_DR_ACTION_FLAGS_ROOT_LEVEL;
#endif
if (attributes->priority != MLX5_FLOW_PRIO_RSVD &&
attributes->priority >= priority_max)
@@ -4545,7 +4550,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ERROR_TYPE_ATTR, NULL,
"must specify exactly one of "
"ingress or egress");
- return 0;
+ return ret;
}
/**
@@ -4561,6 +4566,8 @@ struct field_modify_info modify_tcp[] = {
* Pointer to the list of actions.
* @param[in] external
* This flow rule is created by request external to PMD.
+ * @param[in] hairpin
+ * Number of hairpin TX actions, 0 means classic flow.
* @param[out] error
* Pointer to the error structure.
*
@@ -4571,7 +4578,7 @@ struct field_modify_info modify_tcp[] = {
flow_dv_validate(struct rte_eth_dev *dev, const struct rte_flow_attr *attr,
const struct rte_flow_item items[],
const struct rte_flow_action actions[],
- bool external, struct rte_flow_error *error)
+ bool external, int hairpin, struct rte_flow_error *error)
{
int ret;
uint64_t action_flags = 0;
@@ -4618,12 +4625,15 @@ struct field_modify_info modify_tcp[] = {
struct mlx5_dev_config *dev_conf = &priv->config;
uint16_t queue_index = 0xFFFF;
const struct rte_flow_item_vlan *vlan_m = NULL;
+ int16_t rw_act_num = 0;
+ uint64_t is_root;
if (items == NULL)
return -1;
ret = flow_dv_validate_attributes(dev, attr, external, error);
if (ret < 0)
return ret;
+ is_root = (uint64_t)ret;
for (; items->type != RTE_FLOW_ITEM_TYPE_END; items++) {
int tunnel = !!(item_flags & MLX5_FLOW_LAYER_TUNNEL);
int type = items->type;
@@ -4900,6 +4910,7 @@ struct field_modify_info modify_tcp[] = {
action_flags |= MLX5_FLOW_ACTION_FLAG;
++actions_n;
}
+ rw_act_num += MLX5_ACT_NUM_SET_MARK;
break;
case RTE_FLOW_ACTION_TYPE_MARK:
ret = flow_dv_validate_action_mark(dev, actions,
@@ -4918,6 +4929,7 @@ struct field_modify_info modify_tcp[] = {
action_flags |= MLX5_FLOW_ACTION_MARK;
++actions_n;
}
+ rw_act_num += MLX5_ACT_NUM_SET_MARK;
break;
case RTE_FLOW_ACTION_TYPE_SET_META:
ret = flow_dv_validate_action_set_meta(dev, actions,
@@ -4929,6 +4941,7 @@ struct field_modify_info modify_tcp[] = {
if (!(action_flags & MLX5_FLOW_MODIFY_HDR_ACTIONS))
++actions_n;
action_flags |= MLX5_FLOW_ACTION_SET_META;
+ rw_act_num += MLX5_ACT_NUM_SET_META;
break;
case RTE_FLOW_ACTION_TYPE_SET_TAG:
ret = flow_dv_validate_action_set_tag(dev, actions,
@@ -4940,6 +4953,7 @@ struct field_modify_info modify_tcp[] = {
if (!(action_flags & MLX5_FLOW_MODIFY_HDR_ACTIONS))
++actions_n;
action_flags |= MLX5_FLOW_ACTION_SET_TAG;
+ rw_act_num += MLX5_ACT_NUM_SET_TAG;
break;
case RTE_FLOW_ACTION_TYPE_DROP:
ret = mlx5_flow_validate_action_drop(action_flags,
@@ -5017,6 +5031,7 @@ struct field_modify_info modify_tcp[] = {
return ret;
/* Count VID with push_vlan command. */
action_flags |= MLX5_FLOW_ACTION_OF_SET_VLAN_VID;
+ rw_act_num += MLX5_ACT_NUM_MDF_VID;
break;
case RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP:
case RTE_FLOW_ACTION_TYPE_NVGRE_ENCAP:
@@ -5078,8 +5093,15 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_MAC_SRC ?
MLX5_FLOW_ACTION_SET_MAC_SRC :
MLX5_FLOW_ACTION_SET_MAC_DST;
+ /*
+ * Even if the source and destination MAC addresses have
+ * overlap in the header with 4B alignment, the convert
+ * function will handle them separately and 4 SW actions
+ * will be created. And 2 actions will be added each
+ * time no matter how many bytes of address will be set.
+ */
+ rw_act_num += MLX5_ACT_NUM_MDF_MAC;
break;
-
case RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC:
case RTE_FLOW_ACTION_TYPE_SET_IPV4_DST:
ret = flow_dv_validate_action_modify_ipv4(action_flags,
@@ -5095,6 +5117,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC ?
MLX5_FLOW_ACTION_SET_IPV4_SRC :
MLX5_FLOW_ACTION_SET_IPV4_DST;
+ rw_act_num += MLX5_ACT_NUM_MDF_IPV4;
break;
case RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC:
case RTE_FLOW_ACTION_TYPE_SET_IPV6_DST:
@@ -5117,6 +5140,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC ?
MLX5_FLOW_ACTION_SET_IPV6_SRC :
MLX5_FLOW_ACTION_SET_IPV6_DST;
+ rw_act_num += MLX5_ACT_NUM_MDF_IPV6;
break;
case RTE_FLOW_ACTION_TYPE_SET_TP_SRC:
case RTE_FLOW_ACTION_TYPE_SET_TP_DST:
@@ -5133,6 +5157,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_TP_SRC ?
MLX5_FLOW_ACTION_SET_TP_SRC :
MLX5_FLOW_ACTION_SET_TP_DST;
+ rw_act_num += MLX5_ACT_NUM_MDF_PORT;
break;
case RTE_FLOW_ACTION_TYPE_DEC_TTL:
case RTE_FLOW_ACTION_TYPE_SET_TTL:
@@ -5149,6 +5174,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_SET_TTL ?
MLX5_FLOW_ACTION_SET_TTL :
MLX5_FLOW_ACTION_DEC_TTL;
+ rw_act_num += MLX5_ACT_NUM_MDF_TTL;
break;
case RTE_FLOW_ACTION_TYPE_JUMP:
ret = flow_dv_validate_action_jump(actions,
@@ -5176,6 +5202,7 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ ?
MLX5_FLOW_ACTION_INC_TCP_SEQ :
MLX5_FLOW_ACTION_DEC_TCP_SEQ;
+ rw_act_num += MLX5_ACT_NUM_MDF_TCPSEQ;
break;
case RTE_FLOW_ACTION_TYPE_INC_TCP_ACK:
case RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK:
@@ -5193,10 +5220,13 @@ struct field_modify_info modify_tcp[] = {
RTE_FLOW_ACTION_TYPE_INC_TCP_ACK ?
MLX5_FLOW_ACTION_INC_TCP_ACK :
MLX5_FLOW_ACTION_DEC_TCP_ACK;
+ rw_act_num += MLX5_ACT_NUM_MDF_TCPACK;
break;
- case MLX5_RTE_FLOW_ACTION_TYPE_TAG:
case MLX5_RTE_FLOW_ACTION_TYPE_MARK:
+ break;
+ case MLX5_RTE_FLOW_ACTION_TYPE_TAG:
case MLX5_RTE_FLOW_ACTION_TYPE_COPY_MREG:
+ rw_act_num += MLX5_ACT_NUM_SET_TAG;
break;
case RTE_FLOW_ACTION_TYPE_METER:
ret = mlx5_flow_validate_action_meter(dev,
@@ -5207,6 +5237,8 @@ struct field_modify_info modify_tcp[] = {
return ret;
action_flags |= MLX5_FLOW_ACTION_METER;
++actions_n;
+ /* Meter action will add one more TAG action. */
+ rw_act_num += MLX5_ACT_NUM_SET_TAG;
break;
case RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP:
ret = flow_dv_validate_action_modify_ipv4_dscp
@@ -5220,6 +5252,7 @@ struct field_modify_info modify_tcp[] = {
if (!(action_flags & MLX5_FLOW_MODIFY_HDR_ACTIONS))
++actions_n;
action_flags |= MLX5_FLOW_ACTION_SET_IPV4_DSCP;
+ rw_act_num += MLX5_ACT_NUM_SET_DSCP;
break;
case RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP:
ret = flow_dv_validate_action_modify_ipv6_dscp
@@ -5233,6 +5266,7 @@ struct field_modify_info modify_tcp[] = {
if (!(action_flags & MLX5_FLOW_MODIFY_HDR_ACTIONS))
++actions_n;
action_flags |= MLX5_FLOW_ACTION_SET_IPV6_DSCP;
+ rw_act_num += MLX5_ACT_NUM_SET_DSCP;
break;
default:
return rte_flow_error_set(error, ENOTSUP,
@@ -5305,6 +5339,21 @@ struct field_modify_info modify_tcp[] = {
NULL, "encap is not supported"
" for ingress traffic");
}
+ /* Hairpin flow will add one more TAG action. */
+ if (hairpin > 0)
+ rw_act_num += MLX5_ACT_NUM_SET_TAG;
+ /* extra metadata enabled: one more TAG action will be add. */
+ if (dev_conf->dv_flow_en &&
+ dev_conf->dv_xmeta_en != MLX5_XMETA_MODE_LEGACY &&
+ mlx5_flow_ext_mreg_supported(dev))
+ rw_act_num += MLX5_ACT_NUM_SET_TAG;
+ if ((uint32_t)rw_act_num >=
+ flow_dv_modify_hdr_action_max(dev, is_root)) {
+ return rte_flow_error_set(error, ENOTSUP,
+ RTE_FLOW_ERROR_TYPE_ACTION,
+ NULL, "too many header modify"
+ " actions to support");
+ }
return 0;
}
diff --git a/drivers/net/mlx5/mlx5_flow_verbs.c b/drivers/net/mlx5/mlx5_flow_verbs.c
index 722220c..d20098c 100644
--- a/drivers/net/mlx5/mlx5_flow_verbs.c
+++ b/drivers/net/mlx5/mlx5_flow_verbs.c
@@ -1108,6 +1108,8 @@
* Pointer to the list of actions.
* @param[in] external
* This flow rule is created by request external to PMD.
+ * @param[in] hairpin
+ * Number of hairpin TX actions, 0 means classic flow.
* @param[out] error
* Pointer to the error structure.
*
@@ -1120,6 +1122,7 @@
const struct rte_flow_item items[],
const struct rte_flow_action actions[],
bool external __rte_unused,
+ int hairpin __rte_unused,
struct rte_flow_error *error)
{
int ret;
--
1.8.3.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH v4] net/mlx5: fix header modify action validation
2020-04-21 14:03 ` [dpdk-dev] [PATCH v4] " Bing Zhao
@ 2020-04-21 14:59 ` Raslan Darawsheh
0 siblings, 0 replies; 5+ messages in thread
From: Raslan Darawsheh @ 2020-04-21 14:59 UTC (permalink / raw)
To: Bing Zhao, Slava Ovsiienko; +Cc: Ori Kam, Matan Azrad, dev, stable
Hi,
> -----Original Message-----
> From: Bing Zhao <bingz@mellanox.com>
> Sent: Tuesday, April 21, 2020 5:04 PM
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; Raslan Darawsheh
> <rasland@mellanox.com>
> Cc: Ori Kam <orika@mellanox.com>; Matan Azrad <matan@mellanox.com>;
> dev@dpdk.org; stable@dpdk.org
> Subject: [PATCH v4] net/mlx5: fix header modify action validation
>
> The header modify actions number supported now has some limitation,
> and it is decided by both driver and hardware. If the configuration
> is different or the table to insert the flow is different, the result
> might be different if the flow contains header modify actions.
> Currently, the actual action number could only be calculated in the
> later stage called translate, from user specified value to the driver
> format. And the action numbers checking is missed in the flow
> validation. So PMD will return incorrect result to indicate the
> flow actions are valid by rte_flow_validate but then it will fail
> when calling rte_flow_create.
>
> Adding some simple checking in the validation will help to get rid
> of this incorrect checking. Most of the actions will only consume 1
> SW action field except the MAC address and IPv6 address. And from
> SW POV, the maximal action fields for these will be consumed even if
> only part of such field will be modified because that there is no
> mask in the flow actions and the mask will always be all ONEs.
>
> The metering or extra metadata supports will cost one more action.
>
> Fixes: 9597330c6844 ("net/mlx5: update modify header action translator")
> Cc: viacheslavo@mellanox.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Bing Zhao <bingz@mellanox.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
> v4: rebase to solve the conflict
> v3: change the title of commit message
> v2: take hairpin, metadata and meter into consideration
> ---
> drivers/net/mlx5/mlx5_flow.c | 58 +++++++++++++++++++----------------
> drivers/net/mlx5/mlx5_flow.h | 18 +++++++++++
> drivers/net/mlx5/mlx5_flow_dv.c | 63
> +++++++++++++++++++++++++++++++++-----
> drivers/net/mlx5/mlx5_flow_verbs.c | 3 ++
> 4 files changed, 108 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 109d71e..84aa069 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -2342,6 +2342,7 @@ uint32_t mlx5_flow_adjust_priority(struct
> rte_eth_dev *dev, int32_t priority,
> const struct rte_flow_item items[] __rte_unused,
> const struct rte_flow_action actions[] __rte_unused,
> bool external __rte_unused,
> + int hairpin __rte_unused,
> struct rte_flow_error *error)
> {
> return rte_flow_error_set(error, ENOTSUP,
> @@ -2457,6 +2458,8 @@ uint32_t mlx5_flow_adjust_priority(struct
> rte_eth_dev *dev, int32_t priority,
> * Pointer to the list of actions.
> * @param[in] external
> * This flow rule is created by request external to PMD.
> + * @param[in] hairpin
> + * Number of hairpin TX actions, 0 means classic flow.
> * @param[out] error
> * Pointer to the error structure.
> *
> @@ -2468,13 +2471,14 @@ uint32_t mlx5_flow_adjust_priority(struct
> rte_eth_dev *dev, int32_t priority,
> const struct rte_flow_attr *attr,
> const struct rte_flow_item items[],
> const struct rte_flow_action actions[],
> - bool external, struct rte_flow_error *error)
> + bool external, int hairpin, struct rte_flow_error *error)
> {
> const struct mlx5_flow_driver_ops *fops;
> enum mlx5_flow_drv_type type = flow_get_drv_type(dev, attr);
>
> fops = flow_get_drv_ops(type);
> - return fops->validate(dev, attr, items, actions, external, error);
> + return fops->validate(dev, attr, items, actions, external,
> + hairpin, error);
> }
>
> /**
> @@ -2636,27 +2640,6 @@ uint32_t mlx5_flow_adjust_priority(struct
> rte_eth_dev *dev, int32_t priority,
> }
>
> /**
> - * Validate a flow supported by the NIC.
> - *
> - * @see rte_flow_validate()
> - * @see rte_flow_ops
> - */
> -int
> -mlx5_flow_validate(struct rte_eth_dev *dev,
> - const struct rte_flow_attr *attr,
> - const struct rte_flow_item items[],
> - const struct rte_flow_action actions[],
> - struct rte_flow_error *error)
> -{
> - int ret;
> -
> - ret = flow_drv_validate(dev, attr, items, actions, true, error);
> - if (ret < 0)
> - return ret;
> - return 0;
> -}
> -
> -/**
> * Get RSS action from the action list.
> *
> * @param[in] actions
> @@ -4271,15 +4254,16 @@ uint32_t mlx5_flow_adjust_priority(struct
> rte_eth_dev *dev, int32_t priority,
> const struct rte_flow_action *p_actions_rx = actions;
> uint32_t i;
> uint32_t idx = 0;
> - int hairpin_flow = 0;
> + int hairpin_flow;
> uint32_t hairpin_id = 0;
> struct rte_flow_attr attr_tx = { .priority = 0 };
> - int ret = flow_drv_validate(dev, attr, items, p_actions_rx, external,
> - error);
> + int ret;
>
> + hairpin_flow = flow_check_hairpin_split(dev, attr, actions);
> + ret = flow_drv_validate(dev, attr, items, p_actions_rx,
> + external, hairpin_flow, error);
> if (ret < 0)
> return 0;
> - hairpin_flow = flow_check_hairpin_split(dev, attr, actions);
> if (hairpin_flow > 0) {
> if (hairpin_flow > MLX5_MAX_SPLIT_ACTIONS) {
> rte_errno = EINVAL;
> @@ -4471,6 +4455,26 @@ struct rte_flow *
> }
>
> /**
> + * Validate a flow supported by the NIC.
> + *
> + * @see rte_flow_validate()
> + * @see rte_flow_ops
> + */
> +int
> +mlx5_flow_validate(struct rte_eth_dev *dev,
> + const struct rte_flow_attr *attr,
> + const struct rte_flow_item items[],
> + const struct rte_flow_action actions[],
> + struct rte_flow_error *error)
> +{
> + int hairpin_flow;
> +
> + hairpin_flow = flow_check_hairpin_split(dev, attr, actions);
> + return flow_drv_validate(dev, attr, items, actions,
> + true, hairpin_flow, error);
> +}
> +
> +/**
> * Create a flow.
> *
> * @see rte_flow_create()
> diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
> index 74017cf..e9ed793 100644
> --- a/drivers/net/mlx5/mlx5_flow.h
> +++ b/drivers/net/mlx5/mlx5_flow.h
> @@ -332,6 +332,23 @@ enum mlx5_feature_name {
> #define MLX5_ENCAPSULATION_DECISION_SIZE (sizeof(struct
> rte_flow_item_eth) + \
> sizeof(struct rte_flow_item_ipv4))
>
> +/* Software header modify action numbers of a flow. */
> +#define MLX5_ACT_NUM_MDF_IPV4 1
> +#define MLX5_ACT_NUM_MDF_IPV6 4
> +#define MLX5_ACT_NUM_MDF_MAC 2
> +#define MLX5_ACT_NUM_MDF_VID 1
> +#define MLX5_ACT_NUM_MDF_PORT 2
> +#define MLX5_ACT_NUM_MDF_TTL 1
> +#define MLX5_ACT_NUM_DEC_TTL MLX5_ACT_NUM_MDF_TTL
> +#define MLX5_ACT_NUM_MDF_TCPSEQ 1
> +#define MLX5_ACT_NUM_MDF_TCPACK 1
> +#define MLX5_ACT_NUM_SET_REG 1
> +#define MLX5_ACT_NUM_SET_TAG 1
> +#define MLX5_ACT_NUM_CPY_MREG
> MLX5_ACT_NUM_SET_TAG
> +#define MLX5_ACT_NUM_SET_MARK
> MLX5_ACT_NUM_SET_TAG
> +#define MLX5_ACT_NUM_SET_META
> MLX5_ACT_NUM_SET_TAG
> +#define MLX5_ACT_NUM_SET_DSCP 1
> +
> enum mlx5_flow_drv_type {
> MLX5_FLOW_TYPE_MIN,
> MLX5_FLOW_TYPE_DV,
> @@ -812,6 +829,7 @@ typedef int (*mlx5_flow_validate_t)(struct
> rte_eth_dev *dev,
> const struct rte_flow_item items[],
> const struct rte_flow_action actions[],
> bool external,
> + int hairpin,
> struct rte_flow_error *error);
> typedef struct mlx5_flow *(*mlx5_flow_prepare_t)
> (struct rte_eth_dev *dev, const struct rte_flow_attr *attr,
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> b/drivers/net/mlx5/mlx5_flow_dv.c
> index e156c79..fe6072d 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -434,6 +434,7 @@ struct field_modify_info modify_tcp[] = {
> /* Fetch variable byte size mask from the array. */
> mask = flow_dv_fetch_field((const uint8_t *)item->mask +
> field->offset, field->size);
> + MLX5_ASSERT(mask);
> if (!mask) {
> ++field;
> continue;
> @@ -4490,7 +4491,9 @@ struct field_modify_info modify_tcp[] = {
> * Pointer to error structure.
> *
> * @return
> - * 0 on success, a negative errno value otherwise and rte_errno is set.
> + * - 0 on success and non root table.
> + * - 1 on success and root table.
> + * - a negative errno value otherwise and rte_errno is set.
> */
> static int
> flow_dv_validate_attributes(struct rte_eth_dev *dev,
> @@ -4500,6 +4503,7 @@ struct field_modify_info modify_tcp[] = {
> {
> struct mlx5_priv *priv = dev->data->dev_private;
> uint32_t priority_max = priv->config.flow_prio - 1;
> + int ret = 0;
>
> #ifndef HAVE_MLX5DV_DR
> if (attributes->group)
> @@ -4508,14 +4512,15 @@ struct field_modify_info modify_tcp[] = {
> NULL,
> "groups are not supported");
> #else
> - uint32_t table;
> - int ret;
> + uint32_t table = 0;
>
> ret = mlx5_flow_group_to_table(attributes, external,
> attributes->group, !!priv->fdb_def_rule,
> &table, error);
> if (ret)
> return ret;
> + if (!table)
> + ret = MLX5DV_DR_ACTION_FLAGS_ROOT_LEVEL;
> #endif
> if (attributes->priority != MLX5_FLOW_PRIO_RSVD &&
> attributes->priority >= priority_max)
> @@ -4545,7 +4550,7 @@ struct field_modify_info modify_tcp[] = {
> RTE_FLOW_ERROR_TYPE_ATTR,
> NULL,
> "must specify exactly one of "
> "ingress or egress");
> - return 0;
> + return ret;
> }
>
> /**
> @@ -4561,6 +4566,8 @@ struct field_modify_info modify_tcp[] = {
> * Pointer to the list of actions.
> * @param[in] external
> * This flow rule is created by request external to PMD.
> + * @param[in] hairpin
> + * Number of hairpin TX actions, 0 means classic flow.
> * @param[out] error
> * Pointer to the error structure.
> *
> @@ -4571,7 +4578,7 @@ struct field_modify_info modify_tcp[] = {
> flow_dv_validate(struct rte_eth_dev *dev, const struct rte_flow_attr *attr,
> const struct rte_flow_item items[],
> const struct rte_flow_action actions[],
> - bool external, struct rte_flow_error *error)
> + bool external, int hairpin, struct rte_flow_error *error)
> {
> int ret;
> uint64_t action_flags = 0;
> @@ -4618,12 +4625,15 @@ struct field_modify_info modify_tcp[] = {
> struct mlx5_dev_config *dev_conf = &priv->config;
> uint16_t queue_index = 0xFFFF;
> const struct rte_flow_item_vlan *vlan_m = NULL;
> + int16_t rw_act_num = 0;
> + uint64_t is_root;
>
> if (items == NULL)
> return -1;
> ret = flow_dv_validate_attributes(dev, attr, external, error);
> if (ret < 0)
> return ret;
> + is_root = (uint64_t)ret;
> for (; items->type != RTE_FLOW_ITEM_TYPE_END; items++) {
> int tunnel = !!(item_flags & MLX5_FLOW_LAYER_TUNNEL);
> int type = items->type;
> @@ -4900,6 +4910,7 @@ struct field_modify_info modify_tcp[] = {
> action_flags |= MLX5_FLOW_ACTION_FLAG;
> ++actions_n;
> }
> + rw_act_num += MLX5_ACT_NUM_SET_MARK;
> break;
> case RTE_FLOW_ACTION_TYPE_MARK:
> ret = flow_dv_validate_action_mark(dev, actions,
> @@ -4918,6 +4929,7 @@ struct field_modify_info modify_tcp[] = {
> action_flags |= MLX5_FLOW_ACTION_MARK;
> ++actions_n;
> }
> + rw_act_num += MLX5_ACT_NUM_SET_MARK;
> break;
> case RTE_FLOW_ACTION_TYPE_SET_META:
> ret = flow_dv_validate_action_set_meta(dev,
> actions,
> @@ -4929,6 +4941,7 @@ struct field_modify_info modify_tcp[] = {
> if (!(action_flags &
> MLX5_FLOW_MODIFY_HDR_ACTIONS))
> ++actions_n;
> action_flags |= MLX5_FLOW_ACTION_SET_META;
> + rw_act_num += MLX5_ACT_NUM_SET_META;
> break;
> case RTE_FLOW_ACTION_TYPE_SET_TAG:
> ret = flow_dv_validate_action_set_tag(dev, actions,
> @@ -4940,6 +4953,7 @@ struct field_modify_info modify_tcp[] = {
> if (!(action_flags &
> MLX5_FLOW_MODIFY_HDR_ACTIONS))
> ++actions_n;
> action_flags |= MLX5_FLOW_ACTION_SET_TAG;
> + rw_act_num += MLX5_ACT_NUM_SET_TAG;
> break;
> case RTE_FLOW_ACTION_TYPE_DROP:
> ret = mlx5_flow_validate_action_drop(action_flags,
> @@ -5017,6 +5031,7 @@ struct field_modify_info modify_tcp[] = {
> return ret;
> /* Count VID with push_vlan command. */
> action_flags |=
> MLX5_FLOW_ACTION_OF_SET_VLAN_VID;
> + rw_act_num += MLX5_ACT_NUM_MDF_VID;
> break;
> case RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP:
> case RTE_FLOW_ACTION_TYPE_NVGRE_ENCAP:
> @@ -5078,8 +5093,15 @@ struct field_modify_info modify_tcp[] = {
>
> RTE_FLOW_ACTION_TYPE_SET_MAC_SRC ?
>
> MLX5_FLOW_ACTION_SET_MAC_SRC :
>
> MLX5_FLOW_ACTION_SET_MAC_DST;
> + /*
> + * Even if the source and destination MAC addresses
> have
> + * overlap in the header with 4B alignment, the
> convert
> + * function will handle them separately and 4 SW
> actions
> + * will be created. And 2 actions will be added each
> + * time no matter how many bytes of address will be
> set.
> + */
> + rw_act_num += MLX5_ACT_NUM_MDF_MAC;
> break;
> -
> case RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC:
> case RTE_FLOW_ACTION_TYPE_SET_IPV4_DST:
> ret =
> flow_dv_validate_action_modify_ipv4(action_flags,
> @@ -5095,6 +5117,7 @@ struct field_modify_info modify_tcp[] = {
>
> RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC ?
>
> MLX5_FLOW_ACTION_SET_IPV4_SRC :
>
> MLX5_FLOW_ACTION_SET_IPV4_DST;
> + rw_act_num += MLX5_ACT_NUM_MDF_IPV4;
> break;
> case RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC:
> case RTE_FLOW_ACTION_TYPE_SET_IPV6_DST:
> @@ -5117,6 +5140,7 @@ struct field_modify_info modify_tcp[] = {
>
> RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC ?
>
> MLX5_FLOW_ACTION_SET_IPV6_SRC :
>
> MLX5_FLOW_ACTION_SET_IPV6_DST;
> + rw_act_num += MLX5_ACT_NUM_MDF_IPV6;
> break;
> case RTE_FLOW_ACTION_TYPE_SET_TP_SRC:
> case RTE_FLOW_ACTION_TYPE_SET_TP_DST:
> @@ -5133,6 +5157,7 @@ struct field_modify_info modify_tcp[] = {
>
> RTE_FLOW_ACTION_TYPE_SET_TP_SRC ?
>
> MLX5_FLOW_ACTION_SET_TP_SRC :
>
> MLX5_FLOW_ACTION_SET_TP_DST;
> + rw_act_num += MLX5_ACT_NUM_MDF_PORT;
> break;
> case RTE_FLOW_ACTION_TYPE_DEC_TTL:
> case RTE_FLOW_ACTION_TYPE_SET_TTL:
> @@ -5149,6 +5174,7 @@ struct field_modify_info modify_tcp[] = {
> RTE_FLOW_ACTION_TYPE_SET_TTL ?
>
> MLX5_FLOW_ACTION_SET_TTL :
>
> MLX5_FLOW_ACTION_DEC_TTL;
> + rw_act_num += MLX5_ACT_NUM_MDF_TTL;
> break;
> case RTE_FLOW_ACTION_TYPE_JUMP:
> ret = flow_dv_validate_action_jump(actions,
> @@ -5176,6 +5202,7 @@ struct field_modify_info modify_tcp[] = {
>
> RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ ?
>
> MLX5_FLOW_ACTION_INC_TCP_SEQ :
>
> MLX5_FLOW_ACTION_DEC_TCP_SEQ;
> + rw_act_num += MLX5_ACT_NUM_MDF_TCPSEQ;
> break;
> case RTE_FLOW_ACTION_TYPE_INC_TCP_ACK:
> case RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK:
> @@ -5193,10 +5220,13 @@ struct field_modify_info modify_tcp[] = {
>
> RTE_FLOW_ACTION_TYPE_INC_TCP_ACK ?
>
> MLX5_FLOW_ACTION_INC_TCP_ACK :
>
> MLX5_FLOW_ACTION_DEC_TCP_ACK;
> + rw_act_num += MLX5_ACT_NUM_MDF_TCPACK;
> break;
> - case MLX5_RTE_FLOW_ACTION_TYPE_TAG:
> case MLX5_RTE_FLOW_ACTION_TYPE_MARK:
> + break;
> + case MLX5_RTE_FLOW_ACTION_TYPE_TAG:
> case MLX5_RTE_FLOW_ACTION_TYPE_COPY_MREG:
> + rw_act_num += MLX5_ACT_NUM_SET_TAG;
> break;
> case RTE_FLOW_ACTION_TYPE_METER:
> ret = mlx5_flow_validate_action_meter(dev,
> @@ -5207,6 +5237,8 @@ struct field_modify_info modify_tcp[] = {
> return ret;
> action_flags |= MLX5_FLOW_ACTION_METER;
> ++actions_n;
> + /* Meter action will add one more TAG action. */
> + rw_act_num += MLX5_ACT_NUM_SET_TAG;
> break;
> case RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP:
> ret = flow_dv_validate_action_modify_ipv4_dscp
> @@ -5220,6 +5252,7 @@ struct field_modify_info modify_tcp[] = {
> if (!(action_flags &
> MLX5_FLOW_MODIFY_HDR_ACTIONS))
> ++actions_n;
> action_flags |=
> MLX5_FLOW_ACTION_SET_IPV4_DSCP;
> + rw_act_num += MLX5_ACT_NUM_SET_DSCP;
> break;
> case RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP:
> ret = flow_dv_validate_action_modify_ipv6_dscp
> @@ -5233,6 +5266,7 @@ struct field_modify_info modify_tcp[] = {
> if (!(action_flags &
> MLX5_FLOW_MODIFY_HDR_ACTIONS))
> ++actions_n;
> action_flags |=
> MLX5_FLOW_ACTION_SET_IPV6_DSCP;
> + rw_act_num += MLX5_ACT_NUM_SET_DSCP;
> break;
> default:
> return rte_flow_error_set(error, ENOTSUP,
> @@ -5305,6 +5339,21 @@ struct field_modify_info modify_tcp[] = {
> NULL, "encap is not
> supported"
> " for ingress traffic");
> }
> + /* Hairpin flow will add one more TAG action. */
> + if (hairpin > 0)
> + rw_act_num += MLX5_ACT_NUM_SET_TAG;
> + /* extra metadata enabled: one more TAG action will be add. */
> + if (dev_conf->dv_flow_en &&
> + dev_conf->dv_xmeta_en != MLX5_XMETA_MODE_LEGACY &&
> + mlx5_flow_ext_mreg_supported(dev))
> + rw_act_num += MLX5_ACT_NUM_SET_TAG;
> + if ((uint32_t)rw_act_num >=
> + flow_dv_modify_hdr_action_max(dev, is_root)) {
> + return rte_flow_error_set(error, ENOTSUP,
> + RTE_FLOW_ERROR_TYPE_ACTION,
> + NULL, "too many header modify"
> + " actions to support");
> + }
> return 0;
> }
>
> diff --git a/drivers/net/mlx5/mlx5_flow_verbs.c
> b/drivers/net/mlx5/mlx5_flow_verbs.c
> index 722220c..d20098c 100644
> --- a/drivers/net/mlx5/mlx5_flow_verbs.c
> +++ b/drivers/net/mlx5/mlx5_flow_verbs.c
> @@ -1108,6 +1108,8 @@
> * Pointer to the list of actions.
> * @param[in] external
> * This flow rule is created by request external to PMD.
> + * @param[in] hairpin
> + * Number of hairpin TX actions, 0 means classic flow.
> * @param[out] error
> * Pointer to the error structure.
> *
> @@ -1120,6 +1122,7 @@
> const struct rte_flow_item items[],
> const struct rte_flow_action actions[],
> bool external __rte_unused,
> + int hairpin __rte_unused,
> struct rte_flow_error *error)
> {
> int ret;
> --
> 1.8.3.1
Patch applied to next-net-mlx,
Kindest regards,
Raslan Darawsheh
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-04-21 14:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-19 14:27 [dpdk-dev] [PATCH] net/mlx5: fix the modify checking in flow validate Bing Zhao
2020-04-13 14:37 ` [dpdk-dev] [PATCH v2] " Bing Zhao
2020-04-14 7:53 ` [dpdk-dev] [PATCH v3] net/mlx5: fix header modify action validation Bing Zhao
2020-04-21 14:03 ` [dpdk-dev] [PATCH v4] " Bing Zhao
2020-04-21 14:59 ` 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).