Re: [dpdk-dev] [EXT] [PATCH v3] examples/ipsec-secgw: support flow director feature

["Shetty, Praveen" <praveen.shetty@intel.com>]

Hi Anoob,

See my response inline.

Regards,
Praveen

-----Original Message-----
From: Anoob Joseph <anoobj@marvell.com> 
Sent: Thursday, April 2, 2020 7:09 PM
To: Shetty, Praveen <praveen.shetty@intel.com>; dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>
Cc: Iremonger, Bernard <bernard.iremonger@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>
Subject: RE: [EXT] [PATCH v3] examples/ipsec-secgw: support flow director feature

Hi Praveen,

I've few minor comments. Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Praveen Shetty <praveen.shetty@intel.com>
> Sent: Tuesday, March 31, 2020 6:32 PM
> To: dev@dpdk.org; declan.doherty@intel.com; Anoob Joseph 
> <anoobj@marvell.com>
> Cc: bernard.iremonger@intel.com; konstantin.ananyev@intel.com
> Subject: [EXT] [PATCH v3] examples/ipsec-secgw: support flow director 
> feature
> 
> External Email
> 
> ----------------------------------------------------------------------
> Support load distribution in security gateway application using NIC 
> load distribution feature(Flow Director).
> Flow Director is used to redirect the specified inbound ipsec flow to 
> a specified queue.This is achieved by extending the SA rule syntax to 
> support specification by adding new action_type of <flow-direction> to 
> a specified <port_id> <queue_id>.
> 
> Signed-off-by: Praveen Shetty <praveen.shetty@intel.com>
> ---
> v3 changes:
> Incorporated Anoob review comments on v2.
> 
>  examples/ipsec-secgw/ep0.cfg       | 11 +++++
>  examples/ipsec-secgw/ipsec-secgw.c | 55 +++++++++++++++++++++++
>  examples/ipsec-secgw/ipsec.c       | 67 +++++++++++++++++++++++++++
>  examples/ipsec-secgw/ipsec.h       |  9 ++++
>  examples/ipsec-secgw/sa.c          | 72 ++++++++++++++++++++++++++++--
>  5 files changed, 211 insertions(+), 3 deletions(-)
> 
> diff --git a/examples/ipsec-secgw/ep0.cfg 
> b/examples/ipsec-secgw/ep0.cfg index dfd4aca7d..6f8d5aa53 100644
> --- a/examples/ipsec-secgw/ep0.cfg
> +++ b/examples/ipsec-secgw/ep0.cfg
> @@ -29,6 +29,7 @@ sp ipv4 in esp protect 111 pri 1 dst 
> 192.168.186.0/24 sport
> 0:65535 dport 0:6553  sp ipv4 in esp protect 115 pri 1 dst 
> 192.168.210.0/24 sport
> 0:65535 dport 0:65535  sp ipv4 in esp protect 116 pri 1 dst 
> 192.168.211.0/24 sport 0:65535 dport 0:65535  sp ipv4 in esp protect 
> 115 pri 1 dst
> 192.168.210.0/24 sport 0:65535 dport 0:65535
> +sp ipv4 in esp protect 117 pri 1 dst 192.168.212.0/24 sport 0:65535 
> +dport 0:65535
>  sp ipv4 in esp protect 125 pri 1 dst 192.168.65.0/24 sport 0:65535 
> dport 0:65535 sp ipv4 in esp protect 125 pri 1 dst 192.168.65.0/24 
> sport 0:65535 dport 0:65535 sp ipv4 in esp protect 126 pri 1 dst 
> 192.168.66.0/24 sport 0:65535 dport 0:65535 @@ -61,6 +62,8 @@ sp ipv6 
> in esp protect 125 pri 1 dst
> ffff:0000:0000:0000:aaaa:aaaa:0000:0000/96
>  sport 0:65535 dport 0:65535
>  sp ipv6 in esp protect 126 pri 1 dst
> ffff:0000:0000:0000:bbbb:bbbb:0000:0000/96 \  sport 0:65535 dport 
> 0:65535
> +sp ipv6 in esp protect 127 pri 1 dst
> +ffff:0000:0000:0000:cccc:dddd:0000:0000/96 \ sport 0:65535 dport
> +0:65535
> 
>  #SA rules
>  sa out 5 cipher_algo aes-128-cbc cipher_key 
> 0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0 \ @@ -118,6 +121,9 @@ dst 172.16.1.5
> 
>  sa in 116 cipher_algo null auth_algo null mode ipv4-tunnel src 
> 172.16.2.6 dst
> 172.16.1.6
> 
> +sa in 117 cipher_algo null auth_algo null mode ipv4-tunnel src
> +172.16.2.7 \ dst 172.16.1.7 flow-direction 0 2
> +
>  sa in 125 cipher_algo aes-128-cbc cipher_key 
> c3:c3:c3:c3:c3:c3:c3:c3:c3:c3:c3:\
>  c3:c3:c3:c3:c3 auth_algo sha1-hmac auth_key 
> c3:c3:c3:c3:c3:c3:c3:c3:c3:c3:c3:\
>  c3:c3:c3:c3:c3:c3:c3:c3:c3 mode ipv6-tunnel \ @@ -130,6 +136,11 @@ sa 
> in
> 126 cipher_algo aes-128-cbc cipher_key 
> 4d:4d:4d:4d:4d:4d:4d:4d:4d:4d:4d:\
>  src 2222:2222:2222:2222:2222:2222:2222:6666 \  dst
> 1111:1111:1111:1111:1111:1111:1111:6666
> 
> +sa in 127 cipher_algo null auth_algo null mode ipv6-tunnel \ src
> +2222:2222:2222:2222:2222:2222:2222:7777 \ dst
> +1111:1111:1111:1111:1111:1111:1111:7777 \ flow-direction 0 3
> +
>  #Routing rules
>  rt ipv4 dst 172.16.2.5/32 port 0
>  rt ipv4 dst 172.16.2.6/32 port 1
> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec- 
> secgw/ipsec-secgw.c index ce36e6d9c..4400b075c 100644
> --- a/examples/ipsec-secgw/ipsec-secgw.c
> +++ b/examples/ipsec-secgw/ipsec-secgw.c
> @@ -246,6 +246,30 @@ static struct rte_eth_conf port_conf = {
>  	.txmode = {
>  		.mq_mode = ETH_MQ_TX_NONE,
>  	},
> +	.fdir_conf = {
> +	.mode = RTE_FDIR_MODE_NONE,
> +	.pballoc = RTE_FDIR_PBALLOC_64K,
> +	.status = RTE_FDIR_REPORT_STATUS,
> +	.mask = {
> +		.vlan_tci_mask = 0xFFEF,
> +		.ipv4_mask     = {
> +			.src_ip = 0xFFFFFFFF,
> +			.dst_ip = 0xFFFFFFFF,
> +		},
> +		.ipv6_mask     = {
> +			.src_ip = {0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> +						0xFFFFFFFF},
> +			.dst_ip = {0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> +						0xFFFFFFFF},
> +		},
> +		.src_port_mask = 0xFFFF,
> +		.dst_port_mask = 0xFFFF,
> +		.mac_addr_byte_mask = 0xFF,
> +		.tunnel_type_mask = 1,
> +		.tunnel_id_mask = 0xFFFFFFFF,
> +	},
> +	.drop_queue = 127,
> +	}
>  };
> 
>  struct socket_ctx socket_ctx[NB_SOCKETS]; @@ -1183,6 +1207,28 @@
> ipsec_poll_mode_worker(void)
>  	}
>  }
> 
> +int
> +check_flow_params(uint16_t fdir_portid, uint8_t fdir_qid) {
> +	uint16_t i;
> +	uint16_t portid;
> +	uint8_t queueid;
> +
> +	for (i = 0; i < nb_lcore_params; ++i) {
> +		portid = lcore_params_array[i].port_id;
> +		if (portid == fdir_portid) {
> +			queueid = lcore_params_array[i].queue_id;
> +			if (queueid == fdir_qid)
> +				break;
> +		}
> +
> +		if (i == nb_lcore_params - 1)
> +			return -1;
> +	}
> +
> +	return 1;
> +}
> +
>  static int32_t
>  check_poll_mode_params(struct eh_conf *eh_conf)  { @@ -2813,6 
> +2859,15 @@ main(int32_t argc, char **argv)
> 
>  		sa_check_offloads(portid, &req_rx_offloads[portid],
>  				&req_tx_offloads[portid]);
> +		 /* check if FDIR is configured on the port */
> +		if (check_fdir_configured(portid)) {
> +			/* Enable FDIR */
> +			port_conf.fdir_conf.mode =
> RTE_FDIR_MODE_PERFECT;
> +			/* Disable RSS */
> +			port_conf.rxmode.mq_mode = ETH_MQ_RX_NONE;
> +			port_conf.rx_adv_conf.rss_conf.rss_hf = 0;
> +			port_conf.rx_adv_conf.rss_conf.rss_key = NULL;
> +		}
>  		port_init(portid, req_rx_offloads[portid],
>  				req_tx_offloads[portid]);
>  	}
> diff --git a/examples/ipsec-secgw/ipsec.c 
> b/examples/ipsec-secgw/ipsec.c index d40657102..76ee9dbcf 100644
> --- a/examples/ipsec-secgw/ipsec.c
> +++ b/examples/ipsec-secgw/ipsec.c
> @@ -418,6 +418,73 @@ create_inline_session(struct socket_ctx *skt_ctx, 
> struct ipsec_sa *sa,
>  	return 0;
>  }
> 
> +int
> +create_ipsec_esp_flow(struct ipsec_sa *sa) {
> +	int ret = 0;
> +	struct rte_flow_error err;
> +	if (sa->direction == RTE_SECURITY_IPSEC_SA_DIR_EGRESS)
> +		return 0; /* No Flow director rules for Egress traffic */

[Anoob] Any reason why this is not relevant for Egress. 

[Praveen] we don't see an use case for load distribution across ingress queues for outbound IPsec traffic therefore we have limited this configuration to inbound IPsec processing, as this is the only use case we can verify.

As for the code I would suggest something like,
	/* No flow director rules for Egress traffic */
	if (sa->direction == RTE_SECURITY_IPSEC_SA_DIR_EGRESS)
		return 0;

	...
 
> +	if (sa->flags == TRANSPORT) {
> +		RTE_LOG(ERR, IPSEC,
> +			"No Flow director rule for transport mode:");

[Anoob] Is the ending : required in the line above? And do might need a \n?

[Praveen] Will fix this in v4.

Also, why is one case returning 0 (EGRESS) and another (TRANSPORT) is returning -1? One is treated as error and other is not?

[Praveen] It should be -1(error) for both the cases , will fix this in v4.

 
> +			return -1;
> +	}
> +	sa->action[0].type = RTE_FLOW_ACTION_TYPE_QUEUE;
> +	sa->pattern[0].type = RTE_FLOW_ITEM_TYPE_ETH;
> +	sa->action[0].conf =
> +			&(struct rte_flow_action_queue){
> +				.index = sa->fdir_qid,
> +	};
> +	sa->attr.egress = 0;
> +	sa->attr.ingress = 1;
> +	if (IS_IP6(sa->flags)) {
> +		sa->pattern[1].mask = &rte_flow_item_ipv6_mask;
> +		sa->pattern[1].type = RTE_FLOW_ITEM_TYPE_IPV6;
> +		sa->pattern[1].spec = &sa->ipv6_spec;
> +		memcpy(sa->ipv6_spec.hdr.dst_addr,
> +			sa->dst.ip.ip6.ip6_b, sizeof(sa->dst.ip.ip6.ip6_b));
> +		memcpy(sa->ipv6_spec.hdr.src_addr,
> +			sa->src.ip.ip6.ip6_b, sizeof(sa->src.ip.ip6.ip6_b));
> +		sa->pattern[2].type = RTE_FLOW_ITEM_TYPE_ESP;
> +		sa->pattern[2].spec = &sa->esp_spec;
> +		sa->pattern[2].mask = &rte_flow_item_esp_mask;
> +		sa->esp_spec.hdr.spi = rte_cpu_to_be_32(sa->spi);
> +		sa->pattern[3].type = RTE_FLOW_ITEM_TYPE_END;
> +	} else if (IS_IP4(sa->flags)) {
> +		sa->pattern[1].mask = &rte_flow_item_ipv4_mask;
> +		sa->pattern[1].type = RTE_FLOW_ITEM_TYPE_IPV4;
> +		sa->pattern[1].spec = &sa->ipv4_spec;
> +		sa->ipv4_spec.hdr.dst_addr = sa->dst.ip.ip4;
> +		sa->ipv4_spec.hdr.src_addr = sa->src.ip.ip4;
> +		sa->pattern[2].type = RTE_FLOW_ITEM_TYPE_ESP;
> +		sa->pattern[2].spec = &sa->esp_spec;
> +		sa->pattern[2].mask = &rte_flow_item_esp_mask;
> +		sa->esp_spec.hdr.spi = rte_cpu_to_be_32(sa->spi);
> +		sa->pattern[3].type = RTE_FLOW_ITEM_TYPE_END;
> +	}
> +	sa->action[1].type = RTE_FLOW_ACTION_TYPE_END;
> +
> +	ret = rte_flow_validate(sa->portid, &sa->attr,
> +				sa->pattern, sa->action,
> +				&err);
> +	if (ret < 0) {
> +		RTE_LOG(ERR, IPSEC,
> +			"Flow Validation failed\n");

[Anoob] The above should fit into one line. Also V should be small.

[Praveen] Okay. Will fix this in v4.
 
> +		return ret;
> +	}
> +	sa->flow = rte_flow_create(sa->portid,
> +				&sa->attr, sa->pattern, sa->action,
> +				&err);

[Anoob] Start breaking into multiple lines when you exceed 80 char limits. In the earlier line, &sa->attr should fit into the line above.

Also, having a blank line above rte_flow_create() line would be good.

[Praveen] Okay. Will fix it in v4
 
> +	if (!sa->flow) {
> +		RTE_LOG(ERR, IPSEC,
> +			"Flow Creation failed\n");

[Anoob] Above should fit into one line. Also C should be small.

[Praveen] okay. Will fix this in v4.

> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * queue crypto-ops into PMD queue.
>   */
> diff --git a/examples/ipsec-secgw/ipsec.h 
> b/examples/ipsec-secgw/ipsec.h index f8f29f9b1..b0e9f45cb 100644
> --- a/examples/ipsec-secgw/ipsec.h
> +++ b/examples/ipsec-secgw/ipsec.h
> @@ -144,6 +144,8 @@ struct ipsec_sa {
>  	};
>  	enum rte_security_ipsec_sa_direction direction;
>  	uint16_t portid;
> +	uint8_t fdir_qid;
> +	uint8_t fdir_flag;
> 
>  #define MAX_RTE_FLOW_PATTERN (4)
>  #define MAX_RTE_FLOW_ACTIONS (3)
> @@ -408,5 +410,12 @@ create_lookaside_session(struct ipsec_ctx 
> *ipsec_ctx, struct ipsec_sa *sa,  int  create_inline_session(struct 
> socket_ctx *skt_ctx, struct ipsec_sa *sa,
>  		struct rte_ipsec_session *ips);
> +int
> +check_flow_params(uint16_t fdir_portid, uint8_t fdir_qid);
> +
> +int
> +create_ipsec_esp_flow(struct ipsec_sa *sa);
> 
> +int
> +check_fdir_configured(uint16_t portid);
>  #endif /* __IPSEC_H__ */
> diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c 
> index
> 0eb52d141..ddd275142 100644
> --- a/examples/ipsec-secgw/sa.c
> +++ b/examples/ipsec-secgw/sa.c
> @@ -271,6 +271,7 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
>  	uint32_t type_p = 0;
>  	uint32_t portid_p = 0;
>  	uint32_t fallback_p = 0;
> +	int16_t status_p = 0;
> 
>  	if (strcmp(tokens[0], "in") == 0) {
>  		ri = &nb_sa_in;
> @@ -295,6 +296,7 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
>  	if (atoi(tokens[1]) == INVALID_SPI)
>  		return;
>  	rule->spi = atoi(tokens[1]);
> +	rule->portid = UINT16_MAX;
>  	ips = ipsec_get_primary_session(rule);
> 
>  	for (ti = 2; ti < n_tokens; ti++) {
> @@ -636,9 +638,14 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
>  			INCREMENT_TOKEN_INDEX(ti, n_tokens, status);
>  			if (status->status < 0)
>  				return;
> -			rule->portid = atoi(tokens[ti]);
> -			if (status->status < 0)
> +			if (rule->portid == UINT16_MAX)
> +				rule->portid = atoi(tokens[ti]);
> +			else if (rule->portid != atoi(tokens[ti])) {
> +				APP_CHECK(0, status, "portid %s "
> +				"not matching with already assigned portid
> %u",
> +				tokens[ti], rule->portid);
>  				return;

[Anoob] Alignment for APP_CHECK's broken up parts are not following the convention.

[Praveen] Will fix this in v4.

> +			}
>  			portid_p = 1;
>  			continue;
>  		}
> @@ -681,6 +688,43 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
>  			fallback_p = 1;
>  			continue;
>  		}
> +		if (strcmp(tokens[ti], "flow-direction") == 0) {
> +			if (ips->type ==
> +
> 	RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL ||
> +				ips->type ==
> +
> 	RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL ||
> +				ips->type ==
> +
> 	RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO) {
> +				APP_CHECK(0, status, "Flow Director not "
> +					"supported for security session "
> +					"type:%d", ips->type);
> +					return;
> +			}
> +			rule->fdir_flag = 1;
> +			INCREMENT_TOKEN_INDEX(ti, n_tokens, status);
> +			if (status->status < 0)
> +				return;
> +			if (rule->portid == UINT16_MAX)
> +				rule->portid = atoi(tokens[ti]);
> +			else if (rule->portid != atoi(tokens[ti])) {
> +				APP_CHECK(0, status, "portid %s "
> +				"not matching with already assigned portid
> %u",
> +				tokens[ti], rule->portid);
> +				return;
> +			}
> +			INCREMENT_TOKEN_INDEX(ti, n_tokens, status);
> +			if (status->status < 0)
> +				return;
> +			rule->fdir_qid = atoi(tokens[ti]);
> +			/* validating portid and queueid */
> +			status_p = check_flow_params(rule->portid,
> +					rule->fdir_qid);
> +			if (status_p < 0) {
> +				printf("port id %u / queue id %u is not valid\n",
> +					rule->portid, rule->fdir_qid);
> +			}
> +			continue;
> +		}
> 
>  		/* unrecognizeable input */
>  		APP_CHECK(0, status, "unrecognized input \"%s\"", @@ -719,7
> +763,6 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
>  	if (!type_p || (!portid_p && ips->type !=
>  			RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO)) {
>  		ips->type = RTE_SECURITY_ACTION_TYPE_NONE;
> -		rule->portid = -1;
>  	}
> 
>  	*ri = *ri + 1;
> @@ -823,6 +866,9 @@ print_one_sa_rule(const struct ipsec_sa *sa, int
> inbound)
>  			break;
>  		}
>  	}
> +	if (sa->fdir_flag == 1)
> +		printf("flow-direction %d %d", sa->portid, sa->fdir_qid);
> +
>  	printf("\n");
>  }
> 
> @@ -1141,6 +1187,12 @@ sa_add_rules(struct sa_ctx *sa_ctx, const 
> struct ipsec_sa entries[],
>  			}
>  		}
> 
> +		if (sa->fdir_flag && inbound) {
> +			rc = create_ipsec_esp_flow(sa);
> +			if (rc != 0)
> +				RTE_LOG(ERR, IPSEC_ESP,
> +					"create_ipsec_esp flow failed\n");

[Anoob] Instead of function name, can you give the description of what actually failed?

[Praveen] Can be done , will do it in v4.

> +			}
>  		print_one_sa_rule(sa, inbound);
>  	}
> 
> @@ -1243,6 +1295,20 @@ fill_ipsec_session(struct rte_ipsec_session 
> *ss, struct rte_ipsec_sa *sa)
>  	return rc;
>  }
> 
> +int
> +check_fdir_configured(uint16_t portid) {
> +	struct ipsec_sa *sa = NULL;
> +	uint32_t idx_sa = 0;
> +
> +	for (idx_sa = 0; idx_sa < nb_sa_in; idx_sa++) {
> +		sa = &sa_in[idx_sa];
> +		if (sa->portid == portid)
> +			return sa->fdir_flag;
> +	}
> +	return 0;
> +}
> +
>  /*
>   * Initialise related rte_ipsec_sa object.
>   */
> --
> 2.17.1