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

[Verma, Shally]

HI Declan

>-----Original Message-----
>From: Doherty, Declan [mailto:declan.doherty at intel.com]
>Sent: 09 July 2018 20:25
>To: Verma, Shally <Shally.Verma at cavium.com>; pablo.de.lara.guarch at intel.com
>Cc: dev at dpdk.org; Athreya, Narayana Prasad <NarayanaPrasad.Athreya at cavium.com>; Murthy, Nidadavolu
><Nidadavolu.Murthy at cavium.com>; Sahu, Sunila <Sunila.Sahu at cavium.com>; Gupta, Ashish <Ashish.Gupta at cavium.com>; Kartha,
>Umesh <Umesh.Kartha at cavium.com>
>Subject: Re: [dpdk-dev] [PATCH v4 1/4] lib/cryptodev: add asymmetric algos in cryptodev
>
>External Email
>
>On 06/07/2018 3:28 PM, Verma, Shally wrote:
>> Hi Declan
>>
>>> -----Original Message-----
>>> From: Doherty, Declan [mailto:declan.doherty at intel.com]
>>> Sent: 05 July 2018 20:24
>>> To: Verma, Shally <Shally.Verma at cavium.com>; pablo.de.lara.guarch at intel.com
>>> Cc: dev at dpdk.org; Athreya, Narayana Prasad <NarayanaPrasad.Athreya at cavium.com>; Murthy, Nidadavolu
>>> <Nidadavolu.Murthy at cavium.com>; Sahu, Sunila <Sunila.Sahu at cavium.com>; Gupta, Ashish <Ashish.Gupta at cavium.com>;
>Kartha,
>>> Umesh <Umesh.Kartha at 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 at caviumnetworks.com>
>>>> Signed-off-by: Sunila Sahu <sunila.sahu at caviumnetworks.com>
>>>> Signed-off-by: Ashish Gupta <ashish.gupta at caviumnetworks.com>
>>>> Signed-off-by: Umesh Kartha <umesh.kartha at 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?
>>
>
>Sure I understand the rationale.
>
>>
>>> 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?
>
>I would be a bit concerned with dropping the
>rte_crypto_asym_xx_op_params as if we are following the symmetric model,
>the xform should only specify the immutable data in relation to a
>transform and the operation should provide the mutable input data. I'm
>not sure how this would look like for the different asymmetric
>operations but if there are transforms that don't have immutable data
>then it would make more sense not to have a specific xform data
>structure for that and pass the data through that transformations
>operation structure.

[Shally] Believe there's some misunderstanding here. My suggestion was not to drop asym_xx_op_params, but just an op_type field from op_params if xform types are redefined as suggested above. However, I see below you have another proposal so I will clarify more there.
As a side note, Asym works on similar principle as symmetric i.e. immutable data in xform and mutable in op_params. So, op_params definitely will be there.

>
>>
>> We were about to submit Openssl PMD with asym support today. But I would hold back till we align on this.
>> ...
>>
>
>Ok, I think I had missed part of the complexity of the chained used
>case. I think if we are to follow the symmetric crypto model we end up
>with something along the lines of a fundamental op types - dh, dsa, mod,
>rsa etc.
>
>enum rte_crypto_asym_op_types {
>        RTE_CRYPTO_ASYM_OP_DH,
>        RTE_CRYPTO_ASYM_OP_DSA,
>        RTE_CRYPTO_ASYM_OP_MOD,
>        RTE_CRYPTO_ASYM_OP_RSA,
>};
>
>with a corresponding asym_op as follows.
>
>struct rte_crypto_asym_op {
>        union {
>                struct rte_cryptodev_asym_session *session;
>                /**< Handle for the initialized session context */
>                struct rte_crypto_asym_xform *xform;
>        };
>
>        union {
>                struct rte_crypto_asym_dh_op_data dh;
>                struct rte_crypto_asym_dsa_op_data dsa;
>                struct rte_crypto_asyn_mod_op_data mod;
>                struct rte_crypto_asym_rsa_op_data rsa;
>        } op_data;
>};
>
>
>In terms of the xform definitions I'm not sure that we should have all
>the different transforms defined in the same enum. If we take the below
>approach, all the specifics of each transform could be split out into
>separate headers, giving a cleaner API. This would see a xform types
>enum which would mirror the operation types:
>
>enum rte_crypto_asym_xform_types {
>        RTE_CRYPTO_ASYM_XFORM_DH,
>        RTE_CRYPTO_ASYM_XFORM_DSA,
>        RTE_CRYPTO_ASYM_XFORM_MOD,
>        RTE_CRYPTO_ASYM_XFORM_RSA,
>}
>
>with corresponding xforms structures
>
>struct rte_crypto_asym_xform dh;
>struct rte_crypto_asym_xform dsa;
>struct rte_crypto_asym_xform modlus;
>struct rte_crypto_asym_xform rsa;
>
>then within the xforms would define the sub-type of that xform and have
>definitions of  the relevant session data which is required, with the
>rsa it might look like something like this:
>
>enum rte_crypto_asym_xform_rsa_types {
>        RTE_CRYPTO_ASYM_XFORM_RSA_ENCRYPT,
>        RTE_CRYPTO_ASYM_XFORM_RSA_DECRYPT,
>        RTE_CRYPTO_ASYM_XFORM_RSA_SIGN,
>        RTE_CRYPTO_ASYM_XFORM_RSA_VERIFY,
>}
>
>struct rte_crypto_asym_xform rsa {
>        enum rte_crypto_asym_op_rsa_types type;
>        union {
>                struct rte_crypto_asym_xform_rsa_encrypt_data encrypt;
>                struct rte_crypto_asym_xform_rsa_decrypt_data decrypt;
>                etc...
>        } data;
>};
>
>The need for xform sub-type data structure would be dependent on whether
>there was immutable data associated with a particular transform. Also if
>we take this approach it would allow each operation type to be separated
>out into there own headers.
>

[Shally] I'm assuming you are not suggesting any changes to existing fundamental rte_crypto_asym_op and rte_crypto_asym_xform structures here, as they're 
defined the way you mentioned.  Only change I see having a new xform sub-type in per-xform struct.
Since we already have an enum rte_crypto_asym_op_type {
RTE_CRYPTO_ASYM_OP_ENCRYPT,
RTE_CRYPTO_ASYM_OP_SIGN
RTE_CRYPTO_ASYM_OP_VERIFY
RTE_CRYPTO_ASYM_OP_KEY_PAIR_GENERATE and so on...
}
So, we can use existing one into asym_xx_xform structure so every rte_crypto_asym_xx_xform struct would look like:

struct rte_crypto_xxx_xform {
	enum rte_crypto_asym_op_type;
	/**< Sign/Verify/Encrypt/Decrypt/key generate/ */

	/* other xform params such as public and private key */
}

Since every xform returns bitmask of op_types supported in capability structure. So, app would know which op types are supported on xform.
Also, I don’t see a need a new define such as OP_RSA/OP_DSA as every op come associated with session/xform with same information, so relevant xform op params will be referenced based on that.

Thanks
Shally

>>> ....
>>>
>>>
>>>> + */
>>>> +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?
>>
>The sess_type=RTE_CRYPTO_OP_WITH_SESSION/RTE_CRYPTO_OP_SESSIONLESS
>should be set in the outer crypto_op struct, so if the PMD doesn't
>support the selected sess_type, it should just fail the enqueue and
>possibly log an error.
>
>> Thanks for review.
>> Shally
>>
>>>          __extension__
>>>
>>>
>>>> +#ifdef __cplusplus
>>>> +}
>>>> +#endif
>>>> +
>>>> +#endif /* _RTE_CRYPTO_ASYM_H_ */
>>>>
>>