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

[Akhil Goyal <akhil.goyal@nxp.com>]

On 1/16/2018 2:33 PM, Gujjar, Abhinandan S wrote:
> Hi Akhil,
> 
>> -----Original Message-----
>> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
>> Sent: Tuesday, January 16, 2018 12:56 PM
>> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Doherty, Declan
>> <declan.doherty@intel.com>; Jacob, Jerin
>> <Jerin.JacobKollanukkaran@cavium.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 12:35 PM, Gujjar, Abhinandan S wrote:
>>> 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.
>>
>> Consider this:
>> if (sess_type == RTE_CRYPTO_OP_WITH_SESSION &&
>> 		rte_cryptodev_sym_session_get_private_data == NULL)
>> 	usual case.
>> else if (sess_type = RTE_CRYPTO_OP_WITH_SESSION &&
>> 		rte_cryptodev_sym_session_get_private_data != NULL)
>> 	event case.
>> else if (sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
>> 		private_data_offset != 0)
>> 	event case for sessionless op.
>>
>> I hope all cases can be handled in this way.
> Memory allocated for private data will be continuation of session memory.
> I think, rte_cryptodev_sym_session_get_private_data() will return a valid pointer.
> It could be pointer to private data, in case application has allocated mempool with session + private data.
> It could be again a pointer to a location(may be next session),  in case application has allocated mempool with session only.
> Unless, there is a flag in the session data which will be set by rte_cryptodev_sym_session_set_private_data()
> If this flag is not set, rte_cryptodev_sym_session_get_private_data() will return NULL.
> I am not claiming, I have complete knowledge of different usage case of mempool setup for crypto.
> I am wondering, whether I am missing anything here. Please let me know.

It depends on the implementation of the get/set API. We can set NULL, if 
it is not filled in the set API. If it is set then we have a valid pointer.

-Akhil