Re: [dpdk-dev] [PATCH v3 3/6] lib/cryptodev: add asymmetric crypto capability in cryptodev

["Verma, Shally" <Shally.Verma@cavium.com>]

HI Fiona

Thanks for review. I was just about to send lib patch v4. So for now, I couldn't  consider all input but some of them.
But , in any case, this version is experimental so it is open for further feedback after 1st version go in.

Rest, please see my feedback inline.

>-----Original Message-----
>From: Trahe, Fiona [mailto:fiona.trahe@intel.com]
>Sent: 03 July 2018 19:42
>To: Verma, Shally <Shally.Verma@cavium.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
>Cc: akhil.goyal@nxp.com; dev@dpdk.org; Athreya, Narayana Prasad <NarayanaPrasad.Athreya@cavium.com>; Sahu, Sunila
><Sunila.Sahu@cavium.com>; Gupta, Ashish <Ashish.Gupta@cavium.com>; Trahe, Fiona <fiona.trahe@intel.com>
>Subject: RE: [PATCH v3 3/6] lib/cryptodev: add asymmetric crypto capability in cryptodev
>
>External Email
>
>Hi Shally,
>
>> -----Original Message-----
>> From: Shally Verma [mailto:shally.verma@caviumnetworks.com]
>> Sent: Wednesday, May 16, 2018 7:05 AM
>> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
>> Cc: Trahe, Fiona <fiona.trahe@intel.com>; akhil.goyal@nxp.com; dev@dpdk.org;
>> pathreya@caviumnetworks.com; Sunila Sahu <sunila.sahu@caviumnetworks.com>; Ashish Gupta
>> <ashish.gupta@caviumnetworks.com>
>> Subject: [PATCH v3 3/6] lib/cryptodev: add asymmetric crypto capability in cryptodev
>>
>> Extend cryptodev with asymmetric capability APIs and
>> definitions.
>>
>> changes from v2:
>> - remove redundant xform_type from asym capability struct
>> - rename rte_cryptodev_get_asym_xform_enum to
>> be more consistent with other API names
>>
>> Signed-off-by: Shally Verma <shally.verma@caviumnetworks.com>
>> Signed-off-by: Sunila Sahu <sunila.sahu@caviumnetworks.com>
>> Signed-off-by: Ashish Gupta <ashish.gupta@caviumnetworks.com>
>>
>> ---
>//snip//
>
>> diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
>> index 623459a95..6c13d23f8 100644
>> --- a/lib/librte_cryptodev/rte_cryptodev.h
>> +++ b/lib/librte_cryptodev/rte_cryptodev.h
>> @@ -178,6 +178,35 @@ struct rte_cryptodev_symmetric_capability {
>>       };
>>  };
>>
>> +/**
>> + * Asymmetric Xform Crypto Capability
>> + *
>> + */
>> +struct rte_cryptodev_asymmetric_xfrm_capability {
>> +     enum rte_crypto_asym_xform_type xform_type;
>> +     /**< Transform type: RSA/MODEXP/DH/DSA/MODINV */
>> +
>> +     uint32_t op_types;
>> +     /**< bitmask for supported rte_crypto_asym_op_type */
>> +
>> +     __extension__
>> +     union {
>> +             struct rte_crypto_param_range modlen;
>> +             /**< Range of modulus length supported by modulus based xform.
>> +              * Value 0 mean implementation default
>> +              */
>[Fiona] Some other fields may be necessary here, e.g.
> - A bitmask for supported RSA padding types
> - Whether RSA private-key in quintuple format is supported
> - Which hash-algorithms are supported if RSA padding = OAEP or PSS
> - whether xform chaining is supported for DH keypair gen
>These are not blockers for the first release, but are likely to be
>needed before the experimental label is removed.
>
[Shally] Agree. Part of these capabilities might need to there as per xform capability. 
But  as you indicated, in any case, they are experimental right now, so let's have them added on a requirement basis once current is accepted.

>> +     };
>> +};
>> +
>> +/**
>> + * Asymmetric Crypto Capability
>> + *
>> + */
>> +struct rte_cryptodev_asymmetric_capability {
>> +     struct rte_cryptodev_asymmetric_xfrm_capability xfrm_capa;
>> +};
>[Fiona] Why the  extra indirection?
>Couldn't this be removed and the previous structure be
>renamed rte_cryptodev_asymmetric_capability?
[Shally] it is to keep consistency in rte_cryptodev_capability which uses name asymmetric_capability on the similar line as symmetric.
But again, change is trivial, so if intended will do in subsequent versions.
>
>//snip//
>> @@ -1164,7 +1265,7 @@ int __rte_experimental
>>  rte_cryptodev_asym_session_set_private_data(
>[Fiona] I'd prefer to call this appl_data or appl_priv_data, I think the term private_data is
>over-used, sometimes means PMD data and sometimes means appl data.
>Btw- same is true of sym private_data - but changing that is out of scope for this patch
>
[Shally] ok. I can change it to get_app_priv_data and set_app_priv_data

Thanks
Shally

>
>>                                       struct rte_cryptodev_asym_session *sess,
>>                                       void *data,
>> -                                     uint16_t size)
>> +                                     uint16_t size);
>>
>>  /**
>>   * Get private data of a session.
>> @@ -1178,7 +1279,7 @@ rte_cryptodev_asym_session_set_private_data(
>>   */
>>  void * __rte_experimental
>>  rte_cryptodev_asym_session_get_private_data(
>> -                             struct rte_cryptodev_asym_session *sess)
>> +                             struct rte_cryptodev_asym_session *sess);
>>
>>
>>  #ifdef __cplusplus
>> diff --git a/lib/librte_cryptodev/rte_cryptodev_version.map
>> b/lib/librte_cryptodev/rte_cryptodev_version.map
>> index 62b782444..817cf9f70 100644
>> --- a/lib/librte_cryptodev/rte_cryptodev_version.map
>> +++ b/lib/librte_cryptodev/rte_cryptodev_version.map
>> @@ -89,13 +89,18 @@ DPDK_17.11 {
>>  EXPERIMENTAL {
>>          global:
>>
>> -     rte_cryptodev_asym_get_private_session_size
>> +     rte_cryptodev_asym_capability_get;
>> +     rte_cryptodev_asym_get_private_session_size;
>> +     rte_cryptodev_asym_get_xform_enum;
>> +     rte_crypto_asym_op_strings;
>>       rte_cryptodev_asym_session_clear;
>>       rte_cryptodev_asym_session_create;
>>       rte_cryptodev_asym_session_free;
>>       rte_cryptodev_asym_session_init;
>> -     rte_cryptodev_asym_session_get_private_data
>> -     rte_cryptodev_asym_session_set_private_data
>> +     rte_cryptodev_asym_session_get_private_data;
>> +     rte_cryptodev_asym_session_set_private_data;
>> +     rte_cryptodev_asym_xfrm_capability_check_optype;
>> +     rte_crypto_asym_xform_strings;
>>       rte_cryptodev_sym_session_get_private_data;
>>       rte_cryptodev_sym_session_set_private_data;
>>  } DPDK_17.11;
>> --
>> 2.14.3