Re: [dpdk-dev] [PATCH v4 1/4] lib/cryptodev: add asymmetric algos in cryptodev

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

Hi Declan

>-----Original Message-----
>From: Doherty, Declan [mailto:declan.doherty@intel.com]
>Sent: 05 July 2018 20:24
>To: Verma, Shally <Shally.Verma@cavium.com>; pablo.de.lara.guarch@intel.com
>Cc: dev@dpdk.org; Athreya, Narayana Prasad <NarayanaPrasad.Athreya@cavium.com>; Murthy, Nidadavolu
><Nidadavolu.Murthy@cavium.com>; Sahu, Sunila <Sunila.Sahu@cavium.com>; Gupta, Ashish <Ashish.Gupta@cavium.com>; Kartha,
>Umesh <Umesh.Kartha@cavium.com>
>Subject: Re: [dpdk-dev] [PATCH v4 1/4] lib/cryptodev: add asymmetric algos in cryptodev
>
>External Email
>
>Hey Shally,
>
>just a few things inline below mainly concerned with the need to be able
>to support session-less operations in future PMDs. I think with a few
>minor changes to the API now it should allow session-less to be
>supported later without the need for a major rework of the APIs, I don't
>think this should cause any major rework to your PMD just the adoption
>of some new more explicit op types.
>
>Thanks
>Declan
>
>On 03/07/2018 4:24 PM, Shally Verma wrote:
>> Add rte_crypto_asym.h with supported xfrms
>> and associated op structures and APIs
>>
>> API currently supports:
>> - RSA Encrypt, Decrypt, Sign and Verify
>> - Modular Exponentiation and Inversion
>> - DSA Sign and Verify
>> - Deffie-hellman private key exchange
>> - Deffie-hellman public key exchange
>> - Deffie-hellman shared secret compute
>> - Deffie-hellman public/private key pair generation
>> using xform chain
>>
>> 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>
>> Signed-off-by: Umesh Kartha <umesh.kartha@caviumnetworks.com>
>> ---
>>   lib/librte_cryptodev/Makefile          |   1 +
>>   lib/librte_cryptodev/meson.build       |   3 +-
>>   lib/librte_cryptodev/rte_crypto_asym.h | 496 +++++++++++++++++++++++++++++++++
>>   3 files changed, 499 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_cryptodev/Makefile b/lib/librte_cr
>
>...
>
>> +typedef struct rte_crypto_param_t {
>> +     uint8_t *data;
>> +     /**< pointer to buffer holding data */
>> +     rte_iova_t iova;
>> +     /**< IO address of data buffer */
>> +     size_t length;
>> +     /**< length of data in bytes */
>> +} rte_crypto_param;
>
>What is the intended way for this memory to be allocated, 

[Shally] It should be pointer to flat buffers and added only to input/output data to/from
asymmetric crypto engine.

> it seems like
>there might be a more general requirement in DPDK for IO addressable
>memory (compression? other hardware acceleators implemented on FPGAs)
>than just asymmetric crypto, will we end up needing to support features
>like scatter gather lists in this structure? 

[Shally] I don’t anticipate that we would need to support scatter-gather data buffers as far as it is used for asymmetric.
And I'm not aware if we have requirement to support it for asymmetric processing since data size is usually small for
such operations. Thus, app is expected to send linear buffers for input/output.

Does that answer your question? Or did I miss anything?


>btw I think this is
>probably fine for the moment as it will be expermential but I think it
>will need to be addressed before the removal of the expermential tag.
>

...

>> +     RTE_CRYPTO_ASYM_XFORM_MODINV,
>
>Would prefer if this was _MOD_INV :)
>
>> +     /**< Modular Inverse
>> +      * Perform Modulus inverse b^(-1) mod n
>> +      */
>> +     RTE_CRYPTO_ASYM_XFORM_MODEX,
>
>any this was _MOD_EX :)

[Shally] fine will do name change.

>
>> +     /**< Modular Exponentiation
>> +      * Perform Modular Exponentiation b^e mod n
>> +      */
>> +     RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
>> +     /**< End of list */
>> +};
>> +
>> +/**
>> + * Asymmetric crypto operation type variants
>> + */
>> +enum rte_crypto_asym_op_type {
>> +     RTE_CRYPTO_ASYM_OP_ENCRYPT,
>> +     /**< Asymmetric Encrypt operation */
>> +     RTE_CRYPTO_ASYM_OP_DECRYPT,
>> +     /**< Asymmetric Decrypt operation */
>> +     RTE_CRYPTO_ASYM_OP_SIGN,
>> +     /**< Signature Generation operation */
>> +     RTE_CRYPTO_ASYM_OP_VERIFY,
>> +     /**< Signature Verification operation */
>> +     RTE_CRYPTO_ASYM_OP_PRIVATE_KEY_GENERATE,
>> +     /**< DH Private Key generation operation */
>> +     RTE_CRYPTO_ASYM_OP_PUBLIC_KEY_GENERATE,
>> +     /**< DH Public Key generation operation */
>> +     RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
>> +     /**< DH Shared Secret compute operation */
>> +     RTE_CRYPTO_ASYM_OP_LIST_END
>> +};
>> +
>
>I think that having generic operation types which may or may not apply
>to all of the defined xforms is confusing from a user perspective and in
>the longer term will make it impossible to support session-less
>asymmetric operations. If we instead do something like
>
>        RTE_CRYPTO_ASYM_OP_RSA_ENCRYPT,
>        RTE_CRYPTO_ASYM_OP_RSA_DECRYPT,
>        RTE_CRYPTO_ASYM_OP_RSA_SIGN,
>        RTE_CRYPTO_ASYM_OP_RSA_VERIFY,
>        RTE_CRYPTO_ASYM_OP_DH_KEY_GENERATE,
>        RTE_CRYPTO_ASYM_OP_DH_SHARED_SECRET_COMPUTE,
>        etc...
>
>Then the op type becomes very explicit and will allow session-less
>operations to be supported by PMDs. This shouldn't have any impact on
>your current implementation other than updating the op type.
>

[Shally] Ok, so you suggest to merge xform and op_type (including keeping op_type in one place)  for simplicity sake.

If we take this change, then I will prefer to keep it as xforms rather than op_types, such as,

enum rte_crypto_asym_xform_types {
RTE_CRYPTO_ASYM_XFORM_RSA_ENCRYPT,
RTE_CRYPTO_ASYM_XFORM_RSA_SIGN,
RTE_CRYPTO_ASYM_XFORM_DSA_SIGN,
RTE_CRYPTO_ASYM_XFORM_DSA_VERIFY,
RTE_CRYPTO_ASYM_XFORM_DH_PRIV_KEY_GENERATE,
RTE_CRYPTO_ASYM_XFORM_DH_PUB_KEY_GENERATE,
RTE_CRYPTO_ASYM_XFORM_DH_SHARED_SECRET_COMPUTE, .... and so on..
}

These, when set on a rte_crypto_asym_xform , will work seamlessly for both session and session-less. 
I also see advantage here to support xform chaining. In this case, op_type in rte_crypto_asym_xx_op_params isn't needed and will be removed.
It will add bit of repetition though, like RSA_SIGN / DSA_SIGN, but that would occur in any case, if we are merging two types.
Does that sound okay? 

We were about to submit Openssl PMD with asym support today. But I would hold back till we align on this.
...

>....
>
>
>> + */
>> +struct rte_crypto_dh_xform {
>> +     enum rte_crypto_asym_op_type type;
>> +     /**< Setup xform for key generate or shared secret compute */
>> +
>
>there is an inconsistency here in terms of were the op_type is defined,
>in this case it is in the xform but it other cases RSA, DSA it is
>defined in the operation information itself. I don't know of any reason
>why it is needed in the xform but I think it must be consistent across
>all operations/xforms. Ideally from my perspective it would be in the
>rte_crypto_asym_op structure, see below, as this would allow
>session/session-less operations to be supported seamlessly.
>

[Shally] Reason was xform chaining. As per Fiona feedback, we identified requirement to break DH Key pair
generation in to two  : PRIV_KEY_GENERATION and PUB_KEY_GENERATION, so I had to move this param around.
But this will be addressed once we change xform_types as per suggestion above.

...

>
>...
>
>> +/**
>> + * Asymmetric Cryptographic Operation.
>> + *
>> + * Structure describing asymmetric crypto operation params.
>> + *
>> + */
>> +struct rte_crypto_asym_op {
>> +     struct rte_cryptodev_asym_session *session;
>> +     /**< Handle for the initialised session context */
>> +
>> +     __extension__
>> +     union {
>> +             struct rte_crypto_rsa_op_param rsa;
>> +             struct rte_crypto_mod_op_param modex;
>> +             struct rte_crypto_mod_op_param modinv;
>> +             struct rte_crypto_dh_op_param dh;
>> +             struct rte_crypto_dsa_op_param dsa;
>> +     };
>> +} __rte_cache_aligned;
>> +
>Relating to my comment on position of the op_type and the minor change
>of having an union of session/xform in the rte_crypto_asym_op structure
>would then enable sessionless support to be added seamless in the future
>with minimal effect to the current proposal.

[Shally] Again, this will also be resolved with change to xform_types

>
>struct rte_crypto_asym_op {
>-       struct rte_cryptodev_asym_session *session;
>-       /**< Handle for the initialised session context */
>+       enum rte_crypto_asym_op_type op_type;
>+
>+       union {
>+               struct rte_cryptodev_asym_session *session;
>+               /**< Handle for the initialised session context */
>+               struct rte_crypto_asym_xform *xform;
>+       };
>
[Shally] Ok. Will add this change. But then I'll have to check if crypto PMD has support to reflect which type of mode it supports?

Thanks for review.
Shally

>         __extension__
>
>
>> +#ifdef __cplusplus
>> +}
>> +#endif
>> +
>> +#endif /* _RTE_CRYPTO_ASYM_H_ */
>>