HI Declan, Pablo Could we please close on this asap?
Thanks Shally >-----Original Message----- >From: Verma, Shally >Sent: 06 July 2018 19:59 >To: 'Doherty, Declan' <declan.dohe...@intel.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 > >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? > > >>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_ */ >>>
