RE: [dpdk-dev] [PATCH v3] examples/ipsec-secgw: support more flow patterns and actions

["Zhang, Roy Fan" <roy.fan.zhang@intel.com>]

Hi,

> -----Original Message-----
> From: psatheesh@marvell.com <psatheesh@marvell.com>
> Sent: Friday, June 3, 2022 4:17 AM
> To: Nicolau, Radu <radu.nicolau@intel.com>; Akhil Goyal <gakhil@marvell.com>
> Cc: dev@dpdk.org; Satheesh Paul <psatheesh@marvell.com>
> Subject: [dpdk-dev] [PATCH v3] examples/ipsec-secgw: support more flow
> patterns and actions
> 
> From: Satheesh Paul <psatheesh@marvell.com>
> 
> Added support to create flow rules with count, mark and
> security actions and mark pattern.
> 
> Signed-off-by: Satheesh Paul <psatheesh@marvell.com>
> ---

<snip>

>  .. code-block:: console
> 
> -    flow <ip_ver> <src_ip> <dst_ip> <port> <queue>
> -
> +    flow <mark> <eth> <ip_ver> <src_ip> <dst_ip> <port> <queue> \
> +         <count> <security> <set_mark>
> 
>  where each options means:
> 
> +``<mark>``
> +
> + * Set RTE_FLOW_ITEM_TYPE_MARK pattern item with the given mark value.
> +
> + * Optional: Yes, this pattern is not set by default.
> +
> + * Syntax: *mark X*
> +

<snip>

> +
> +``<set_mark>``
> +
> + * Set RTE_FLOW_ACTION_TYPE_MARK action with the given mark value.
> +
> + * Optional: yes, this action is not set by default.
> +
> + * Syntax: *set_mark X*
> +
>  Example flow rules:

I feel "mark" and "set_mark" are duplicated?
From the implementation below it looks there are slight difference in between
But we may need better description for both.

> 
>  .. code-block:: console
> @@ -948,6 +988,18 @@ Example flow rules:
> 
>      flow ipv6 dst 1111:1111:1111:1111:1111:1111:1111:5555/116 port 1
> queue 0
> 
> +    flow mark 123 ipv4 dst 192.168.0.0/16 port 0 queue 0 count
> +
> +    flow eth ipv4 dst 192.168.0.0/16 port 0 queue 0 count
> +
> +    flow ipv4 dst 192.168.0.0/16 port 0 queue 0 count
> +
> +    flow ipv4 dst 192.168.0.0/16 port 0 queue 0
> +
> +    flow port 0 security set_mark 123
> +
> +    flow ipv4 dst 1.1.0.0/16 port 0 count set_mark 123 security
> +
> 
>  Neighbour rule syntax
>  ^^^^^^^^^^^^^^^^^^^^^
> diff --git a/examples/ipsec-secgw/flow.c b/examples/ipsec-secgw/flow.c
> index 1a1ec7861c..2088876999 100644
> --- a/examples/ipsec-secgw/flow.c
> +++ b/examples/ipsec-secgw/flow.c
> @@ -15,7 +15,9 @@
>  #define FLOW_RULES_MAX 128
> 
>  struct flow_rule_entry {
> +	uint8_t is_eth;
>  	uint8_t is_ipv4;
> +	uint8_t is_ipv6;
>  	RTE_STD_C11
>  	union {
>  		struct {
> @@ -27,8 +29,15 @@ struct flow_rule_entry {
>  			struct rte_flow_item_ipv6 mask;
>  		} ipv6;
>  	};
> +	struct rte_flow_item_mark mark_val;
>  	uint16_t port;
>  	uint16_t queue;
> +	bool is_queue_set;
> +	bool enable_count;
> +	bool enable_mark;
> +	bool set_security_action;
> +	bool set_mark_action;
> +	uint32_t mark_action_val;
>  	struct rte_flow *flow;
>  } flow_rule_tbl[FLOW_RULES_MAX];
> 
> @@ -64,8 +73,9 @@ ipv4_addr_cpy(rte_be32_t *spec, rte_be32_t *mask, char
> *token,
>  	memcpy(mask, &rte_flow_item_ipv4_mask.hdr.src_addr, sizeof(ip));
> 
>  	*spec = ip.s_addr;
> +
>  	if (depth < 32)
> -		*mask = *mask << (32-depth);
> +		*mask = htonl(*mask << (32 - depth));
> 
>  	return 0;
>  }
> @@ -124,7 +134,7 @@ parse_flow_tokens(char **tokens, uint32_t n_tokens,
>  		  struct parse_status *status)
>  {
>  	struct flow_rule_entry *rule;
> -	uint32_t ti;
> +	uint32_t ti = 0;
> 
>  	if (nb_flow_rule >= FLOW_RULES_MAX) {
>  		printf("Too many flow rules\n");
> @@ -134,49 +144,73 @@ parse_flow_tokens(char **tokens, uint32_t
> n_tokens,
>  	rule = &flow_rule_tbl[nb_flow_rule];
>  	memset(rule, 0, sizeof(*rule));
> 
> -	if (strcmp(tokens[0], "ipv4") == 0) {
> -		rule->is_ipv4 = 1;
> -	} else if (strcmp(tokens[0], "ipv6") == 0) {
> -		rule->is_ipv4 = 0;
> -	} else {
> -		APP_CHECK(0, status, "unrecognized input \"%s\"", tokens[0]);
> -		return;
> -	}
> -
> -	for (ti = 1; ti < n_tokens; ti++) {
> -		if (strcmp(tokens[ti], "src") == 0) {
> +	for (ti = 0; ti < n_tokens; ti++) {
> +		if (strcmp(tokens[ti], "mark") == 0) {
>  			INCREMENT_TOKEN_INDEX(ti, n_tokens, status);
> +			if (status->status < 0)
> +				return;
> +			APP_CHECK_TOKEN_IS_NUM(tokens, ti, status);
>  			if (status->status < 0)
>  				return;
> 
> -			if (rule->is_ipv4) {
> +			rule->mark_val.id = atoi(tokens[ti]);
> +			rule->enable_mark = true;
> +			continue;
> +		}
> +		if (strcmp(tokens[ti], "eth") == 0) {
> +			rule->is_eth = true;
> +			continue;
> +		}
> +
> +		if (strcmp(tokens[ti], "ipv4") == 0) {
> +			rule->is_ipv4 = true;
> +			INCREMENT_TOKEN_INDEX(ti, n_tokens, status);
> +			if (status->status < 0)
> +				return;
> +			if (strcmp(tokens[ti], "src") == 0) {
> +				INCREMENT_TOKEN_INDEX(ti, n_tokens,
> status);
> +				if (status->status < 0)
> +					return;
>  				if (ipv4_addr_cpy(&rule-
> >ipv4.spec.hdr.src_addr,
>  						  &rule-
> >ipv4.mask.hdr.src_addr,
>  						  tokens[ti], status))
>  					return;
> -			} else {
> -				if (ipv6_addr_cpy(rule->ipv6.spec.hdr.src_addr,
> -						  rule->ipv6.mask.hdr.src_addr,
> +			}
> +			if (strcmp(tokens[ti], "dst") == 0) {
> +				INCREMENT_TOKEN_INDEX(ti, n_tokens,
> status);
> +				if (status->status < 0)
> +					return;
> +				if (ipv4_addr_cpy(&rule-
> >ipv4.spec.hdr.dst_addr,
> +						  &rule-
> >ipv4.mask.hdr.dst_addr,
>  						  tokens[ti], status))
>  					return;
>  			}
> +			continue;
>  		}
> -		if (strcmp(tokens[ti], "dst") == 0) {
> +		if (strcmp(tokens[ti], "ipv6") == 0) {
> +			rule->is_ipv6 = true;
>  			INCREMENT_TOKEN_INDEX(ti, n_tokens, status);
>  			if (status->status < 0)
>  				return;
> -
> -			if (rule->is_ipv4) {
> -				if (ipv4_addr_cpy(&rule-
> >ipv4.spec.hdr.dst_addr,
> -						  &rule-
> >ipv4.mask.hdr.dst_addr,
> +			if (strcmp(tokens[ti], "src") == 0) {
> +				INCREMENT_TOKEN_INDEX(ti, n_tokens,
> status);
> +				if (status->status < 0)
> +					return;
> +				if (ipv6_addr_cpy(rule->ipv6.spec.hdr.src_addr,
> +						  rule->ipv6.mask.hdr.src_addr,
>  						  tokens[ti], status))
>  					return;
> -			} else {
> +			}
> +			if (strcmp(tokens[ti], "dst") == 0) {
> +				INCREMENT_TOKEN_INDEX(ti, n_tokens,
> status);
> +				if (status->status < 0)
> +					return;
>  				if (ipv6_addr_cpy(rule->ipv6.spec.hdr.dst_addr,
>  						  rule->ipv6.mask.hdr.dst_addr,
>  						  tokens[ti], status))
>  					return;
>  			}
> +			continue;
>  		}
> 
>  		if (strcmp(tokens[ti], "port") == 0) {
> @@ -188,6 +222,7 @@ parse_flow_tokens(char **tokens, uint32_t n_tokens,
>  				return;
> 
>  			rule->port = atoi(tokens[ti]);
> +			continue;
>  		}
> 
>  		if (strcmp(tokens[ti], "queue") == 0) {
> @@ -199,50 +234,129 @@ parse_flow_tokens(char **tokens, uint32_t
> n_tokens,
>  				return;
> 
>  			rule->queue = atoi(tokens[ti]);
> +			rule->is_queue_set = true;
> +			continue;
> +		}
> +
> +		if (strcmp(tokens[ti], "count") == 0) {
> +			rule->enable_count = true;
> +			continue;
> +		}
> +
> +		if (strcmp(tokens[ti], "security") == 0) {
> +			rule->set_security_action = true;
> +			continue;
>  		}
> +
> +		if (strcmp(tokens[ti], "set_mark") == 0) {
> +			INCREMENT_TOKEN_INDEX(ti, n_tokens, status);
> +			if (status->status < 0)
> +				return;
> +			APP_CHECK_TOKEN_IS_NUM(tokens, ti, status);
> +			if (status->status < 0)
> +				return;
> +
> +			rule->set_mark_action = true;
> +			rule->mark_action_val = atoi(tokens[ti]);
> +			continue;
> +		}
> +
> +		sprintf(status->parse_msg, "Unrecognized input:%s\n",
> tokens[ti]);
> +		status->status = -1;
> +		return;
>  	}
> +	printf("\n");
> 
>  	nb_flow_rule++;
>  }
> 
> -#define MAX_RTE_FLOW_PATTERN (3)
> -#define MAX_RTE_FLOW_ACTIONS (2)
> +#define MAX_RTE_FLOW_PATTERN (4)
> +#define MAX_RTE_FLOW_ACTIONS (5)
> 
>  static void
>  flow_init_single(struct flow_rule_entry *rule)
>  {
> -	struct rte_flow_item pattern[MAX_RTE_FLOW_PATTERN] = {};
>  	struct rte_flow_action action[MAX_RTE_FLOW_ACTIONS] = {};
> +	struct rte_flow_item pattern[MAX_RTE_FLOW_PATTERN] = {};
> +	struct rte_flow_action_queue queue_action;
> +	struct rte_flow_action_mark mark_action;
> +	int ret, pattern_idx = 0, act_idx = 0;
> +	struct rte_flow_item_mark mark_mask;
>  	struct rte_flow_attr attr = {};
> -	struct rte_flow_error err;
> -	int ret;
> +	struct rte_flow_error err = {};
> 
>  	attr.egress = 0;
>  	attr.ingress = 1;
> 
> -	action[0].type = RTE_FLOW_ACTION_TYPE_QUEUE;
> -	action[0].conf = &(struct rte_flow_action_queue) {
> -				.index = rule->queue,
> -	};
> -	action[1].type = RTE_FLOW_ACTION_TYPE_END;
> +	if (rule->is_queue_set) {
> +		queue_action.index = rule->queue;
> +		action[act_idx].type = RTE_FLOW_ACTION_TYPE_QUEUE;
> +		action[act_idx].conf = &queue_action;
> +		act_idx++;
> +	}
> +
> +	if (rule->enable_count) {
> +		action[act_idx].type = RTE_FLOW_ACTION_TYPE_COUNT;
> +		act_idx++;
> +	}
> +
> +	if (rule->set_security_action) {
> +		action[act_idx].type = RTE_FLOW_ACTION_TYPE_SECURITY;
> +		action[act_idx].conf = NULL;
> +		act_idx++;
> +	}
> +
> +	if (rule->set_mark_action) {
> +		mark_action.id = rule->mark_action_val;
> +		action[act_idx].type = RTE_FLOW_ACTION_TYPE_MARK;
> +		action[act_idx].conf = &mark_action;
> +		act_idx++;
> +	}
> 
> -	pattern[0].type = RTE_FLOW_ITEM_TYPE_ETH;
> +	action[act_idx].type = RTE_FLOW_ACTION_TYPE_END;
> +	action[act_idx].conf = NULL;
> +
> +	if (rule->enable_mark) {
> +		mark_mask.id = UINT32_MAX;
> +		pattern[pattern_idx].type = RTE_FLOW_ITEM_TYPE_MARK;
> +		pattern[pattern_idx].spec = &rule->mark_val;
> +		pattern[pattern_idx].mask = &mark_mask;
> +		pattern_idx++;
> +	}
> +
> +	if (rule->is_eth) {
> +		pattern[pattern_idx].type = RTE_FLOW_ITEM_TYPE_ETH;
> +		pattern_idx++;
> +	}
> 
>  	if (rule->is_ipv4) {
> -		pattern[1].type = RTE_FLOW_ITEM_TYPE_IPV4;
> -		pattern[1].spec = &rule->ipv4.spec;
> -		pattern[1].mask = &rule->ipv4.mask;
> -	} else {
> -		pattern[1].type = RTE_FLOW_ITEM_TYPE_IPV6;
> -		pattern[1].spec = &rule->ipv6.spec;
> -		pattern[1].mask = &rule->ipv6.mask;
> +		pattern[pattern_idx].type = RTE_FLOW_ITEM_TYPE_IPV4;
> +		pattern[pattern_idx].spec = &rule->ipv4.spec;
> +		pattern[pattern_idx].mask = &rule->ipv4.mask;
> +		pattern_idx++;
> +	}
> +
> +	if (rule->is_ipv6) {
> +		pattern[pattern_idx].type = RTE_FLOW_ITEM_TYPE_IPV6;
> +		pattern[pattern_idx].spec = &rule->ipv6.spec;
> +		pattern[pattern_idx].mask = &rule->ipv6.mask;
> +		pattern_idx++;
> +	}
> +
> +	if (rule->set_security_action) {
> +		pattern[pattern_idx].type = RTE_FLOW_ITEM_TYPE_ESP;
> +		pattern[pattern_idx].spec = NULL;
> +		pattern[pattern_idx].mask = NULL;
> +		pattern[pattern_idx].last = NULL;
> +		pattern_idx++;
>  	}
> 
> -	pattern[2].type = RTE_FLOW_ITEM_TYPE_END;
> +	pattern[pattern_idx].type = RTE_FLOW_ITEM_TYPE_END;
> 
>  	ret = rte_flow_validate(rule->port, &attr, pattern, action, &err);
>  	if (ret < 0) {
>  		RTE_LOG(ERR, IPSEC, "Flow validation failed %s\n",
> err.message);
> +		rule->flow = 0;
>  		return;
>  	}
> 
> @@ -251,6 +365,56 @@ flow_init_single(struct flow_rule_entry *rule)
>  		RTE_LOG(ERR, IPSEC, "Flow creation return %s\n",
> err.message);
>  }
> 
> +void
> +flow_print_counters(void)
> +{
> +	struct rte_flow_query_count count_query;
> +	struct rte_flow_action action;
> +	struct flow_rule_entry *rule;
> +	struct rte_flow_error error;
> +	int i = 0, ret = 0;
> +
> +	action.type = RTE_FLOW_ACTION_TYPE_COUNT;
> +
> +	for (i = 0; i < nb_flow_rule; i++) {
> +		rule = &flow_rule_tbl[i];
> +		if (!rule->flow || !rule->enable_count)
> +			continue;
> +
> +		/* Poisoning to make sure PMDs update it in case of error. */
> +		memset(&error, 0x55, sizeof(error));
> +		memset(&count_query, 0, sizeof(count_query));
> +		ret = rte_flow_query(rule->port, rule->flow, &action,
> +				     &count_query, &error);
> +		if (ret)
> +			RTE_LOG(ERR, IPSEC,
> +				"Failed to get flow counter "
> +				" for port %u, err msg: %s\n",
> +				rule->port, error.message);
> +
> +		printf("Flow #%3d:", i);
> +		if (rule->is_ipv4) {
> +			printf(" spec ipv4 ");
> +			ipv4_hdr_print(&rule->ipv4.spec.hdr);
> +		}
> +		if (rule->is_ipv6) {
> +			printf(" spec ipv6 ");
> +			ipv6_hdr_print(&rule->ipv6.spec.hdr);
> +		}
> +
> +		if (rule->set_security_action)
> +			printf(" Security action set,");
> +
> +		if (rule->enable_mark)
> +			printf(" Mark Enabled");
> +
> +		printf(" Port: %d,", rule->port);
> +		if (rule->is_queue_set)
> +			printf(" Queue: %d", rule->queue);
> +		printf(" Hits: %"PRIu64"\n", count_query.hits);
> +	}
> +}
> +
>  void
>  flow_init(void)
>  {
> @@ -264,21 +428,37 @@ flow_init(void)
> 
>  	for (i = 0; i < nb_flow_rule; i++) {
>  		rule = &flow_rule_tbl[i];
> +		printf("Flow #%3d: ", i);
>  		if (rule->is_ipv4) {
> -			printf("Flow #%3d: spec ipv4 ", i);
> +			printf("spec ipv4 ");
>  			ipv4_hdr_print(&rule->ipv4.spec.hdr);
>  			printf("\n");
> -			printf("           mask ipv4 ");
> +			printf(" mask ipv4 ");
>  			ipv4_hdr_print(&rule->ipv4.mask.hdr);
> -		} else {
> -			printf("Flow #%3d: spec ipv6 ", i);
> +		}
> +		if (rule->is_ipv6) {
> +			printf("spec ipv6 ");
>  			ipv6_hdr_print(&rule->ipv6.spec.hdr);
>  			printf("\n");
> -			printf("           mask ipv6 ");
> +			printf(" mask ipv6 ");
>  			ipv6_hdr_print(&rule->ipv6.mask.hdr);
>  		}
> 
> -		printf("\tPort: %d, Queue: %d", rule->port, rule->queue);
> +		if (rule->enable_mark)
> +			printf(", Mark enabled");
> +
> +		printf("\tPort: %d,", rule->port);
> +		if (rule->is_queue_set)
> +			printf(" Queue: %d,", rule->queue);
> +
> +		if (rule->set_security_action)
> +			printf(" Security action set,");
> +
> +		if (rule->set_mark_action)
> +			printf(" Mark: %d,", rule->mark_action_val);
> +
> +		if (rule->enable_count)
> +			printf(" Counter enabled,");
> 
>  		if (rule->flow == NULL)
>  			printf(" [UNSUPPORTED]");
> diff --git a/examples/ipsec-secgw/flow.h b/examples/ipsec-secgw/flow.h
> index 1b1b4774e4..9492d06346 100644
> --- a/examples/ipsec-secgw/flow.h
> +++ b/examples/ipsec-secgw/flow.h
> @@ -11,5 +11,6 @@ void parse_flow_tokens(char **tokens, uint32_t n_tokens,
>  		       struct parse_status *status);
> 
>  void flow_init(void);
> +void flow_print_counters(void);
> 
>  #endif /* _FLOW_H_ */
> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-
> secgw.c
> index 42b5081840..244453e06e 100644
> --- a/examples/ipsec-secgw/ipsec-secgw.c
> +++ b/examples/ipsec-secgw/ipsec-secgw.c
> @@ -3271,7 +3271,6 @@ ipsec_secgw_telemetry_init(void)
>  		"Optional Parameters: int <logical core id>");
>  }
> 
> -
>  int32_t
>  main(int32_t argc, char **argv)
>  {
> @@ -3512,6 +3511,8 @@ main(int32_t argc, char **argv)
>  		printf(" Done\n");
>  	}
> 
> +	flow_print_counters();
> +
>  	RTE_ETH_FOREACH_DEV(portid) {
>  		if ((enabled_port_mask & (1 << portid)) == 0)
>  			continue;
> --
> 2.35.3