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

Message ID 1575808249-31135-2-git-send-email-anoobj@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: akhil goyal
Headers
Series add eventmode to ipsec-secgw |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/Intel-compilation fail Compilation issues
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Anoob Joseph Dec. 8, 2019, 12:30 p.m. UTC
  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.

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

Comments

Ananyev, Konstantin Dec. 16, 2019, 2:20 p.m. UTC | #1
> 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
  
Anoob Joseph Dec. 16, 2019, 3:58 p.m. UTC | #2
Hi Konstantin,

Thanks for the review. Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Monday, December 16, 2019 7:51 PM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal <akhil.goyal@nxp.com>;
> Nicolau, Radu <radu.nicolau@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: Ankur Dwivedi <adwivedi@marvell.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Narayana Prasad Raju Athreya
> <pathreya@marvell.com>; Archana Muniganti <marchana@marvell.com>;
> Tejasree Kondoj <ktejasree@marvell.com>; Vamsi Krishna Attunuru
> <vattunuru@marvell.com>; Lukas Bartosik <lbartosik@marvell.com>;
> dev@dpdk.org
> Subject: [EXT] RE: [PATCH 01/14] examples/ipsec-secgw: add default rte_flow
> for inline Rx
> 
> External Email
> 
> ----------------------------------------------------------------------
> 
> > 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).

[Anoob] Yes. Usually the dependency is not added in the commit header. I'll update the v2 cover letter with such details.
 
> 
> >
> > 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.
 
> 
> > +
> >  #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
  
Lukas Bartosik [C] Jan. 9, 2020, 12:01 p.m. UTC | #3
Hi Konstantin,

Please see my question inline.

Thanks,
Lukasz

On 16.12.2019 16:58, Anoob Joseph wrote:
> Hi Konstantin,
> 
> Thanks for the review. Please see inline.
> 
> Thanks,
> Anoob
> 
>> -----Original Message-----
>> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
>> Sent: Monday, December 16, 2019 7:51 PM
>> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal <akhil.goyal@nxp.com>;
>> Nicolau, Radu <radu.nicolau@intel.com>; Thomas Monjalon
>> <thomas@monjalon.net>
>> Cc: Ankur Dwivedi <adwivedi@marvell.com>; Jerin Jacob Kollanukkaran
>> <jerinj@marvell.com>; Narayana Prasad Raju Athreya
>> <pathreya@marvell.com>; Archana Muniganti <marchana@marvell.com>;
>> Tejasree Kondoj <ktejasree@marvell.com>; Vamsi Krishna Attunuru
>> <vattunuru@marvell.com>; Lukas Bartosik <lbartosik@marvell.com>;
>> dev@dpdk.org
>> Subject: [EXT] RE: [PATCH 01/14] examples/ipsec-secgw: add default rte_flow
>> for inline Rx
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>>
>>> 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).
> 
> [Anoob] Yes. Usually the dependency is not added in the commit header. I'll update the v2 cover letter with such details.
>  
>>
>>>
>>> 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.

>>
>>> +
>>>  #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
>
  
Ananyev, Konstantin Jan. 9, 2020, 7:09 p.m. UTC | #4
> >>>
> >>> 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.  

> 
> >>
> >>> +
> >>>  #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
> >
  
Ananyev, Konstantin Jan. 13, 2020, 11:40 a.m. UTC | #5
> > >>>
> > >>> 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
> > >
  

Patch

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];
+
 #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) {
+			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);
+		if (flow == NULL) {
+			RTE_LOG(ERR, IPSEC,
+				"Failed to create ipsec flow %s\n",
+				err.message);
+			ret = -rte_errno;
+			goto exit;
+		}
+		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);
 	}
 
 	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;
+
 			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,