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

[Verma, Shally]

>-----Original Message-----
>From: Trahe, Fiona [mailto:fiona.trahe at intel.com]
>Sent: 09 July 2018 22:15
>To: Doherty, Declan <declan.doherty at intel.com>; Verma, Shally <Shally.Verma at cavium.com>; De Lara Guarch, Pablo
><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>; Trahe, Fiona <fiona.trahe at intel.com>
>Subject: RE: [dpdk-dev] [PATCH v4 1/4] lib/cryptodev: add asymmetric algos in cryptodev
>
>External Email
>
>Hi Shally,  Declan, Pablo,
>
>I'm concerned about rushing in significant last-minute changes, but would like to see this API in 18.08.
>So I suggest the patchset is applied with the caveat that it is experimental and will continue to be so
>for the next release, in which the remaining open issues should be addressed.
>The main areas of concern are:
> - the structures for xforms and ops and rework needed to cater for sessionless
> - capabilities
>
Sounds good to me. If that’s fine with everyone, I will send current openssl PMD patch. Please confirm.

Thanks
Shally
>
>Regards,
>Fiona
>
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Doherty, Declan
>> Sent: Monday, July 9, 2018 3:55 PM
>> To: Verma, Shally <Shally.Verma at cavium.com>; De Lara Guarch, Pablo
>> <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
>>
>> 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.
>>
>> >
>> > 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.
>>
>> >> ....
>> >>
>> >>
>> >>> + */
>> >>> +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_ */
>> >>>
>> >