On 12/10/2021 11:50, Anoob Joseph wrote:

Hi Ray, Akhil,

Please see inline.

Thanks,
Anoob

quoted

-----Original Message-----
From: Akhil Goyal <redacted>
Sent: Tuesday, October 12, 2021 3:49 PM
To: Kinsella, Ray <redacted>; dev@dpdk.org
Cc: thomas@monjalon.net; david.marchand@redhat.com;
hemant.agrawal@nxp.com; Anoob Joseph [off-list ref];
pablo.de.lara.guarch@intel.com; fiona.trahe@intel.com;
declan.doherty@intel.com; matan@nvidia.com; g.singh@nxp.com;
roy.fan.zhang@intel.com; jianjay.zhou@huawei.com; asomalap@amd.com;
ruifeng.wang@arm.com; konstantin.ananyev@intel.com;
radu.nicolau@intel.com; ajit.khaparde@broadcom.com; Nagadheeraj Rottela
[off-list ref]; Ankur Dwivedi [off-list ref];
ciara.power@intel.com; Stephen Hemminger [off-list ref];
Yigit, Ferruh [off-list ref]
Subject: RE: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END
enumerators

quoted

On 08/10/2021 21:45, Akhil Goyal wrote:

quoted

Remove *_LIST_END enumerators from asymmetric crypto lib to avoid
ABI breakage for every new addition in enums.

Signed-off-by: Akhil Goyal <redacted>
---
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

quoted

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)

quoted

 		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++) {

quoted

 			if (tc.modex.xform_type ==

RTE_CRYPTO_ASYM_XFORM_RSA) {

quoted

 				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++) {

quoted

 		/* check supported operations */
 		if

(rte_cryptodev_asym_xform_capability_check_optype(capa, i))

quoted

 			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,

quoted

 			err = -EINVAL;
 			goto error;
 		}
-	} else if (xform->xform_type >=

RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END

quoted

+	} else if (xform->xform_type > RTE_CRYPTO_ASYM_XFORM_ECPM
 			|| xform->xform_type <=

RTE_CRYPTO_ASYM_XFORM_NONE) {

quoted

 		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

quoted

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.

quoted

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.

quoted

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.

[Anoob] Having LIST_END would preclude new additions to asymmetric algos? If yes, then I would suggest we address it now.

Not at all - but it can be problematic, if two versions of DPDK disagree with the value of LIST_END.

Looking at the "problematic changes", we only have 2-3 application & PMD changes. For unit test application, we could may be do something like,

The essental functionality not that different, I am just not sure that the verbosity below is helping. 
What you are really trying to guard against is people using LIST_END to size arrays.

-               for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
+               enum rte_crypto_asym_op_type types[] = {
+                               RTE_CRYPTO_ASYM_OP_ENCRYPT,
+                               RTE_CRYPTO_ASYM_OP_DECRYPT,
+                               RTE_CRYPTO_ASYM_OP_SIGN,
+                               RTE_CRYPTO_ASYM_OP_VERIFY,
+                               RTE_CRYPTO_ASYM_OP_PRIVATE_KEY_GENERATE,
+                               RTE_CRYPTO_ASYM_OP_PUBLIC_KEY_GENERATE,
+                               RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
+               };
+               for (i = 0; i <= RTE_DIM(types); i++) {
                        if (tc.modex.xform_type == RTE_CRYPTO_ASYM_XFORM_RSA) {
-                               if (tc.rsa_data.op_type_flags & (1 << i)) {
+                               if (tc.rsa_data.op_type_flags & (1 << types[i])) {
                                        if (tc.rsa_data.key_exp) {
                                                status = test_cryptodev_asym_op(
                                                        &testsuite_params, &tc,
-                                                       test_msg, sessionless, i,
+                                                       test_msg, sessionless, types[i],
                                                        RTE_RSA_KEY_TYPE_EXP);
                                        }
                                        if (status)
                                                break;
-                                       if (tc.rsa_data.key_qt && (i ==
+                                       if (tc.rsa_data.key_qt && (types[i] ==
                                                        RTE_CRYPTO_ASYM_OP_DECRYPT ||
-                                                       i == RTE_CRYPTO_ASYM_OP_SIGN)) {
+                                                       types[i] == RTE_CRYPTO_ASYM_OP_SIGN)) {
                                                status = test_cryptodev_asym_op(
                                                        &testsuite_params,
-                                                       &tc, test_msg, sessionless, i,
+                                                       &tc, test_msg, sessionless, types[i],
                                                        RTE_RSA_KET_TYPE_QT);
                                        }
                                        if (status)

This way, application would only use the ones which it is designed to work with. For QAT driver changes, we could have an overload if condition (if alg == x || alg = y || ...) to get the same effect.