Re: [dpdk-dev] [PATCH 1/2] lib/cryptodev: add support to set session private data

["Gujjar, Abhinandan S" <abhinandan.gujjar@intel.com>]

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Tuesday, January 16, 2018 11:55 AM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Doherty, Declan
> <declan.doherty@intel.com>
> Cc: dev@dpdk.org; Vangati, Narender <narender.vangati@intel.com>; Rao,
> Nikhil <nikhil.rao@intel.com>
> Subject: Re: [PATCH 1/2] lib/cryptodev: add support to set session private data
> 
> Hi Abhinandan,
> On 1/16/2018 11:39 AM, Gujjar, Abhinandan S wrote:
> >>> diff --git a/lib/librte_cryptodev/rte_crypto.h
> >>> b/lib/librte_cryptodev/rte_crypto.h
> >>> index bbc510d..3a98cbf 100644
> >>> --- a/lib/librte_cryptodev/rte_crypto.h
> >>> +++ b/lib/librte_cryptodev/rte_crypto.h
> >>> @@ -62,6 +62,18 @@ enum rte_crypto_op_sess_type {
> >>>    	RTE_CRYPTO_OP_SECURITY_SESSION	/**< Security session crypto
> >> operation */
> >>>    };
> >>>
> >>> +/** Private data types for cryptographic operation
> >>> + * @see rte_crypto_op::private_data_type */ enum
> >>> +rte_crypto_op_private_data_type {
> >>> +	RTE_CRYPTO_OP_PRIVATE_DATA_NONE,
> >>> +	/**< No private data */
> >>> +	RTE_CRYPTO_OP_PRIVATE_DATA_OP,
> >>> +	/**< Private data is part of rte_crypto_op and indicated by
> >>> +	 * private_data_offset */
> >>> +	RTE_CRYPTO_OP_PRIVATE_DATA_SESSION
> >>> +	/**< Private data is available at session */ };
> >>> +
> >> We may get away with this enum. If private_data_offset is "0", then
> >> we can check with the session if it has priv data or not.
> > Right now,  Application uses 'rte_crypto_op_private_data_type' to
> > indicate rte_cryptodev_sym_session_set_private_data()
> > was called to set the private data. Otherwise, how do you indicate there is a
> private data associated with the session?
> > Any suggestions?
> For session based flows, the first choice to store the private data should be in
> the session. So RTE_CRYPTO_OP_WITH_SESSION or
> RTE_CRYPTO_OP_SECURITY_SESSION can be used to call
> rte_cryptodev_sym_session_set_private_data or
> rte_security_session_set_private_data.
Case 1: private_data_offset is "0" and sess_type = RTE_CRYPTO_OP_WITH_SESSION -> usual case
Case 2: private_data_offset is "0" and sess_type = RTE_CRYPTO_OP_WITH_SESSION + event case (access private data)
Differentiating between case 1 & 2 will be done by checking 
rte_crypto_op_private_data_type == RTE_CRYPTO_OP_PRIVATE_DATA_SESSION.
> >>>    /**
> >>>     * Cryptographic Operation.
> >>>     *
> >>> @@ -84,8 +96,11 @@ struct rte_crypto_op {
> >>>    	 */
> >>>    	uint8_t sess_type;
> >>>    	/**< operation session type */
> >>> -
> >>> -	uint8_t reserved[5];
> >>> +	uint8_t private_data_type;
> >>> +	/**< Private data type. @see enum rte_crypto_op_private_data_type
> >> */
> >>> +	uint16_t private_data_offset;
> >>> +	/**< Offset to indicate start of private data */
> >> It is better to add some more information in the comments just like
> description.
> >> While viewing the code, it is not explicit that who is the intended
> >> user of this private data.
> > The propose APIs are generic, that’s that reason eventdev was not
> > mentioned in the comments of this patch & mentioned only in the description.
> it may be written as, "Offset to indicate start of private data (if any). The private
> data may be used by the application to store information which should remain
> untouched in the library/driver"
Ok
> >>> +	uint8_t reserved[3];
> >>>    	/**< Reserved bytes to fill 64 bits for future additions */
> >>>    	struct rte_mempool *mempool;
> >>>    	/**< crypto operation mempool which operation is allocated from
> >>> */
> 
> <snip>
> >>> +
> >>> +/**
> >>> + * Get private data of a session.
> >>> + *
> >>> + * @param	sess		Session pointer allocated by
> >>> + *				*rte_cryptodev_sym_session_create*.
> >>> + *
> >>> + * @return
> >>> + *  - On success return pointer to private data.
> >>> + *  - On failure returns NULL.
> >>> + */
> >>> +void *rte_cryptodev_sym_session_get_private_data(
> >>> +				const struct rte_cryptodev_sym_session *sess);
> >>> +
> >> same here.
> > This doesn’t fit into a single line or in the next line aligned with bracket.
> void * should be in a separate line.
ok
> 
> 
> -Akhil
Abhinandan