RE: [EXT] [PATCH v2 02/14] cryptodev: reduce number of comments in asym xform

["Kusztal, ArkadiuszX" <arkadiuszx.kusztal@intel.com>]

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, May 26, 2022 11:52 AM
> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; dev@dpdk.org
> Cc: Zhang, Roy Fan <roy.fan.zhang@intel.com>
> Subject: RE: [EXT] [PATCH v2 02/14] cryptodev: reduce number of comments in
> asym xform
> 
> > - Reduced number of comments in asymmetric xform.
> > Information describing basic functionality of well known algorithms
> > are unnecessary.
> > - Removed NONE asymetric xform.
> 
> I commented on v1 not to remove this and I do not see comment from your side
> for removing it.
[Arek] - yeah, sorry for that, I will keep it in v3.
> 
> >
> > Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> > ---
> >  app/test/test_cryptodev_asym.c  |   2 -
> >  lib/cryptodev/rte_crypto_asym.h | 114 ++++++++++++++++-----------------------
> -
> >  lib/cryptodev/rte_cryptodev.c   |   1 -
> >  3 files changed, 46 insertions(+), 71 deletions(-)
> >
> > diff --git a/app/test/test_cryptodev_asym.c
> > b/app/test/test_cryptodev_asym.c index 573af2a537..5aa9d65395 100644
> > --- a/app/test/test_cryptodev_asym.c
> > +++ b/app/test/test_cryptodev_asym.c
> > @@ -288,7 +288,6 @@ test_cryptodev_asym_ver(struct rte_crypto_op *op,
> >  		break;
> >  	case RTE_CRYPTO_ASYM_XFORM_DH:
> >  	case RTE_CRYPTO_ASYM_XFORM_DSA:
> > -	case RTE_CRYPTO_ASYM_XFORM_NONE:
> >  	case RTE_CRYPTO_ASYM_XFORM_UNSPECIFIED:
> >  	default:
> >  		break;
> > @@ -440,7 +439,6 @@ test_cryptodev_asym_op(struct
> > crypto_testsuite_params_asym *ts_params,
> >  		break;
> >  	case RTE_CRYPTO_ASYM_XFORM_DH:
> >  	case RTE_CRYPTO_ASYM_XFORM_DSA:
> > -	case RTE_CRYPTO_ASYM_XFORM_NONE:
> >  	case RTE_CRYPTO_ASYM_XFORM_UNSPECIFIED:
> >  	default:
> >  		snprintf(test_msg, ASYM_TEST_MSG_LEN, diff --git
> > a/lib/cryptodev/rte_crypto_asym.h b/lib/cryptodev/rte_crypto_asym.h
> > index 7206652458..66ffb29743 100644
> > --- a/lib/cryptodev/rte_crypto_asym.h
> > +++ b/lib/cryptodev/rte_crypto_asym.h
> > @@ -38,6 +38,40 @@ extern const char *  rte_crypto_asym_op_strings[];
> >
> >  /**
> > + * Buffer to hold crypto params required for asym operations.
> > + *
> > + * These buffers can be used for both input to PMD and output from PMD.
> > When
> > + * used for output from PMD, application has to ensure the buffer is
> > +large
> > + * enough to hold the target data.
> > + *
> > + * If an operation requires the PMD to generate a random number,
> > + * and the device supports CSRNG, 'data' should be set to NULL.
> > + * The crypto parameter in question will not be used by the PMD,
> > + * as it is internally generated.
> > + */
> > +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;
> > +
> > +/** Unsigned big-integer in big-endian format */ typedef
> > +rte_crypto_param rte_crypto_uint;
> > +
> > +/**
> > + * Structure for elliptic curve point  */ struct rte_crypto_ec_point
> > +{
> > +	rte_crypto_param x;
> > +	/**< X coordinate */
> > +	rte_crypto_param y;
> > +	/**< Y coordinate */
> > +};
> 
> Why is this movement of code done?
[Arek] - well this was to be part of previous patches accepted in last release. It just look more clear having this structs on top. But this is matter of opinion, can be dropped from v3.
But we need to keep in mind that there is already structure like crypto_param -> crypto_vec, and we discussed last release to unify it. Especially that tot_len in rte_crypto_vec would be bit more useful in asymmetric cryptography than symmetric. But definitely not before RC1
> 
> > +
> > +/**
> >   * List of elliptic curves. This enum aligns with
> >   * TLS "Supported Groups" registry (previously known  as
> >   * NamedCurve registry). FFDH groups are not, and will not @@ -55,46
> > +89,23 @@ enum rte_crypto_curve_id {  };
> >
> >  /**
> > - * Asymmetric crypto transformation types.
> > - * Each xform type maps to one asymmetric algorithm
> > - * performing specific operation
> > - *
> > + * Asymmetric crypto algorithms
> 
> Since you are deferring the change for xform, it is better to keep the Above
> description as well. So no need for this above change.
+1
> 
> >   */
> >  enum rte_crypto_asym_xform_type {
> > -	RTE_CRYPTO_ASYM_XFORM_UNSPECIFIED = 0,
> > +	RTE_CRYPTO_ASYM_XFORM_UNSPECIFIED,
> >  	/**< 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
> > -	 */
> > +	/**< RSA */
> 
> I believe it is better to have a short one line description is good to have For
> someone who is new to asymmetric cryptography.
[Arek] - ok, so this patch probably could be dropped considering above comments.
> 
> You can remove "* Refer to rte_crypto_asym_op_type "
> 
> >  	RTE_CRYPTO_ASYM_XFORM_DH,
> > -	/**< Diffie-Hellman.
> > -	 * Performs Key Generate and Shared Secret Compute.
> > -	 * Refer to rte_crypto_asym_op_type
> > -	 */
> > +	/**< Diffie-Hellman */
> >  	RTE_CRYPTO_ASYM_XFORM_DSA,
> > -	/**< Digital Signature Algorithm
> > -	 * Performs Signature Generation and Verification.
> > -	 * Refer to rte_crypto_asym_op_type
> > -	 */
> > +	/**< Digital Signature Algorithm */
> >  	RTE_CRYPTO_ASYM_XFORM_MODINV,
> > -	/**< Modular Multiplicative Inverse
> > -	 * Perform Modular Multiplicative Inverse b^(-1) mod n
> > -	 */
> > +	/**< Modular Multiplicative Inverse */
> >  	RTE_CRYPTO_ASYM_XFORM_MODEX,
> > -	/**< Modular Exponentiation
> > -	 * Perform Modular Exponentiation b^e mod n
> > -	 */
> > +	/**< Modular Exponentiation */
> >  	RTE_CRYPTO_ASYM_XFORM_ECDSA,
> > -	/**< Elliptic Curve Digital Signature Algorithm
> > -	 * Perform Signature Generation and Verification.
> > -	 */
> > +	/**< Elliptic Curve Digital Signature Algorithm */
> >  	RTE_CRYPTO_ASYM_XFORM_ECPM,
> >  	/**< Elliptic Curve Point Multiplication */
> >  	RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > @@ -126,11 +137,12 @@ enum rte_crypto_asym_op_type {
> >   * Padding types for RSA signature.
> >   */
> >  enum rte_crypto_rsa_padding_type {
> > -	RTE_CRYPTO_RSA_PADDING_NONE = 0,
> > +	RTE_CRYPTO_RSA_PADDING_NONE,
> >  	/**< RSA no padding scheme */
> >  	RTE_CRYPTO_RSA_PADDING_PKCS1_5,
> > -	/**< RSA PKCS#1 PKCS1-v1_5 padding scheme. For signatures block type
> > 01,
> > -	 * for encryption block type 02 are used.
> > +	/**< RSA PKCS#1 PKCS1-v1_5 padding scheme.
> > +	 * For signatures block type 01, for encryption
> > +	 * block type 02 are used.
> >  	 */
> >  	RTE_CRYPTO_RSA_PADDING_OAEP,
> >  	/**< RSA PKCS#1 OAEP padding scheme */ @@ -156,40 +168,6 @@
> enum
> > rte_crypto_rsa_priv_key_type {  };
> >
> >  /**
> > - * Buffer to hold crypto params required for asym operations.
> > - *
> > - * These buffers can be used for both input to PMD and output from PMD.
> > When
> > - * used for output from PMD, application has to ensure the buffer is
> > large
> > - * enough to hold the target data.
> > - *
> > - * If an operation requires the PMD to generate a random number,
> > - * and the device supports CSRNG, 'data' should be set to NULL.
> > - * The crypto parameter in question will not be used by the PMD,
> > - * as it is internally generated.
> > - */
> > -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;
> > -
> > -/** Unsigned big-integer in big-endian format */ -typedef
> > rte_crypto_param rte_crypto_uint;
> > -
> > -/**
> > - * Structure for elliptic curve point
> > - */
> > -struct rte_crypto_ec_point {
> > -	rte_crypto_param x;
> > -	/**< X coordinate */
> > -	rte_crypto_param y;
> > -	/**< Y coordinate */
> > -};
> > -
> > -/**
> >   * Structure describing RSA private key in quintuple format.
> >   * See PKCS V1.5 RSA Cryptography Standard.
> >   */
> > diff --git a/lib/cryptodev/rte_cryptodev.c
> > b/lib/cryptodev/rte_cryptodev.c index e16e6802aa..691625bd04 100644
> > --- a/lib/cryptodev/rte_cryptodev.c
> > +++ b/lib/cryptodev/rte_cryptodev.c
> > @@ -160,7 +160,6 @@ rte_crypto_aead_operation_strings[] = {
> >   * Asymmetric crypto transform operation strings identifiers.
> >   */
> >  const char *rte_crypto_asym_xform_strings[] = {
> > -	[RTE_CRYPTO_ASYM_XFORM_NONE]	= "none",
> >  	[RTE_CRYPTO_ASYM_XFORM_RSA]	= "rsa",
> >  	[RTE_CRYPTO_ASYM_XFORM_MODEX]	= "modexp",
> >  	[RTE_CRYPTO_ASYM_XFORM_MODINV]	= "modinv",
> > --
> > 2.13.6