>-----Original Message----- >From: Trahe, Fiona [mailto:fiona.tr...@intel.com] >Sent: 09 July 2018 22:15 >To: Doherty, Declan <declan.dohe...@intel.com>; Verma, Shally ><shally.ve...@cavium.com>; De Lara Guarch, Pablo ><pablo.de.lara.gua...@intel.com> >Cc: dev@dpdk.org; Athreya, Narayana Prasad ><narayanaprasad.athr...@cavium.com>; Murthy, Nidadavolu ><nidadavolu.mur...@cavium.com>; Sahu, Sunila <sunila.s...@cavium.com>; Gupta, >Ashish <ashish.gu...@cavium.com>; Kartha, >Umesh <umesh.kar...@cavium.com>; Trahe, Fiona <fiona.tr...@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-boun...@dpdk.org] On Behalf Of Doherty, Declan >> Sent: Monday, July 9, 2018 3:55 PM >> To: Verma, Shally <shally.ve...@cavium.com>; De Lara Guarch, Pablo >> <pablo.de.lara.gua...@intel.com> >> Cc: dev@dpdk.org; Athreya, Narayana Prasad >> <narayanaprasad.athr...@cavium.com>; Murthy, >> Nidadavolu <nidadavolu.mur...@cavium.com>; Sahu, Sunila >> <sunila.s...@cavium.com>; Gupta, >> Ashish <ashish.gu...@cavium.com>; Kartha, Umesh <umesh.kar...@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.dohe...@intel.com] >> >> Sent: 05 July 2018 20:24 >> >> To: Verma, Shally <shally.ve...@cavium.com>; >> >> pablo.de.lara.gua...@intel.com >> >> Cc: dev@dpdk.org; Athreya, Narayana Prasad >> >> <narayanaprasad.athr...@cavium.com>; Murthy, >> Nidadavolu >> >> <nidadavolu.mur...@cavium.com>; Sahu, Sunila <sunila.s...@cavium.com>; >> >> Gupta, Ashish >> <ashish.gu...@cavium.com>; Kartha, >> >> Umesh <umesh.kar...@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.ve...@caviumnetworks.com> >> >>> Signed-off-by: Sunila Sahu <sunila.s...@caviumnetworks.com> >> >>> Signed-off-by: Ashish Gupta <ashish.gu...@caviumnetworks.com> >> >>> Signed-off-by: Umesh Kartha <umesh.kar...@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_ */ >> >>> >> >
