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

[Lukas Bartosik <lbartosik@marvell.com>]

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
>