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

[Gujjar, Abhinandan S]

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal at nxp.com]
> Sent: Monday, January 15, 2018 5:49 PM
> To: Gujjar, Abhinandan S <abhinandan.gujjar at intel.com>; Doherty, Declan
> <declan.doherty at intel.com>
> Cc: dev at dpdk.org; Vangati, Narender <narender.vangati at intel.com>; Rao,
> Nikhil <nikhil.rao at intel.com>
> Subject: Re: [PATCH 1/2] lib/cryptodev: add support to set session private data
> 
> Hi Abhinandan,
> On 1/15/2018 5:21 PM, Abhinandan Gujjar wrote:
> > Update rte_crypto_op to indicate private data type and offset.
> >
> > The application may want to store private data along with the
> > rte_cryptodev that is transparent to the rte_cryptodev layer.
> > For e.g., If an eventdev based application is submitting a
> > rte_cryptodev_sym_session operation and wants to indicate event
> > information required to construct a new event that will be enqueued to
> > eventdev after completion of the rte_cryptodev_sym_session operation.
> > This patch provides a mechanism for the application to associate this
> > information with the rte_cryptodev_sym_session session.
> > The application can set the private data using
> > rte_cryptodev_sym_session_set_private_data() and retrieve it using
> > rte_cryptodev_sym_session_get_private_data().
> >
> > Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar at intel.com>
> > Signed-off-by: Nikhil Rao <nikhil.rao at intel.com>
> Subject line should be cryptodev:....
> similar comment for 2/2
Ok.
> > ---
> >   lib/librte_cryptodev/rte_crypto.h    | 19 +++++++++++++++++--
> >   lib/librte_cryptodev/rte_cryptodev.h | 30
> ++++++++++++++++++++++++++++++
> >   2 files changed, 47 insertions(+), 2 deletions(-)
> >
> > 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?
> >   /**
> >    * 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.
> > +	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 */
> > diff --git a/lib/librte_cryptodev/rte_cryptodev.h
> > b/lib/librte_cryptodev/rte_cryptodev.h
> > index dade554..56958a6 100644
> > --- a/lib/librte_cryptodev/rte_cryptodev.h
> > +++ b/lib/librte_cryptodev/rte_cryptodev.h
> > @@ -1033,6 +1033,36 @@ struct rte_cryptodev_sym_session *
> >    */
> >   const char *rte_cryptodev_driver_name_get(uint8_t driver_id);
> >
> > +/**
> > + * Set private data for a session.
> > + *
> > + * @param	sess		Session pointer allocated by
> > + *				*rte_cryptodev_sym_session_create*.
> > + * @param	data		Pointer to the private data.
> > + * @param	size		Size of the private data.
> > + *
> > + * @return
> > + *  - On success, zero.
> > + *  - On failure, a negative value.
> > + */
> > +int rte_cryptodev_sym_session_set_private_data(
> > +				struct rte_cryptodev_sym_session *sess,
> > +				void *data,
> > +				uint16_t size);
> Looks like this is an RFC patch. There is no implementation of this API and the
> formatting is also incorrect. Please correct formatting while sending the
> complete patch.
> Same comment for 2/2 patch
Ok
> > +
> > +/**
> > + * 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.
> >   #ifdef __cplusplus
> >   }
> >   #endif
> >
> 
> -Akhil
Abhinandan