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

[Anoob Joseph <anoobj@marvell.com>]

Hi Praveen,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Shetty, Praveen <praveen.shetty@intel.com>
> Sent: Thursday, April 2, 2020 11:27 PM
> To: Anoob Joseph <anoobj@marvell.com>; dev@dpdk.org; Doherty, Declan
> <declan.doherty@intel.com>; Akhil Goyal <akhil.goyal@nxp.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 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.

[Anoob] Why do you say load distribution for ingress queues is not required but is required for egress? I would say the use case is the same in either direction.

Said that, adding just egress should be fine. I leave this to Akhil's judgement.

> 
> 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