RE: [EXT] [PATCH] crypto: use single buffer for asymmetric session

["Power, Ciara" <ciara.power@intel.com>]

Hi Anoob,

Thanks for the review, apologies for the delay in reply.
Comments inline.

Thanks,
Ciara

>-----Original Message-----
>From: Anoob Joseph <anoobj@marvell.com>
>Sent: Monday 13 December 2021 16:34
>To: Power, Ciara <ciara.power@intel.com>; dev@dpdk.org
>Cc: Zhang, Roy Fan <roy.fan.zhang@intel.com>; Akhil Goyal
><gakhil@marvell.com>; Doherty, Declan <declan.doherty@intel.com>; Ankur
>Dwivedi <adwivedi@marvell.com>; Tejasree Kondoj
><ktejasree@marvell.com>; Griffin, John <john.griffin@intel.com>; Trahe,
>Fiona <fiona.trahe@intel.com>; Jain, Deepak K <deepak.k.jain@intel.com>;
>Ray Kinsella <mdr@ashroe.eu>
>Subject: RE: [EXT] [PATCH] crypto: use single buffer for asymmetric session
>
>Hi Ciara,
>
>+1 to the overall approach. Few comments inline.
>
>Thanks,
>Anoob
>
>> -----Original Message-----
>> From: Ciara Power <ciara.power@intel.com>
>> Sent: Monday, December 13, 2021 8:34 PM
>> To: dev@dpdk.org
>> Cc: roy.fan.zhang@intel.com; Akhil Goyal <gakhil@marvell.com>; Ciara
>> Power <ciara.power@intel.com>; Declan Doherty
>> <declan.doherty@intel.com>; Ankur Dwivedi <adwivedi@marvell.com>;
>> Anoob Joseph <anoobj@marvell.com>; Tejasree Kondoj
>> <ktejasree@marvell.com>; John Griffin <john.griffin@intel.com>; Fiona
>> Trahe <fiona.trahe@intel.com>; Deepak Kumar Jain
>> <deepak.k.jain@intel.com>; Ray Kinsella <mdr@ashroe.eu>
>> Subject: [EXT] [PATCH] crypto: use single buffer for asymmetric
>> session
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> Rather than using a session buffer that contains pointers to private
>> session data elsewhere, have a single session buffer.
>> This session is created for a driver ID, and the mempool element
>> contains space for the max session private data needed for any driver.
>>
>> Signed-off-by: Ciara Power <ciara.power@intel.com>
>>
>> ---
>> Hiding the asym session structure by moving it to an internal header
>> will be implemented in a later version of this patch.
>> ---
>>  app/test-crypto-perf/cperf_ops.c              |  14 +-
>>  app/test/test_cryptodev_asym.c                | 204 ++++--------------
>>  drivers/crypto/cnxk/cn10k_cryptodev_ops.c     |   6 +-
>>  drivers/crypto/cnxk/cn9k_cryptodev_ops.c      |   6 +-
>>  drivers/crypto/cnxk/cnxk_cryptodev_ops.c      |  11 +-
>>  drivers/crypto/octeontx/otx_cryptodev_ops.c   |  29 +--
>>  drivers/crypto/octeontx2/otx2_cryptodev_ops.c |  25 +--
>>  drivers/crypto/openssl/rte_openssl_pmd.c      |   5 +-
>>  drivers/crypto/openssl/rte_openssl_pmd_ops.c  |  23 +-
>>  drivers/crypto/qat/qat_asym.c                 |  35 +--
>>  lib/cryptodev/cryptodev_pmd.h                 |  11 +-
>>  lib/cryptodev/cryptodev_trace_points.c        |   3 +
>>  lib/cryptodev/rte_cryptodev.c                 | 199 +++++++++++------
>>  lib/cryptodev/rte_cryptodev.h                 | 107 ++++++---
>>  lib/cryptodev/rte_cryptodev_trace.h           |  12 ++
>>  lib/cryptodev/version.map                     |   6 +-
>>  16 files changed, 302 insertions(+), 394 deletions(-)
>>
>
>[snip]
>
>> diff --git a/lib/cryptodev/rte_cryptodev.h
>> b/lib/cryptodev/rte_cryptodev.h index 59ea5a54df..11a62bb555 100644
>> --- a/lib/cryptodev/rte_cryptodev.h
>> +++ b/lib/cryptodev/rte_cryptodev.h
>> @@ -919,9 +919,15 @@ struct rte_cryptodev_sym_session {  };
>>
>>  /** Cryptodev asymmetric crypto session */ -struct
>> rte_cryptodev_asym_session {
>> -	__extension__ void *sess_private_data[0];
>> -	/**< Private asymmetric session material */
>> +__extension__ struct rte_cryptodev_asym_session {
>> +	uint8_t driver_id;
>> +	/**< Session driver ID. */
>> +	uint8_t max_priv_session_sz;
>> +	/**< size of private session data used when creating mempool */
>> +	uint16_t user_data_sz;
>> +	/**< session user data will be placed after sess_data */
>> +	uint8_t padding[4];
>> +	uint8_t sess_private_data[0];
>>  };
>
>[Anoob] Should we add a uint64_t member to hold IOVA address of, may be,
>rte_cryptodev_asym_session()? IOVA address could be required for
>hardware PMDs. And typically rte_mempool_virt2iova() used to help in that.
>Also, did you consider whether this layout of crypto session can be kept
>uniform across sym, asym & security? There is no asym specific field in this
>struct, right?
>
[CP] 
Do you think mempool would not be used for session in some cases? I guess the IOVA address would be needed in that case.

Yes, I believe this approach for session could be applicable to the sym and security sessions also, but for now we are only doing the asym session change, but I think they should all be aligned in future releases.

<snip>
>> -/**
>> - * Initialize asymmetric session on a device with specific asymmetric
>> xform
>> - *
>> - * @param   dev_id   ID of device that we want the session to be used on
>> - * @param   sess     Session to be set up on a device
>> - * @param   xforms   Asymmetric crypto transform operations to apply on
>> flow
>> - *                   processed with this session
>> - * @param   mempool  Mempool to be used for internal allocation.
>> - *
>> - * @return
>> - *  - On success, zero.
>> - *  - -EINVAL if input parameters are invalid.
>> - *  - -ENOTSUP if crypto device does not support the crypto transform.
>
>[Anoob] API rte_cryptodev_asym_session_create() returning NULL is treated
>as an error. But error can be either due to -EINVAL/-ENOMEM/-ENOTSUP, in
>which -ENOTSUP is typically used by PMD to declare unsupported
>combinations of xforms. Should we clarify this in the API description?
>
>Also, none of rte_cryptodev_asym_session_create() calls in validation tests
>consider the API returning NULL due to -ENOTSUP. For sym crypto test cases,
>API returning -ENOTSUP was used to skip the test. Can you update the tests
>such that returning NULL would mean test is skipped? Agreed that current
>code also doesn't handle -ENOTSUP case returned by init API.
>
[CP] 
Skipping for NULL return would mean it would also count as skipped for error cases other than -ENOTSUP, which isn't ideal.
In v2 I will change rte_cryptodev_asym_session_create() to return an int value (and just pass in session to be used as input),
this will allow the error returns to be treated correctly.

>> - *  - -ENOMEM if the private session could not be allocated.
>> - */
>> -__rte_experimental
>> -int
>> -rte_cryptodev_asym_session_init(uint8_t dev_id,
>> -			struct rte_cryptodev_asym_session *sess,
>> -			struct rte_crypto_asym_xform *xforms,
>> -			struct rte_mempool *mempool);
>> -
>>  /**
>>   * Frees private data for the device id, based on its device type,
>>   * returning it to its mempool. It is the application's
>> responsibility @@ -
>> 1075,14 +1088,13 @@ rte_cryptodev_sym_session_clear(uint8_t dev_id,
>>  			struct rte_cryptodev_sym_session *sess);
>>
>
>[snip]
>
>> diff --git a/lib/cryptodev/version.map b/lib/cryptodev/version.map
>> index
>> c50745fa8c..00b1c9ae35 100644
>> --- a/lib/cryptodev/version.map
>> +++ b/lib/cryptodev/version.map
>> @@ -58,7 +58,6 @@ EXPERIMENTAL {
>>  	rte_cryptodev_asym_session_clear;
>>  	rte_cryptodev_asym_session_create;
>>  	rte_cryptodev_asym_session_free;
>> -	rte_cryptodev_asym_session_init;
>>  	rte_cryptodev_asym_xform_capability_check_modlen;
>>  	rte_cryptodev_asym_xform_capability_check_optype;
>>  	rte_cryptodev_sym_cpu_crypto_process;
>> @@ -104,6 +103,11 @@ EXPERIMENTAL {
>>  	rte_cryptodev_remove_deq_callback;
>>  	rte_cryptodev_remove_enq_callback;
>>
>> +	# added 22.03
>
>+1 for get & set user_data API. Ideally it should have been a separate series
>but I agree that it's better getting addressed along with the session rework.
>
[CP] 
Agreed, for v2 I have split it into its own patch in this patchset rather than squashing it with other session changes.