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

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

-----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");
> > >  }
> > >