Hi Thomas, Ray,

Please see inline.

Thanks,
Anoob

-----Original Message-----
From: Thomas Monjalon <redacted>
Sent: Tuesday, October 12, 2021 8:37 PM
To: Anoob Joseph <redacted>; Akhil Goyal
[off-list ref]; dev@dpdk.org; Kinsella, Ray [off-list ref]
Cc: david.marchand@redhat.com; hemant.agrawal@nxp.com;
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];
bruce.richardson@intel.com
Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove
LIST_END enumerators

12/10/2021 16:47, Kinsella, Ray:

quoted

On 12/10/2021 15:18, Anoob Joseph wrote:

quoted

From: Thomas Monjalon <redacted>

quoted

12/10/2021 15:38, Anoob Joseph:

quoted

From: Thomas Monjalon <redacted>

quoted

12/10/2021 13:34, Anoob Joseph:

quoted

From: Kinsella, Ray <redacted>

quoted

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

quoted

From: Akhil Goyal <redacted>

quoted

quoted

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

quoted

Remove *_LIST_END enumerators from asymmetric crypto

lib to

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

avoid ABI breakage for every new addition in enums.

Signed-off-by: Akhil Goyal <redacted>
---
-	} else if (xform->xform_type >=

RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END

quoted

+	} else if (xform->xform_type >

RTE_CRYPTO_ASYM_XFORM_ECPM
[...]

quoted

quoted

quoted

quoted

quoted

So I am not sure that this is an improvement.

Indeed, it is not an improvement.

quoted

quoted

quoted

quoted

quoted

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?

quoted

quoted

Yes we did this same exercise for symmetric crypto enums

earlier.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

here.

quoted

My 2c is that from an ABI PoV

RTE_CRYPTO_ASYM_OP_LIST_END is

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

not better or worse, than

RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?

quoted

quoted

quoted

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?

quoted

quoted

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.

quoted

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.

[Anoob] Our problem is application using LIST_END (which comes
from library)

to determine the number of iterations for the loop. My suggestion
is to modify the UT such that, we could use RTE_DIM(types) (which
comes from application) to determine iterations of loop. This
would solve the

problem, right?

quoted

quoted

The problem is not the application.
Are you asking the app to define DPDK types?

[Anoob] I didn't understand how you concluded that.

Because you define a specific array in the test app.

quoted

The app is supposed to test "n" asymmetric features supported by

DPDK.

quoted

quoted

quoted

Currently, it does that by looping from 0 to LIST_END which happens
to give you the first n features. Now, if we add any new asymmetric
feature, LIST_END value would change. Isn't that the very reason
why we removed LIST_END from symmetric library and applications?

Yes

quoted

Now coming to what I proposed, the app is supposed to test "n"
asymmetric

features. LIST_END helps in doing the loops. If we remove LIST_END,
then application will not be in a position to do a loop. My
suggestion is, we list the types that are supposed to be tested by
the app, and let that array be used as feature list.

quoted

PS: Just to reiterate, my proposal is just a local array which
would hold DPDK

defined RTE enum values for the features that would be tested by
this app/function.

I am more concerned by the general case than the test app.
I think a function returning a number is more app-friendly.

[Anoob] Indeed. But there are 3 LIST_ENDs removed with this patch. Do

you propose 3 new APIs to just get max number?

quoted

1 API returning a single "info" structure perhaps - as being the most

extensible?

Or 3 iterators (foreach construct).
Instead of just returning a size, we can have an iterator for each enum which
needs to be iterated.

[Anoob] Something like this?

diff --git a/app/test/test_cryptodev_asym.c b/app/test/test_cryptodev_asym.c
index 847b074a4f..68a6197851 100644
--- a/app/test/test_cryptodev_asym.c
+++ b/app/test/test_cryptodev_asym.c

@@ -542,7 +542,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++) {
+               RTE_CRYPTO_ASYM_FOREACH_OP_TYPE(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) {

diff --git a/lib/cryptodev/rte_crypto_asym.h b/lib/cryptodev/rte_crypto_asym.h
index 9c866f553f..5627dcaff1 100644
--- a/lib/cryptodev/rte_crypto_asym.h
+++ b/lib/cryptodev/rte_crypto_asym.h

@@ -119,6 +119,11 @@ enum rte_crypto_asym_op_type {
        RTE_CRYPTO_ASYM_OP_LIST_END
 };

+#define RTE_CRYPTO_ASYM_FOREACH_OP_TYPE(i) \
+       for (i = RTE_CRYPTO_ASYM_OP_ENCRYPT; \
+            i <= RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE; \
+            i++)
+
 /**
  * Padding types for RSA signature.
  */