[v3] examples/ipsec-secgw: support flow director feature

Message ID 20200331130211.24761-1-praveen.shetty@intel.com (mailing list archive)
State Superseded, archived
Delegated to: akhil goyal
Headers
Series [v3] examples/ipsec-secgw: support flow director feature |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail apply issues

Commit Message

Shetty, Praveen March 31, 2020, 1:02 p.m. UTC
  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(-)
  

Comments

Akhil Goyal April 1, 2020, 1:19 p.m. UTC | #1
Hi Praveen,

Sorry for being late to reply on this, Please delegate the patches properly from next time in patchworks.
This patch was neither delegated to me, nor I was in to/cc. So it got missed.

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

Please add documentation (doc/guides/sample_app_ug/ipsec_secgw.rst) changes
to explain the new parameter.

> Signed-off-by: Praveen Shetty <praveen.shetty@intel.com>
> ---
> v3 changes:
> Incorporated Anoob review comments on v2.
> 



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

Fdir_conf is a deprecated parameter. It is not good to introduce 
Something new in the application with a deprecated parameter.
Please use the recommended way to configure flows.

> +	.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 */
> +	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, 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");
> +		return ret;
> +	}
> +	sa->flow = rte_flow_create(sa->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 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;
> +			}
>  			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;
> +			}
It means it is supported in cpu crypto as well? Better to have a check for the supported
Action types, as in the future there may be some other action types.

> +			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);

Better to print like below.
printf("flow-direction port %d queue %d ", sa->portid, sa->fdir_qid)

> +
>  	printf("\n");
>  }
>
  
Anoob Joseph April 1, 2020, 1:26 p.m. UTC | #2
Hi Akhil, Praveen,

Can't rte_flow and RSS co-exist? In rte_flow there is an ACTION type RSS in addition to QUEUE. With this patch, if rte_flow is enabled on any SA, then RSS would be disabled for the entire port. Is that the right behavior? And if we have to address this later, what would be the course of action?

Also, is flow director the right name we should use? Internally it is rte_flow, right? 

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Wednesday, April 1, 2020 6:50 PM
> To: Praveen Shetty <praveen.shetty@intel.com>; dev@dpdk.org;
> declan.doherty@intel.com; Anoob Joseph <anoobj@marvell.com>
> Cc: bernard.iremonger@intel.com; konstantin.ananyev@intel.com
> Subject: [EXT] RE: [dpdk-dev] [PATCH v3] examples/ipsec-secgw: support flow
> director feature
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Praveen,
> 
> Sorry for being late to reply on this, Please delegate the patches properly from
> next time in patchworks.
> This patch was neither delegated to me, nor I was in to/cc. So it got missed.
> 
> >
> > 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>.
> >
> 
> Please add documentation (doc/guides/sample_app_ug/ipsec_secgw.rst)
> changes to explain the new parameter.
> 
> > Signed-off-by: Praveen Shetty <praveen.shetty@intel.com>
> > ---
> > v3 changes:
> > Incorporated Anoob review comments on v2.
> >
> 
> 
> 
> > 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 = {
> 
> Fdir_conf is a deprecated parameter. It is not good to introduce Something new
> in the application with a deprecated parameter.
> Please use the recommended way to configure flows.
> 
> > +	.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 */
> > +	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, 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");
> > +		return ret;
> > +	}
> > +	sa->flow = rte_flow_create(sa->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 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;
> > +			}
> >  			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;
> > +			}
> It means it is supported in cpu crypto as well? Better to have a check for the
> supported Action types, as in the future there may be some other action types.
> 
> > +			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);
> 
> Better to print like below.
> printf("flow-direction port %d queue %d ", sa->portid, sa->fdir_qid)
> 
> > +
> >  	printf("\n");
> >  }
> >
  
Akhil Goyal April 1, 2020, 1:53 p.m. UTC | #3
> -----Original Message-----
> From: Anoob Joseph <anoobj@marvell.com>
> Sent: Wednesday, April 1, 2020 6:57 PM
> To: Akhil Goyal <akhil.goyal@nxp.com>; Praveen Shetty
> <praveen.shetty@intel.com>; dev@dpdk.org; declan.doherty@intel.com
> Cc: bernard.iremonger@intel.com; konstantin.ananyev@intel.com
> Subject: RE: [dpdk-dev] [PATCH v3] examples/ipsec-secgw: support flow director
> feature
> 
> Hi Akhil, Praveen,
> 
> Can't rte_flow and RSS co-exist? In rte_flow there is an ACTION type RSS in
> addition to QUEUE. With this patch, if rte_flow is enabled on any SA, then RSS
> would be disabled for the entire port. Is that the right behavior? And if we have
> to address this later, what would be the course of action?
> 
Yes they can co-exist I believe. What this patch is doing is assigning a fixed queue to
A flow which user can control for an SA. RSS is based on hash and user doesnot have
Control on it.
Removing RSS on entire port is not desirable and it should not be done. Probably there
Should be a mechanism to disable RSS on that particular flow.

> Also, is flow director the right name we should use? Internally it is rte_flow, right?
Name can be anything, I don't feel issue in either flow director or rte_flow.

> 
> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: Akhil Goyal <akhil.goyal@nxp.com>
> > Sent: Wednesday, April 1, 2020 6:50 PM
> > To: Praveen Shetty <praveen.shetty@intel.com>; dev@dpdk.org;
> > declan.doherty@intel.com; Anoob Joseph <anoobj@marvell.com>
> > Cc: bernard.iremonger@intel.com; konstantin.ananyev@intel.com
> > Subject: [EXT] RE: [dpdk-dev] [PATCH v3] examples/ipsec-secgw: support flow
> > director feature
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > Hi Praveen,
> >
> > Sorry for being late to reply on this, Please delegate the patches properly from
> > next time in patchworks.
> > This patch was neither delegated to me, nor I was in to/cc. So it got missed.
> >
> > >
> > > 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>.
> > >
> >
> > Please add documentation (doc/guides/sample_app_ug/ipsec_secgw.rst)
> > changes to explain the new parameter.
> >
> > > Signed-off-by: Praveen Shetty <praveen.shetty@intel.com>
> > > ---
> > > v3 changes:
> > > Incorporated Anoob review comments on v2.
> > >
> >
> >
> >
> > > 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 = {
> >
> > Fdir_conf is a deprecated parameter. It is not good to introduce Something
> new
> > in the application with a deprecated parameter.
> > Please use the recommended way to configure flows.
> >
> > > +	.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 */
> > > +	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, 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");
> > > +		return ret;
> > > +	}
> > > +	sa->flow = rte_flow_create(sa->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 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;
> > > +			}
> > >  			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;
> > > +			}
> > It means it is supported in cpu crypto as well? Better to have a check for the
> > supported Action types, as in the future there may be some other action types.
> >
> > > +			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);
> >
> > Better to print like below.
> > printf("flow-direction port %d queue %d ", sa->portid, sa->fdir_qid)
> >
> > > +
> > >  	printf("\n");
> > >  }
> > >
  
Shetty, Praveen April 2, 2020, 11:15 a.m. UTC | #4
-----Original Message-----
From: Akhil Goyal <akhil.goyal@nxp.com> 
Sent: Wednesday, April 1, 2020 7:24 PM
To: Anoob Joseph <anoobj@marvell.com>; 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: [dpdk-dev] [PATCH v3] examples/ipsec-secgw: support flow director feature



> -----Original Message-----
> From: Anoob Joseph <anoobj@marvell.com>
> Sent: Wednesday, April 1, 2020 6:57 PM
> To: Akhil Goyal <akhil.goyal@nxp.com>; Praveen Shetty 
> <praveen.shetty@intel.com>; dev@dpdk.org; declan.doherty@intel.com
> Cc: bernard.iremonger@intel.com; konstantin.ananyev@intel.com
> Subject: RE: [dpdk-dev] [PATCH v3] examples/ipsec-secgw: support flow 
> director feature
> 
> Hi Akhil, Praveen,
> 
> Can't rte_flow and RSS co-exist? In rte_flow there is an ACTION type 
> RSS in addition to QUEUE. With this patch, if rte_flow is enabled on 
> any SA, then RSS would be disabled for the entire port. Is that the 
> right behavior? And if we have to address this later, what would be the course of action?
> 
Yes they can co-exist I believe. What this patch is doing is assigning a fixed queue to A flow which user can control for an SA. RSS is based on hash and user doesnot have Control on it.
Removing RSS on entire port is not desirable and it should not be done. Probably there Should be a mechanism to disable RSS on that particular flow.

[Praveen]  We will remove the code which disables RSS on entire port in V4. 
meanwhile we will also explore a way to disable the RSS on the queue which the SA is associated with. 

future the idea would be only to disable the queue which the SA is associated with
> Also, is flow director the right name we should use? Internally it is rte_flow, right?
Name can be anything, I don't feel issue in either flow director or rte_flow.

> 
> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: Akhil Goyal <akhil.goyal@nxp.com>
> > Sent: Wednesday, April 1, 2020 6:50 PM
> > To: Praveen Shetty <praveen.shetty@intel.com>; dev@dpdk.org; 
> > declan.doherty@intel.com; Anoob Joseph <anoobj@marvell.com>
> > Cc: bernard.iremonger@intel.com; konstantin.ananyev@intel.com
> > Subject: [EXT] RE: [dpdk-dev] [PATCH v3] examples/ipsec-secgw: 
> > support flow director feature
> >
> > External Email
> >
> > --------------------------------------------------------------------
> > --
> > Hi Praveen,
> >
> > Sorry for being late to reply on this, Please delegate the patches 
> > properly from next time in patchworks.
> > This patch was neither delegated to me, nor I was in to/cc. So it got missed.

[Praveen] sorry , I forgot to include you. Will do it from next time.
> >
> > >
> > > 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>.
> > >
> >
> > Please add documentation (doc/guides/sample_app_ug/ipsec_secgw.rst)
> > changes to explain the new parameter.

[Praveen] Will do it in v4.

> >
> > > Signed-off-by: Praveen Shetty <praveen.shetty@intel.com>
> > > ---
> > > v3 changes:
> > > Incorporated Anoob review comments on v2.
> > >
> >
> >
> >
> > > 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 = {
> >
> > Fdir_conf is a deprecated parameter. It is not good to introduce 
> > Something
> new
> > in the application with a deprecated parameter.
> > Please use the recommended way to configure flows.

[Praveen] We will check and do it in v4.

> >
> > > +	.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 */
> > > +	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, 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");
> > > +		return ret;
> > > +	}
> > > +	sa->flow = rte_flow_create(sa->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 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;
> > > +			}
> > >  			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;
> > > +			}
> > It means it is supported in cpu crypto as well? 
[Praveen] As of now we have validated only on "RTE_SECURITY_ACTION_TYPE_NONE" and CPU crypto is independent of the IO device similar to action type NONE.
And also it should be supported in other crypto devices as well but we have not included them here because we have not validated.

> >Better to have a check for the supported Action types, as in the future there may be some other action types.
[Praveen] We will fix this in V4.

> >
> > > +			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);
> >
> > Better to print like below.
> > printf("flow-direction port %d queue %d ", sa->portid, sa->fdir_qid)

[Praveen] Will do it in V4.

> >
> > > +
> > >  	printf("\n");
> > >  }
> > >
  
Anoob Joseph April 2, 2020, 1:39 p.m. UTC | #5
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. 

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?

Also, why is one case returning 0 (EGRESS) and another (TRANSPORT) is returning -1? One is treated as error and other is not?
 
> +			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.
 
> +		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.
 
> +	if (!sa->flow) {
> +		RTE_LOG(ERR, IPSEC,
> +			"Flow Creation failed\n");

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

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

> +			}
>  			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?

> +			}
>  		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
  
Shetty, Praveen April 2, 2020, 5:56 p.m. UTC | #6
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
  
Anoob Joseph April 3, 2020, 5:14 a.m. UTC | #7
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
  
Akhil Goyal April 3, 2020, 5:52 a.m. UTC | #8
> > > 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.
> 

I believe it does not matter for EGRESS in most hardwares,
INGRESS flows should have distribution. I think your comments are just reverse but
The code is inline with my understanding.
  
Shetty, Praveen April 3, 2020, 10:21 a.m. UTC | #9
-----Original Message-----
From: Akhil Goyal <akhil.goyal@nxp.com> 
Sent: Friday, April 3, 2020 11:22 AM
To: Anoob Joseph <anoobj@marvell.com>; 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

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

I believe it does not matter for EGRESS in most hardwares, INGRESS flows should have distribution. I think your comments are just reverse but The code is inline with my understanding.

[Praveen] 
Current implementation is only for ingress traffic load distribution therefore it is applicable only for inbound IPsec traffic.
  
Anoob Joseph April 3, 2020, 10:28 a.m. UTC | #10
Hi Akhil, Praveen,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Shetty, Praveen <praveen.shetty@intel.com>
> Sent: Friday, April 3, 2020 3:51 PM
> To: Akhil Goyal <akhil.goyal@nxp.com>; Anoob Joseph <anoobj@marvell.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
> 
> 
> 
> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Friday, April 3, 2020 11:22 AM
> To: Anoob Joseph <anoobj@marvell.com>; 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
> 
> > > > 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.
> >
> 
> I believe it does not matter for EGRESS in most hardwares, INGRESS flows
> should have distribution. I think your comments are just reverse but The code is
> inline with my understanding.
> 
> [Praveen]
> Current implementation is only for ingress traffic load distribution therefore it is
> applicable only for inbound IPsec traffic.
> 

[Anoob] Yes. I got it reverse. Meant egress instead of ingress and the other way round as well.

I was asking for the rationale behind limiting the scope. Anyway, that can be taken up separately.
  

Patch

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 */
+	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, 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");
+		return ret;
+	}
+	sa->flow = rte_flow_create(sa->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 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;
+			}
 			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");
+			}
 		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.
  */