Re: [dpdk-dev] [PATCH v4 05/10] ipsec: add SA data-path API

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

Hi Akhil,

> 
> Hi Konstantin,
> 
> Sorry for a late review. I was on unplanned leave for more than 2 weeks.

No worries, thanks for review.
Comments/answers inline.
Konstantin

> > +/**
> > + * Checks that inside given rte_ipsec_session crypto/security fields
> > + * are filled correctly and setups function pointers based on these values.
> it means user need not fill rte_ipsec_sa_pkt_fun, 

Yes.

> specify this in the comments.

Ok.

> > + * @param ss
> > + *   Pointer to the *rte_ipsec_session* object
> > + * @return
> > + *   - Zero if operation completed successfully.
> > + *   - -EINVAL if the parameters are invalid.
> > + */
> > +int __rte_experimental
> > +rte_ipsec_session_prepare(struct rte_ipsec_session *ss);
> > +
> > +/**
> > + * For input mbufs and given IPsec session prepare crypto ops that can be
> > + * enqueued into the cryptodev associated with given session.
> > + * expects that for each input packet:
> > + *      - l2_len, l3_len are setup correctly
> > + * Note that erroneous mbufs are not freed by the function,
> > + * but are placed beyond last valid mbuf in the *mb* array.
> > + * It is a user responsibility to handle them further.
> How will the user know how many mbufs are correctly processed.

Function return value contains number of successfully processed packets,
see comments below.
As an example, let say at input mb[]={A, B, C, D}, num=4, and prepare()
was able to successfully process A, B, D mbufs but failed to process C. 
Then return value will be 3, and mb[]={A, B, D, C}. 

> > + * @param ss
> > + *   Pointer to the *rte_ipsec_session* object the packets belong to.
> > + * @param mb
> > + *   The address of an array of *num* pointers to *rte_mbuf* structures
> > + *   which contain the input packets.
> > + * @param cop
> > + *   The address of an array of *num* pointers to the output *rte_crypto_op*
> > + *   structures.
> > + * @param num
> > + *   The maximum number of packets to process.
> > + * @return
> > + *   Number of successfully processed packets, with error code set in rte_errno.
> > + */
> > +static inline uint16_t __rte_experimental
> > +rte_ipsec_pkt_crypto_prepare(const struct rte_ipsec_session *ss,
> > +	struct rte_mbuf *mb[], struct rte_crypto_op *cop[], uint16_t num)
> > +{
> > +	return ss->pkt_func.prepare(ss, mb, cop, num);
> > +}
> > +
> > +/**
> > + * Finalise processing of packets after crypto-dev finished with them or
> > + * process packets that are subjects to inline IPsec offload.
> > + * Expects that for each input packet:
> > + *      - l2_len, l3_len are setup correctly
> > + * Output mbufs will be:
> > + * inbound - decrypted & authenticated, ESP(AH) related headers removed,
> > + * *l2_len* and *l3_len* fields are updated.
> > + * outbound - appropriate mbuf fields (ol_flags, tx_offloads, etc.)
> > + * properly setup, if necessary - IP headers updated, ESP(AH) fields added,
> > + * Note that erroneous mbufs are not freed by the function,
> > + * but are placed beyond last valid mbuf in the *mb* array.
> same question

Same answer as above.

> > + * It is a user responsibility to handle them further.
> > + * @param ss
> > + *   Pointer to the *rte_ipsec_session* object the packets belong to.
> > + * @param mb
> > + *   The address of an array of *num* pointers to *rte_mbuf* structures
> > + *   which contain the input packets.
> > + * @param num
> > + *   The maximum number of packets to process.
> > + * @return
> > + *   Number of successfully processed packets, with error code set in rte_errno.
> > + */
> > +static inline uint16_t __rte_experimental
> > +rte_ipsec_pkt_process(const struct rte_ipsec_session *ss, struct rte_mbuf *mb[],
> > +	uint16_t num)
> > +{
> > +	return ss->pkt_func.process(ss, mb, num);
> > +}
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* _RTE_IPSEC_H_ */


> > +
> > +int
> > +ipsec_sa_pkt_func_select(const struct rte_ipsec_session *ss,
> > +	const struct rte_ipsec_sa *sa, struct rte_ipsec_sa_pkt_func *pf)
> > +{
> > +	int32_t rc;
> > +
> > +	RTE_SET_USED(sa);
> > +
> > +	rc = 0;
> > +	pf[0] = (struct rte_ipsec_sa_pkt_func) { 0 };
> > +
> > +	switch (ss->type) {
> > +	default:
> > +		rc = -ENOTSUP;
> > +	}
> > +
> > +	return rc;
> > +}
> Is this a dummy function? Will it be updated later?

Yes it is a dummy function in that patch, will be filled in patch #6.

 I believe should
> have appropriate comments in that case.