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 6:32 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
> 
> On 1/16/2018 5:59 PM, Gujjar, Abhinandan S wrote:
> > Hi Akhil,
> >
> >> -----Original Message-----
> >> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> >> Sent: Tuesday, January 16, 2018 5:30 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 5:06 PM, Gujjar, Abhinandan S wrote:
> >>> Hi Akhil,
> >>>
> >>>> -----Original Message-----
> >>>> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> >>>> Sent: Tuesday, January 16, 2018 2:52 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
> >>>>
> >>>> 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.
> >>> The plan is to store private data after "sess * nb_drivers ".
> >>> As you said, if it is implementation specific, flag may be again
> >>> required at struct rte_cryptodev_sym_session
> >> I think my previous statement was not clear.
> >> My point is that whatever we set in the
> >> rte_cryptodev_sym_session_set_private_data() is a valid value when we
> >> call this API explicitly. And before calling the set API, the values
> >> are zero or any invalid value. So if application calls the get API
> >> before setting it with set API, it will get an invalid value(may be NULL or zero
> or whatever).
> > Thanks for clarifying. At this time, without calling set API and calling get API
> will get some value.
> > How do you validating whether the data is valid or not?
> > Since application has called set API and same is indicated by
> > private_data_type flag, I thought data got by get API can be safely assume to
> be valid.
> > Not sure, if you have better way to validate the data from get API.
> 
> Got your point, we cannot validate in the library that private data is valid or not
> as we do not know the values of the data.
> 
> However, got one more option to work with.
> You can have a priv_data_offset (similar to crypto_op) in the
> rte_cryptodev_sym_session. In that way it will look similar in both the cases and
> we do not have to make any assumption that the priv data is present after "sess
> * nb_drivers ".
> So in this way we can know if offset is zero, then data is not valid.
> And procedure will also be same in both the cases.
> If it is in crypto_op, then we set the priv_data_off in crypto_op, and if it is there
> in session, then we set the priv_data_off in session.

I guess, you are suggesting below changes:
diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
index 56958a6..057c39a 100644
--- a/lib/librte_cryptodev/rte_cryptodev.h
+++ b/lib/librte_cryptodev/rte_cryptodev.h
@@ -892,6 +892,8 @@ struct rte_cryptodev_data {
 
 /** Cryptodev symmetric crypto session */
 struct rte_cryptodev_sym_session {
+       uint16_t private_data_offset;
+       /**< Private data offset */
        __extension__ void *sess_private_data[0];
        /**< Private session material */
 };
I am ok with this.

Declan/Pablo,
Is this ok? Do you see any impact on performance or anything else has to be considered?

> 
> >
> >>
> >>> OR
> >>> If it is planned to store at PMD's sess_private_data, it requires
> >>> additional ops as well in rte_cryptodev_ops.
> >>> We wanted to have a simple design with minimal changes to cryptodev
> >>> and
> >> security,
> >>> that’s reason for existing design.
> >>> It will be good, if other folks chime in and share there opinion.
> >>> This will make the implementation part more clear.
> >>>>
> >>>> -Akhil
> >>> -Abhinandan
> >>>
> > Abhinandan
> >
Abhinandan