Re: [dpdk-dev] [PATCH v3 1/6] lib/cryptodev: add asymmetric algos in cryptodev

["De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>]

Hi Shally,

> -----Original Message-----
> From: Shally Verma [mailto:shally.verma@caviumnetworks.com]
> Sent: Wednesday, May 16, 2018 7:05 AM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Cc: Trahe, Fiona <fiona.trahe@intel.com>; akhil.goyal@nxp.com;
> dev@dpdk.org; pathreya@caviumnetworks.com; Sunila Sahu
> <sunila.sahu@caviumnetworks.com>; Ashish Gupta
> <ashish.gupta@caviumnetworks.com>
> Subject: [PATCH v3 1/6] lib/cryptodev: add asymmetric algos in cryptodev
> 
> 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@caviumnetworks.com>
> Signed-off-by: Sunila Sahu <sunila.sahu@caviumnetworks.com>
> Signed-off-by: Ashish Gupta <ashish.gupta@caviumnetworks.com>

I have some comments below, but apart from those, could you send a rebased version of this API?

Thanks,
Pablo

> ---
>  lib/librte_cryptodev/Makefile          |   2 +-
>  lib/librte_cryptodev/meson.build       |   3 +-
>  lib/librte_cryptodev/rte_crypto_asym.h | 519
> +++++++++++++++++++++++++++++++++
>  3 files changed, 522 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_cryptodev/Makefile b/lib/librte_cryptodev/Makefile index
> bba8dee9f..138e627dc 100644
> --- a/lib/librte_cryptodev/Makefile
> +++ b/lib/librte_cryptodev/Makefile
> @@ -23,7 +23,7 @@ SYMLINK-y-include += rte_crypto.h  SYMLINK-y-include +=
> rte_crypto_sym.h  SYMLINK-y-include += rte_cryptodev.h  SYMLINK-y-include +=
> rte_cryptodev_pmd.h
> -
> +SYMLINK-y-include += rte_crypto_asym.h

Leave the blank space that was present before.

>  # versioning export map
>  EXPORT_MAP := rte_cryptodev_version.map
> 

...

> --- /dev/null
> +++ b/lib/librte_cryptodev/rte_crypto_asym.h

...

> +
> +#include <string.h>
> +#include <stdint.h>

Leave a blank space between non-DPDK and DPDK libraries.

> +#include <rte_memory.h>
> +#include <rte_mempool.h>
> +#include <rte_common.h>
> +

...

> +struct rte_crypto_rsa_xform {
> +	rte_crypto_param n;
> +	/**< n - Prime modulus
> +	 * Prime modulus data of RSA operation in Octet-string network
> +	 * byte order format.
> +	 */
> +
> +	rte_crypto_param e;
> +	/**< e - Public key exponent
> +	 * Public key exponent used for RSA public key operations in Octet-
> +	 * string network byte order format.
> +	 */
> +
> +	enum rte_crypto_rsa_priv_key_type key_type;
> +

Needs RTE_STD_C11/extension, before the union.

> +	union {
> +			rte_crypto_param d;
> +			/**< d - Private key exponent
> +			 * Private key exponent used for RSA
> +			 * private key operations in
> +			 * Octet-string  network byte order format.
> +			 */
> +
> +			struct rte_crypto_rsa_priv_key_qt qt;
> +			/**< qt - Private key in quintuple format */
> +	};
> +};

...

> +/**
> + * 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 */
> +

Looking at the xform structure, it looks like a chain of xforms is possible.
Looking at this union, this case wouldn't be possible, as only one item from the union can be set.

> +	__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;
> +
> +/**
> + * Reset the fields of an asymmetric operation to their default values.
> + *
> + * @param	op	The crypto operation to be reset.
> + */
> +static inline void
> +__rte_crypto_asym_op_reset(struct rte_crypto_asym_op *op) {
> +	memset(op, 0, sizeof(*op));
> +}
> +
> +/**
> + * Attach a session to an asymmetric crypto operation
> + *
> + * @param	asym_op	crypto operation
> + * @param	sess	cryptodev session
> + */
> +static inline int
> +__rte_crypto_op_attach_asym_session(struct rte_crypto_asym_op *asym_op,
> +		struct rte_cryptodev_asym_session *sess) {
> +	asym_op->session = sess;
> +	return 0;
> +}

I think we should get rid of these two functions, as they are just one line, used just once in the code.
I know it is also done in symmetric, I think it can be removed from there too.


> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_CRYPTO_ASYM_H_ */
> --
> 2.14.3