Hi Anoob,
Thanks for the review, apologies for the delay in reply.
Comments inline.
Thanks,
Ciara
>-----Original Message-----
>From: Anoob Joseph <ano...@marvell.com>
>Sent: Monday 13 December 2021 16:34
>To: Power, Ciara <ciara.po...@intel.com>; dev@dpdk.org
>Cc: Zhang, Roy Fan <roy.fan.zh...@intel.com>; Akhil Goyal
><gak...@marvell.com>; Doherty, Declan <declan.dohe...@intel.com>; Ankur
>Dwivedi <adwiv...@marvell.com>; Tejasree Kondoj
><ktejas...@marvell.com>; Griffin, John <john.grif...@intel.com>; Trahe,
>Fiona <fiona.tr...@intel.com>; Jain, Deepak K <deepak.k.j...@intel.com>;
>Ray Kinsella <m...@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.po...@intel.com>
>> Sent: Monday, December 13, 2021 8:34 PM
>> To: dev@dpdk.org
>> Cc: roy.fan.zh...@intel.com; Akhil Goyal <gak...@marvell.com>; Ciara
>> Power <ciara.po...@intel.com>; Declan Doherty
>> <declan.dohe...@intel.com>; Ankur Dwivedi <adwiv...@marvell.com>;
>> Anoob Joseph <ano...@marvell.com>; Tejasree Kondoj
>> <ktejas...@marvell.com>; John Griffin <john.grif...@intel.com>; Fiona
>> Trahe <fiona.tr...@intel.com>; Deepak Kumar Jain
>> <deepak.k.j...@intel.com>; Ray Kinsella <m...@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.po...@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.
