Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Akhil Goyal <gakhil@marvell.com>]

> 
> On 08/10/2021 21:45, Akhil Goyal wrote:
> > Remove *_LIST_END enumerators from asymmetric crypto
> > lib to avoid ABI breakage for every new addition in
> > enums.
> >
> > Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > ---
> > v2: no change
> >
> >  app/test/test_cryptodev_asym.c  | 4 ++--
> >  drivers/crypto/qat/qat_asym.c   | 2 +-
> >  lib/cryptodev/rte_crypto_asym.h | 4 ----
> >  3 files changed, 3 insertions(+), 7 deletions(-)
> >
> > diff --git a/app/test/test_cryptodev_asym.c
> b/app/test/test_cryptodev_asym.c
> > index 9d19a6d6d9..603b2e4609 100644
> > --- a/app/test/test_cryptodev_asym.c
> > +++ b/app/test/test_cryptodev_asym.c
> > @@ -541,7 +541,7 @@ test_one_case(const void *test_case, int
> sessionless)
> >  		printf("  %u) TestCase %s %s\n", test_index++,
> >  			tc.modex.description, test_msg);
> >  	} else {
> > -		for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> > +		for (i = 0; i <=
> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE; i++) {
> >  			if (tc.modex.xform_type ==
> RTE_CRYPTO_ASYM_XFORM_RSA) {
> >  				if (tc.rsa_data.op_type_flags & (1 << i)) {
> >  					if (tc.rsa_data.key_exp) {
> > @@ -1027,7 +1027,7 @@ static inline void print_asym_capa(
> >  			rte_crypto_asym_xform_strings[capa->xform_type]);
> >  	printf("operation supported -");
> >
> > -	for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> > +	for (i = 0; i <= RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE;
> i++) {
> >  		/* check supported operations */
> >  		if
> (rte_cryptodev_asym_xform_capability_check_optype(capa, i))
> >  			printf(" %s",
> > diff --git a/drivers/crypto/qat/qat_asym.c b/drivers/crypto/qat/qat_asym.c
> > index 85973812a8..026625a4d2 100644
> > --- a/drivers/crypto/qat/qat_asym.c
> > +++ b/drivers/crypto/qat/qat_asym.c
> > @@ -742,7 +742,7 @@ qat_asym_session_configure(struct rte_cryptodev
> *dev,
> >  			err = -EINVAL;
> >  			goto error;
> >  		}
> > -	} else if (xform->xform_type >=
> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > +	} else if (xform->xform_type > RTE_CRYPTO_ASYM_XFORM_ECPM
> >  			|| xform->xform_type <=
> RTE_CRYPTO_ASYM_XFORM_NONE) {
> >  		QAT_LOG(ERR, "Invalid asymmetric crypto xform");
> >  		err = -EINVAL;
> > diff --git a/lib/cryptodev/rte_crypto_asym.h
> b/lib/cryptodev/rte_crypto_asym.h
> > index 9c866f553f..5edf658572 100644
> > --- a/lib/cryptodev/rte_crypto_asym.h
> > +++ b/lib/cryptodev/rte_crypto_asym.h
> > @@ -94,8 +94,6 @@ enum rte_crypto_asym_xform_type {
> >  	 */
> >  	RTE_CRYPTO_ASYM_XFORM_ECPM,
> >  	/**< Elliptic Curve Point Multiplication */
> > -	RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > -	/**< End of list */
> >  };
> >
> >  /**
> > @@ -116,7 +114,6 @@ enum rte_crypto_asym_op_type {
> >  	/**< DH Public Key generation operation */
> >  	RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
> >  	/**< DH Shared Secret compute operation */
> > -	RTE_CRYPTO_ASYM_OP_LIST_END
> >  };
> >
> >  /**
> > @@ -133,7 +130,6 @@ enum rte_crypto_rsa_padding_type {
> >  	/**< RSA PKCS#1 OAEP padding scheme */
> >  	RTE_CRYPTO_RSA_PADDING_PSS,
> >  	/**< RSA PKCS#1 PSS padding scheme */
> > -	RTE_CRYPTO_RSA_PADDING_TYPE_LIST_END
> >  };
> >
> >  /**
> 
> So I am not sure that this is an improvement.
> The cryptodev issue we had, was that _LIST_END was being used to size
> arrays.
> And that broke when new algorithms got added. Is that an issue, in this case?

Yes we did this same exercise for symmetric crypto enums earlier.
Asym enums were left as it was experimental at that point.
They are still experimental, but thought of making this uniform
throughout DPDK enums.

> 
> I am not sure that swapping out _LIST_END, and then littering the code with
> RTE_CRYPTO_ASYM_XFORM_ECPM and
> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an improvement
> here.
> 
> My 2c is that from an ABI PoV RTE_CRYPTO_ASYM_OP_LIST_END is not
> better or worse,
> than RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?
> 
> Interested to hear other thoughts.

I don’t have any better solution for avoiding ABI issues for now.
The change is for avoiding ABI breakage. But we can drop this patch
For now as asym is still experimental.