Re: [dpdk-dev] [PATCH 01/14] examples/ipsec-secgw: add default rte_flow for inline Rx

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

 
> From: Ankur Dwivedi <adwivedi@marvell.com>
> 
> The default flow created would enable security processing on all ESP
> packets. If the default flow is created, SA based rte_flow creation
> would be skipped.

I suppose that one depends on:
http://patches.dpdk.org/patch/63621/
http://patches.dpdk.org/cover/63625/
to work as expected?
If so probably worth to mention in that header
or in cover letter (or both). 

> 
> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> ---
>  examples/ipsec-secgw/ipsec-secgw.c | 56 ++++++++++++++++++++++++++++++++++++++
>  examples/ipsec-secgw/ipsec.c       |  8 ++++++
>  examples/ipsec-secgw/ipsec.h       |  6 ++++
>  3 files changed, 70 insertions(+)
> 
> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
> index 3b5aaf6..7506922 100644
> --- a/examples/ipsec-secgw/ipsec-secgw.c
> +++ b/examples/ipsec-secgw/ipsec-secgw.c
> @@ -128,6 +128,8 @@ struct ethaddr_info ethaddr_tbl[RTE_MAX_ETHPORTS] = {
>  	{ 0, ETHADDR(0x00, 0x16, 0x3e, 0x49, 0x9e, 0xdd) }
>  };
> 
> +struct flow_info flow_info_tbl[RTE_MAX_ETHPORTS];

Need to be initialized with zeroes somewhere.

> +
>  #define CMD_LINE_OPT_CONFIG		"config"
>  #define CMD_LINE_OPT_SINGLE_SA		"single-sa"
>  #define CMD_LINE_OPT_CRYPTODEV_MASK	"cryptodev_mask"
> @@ -2406,6 +2408,55 @@ reassemble_init(void)
>  	return rc;
>  }
> 
> +static int
> +create_default_ipsec_flow(uint16_t port_id, uint64_t rx_offloads)
> +{
> +	int ret = 0;
> +
> +	/* Add the default ipsec flow to detect all ESP packets for rx */
> +	if (rx_offloads & DEV_RX_OFFLOAD_SECURITY) {
> +		struct rte_flow_action action[2];
> +		struct rte_flow_item pattern[2];
> +		struct rte_flow_attr attr = {0};
> +		struct rte_flow_error err;
> +		struct rte_flow *flow;
> +
> +		pattern[0].type = RTE_FLOW_ITEM_TYPE_ESP;
> +		pattern[0].spec = NULL;
> +		pattern[0].mask = NULL;
> +		pattern[0].last = NULL;
> +		pattern[1].type = RTE_FLOW_ITEM_TYPE_END;
> +
> +		action[0].type = RTE_FLOW_ACTION_TYPE_SECURITY;
> +		action[0].conf = NULL;
> +		action[1].type = RTE_FLOW_ACTION_TYPE_END;
> +		action[1].conf = NULL;
> +
> +		attr.egress = 0;
> +		attr.ingress = 1;
> +
> +		ret = rte_flow_validate(port_id, &attr, pattern, action, &err);
> +		if (ret) {

As I understand, flow_validate() is used here to query does this capability
(multiple security sessions for same flow) is supported by PMD/HW?
If so, then probably no need for error message if it doesn't. 

> +			RTE_LOG(ERR, IPSEC,
> +				"Failed to validate ipsec flow %s\n",
> +				err.message);
> +			goto exit;
> +		}
> +
> +		flow = rte_flow_create(port_id, &attr, pattern, action, &err);

Same question as for http://patches.dpdk.org/patch/63621/,
why do you need it at all?
What it will enable/disable? 

> +		if (flow == NULL) {
> +			RTE_LOG(ERR, IPSEC,
> +				"Failed to create ipsec flow %s\n",
> +				err.message);
> +			ret = -rte_errno;
> +			goto exit;

Why not just 'return ret;' here?

> +		}
> +		flow_info_tbl[port_id].rx_def_flow = flow;
> +	}
> +exit:
> +	return ret;
> +}
> +
>  int32_t
>  main(int32_t argc, char **argv)
>  {
> @@ -2478,6 +2529,11 @@ main(int32_t argc, char **argv)
> 
>  		sa_check_offloads(portid, &req_rx_offloads, &req_tx_offloads);
>  		port_init(portid, req_rx_offloads, req_tx_offloads);
> +		/* Create default ipsec flow for the ethernet device */
> +		ret = create_default_ipsec_flow(portid, req_rx_offloads);
> +		if (ret)
> +			printf("Cannot create default flow, err=%d, port=%d\n",
> +					ret, portid);

Again it is an optional feature, so not sure if we need to report it for every port.
Might be better to do visa-versa: LOG(INFO, ...) when  create_default() was successfull.

>  	}
> 
>  	cryptodevs_init();
> diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c
> index d4b5712..e529f68 100644
> --- a/examples/ipsec-secgw/ipsec.c
> +++ b/examples/ipsec-secgw/ipsec.c
> @@ -261,6 +261,12 @@ create_inline_session(struct socket_ctx *skt_ctx, struct ipsec_sa *sa,
>  			unsigned int i;
>  			unsigned int j;
> 
> +			/*
> +			 * Don't create flow if default flow is already created
> +			 */
> +			if (flow_info_tbl[sa->portid].rx_def_flow)
> +				goto set_cdev_id;

As a nit: would be great to avoid introducing extra gotos.

> +

As I can see, that block of code is for RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO only.
Is that what intended?
BTW, for RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL, it seems rte_flow is 
never created anyway inside that function. 

>  			ret = rte_eth_dev_info_get(sa->portid, &dev_info);
>  			if (ret != 0) {
>  				RTE_LOG(ERR, IPSEC,
> @@ -396,6 +402,8 @@ create_inline_session(struct socket_ctx *skt_ctx, struct ipsec_sa *sa,
>  		ips->security.ol_flags = sec_cap->ol_flags;
>  		ips->security.ctx = sec_ctx;
>  	}
> +
> +set_cdev_id:
>  	sa->cdev_id_qp = 0;
> 
>  	return 0;
> diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
> index 8e07521..28ff07d 100644
> --- a/examples/ipsec-secgw/ipsec.h
> +++ b/examples/ipsec-secgw/ipsec.h
> @@ -81,6 +81,12 @@ struct app_sa_prm {
> 
>  extern struct app_sa_prm app_sa_prm;
> 
> +struct flow_info {
> +	struct rte_flow *rx_def_flow;
> +};
> +
> +extern struct flow_info flow_info_tbl[RTE_MAX_ETHPORTS];
> +
>  enum {
>  	IPSEC_SESSION_PRIMARY = 0,
>  	IPSEC_SESSION_FALLBACK = 1,
> --
> 2.7.4