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

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

> > >>>
> > >>> 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.
> > >
> > > [Anoob] Will add it in v2.
> >
> > [Lukasz] Is there any reason to initialize flow_info_tbl explicitly with zeros ? As a global array it will be automatically
> > zeroized by the compiler.
> 
> I think, it wouldn't.
> Only static ones will be silently initialized by compiler.
> Otherwise it could be anything.

Actually as pointed by Ferruh:
Compiler wouldn't zero it out,  but it will make it a'common' symbol
and let linker to decide.
As there is no other symbols for that var, linker should put it into .bss.
So it seems I was too conservative, and it is safe not to have explicit initialization here.
Konstantin

> 
> >
> > >>
> > >>> +
> > >>>  #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.
> > >
> > > [Anoob] Yes. Will remove the error log.
> > >
> > >>
> > >>> +			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?
> > >
> > > [Anoob] Your followup question there accurately describes the usage. If the application wants to enable H/w IPsec processing only on a
> > specific SPI range, it will be allowed so with this kind of flow.
> > >
> > > Let's say, application wants to allow H/w processing only for SPI 1-8192. In that case, either 8192 rte_flows need to be created, or one
> > rte_flow rule with SPI 1-8192 range can be created. Any SPI outside the range won't match the rule and rte_flow could have further rules
> to
> > act on such packets.
> > >
> > >>
> > >>> +		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?
> > >
> > > [Anoob] Will fix in v2.
> > >
> > >>
> > >>> +		}
> > >>> +		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.
> > >
> > > [Anoob] Will update in v2.
> > >
> > >>
> > >>>  	}
> > >>>
> > >>>  	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.
> > >
> > > [Anoob] So, set the cdev_id and return here itself?
> > >
> > > Will make that change in v2.
> > >
> > >>
> > >>> +
> > >>
> > >> As I can see, that block of code is for
> > >> RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO only.
> > >> Is that what intended?
> > >
> > > [Anoob] Yes
> > >
> > >> BTW, for RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL, it seems rte_flow
> > >> is never created anyway inside that function.
> > >
> > > [Anoob] Yes. Current ipsec-secgw doesn't have rte_flow creation for inline protocol. It is done only for inline crypto. The default flow
> that
> > we are adding is applicable for both inline crypto & inline protocol. Hence adding the extra check in inline crypto path to avoid creating
> > duplicate rte_flows.
> > >
> > >>
> > >>>  			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
> > >