From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 1787E1BE57 for ; Thu, 5 Jul 2018 16:54:32 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Jul 2018 07:54:31 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,312,1526367600"; d="scan'208";a="54659423" Received: from dwdohert-mobl.ger.corp.intel.com (HELO [163.33.176.228]) ([163.33.176.228]) by orsmga008.jf.intel.com with ESMTP; 05 Jul 2018 07:54:30 -0700 To: Shally Verma , pablo.de.lara.guarch@intel.com Cc: dev@dpdk.org, pathreya@caviumnetworks.com, nmurthy@caviumnetworks.com, Sunila Sahu , Ashish Gupta , Umesh Kartha References: <1530631466-26427-1-git-send-email-shally.verma@caviumnetworks.com> <1530631466-26427-2-git-send-email-shally.verma@caviumnetworks.com> From: "Doherty, Declan" Message-ID: <9905afc5-3571-d6c8-c2b0-7c9804f0dbd6@intel.com> Date: Thu, 5 Jul 2018 15:54:29 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <1530631466-26427-2-git-send-email-shally.verma@caviumnetworks.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 1/4] lib/cryptodev: add asymmetric algos in cryptodev X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Jul 2018 14:54:33 -0000 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 > Signed-off-by: Sunila Sahu > Signed-off-by: Ashish Gupta > Signed-off-by: Umesh Kartha > --- > 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, 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? 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. > + > +/** asym xform type name strings */ > +extern const char * > +rte_crypto_asym_xform_strings[]; > + > +/** asym operations type name strings */ > +extern const char * > +rte_crypto_asym_op_strings[]; > + > +/** > + * Asymmetric crypto transformation types. > + * Each xform type maps to one asymmetric algorithm > + * performing specific operation > + * > + */ > +enum rte_crypto_asym_xform_type { > + RTE_CRYPTO_ASYM_XFORM_UNSPECIFIED = 0, > + /**< Invalid xform. */ > + RTE_CRYPTO_ASYM_XFORM_NONE, > + /**< Xform type None. > + * May be supported by PMD to support > + * passthrough op for debugging purpose. > + * if xform_type none , op_type is disregarded. > + */ > + RTE_CRYPTO_ASYM_XFORM_RSA, > + /**< RSA. Performs Encrypt, Decrypt, Sign and Verify. > + * Refer to rte_crypto_asym_op_type > + */ > + RTE_CRYPTO_ASYM_XFORM_DH, > + /**< Deffie-Hellman. > + * Performs Key Generate and Shared Secret Compute. > + * Refer to rte_crypto_asym_op_type > + */ > + RTE_CRYPTO_ASYM_XFORM_DSA, > + /**< Digital Signature Algorithm > + * Performs Signature Generation and Verification. > + * Refer to rte_crypto_asym_op_type > + */ > + 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 :) > + /**< 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. > +/** .... > + */ > +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. > + rte_crypto_param p; ... > +struct rte_crypto_rsa_op_param { > + enum rte_crypto_asym_op_type op_type; > + /**< Type of RSA operation for transform */; > + see previous comment above ... > + > +/** > + * DSA Operations params > + * > + */ > +struct rte_crypto_dsa_op_param { > + enum rte_crypto_asym_op_type op_type; > + /**< Signature Generation or Verification */ see previous comment 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. 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; + }; __extension__ > +#ifdef __cplusplus > +} > +#endif > + > +#endif /* _RTE_CRYPTO_ASYM_H_ */ >