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

[Akhil Goyal]

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.
>>>    /**
>>>     * 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"
>>> +	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.


-Akhil