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

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

[Abhinandan Gujjar]

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@intel.com>
Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
---
 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 */
+};
+
 /**
  * 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 */
+	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);
+
+/**
+ * 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);
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.9.1

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

[Akhil Goyal]

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@intel.com>
> Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
Subject line should be cryptodev:....
similar comment for 2/2
> ---
>   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.
>   /**
>    * 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.
> +	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
> +
> +/**
> + * 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.
>   #ifdef __cplusplus
>   }
>   #endif
> 

-Akhil

Re: [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@nxp.com]
> Sent: Monday, January 15, 2018 5:49 PM
> 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/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@intel.com>
> > Signed-off-by: Nikhil Rao <nikhil.rao@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

Re: [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

Re: [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@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

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

[Akhil Goyal]

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.


-Akhil

Re: [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@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.
> 
> 
> -Akhil
-Abhinandan

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

[Akhil Goyal]

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

Re: [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@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
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

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

[Akhil Goyal]

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).

> 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
> 

Re: [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@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.

> 
> > 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

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

[Akhil Goyal]

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.

> 
>>
>>> 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
> 

Re: [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@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

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

[De Lara Guarch, Pablo]

Hi Abhinandan,

> -----Original Message-----
> From: Gujjar, Abhinandan S
> Sent: Wednesday, January 17, 2018 6:35 AM
> To: Akhil Goyal <akhil.goyal@nxp.com>; Doherty, Declan
> <declan.doherty@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@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 Akhil,
> 

...

> 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?

This is breaking ABI, and since there is a zero length array, this latter has to be at the end of the structure.
Therefore, this is not a valid option unless ABI deprecation is announced and then it could be merged in the next release.

Pablo

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

[Gujjar, Abhinandan S]

Hi Akhil,

> -----Original Message-----
> From: De Lara Guarch, Pablo
> Sent: Wednesday, January 17, 2018 3:16 PM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Akhil Goyal
> <akhil.goyal@nxp.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,
> 
> > -----Original Message-----
> > From: Gujjar, Abhinandan S
> > Sent: Wednesday, January 17, 2018 6:35 AM
> > To: Akhil Goyal <akhil.goyal@nxp.com>; Doherty, Declan
> > <declan.doherty@intel.com>; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@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 Akhil,
> >
> 
> ...
> 
> > 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?
> 
> This is breaking ABI, and since there is a zero length array, this latter has to be at
> the end of the structure.
> Therefore, this is not a valid option unless ABI deprecation is announced and
> then it could be merged in the next release.
What is your opinion on this?
Should we consider retaining the enum rte_crypto_op_private_data_type?
> 
> Pablo
Abhinandan

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

[Akhil Goyal]

Hi Abhinandan,
On 1/17/2018 3:35 PM, Gujjar, Abhinandan S wrote:
> Hi Akhil,
> 
>> -----Original Message-----
>> From: De Lara Guarch, Pablo
>> Sent: Wednesday, January 17, 2018 3:16 PM
>> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Akhil Goyal
>> <akhil.goyal@nxp.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,
>>
>>> -----Original Message-----
>>> From: Gujjar, Abhinandan S
>>> Sent: Wednesday, January 17, 2018 6:35 AM
>>> To: Akhil Goyal <akhil.goyal@nxp.com>; Doherty, Declan
>>> <declan.doherty@intel.com>; De Lara Guarch, Pablo
>>> <pablo.de.lara.guarch@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 Akhil,
>>>
>>
>> ...
>>
>>> 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?
>>
>> This is breaking ABI, and since there is a zero length array, this latter has to be at
>> the end of the structure.
>> Therefore, this is not a valid option unless ABI deprecation is announced and
>> then it could be merged in the next release.
> What is your opinion on this?
> Should we consider retaining the enum rte_crypto_op_private_data_type?

As per our previous discussion we are anyway pushing crypto adapter to 
next release, then we do have time for the deprecation notice to be sent.
Or you can reserve the first byte of private data (internal to library) 
in the session to check whether the private data is valid or not.

IMO, private data offset in session is a better approach instead of 
adding one more enum. Others can suggest.

-Akhil
>>
>> Pablo
> Abhinandan
> 

Re: [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@nxp.com]
> Sent: Wednesday, January 17, 2018 4:23 PM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@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/17/2018 3:35 PM, Gujjar, Abhinandan S wrote:
> > Hi Akhil,
> >
> >> -----Original Message-----
> >> From: De Lara Guarch, Pablo
> >> Sent: Wednesday, January 17, 2018 3:16 PM
> >> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Akhil Goyal
> >> <akhil.goyal@nxp.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,
> >>
> >>> -----Original Message-----
> >>> From: Gujjar, Abhinandan S
> >>> Sent: Wednesday, January 17, 2018 6:35 AM
> >>> To: Akhil Goyal <akhil.goyal@nxp.com>; Doherty, Declan
> >>> <declan.doherty@intel.com>; De Lara Guarch, Pablo
> >>> <pablo.de.lara.guarch@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 Akhil,
> >>>
> >>
> >> ...
> >>
> >>> 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?
> >>
> >> This is breaking ABI, and since there is a zero length array, this
> >> latter has to be at the end of the structure.
> >> Therefore, this is not a valid option unless ABI deprecation is
> >> announced and then it could be merged in the next release.
> > What is your opinion on this?
> > Should we consider retaining the enum rte_crypto_op_private_data_type?
> 
> As per our previous discussion we are anyway pushing crypto adapter to next
> release, then we do have time for the deprecation notice to be sent.
Not sure, it is really worth breaking ABI or have an enum.
> Or you can reserve the first byte of private data (internal to library) in the session
> to check whether the private data is valid or not.
Regarding reserving the first byte which validates the rest of the metadata data,
unless this byte is also included part of rte_cryptodev_sym_session_create()
i.e. 
memset(sess, 0, (sizeof(void *) * nb_drivers + private_data_flag));
and in
rte_cryptodev_get_header_session_size(void)
{
	/*
	 * Header contains pointers to the private data
	 * of all registered drivers
	 */
	return (sizeof(void *) * nb_drivers + private_data_flag);
}
Without above changes, the flag content can't be just trusted. Do you agree?

Pablo/Declan,
Hope the changes are ok? ABI breakage or anything has to be considered again?
> 
> IMO, private data offset in session is a better approach instead of adding one
> more enum. Others can suggest.
@Others, please provide your inputs so that I can prepare the next patch.

-Abhinandan
> 
> -Akhil
> >>
> >> Pablo
> > Abhinandan
> >

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

[Gujjar, Abhinandan S]

Hi All,

> -----Original Message-----
> From: Gujjar, Abhinandan S
> Sent: Thursday, January 18, 2018 12:22 PM
> To: Akhil Goyal <akhil.goyal@nxp.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@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 Akhil,
> 
> > -----Original Message-----
> > From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> > Sent: Wednesday, January 17, 2018 4:23 PM
> > To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; De Lara
> > Guarch, Pablo <pablo.de.lara.guarch@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/17/2018 3:35 PM, Gujjar, Abhinandan S wrote:
> > > Hi Akhil,
> > >
> > >> -----Original Message-----
> > >> From: De Lara Guarch, Pablo
> > >> Sent: Wednesday, January 17, 2018 3:16 PM
> > >> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Akhil Goyal
> > >> <akhil.goyal@nxp.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,
> > >>
> > >>> -----Original Message-----
> > >>> From: Gujjar, Abhinandan S
> > >>> Sent: Wednesday, January 17, 2018 6:35 AM
> > >>> To: Akhil Goyal <akhil.goyal@nxp.com>; Doherty, Declan
> > >>> <declan.doherty@intel.com>; De Lara Guarch, Pablo
> > >>> <pablo.de.lara.guarch@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 Akhil,
> > >>>
> > >>
> > >> ...
> > >>
> > >>> 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?
> > >>
> > >> This is breaking ABI, and since there is a zero length array, this
> > >> latter has to be at the end of the structure.
> > >> Therefore, this is not a valid option unless ABI deprecation is
> > >> announced and then it could be merged in the next release.
> > > What is your opinion on this?
> > > Should we consider retaining the enum rte_crypto_op_private_data_type?
> >
> > As per our previous discussion we are anyway pushing crypto adapter to
> > next release, then we do have time for the deprecation notice to be sent.
> Not sure, it is really worth breaking ABI or have an enum.
> > Or you can reserve the first byte of private data (internal to
> > library) in the session to check whether the private data is valid or not.
> Regarding reserving the first byte which validates the rest of the metadata data,
> unless this byte is also included part of rte_cryptodev_sym_session_create()
> i.e.
> memset(sess, 0, (sizeof(void *) * nb_drivers + private_data_flag)); and in
> rte_cryptodev_get_header_session_size(void)
> {
> 	/*
> 	 * Header contains pointers to the private data
> 	 * of all registered drivers
> 	 */
> 	return (sizeof(void *) * nb_drivers + private_data_flag); } Without above
> changes, the flag content can't be just trusted. Do you agree?
I will send the next patch based on above approach.
> 
> Pablo/Declan,
> Hope the changes are ok? ABI breakage or anything has to be considered again?
> >
> > IMO, private data offset in session is a better approach instead of
> > adding one more enum. Others can suggest.
> @Others, please provide your inputs so that I can prepare the next patch.
> 
> -Abhinandan
> >
> > -Akhil
> > >>
> > >> Pablo
> > > Abhinandan
> > >