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

[Anoob Joseph <anoobj@marvell.com>]

Hi Praveen,

You need to rebase to the latest dpdk-next-crypto code base. This patch is not applying cleanly.

There were few patches from Intel removing the existing FDIR and adding some new implementation. Hope this patch takes that also into account. Also, why do we need to call the feature flow director. I would assume rte_flow per SA is what is being attempted here.

Few comments. See inline.

Thanks,
Anoob

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Praveen Shetty
> Sent: Thursday, March 19, 2020 9:52 PM
> To: dev@dpdk.org; declan.doherty@intel.com; bernard.iremonger@intel.co;
> konstantin.ananyev@intel.com
> Subject: [dpdk-dev] [PATCH v2] examples/ipsec-secgw: support flow director
> feature
> 
> 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>
> ---
> v2 changes:
> added more details in commit message.
> added a check to throw an error if the security session type is other than
> LOOKASIDE_NONE
> 
>  examples/ipsec-secgw/ep0.cfg       | 11 +++++
>  examples/ipsec-secgw/ipsec-secgw.c | 56 ++++++++++++++++++++++++-
>  examples/ipsec-secgw/ipsec.c       | 67 ++++++++++++++++++++++++++++++
>  examples/ipsec-secgw/ipsec.h       | 11 +++++
>  examples/ipsec-secgw/sa.c          | 60 +++++++++++++++++++++++++-
>  5 files changed, 202 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 4799bc90c..132484422 100644
> --- a/examples/ipsec-secgw/ipsec-secgw.c
> +++ b/examples/ipsec-secgw/ipsec-secgw.c
> @@ -166,7 +166,6 @@ static const struct option lgopts[] = {
>  	{CMD_LINE_OPT_FRAG_TTL, 1, 0, CMD_LINE_OPT_FRAG_TTL_NUM},
>  	{NULL, 0, 0, 0}
>  };
> -

[Anoob] I would assume the above change is not intentional.
 
>  /* mask of enabled ports */
>  static uint32_t enabled_port_mask;
>  static uint64_t enabled_cryptodev_mask = UINT64_MAX; @@ -259,6 +258,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,
> +	}
>  };
> 
>  static struct socket_ctx socket_ctx[NB_SOCKETS]; @@ -1184,7 +1207,6 @@
> main_loop(__attribute__((unused)) void *dummy)
> 
>  			if (nb_rx > 0)
>  				process_pkts(qconf, pkts, nb_rx, portid);
> -

[Anoob] I would assume the above change is not intentional.
 
>  			/* dequeue and process completed crypto-ops */
>  			if (UNPROTECTED_PORT(portid))
>  				drain_inbound_crypto_queues(qconf,
> @@ -1196,6 +1218,27 @@ main_loop(__attribute__((unused)) void *dummy)
>  	}
>  }
> 
> +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_params(void)
>  {
> @@ -2503,6 +2546,15 @@ main(int32_t argc, char **argv)
>  			continue;
> 
>  		sa_check_offloads(portid, &req_rx_offloads,
> &req_tx_offloads);
> +		/* 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;
> +		}

[Anoob] This would mean, once FDIR is enabled for one SA, then the port init is changed. RSS is disabled and FDIR gets enabled. Can you confirm if I understood the code correctly?
 
>  		port_init(portid, req_rx_offloads, req_tx_offloads);
>  	}
> 
> diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c index
> 6e8120702..363809cfd 100644
> --- a/examples/ipsec-secgw/ipsec.c
> +++ b/examples/ipsec-secgw/ipsec.c
> @@ -415,6 +415,73 @@ create_inline_session(struct socket_ctx *skt_ctx,
> struct ipsec_sa *sa,
>  	return 0;
>  }
> 
> +int
> +create_ipsec_esp_flow(struct ipsec_sa *sa) {

[Anoob] Why just ESP? Can't the same rte_flow rules be used for doing BYPASS/TRANSPORT?
 
> +	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 */
> +	if (sa->flags == TRANSPORT) {
> +		RTE_LOG(ERR, IPSEC,
> +			"No Flow director rule for transport mode:");
> +			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, IPV6_ADDR_LEN);
> +		memcpy(sa->ipv6_spec.hdr.src_addr,
> +				sa->src.ip.ip6.ip6_b, IPV6_ADDR_LEN);
> +		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->fdir_portid, &sa->attr,
> +				sa->pattern, sa->action,
> +				&err);
> +	if (ret < 0) {
> +		RTE_LOG(ERR, IPSEC,
> +			"Flow Validation failed\n");
> +		return ret;
> +	}
> +	sa->flow = rte_flow_create(sa->fdir_portid,
> +				&sa->attr, sa->pattern, sa->action,
> +				&err);
> +	if (!sa->flow) {
> +		RTE_LOG(ERR, IPSEC,
> +			"Flow Creation failed\n");
> +		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 4f2fd6184..00147895a 100644
> --- a/examples/ipsec-secgw/ipsec.h
> +++ b/examples/ipsec-secgw/ipsec.h
> @@ -46,6 +46,8 @@
> 
>  #define IP6_VERSION (6)
> 
> +#define IPV6_ADDR_LEN   16

[Anoob] Can't we do a sizeof of some structure to get the same? Having an application specific macro may not be the right approach.
 
> +
>  struct rte_crypto_xform;
>  struct ipsec_xform;
>  struct rte_mbuf;
> @@ -138,6 +140,9 @@ struct ipsec_sa {
>  	};
>  	enum rte_security_ipsec_sa_direction direction;
>  	uint16_t portid;
> +	uint16_t fdir_portid;

[Anoob] The existing member 'portid' above 'fdir_portid' is used for a similar rte_flow creation for inline IPsec. Can't we use the same here also?
 
> +	uint8_t fdir_qid;
> +	uint8_t fdir_flag;
> 
>  #define MAX_RTE_FLOW_PATTERN (4)
>  #define MAX_RTE_FLOW_ACTIONS (3)
> @@ -383,5 +388,11 @@ 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);

[Anoob] Better to stick to the convention followed.
 
>  #endif /* __IPSEC_H__ */
> diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c index
> 4822d6bda..204a685fb 100644
> --- a/examples/ipsec-secgw/sa.c
> +++ b/examples/ipsec-secgw/sa.c
> @@ -20,6 +20,9 @@
>  #include <rte_random.h>
>  #include <rte_ethdev.h>
>  #include <rte_malloc.h>
> +#include <rte_common.h>
> +#include <rte_string_fns.h>
> +#include <rte_ethdev_driver.h>
> 
>  #include "ipsec.h"
>  #include "esp.h"
> @@ -271,6 +274,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;
> @@ -681,6 +685,38 @@ 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_CPU_CRYPTO
> ||
> +				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;
> +			rule->fdir_portid = atoi(tokens[ti]);
> +			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->fdir_portid,
> +					rule->fdir_qid);
> +			if (status_p < 0) {
> +				printf("port id %u / queue id %u is not valid\n",
> +					rule->fdir_portid, rule->fdir_qid);
> +			}
> +			continue;
> +		}
> 
>  		/* unrecognizeable input */
>  		APP_CHECK(0, status, "unrecognized input \"%s\"", @@ -823,6
> +859,9 @@ print_one_sa_rule(const struct ipsec_sa *sa, int inbound)
>  			break;
>  		}
>  	}
> +	if (sa->fdir_flag == 1)
> +		printf("flow-direction %d %d", sa->fdir_portid, sa->fdir_qid);
> +
>  	printf("\n");
>  }
> 
> @@ -1153,7 +1192,12 @@ sa_add_rules(struct sa_ctx *sa_ctx, const struct
> ipsec_sa entries[],
>  				return -EINVAL;
>  			}
>  		}
> -
> +		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");
> +			}
>  		print_one_sa_rule(sa, inbound);
>  	}
> 
> @@ -1256,6 +1300,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->fdir_portid == portid)
> +			return sa->fdir_flag;
> +	}
> +	return 0;
> +}
> +
>  /*
>   * Initialise related rte_ipsec_sa object.
>   */
> --
> 2.17.1