[v2,1/3] cryptodev: remove LIST_END enumerators
Checks
Commit Message
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(-)
Comments
> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Friday, October 8, 2021 9:45 PM
> To: dev@dpdk.org
> Cc: thomas@monjalon.net; david.marchand@redhat.com;
> hemant.agrawal@nxp.com; anoobj@marvell.com; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Trahe, Fiona <fiona.trahe@intel.com>;
> Doherty, Declan <declan.doherty@intel.com>; matan@nvidia.com;
> g.singh@nxp.com; Zhang, Roy Fan <roy.fan.zhang@intel.com>;
> jianjay.zhou@huawei.com; asomalap@amd.com; ruifeng.wang@arm.com;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>; Nicolau, Radu
> <radu.nicolau@intel.com>; ajit.khaparde@broadcom.com;
> rnagadheeraj@marvell.com; adwivedi@marvell.com; Power, Ciara
> <ciara.power@intel.com>; Akhil Goyal <gakhil@marvell.com>
> Subject: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators
>
> 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>
> ---
Acked-by: Fan Zhang <roy.fan.zhang@intel.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?
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.
>
> 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.
Hi Ray, Akhil,
Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Tuesday, October 12, 2021 3:49 PM
> To: Kinsella, Ray <mdr@ashroe.eu>; dev@dpdk.org
> Cc: thomas@monjalon.net; david.marchand@redhat.com;
> hemant.agrawal@nxp.com; Anoob Joseph <anoobj@marvell.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
> <rnagadheeraj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>;
> ciara.power@intel.com; Stephen Hemminger <stephen@networkplumber.org>;
> Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: RE: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END
> enumerators
>
> >
> > 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.
[Anoob] Having LIST_END would preclude new additions to asymmetric algos? If yes, then I would suggest we address it now.
Looking at the "problematic changes", we only have 2-3 application & PMD changes. For unit test application, we could may be do something like,
- 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.
On 12/10/2021 11:50, Anoob Joseph wrote:
> Hi Ray, Akhil,
>
> Please see inline.
>
> Thanks,
> Anoob
>
>> -----Original Message-----
>> From: Akhil Goyal <gakhil@marvell.com>
>> Sent: Tuesday, October 12, 2021 3:49 PM
>> To: Kinsella, Ray <mdr@ashroe.eu>; dev@dpdk.org
>> Cc: thomas@monjalon.net; david.marchand@redhat.com;
>> hemant.agrawal@nxp.com; Anoob Joseph <anoobj@marvell.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
>> <rnagadheeraj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>;
>> ciara.power@intel.com; Stephen Hemminger <stephen@networkplumber.org>;
>> Yigit, Ferruh <ferruh.yigit@intel.com>
>> Subject: RE: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END
>> enumerators
>>
>>>
>>> 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.
>
> [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.
>
Hi Ray,
Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Kinsella, Ray <mdr@ashroe.eu>
> Sent: Tuesday, October 12, 2021 4:58 PM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal <gakhil@marvell.com>;
> dev@dpdk.org
> Cc: thomas@monjalon.net; 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
> <rnagadheeraj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>;
> ciara.power@intel.com; Stephen Hemminger <stephen@networkplumber.org>;
> Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END
> enumerators
>
>
>
> On 12/10/2021 11:50, Anoob Joseph wrote:
> > Hi Ray, Akhil,
> >
> > Please see inline.
> >
> > Thanks,
> > Anoob
> >
> >> -----Original Message-----
> >> From: Akhil Goyal <gakhil@marvell.com>
> >> Sent: Tuesday, October 12, 2021 3:49 PM
> >> To: Kinsella, Ray <mdr@ashroe.eu>; dev@dpdk.org
> >> Cc: thomas@monjalon.net; david.marchand@redhat.com;
> >> hemant.agrawal@nxp.com; Anoob Joseph <anoobj@marvell.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 <rnagadheeraj@marvell.com>; Ankur Dwivedi
> >> <adwivedi@marvell.com>; ciara.power@intel.com; Stephen Hemminger
> >> <stephen@networkplumber.org>; Yigit, Ferruh <ferruh.yigit@intel.com>
> >> Subject: RE: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove
> >> LIST_END enumerators
> >>
> >>>
> >>> 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.
> >
> > [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.
[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?
>
> >
> > - 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.
> >
12/10/2021 13:34, Anoob Joseph:
> From: Kinsella, Ray <mdr@ashroe.eu>
> > On 12/10/2021 11:50, Anoob Joseph wrote:
> > > From: 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>
> > >>>> ---
> > >>>> - } else if (xform->xform_type >=
> > >>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > >>>> + } else if (xform->xform_type > RTE_CRYPTO_ASYM_XFORM_ECPM
[...]
> > >>>
> > >>> So I am not sure that this is an improvement.
Indeed, it is not 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.
> > >
> > > [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.
>
> [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?
The problem is not the application.
Are you asking the app to define DPDK types?
The problem is in DPDK API. We must not suggest a size for enums.
If we really need a size, then it must be explicit and updated in the lib binary
(through a function) when the size increases.
> > > - 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.
Hi Thomas,
Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, October 12, 2021 5:22 PM
> To: Kinsella, Ray <mdr@ashroe.eu>; Akhil Goyal <gakhil@marvell.com>;
> dev@dpdk.org; Anoob Joseph <anoobj@marvell.com>
> 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
> <rnagadheeraj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>;
> ciara.power@intel.com; Stephen Hemminger <stephen@networkplumber.org>;
> Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END
> enumerators
>
> 12/10/2021 13:34, Anoob Joseph:
> > From: Kinsella, Ray <mdr@ashroe.eu>
> > > On 12/10/2021 11:50, Anoob Joseph wrote:
> > > > From: 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>
> > > >>>> ---
> > > >>>> - } else if (xform->xform_type >=
> > > >>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > > >>>> + } else if (xform->xform_type >
> RTE_CRYPTO_ASYM_XFORM_ECPM
> [...]
> > > >>>
> > > >>> So I am not sure that this is an improvement.
>
> Indeed, it is not 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.
> > > >
> > > > [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.
> >
> > [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?
>
> The problem is not the application.
> Are you asking the app to define DPDK types?
[Anoob] I didn't understand how you concluded that. The app is supposed to test "n" asymmetric features supported by DPDK. 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?
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.
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.
> > > > + enum rte_crypto_asym_op_type types[] = {
>
> The problem is in DPDK API. We must not suggest a size for enums.
[Anoob] So agreed that LIST_END should be removed?
> If we really need a size, then it must be explicit and updated in the lib binary
> (through a function) when the size increases.
[Anoob] Precisely my thoughts. The loop with LIST_END done in application is not correct.
>
>
>
> > > > - 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.
>
>
>
12/10/2021 15:38, Anoob Joseph:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 12/10/2021 13:34, Anoob Joseph:
> > > From: Kinsella, Ray <mdr@ashroe.eu>
> > > > On 12/10/2021 11:50, Anoob Joseph wrote:
> > > > > From: 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>
> > > > >>>> ---
> > > > >>>> - } else if (xform->xform_type >=
> > > > >>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > > > >>>> + } else if (xform->xform_type >
> > RTE_CRYPTO_ASYM_XFORM_ECPM
> > [...]
> > > > >>>
> > > > >>> So I am not sure that this is an improvement.
> >
> > Indeed, it is not 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.
> > > > >
> > > > > [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.
> > >
> > > [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?
> >
> > 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.
> The app is supposed to test "n" asymmetric features supported by DPDK. 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
> 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.
>
> 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.
> > > > > + enum rte_crypto_asym_op_type types[] = {
>
> >
> > The problem is in DPDK API. We must not suggest a size for enums.
>
> [Anoob] So agreed that LIST_END should be removed?
Yes
> > If we really need a size, then it must be explicit and updated in the lib binary
> > (through a function) when the size increases.
>
> [Anoob] Precisely my thoughts. The loop with LIST_END done in application is not correct.
> >
> >
> >
> > > > > - 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.
Hi Thomas,
Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, October 12, 2021 7:25 PM
> To: Kinsella, Ray <mdr@ashroe.eu>; Akhil Goyal <gakhil@marvell.com>;
> dev@dpdk.org; Anoob Joseph <anoobj@marvell.com>
> 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
> <rnagadheeraj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>;
> ciara.power@intel.com; Stephen Hemminger <stephen@networkplumber.org>;
> Yigit, Ferruh <ferruh.yigit@intel.com>; bruce.richardson@intel.com
> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END
> enumerators
>
> 12/10/2021 15:38, Anoob Joseph:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 12/10/2021 13:34, Anoob Joseph:
> > > > From: Kinsella, Ray <mdr@ashroe.eu>
> > > > > On 12/10/2021 11:50, Anoob Joseph wrote:
> > > > > > From: 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>
> > > > > >>>> ---
> > > > > >>>> - } else if (xform->xform_type >=
> > > > > >>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > > > > >>>> + } else if (xform->xform_type >
> > > RTE_CRYPTO_ASYM_XFORM_ECPM
> > > [...]
> > > > > >>>
> > > > > >>> So I am not sure that this is an improvement.
> > >
> > > Indeed, it is not 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.
> > > > > >
> > > > > > [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.
> > > >
> > > > [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?
> > >
> > > 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.
>
> > The app is supposed to test "n" asymmetric features supported by DPDK.
> 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
>
> > 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.
> >
> > 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?
>
> > > > > > + enum rte_crypto_asym_op_type types[] = {
> >
> > >
> > > The problem is in DPDK API. We must not suggest a size for enums.
> >
> > [Anoob] So agreed that LIST_END should be removed?
>
> Yes
>
> > > If we really need a size, then it must be explicit and updated in
> > > the lib binary (through a function) when the size increases.
> >
> > [Anoob] Precisely my thoughts. The loop with LIST_END done in application is
> not correct.
> > >
> > >
> > >
> > > > > > - 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.
>
>
On 12/10/2021 15:18, Anoob Joseph wrote:
> Hi Thomas,
>
> Please see inline.
>
> Thanks,
> Anoob
>
>> -----Original Message-----
>> From: Thomas Monjalon <thomas@monjalon.net>
>> Sent: Tuesday, October 12, 2021 7:25 PM
>> To: Kinsella, Ray <mdr@ashroe.eu>; Akhil Goyal <gakhil@marvell.com>;
>> dev@dpdk.org; Anoob Joseph <anoobj@marvell.com>
>> 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
>> <rnagadheeraj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>;
>> ciara.power@intel.com; Stephen Hemminger <stephen@networkplumber.org>;
>> Yigit, Ferruh <ferruh.yigit@intel.com>; bruce.richardson@intel.com
>> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END
>> enumerators
>>
>> 12/10/2021 15:38, Anoob Joseph:
>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>> 12/10/2021 13:34, Anoob Joseph:
>>>>> From: Kinsella, Ray <mdr@ashroe.eu>
>>>>>> On 12/10/2021 11:50, Anoob Joseph wrote:
>>>>>>> From: 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>
>>>>>>>>>> ---
>>>>>>>>>> - } else if (xform->xform_type >=
>>>>>>>>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
>>>>>>>>>> + } else if (xform->xform_type >
>>>> RTE_CRYPTO_ASYM_XFORM_ECPM
>>>> [...]
>>>>>>>>>
>>>>>>>>> So I am not sure that this is an improvement.
>>>>
>>>> Indeed, it is not 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.
>>>>>>>
>>>>>>> [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.
>>>>>
>>>>> [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?
>>>>
>>>> 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.
>>
>>> The app is supposed to test "n" asymmetric features supported by DPDK.
>> 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
>>
>>> 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.
>>>
>>> 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?
1 API returning a single "info" structure perhaps - as being the most extensible?
>
>>
>>>>>>> + enum rte_crypto_asym_op_type types[] = {
>>>
>>>>
>>>> The problem is in DPDK API. We must not suggest a size for enums.
>>>
>>> [Anoob] So agreed that LIST_END should be removed?
>>
>> Yes
>>
>>>> If we really need a size, then it must be explicit and updated in
>>>> the lib binary (through a function) when the size increases.
>>>
>>> [Anoob] Precisely my thoughts. The loop with LIST_END done in application is
>> not correct.
>>>>
>>>>
>>>>
>>>>>>> - 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.
>>
>>
>
12/10/2021 16:47, Kinsella, Ray:
> On 12/10/2021 15:18, Anoob Joseph wrote:
> > From: Thomas Monjalon <thomas@monjalon.net>
> >> 12/10/2021 15:38, Anoob Joseph:
> >>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>> 12/10/2021 13:34, Anoob Joseph:
> >>>>> From: Kinsella, Ray <mdr@ashroe.eu>
> >>>>>> On 12/10/2021 11:50, Anoob Joseph wrote:
> >>>>>>> From: 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>
> >>>>>>>>>> ---
> >>>>>>>>>> - } else if (xform->xform_type >=
> >>>>>>>>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> >>>>>>>>>> + } else if (xform->xform_type >
> >>>> RTE_CRYPTO_ASYM_XFORM_ECPM
> >>>> [...]
> >>>>>>>>>
> >>>>>>>>> So I am not sure that this is an improvement.
> >>>>
> >>>> Indeed, it is not 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.
> >>>>>>>
> >>>>>>> [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.
> >>>>>
> >>>>> [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?
> >>>>
> >>>> 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.
> >>
> >>> The app is supposed to test "n" asymmetric features supported by DPDK.
> >> 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
> >>
> >>> 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.
> >>>
> >>> 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?
>
> 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.
Feel free to consider the alternative which fits the best in cryptodev.
Hi Thomas, Ray,
Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, October 12, 2021 8:37 PM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal
> <gakhil@marvell.com>; dev@dpdk.org; Kinsella, Ray <mdr@ashroe.eu>
> 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 <rnagadheeraj@marvell.com>; Ankur Dwivedi
> <adwivedi@marvell.com>; ciara.power@intel.com; Stephen Hemminger
> <stephen@networkplumber.org>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> 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:
> > On 12/10/2021 15:18, Anoob Joseph wrote:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > >> 12/10/2021 15:38, Anoob Joseph:
> > >>> From: Thomas Monjalon <thomas@monjalon.net>
> > >>>> 12/10/2021 13:34, Anoob Joseph:
> > >>>>> From: Kinsella, Ray <mdr@ashroe.eu>
> > >>>>>> On 12/10/2021 11:50, Anoob Joseph wrote:
> > >>>>>>> From: 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>
> > >>>>>>>>>> ---
> > >>>>>>>>>> - } else if (xform->xform_type >=
> > >>>>>>>>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > >>>>>>>>>> + } else if (xform->xform_type >
> > >>>> RTE_CRYPTO_ASYM_XFORM_ECPM
> > >>>> [...]
> > >>>>>>>>>
> > >>>>>>>>> So I am not sure that this is an improvement.
> > >>>>
> > >>>> Indeed, it is not 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.
> > >>>>>>>
> > >>>>>>> [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.
> > >>>>>
> > >>>>> [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?
> > >>>>
> > >>>> 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.
> > >>
> > >>> The app is supposed to test "n" asymmetric features supported by
> DPDK.
> > >> 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
> > >>
> > >>> 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.
> > >>>
> > >>> 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?
> >
> > 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.
*/
13/10/2021 07:36, Anoob Joseph:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 12/10/2021 16:47, Kinsella, Ray:
> > > On 12/10/2021 15:18, Anoob Joseph wrote:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > >> 12/10/2021 15:38, Anoob Joseph:
> > > >>> From: Thomas Monjalon <thomas@monjalon.net>
> > > >>>> 12/10/2021 13:34, Anoob Joseph:
> > > >>>>> From: Kinsella, Ray <mdr@ashroe.eu>
> > > >>>>>> On 12/10/2021 11:50, Anoob Joseph wrote:
> > > >>>>>>> From: 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>
> > > >>>>>>>>>> ---
> > > >>>>>>>>>> - } else if (xform->xform_type >=
> > > >>>>>>>>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > > >>>>>>>>>> + } else if (xform->xform_type >
> > > >>>> RTE_CRYPTO_ASYM_XFORM_ECPM
> > > >>>> [...]
> > > >>>>>>>>>
> > > >>>>>>>>> So I am not sure that this is an improvement.
> > > >>>>
> > > >>>> Indeed, it is not 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.
> > > >>>>>>>
> > > >>>>>>> [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.
> > > >>>>>
> > > >>>>> [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?
> > > >>>>
> > > >>>> 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.
> > > >>
> > > >>> The app is supposed to test "n" asymmetric features supported by
> > DPDK.
> > > >> 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
> > > >>
> > > >>> 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.
> > > >>>
> > > >>> 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?
> > >
> > > 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++)
You must not use enum values in the .h, otherwise ABI compatibility is not ensured.
Yes you can do a macro, but it must call functions, not using direct values.
Hi Akhil, Ray, Thomas,
Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, October 13, 2021 12:32 PM
> To: Akhil Goyal <gakhil@marvell.com>; dev@dpdk.org; Kinsella, Ray
> <mdr@ashroe.eu>; Anoob Joseph <anoobj@marvell.com>
> 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 <rnagadheeraj@marvell.com>; Ankur Dwivedi
> <adwivedi@marvell.com>; ciara.power@intel.com; Stephen Hemminger
> <stephen@networkplumber.org>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> bruce.richardson@intel.com
> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove
> LIST_END enumerators
>
> 13/10/2021 07:36, Anoob Joseph:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 12/10/2021 16:47, Kinsella, Ray:
> > > > On 12/10/2021 15:18, Anoob Joseph wrote:
> > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > >> 12/10/2021 15:38, Anoob Joseph:
> > > > >>> From: Thomas Monjalon <thomas@monjalon.net>
> > > > >>>> 12/10/2021 13:34, Anoob Joseph:
> > > > >>>>> From: Kinsella, Ray <mdr@ashroe.eu>
> > > > >>>>>> On 12/10/2021 11:50, Anoob Joseph wrote:
> > > > >>>>>>> From: 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>
> > > > >>>>>>>>>> ---
> > > > >>>>>>>>>> - } else if (xform->xform_type >=
> > > > >>>>>>>>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > > > >>>>>>>>>> + } else if (xform->xform_type >
> > > > >>>> RTE_CRYPTO_ASYM_XFORM_ECPM
> > > > >>>> [...]
> > > > >>>>>>>>>
> > > > >>>>>>>>> So I am not sure that this is an improvement.
> > > > >>>>
> > > > >>>> Indeed, it is not 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.
> > > > >>>>>>>
> > > > >>>>>>> [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.
> > > > >>>>>
> > > > >>>>> [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?
> > > > >>>>
> > > > >>>> 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.
> > > > >>
> > > > >>> The app is supposed to test "n" asymmetric features supported
> > > > >>> by
> > > DPDK.
> > > > >> 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
> > > > >>
> > > > >>> 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.
> > > > >>>
> > > > >>> 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?
> > > >
> > > > 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++)
>
> You must not use enum values in the .h, otherwise ABI compatibility is not
> ensured.
> Yes you can do a macro, but it must call functions, not using direct values.
>
[Anoob] Understood. Will do that.
@Ray, @Akhil, you are also in agreement, right?
On 13/10/2021 08:04, Anoob Joseph wrote:
> Hi Akhil, Ray, Thomas,
>
> Please see inline.
>
> Thanks,
> Anoob
>
>> -----Original Message-----
>> From: Thomas Monjalon <thomas@monjalon.net>
>> Sent: Wednesday, October 13, 2021 12:32 PM
>> To: Akhil Goyal <gakhil@marvell.com>; dev@dpdk.org; Kinsella, Ray
>> <mdr@ashroe.eu>; Anoob Joseph <anoobj@marvell.com>
>> 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 <rnagadheeraj@marvell.com>; Ankur Dwivedi
>> <adwivedi@marvell.com>; ciara.power@intel.com; Stephen Hemminger
>> <stephen@networkplumber.org>; Yigit, Ferruh <ferruh.yigit@intel.com>;
>> bruce.richardson@intel.com
>> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove
>> LIST_END enumerators
>>
>> 13/10/2021 07:36, Anoob Joseph:
>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>> 12/10/2021 16:47, Kinsella, Ray:
>>>>> On 12/10/2021 15:18, Anoob Joseph wrote:
>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>> 12/10/2021 15:38, Anoob Joseph:
>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>>>> 12/10/2021 13:34, Anoob Joseph:
>>>>>>>>>> From: Kinsella, Ray <mdr@ashroe.eu>
>>>>>>>>>>> On 12/10/2021 11:50, Anoob Joseph wrote:
>>>>>>>>>>>> From: 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>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>> - } else if (xform->xform_type >=
>>>>>>>>>>>>>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
>>>>>>>>>>>>>>> + } else if (xform->xform_type >
>>>>>>>>> RTE_CRYPTO_ASYM_XFORM_ECPM
>>>>>>>>> [...]
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So I am not sure that this is an improvement.
>>>>>>>>>
>>>>>>>>> Indeed, it is not 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.
>>>>>>>>>>>>
>>>>>>>>>>>> [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.
>>>>>>>>>>
>>>>>>>>>> [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?
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>
>>>>>>>> The app is supposed to test "n" asymmetric features supported
>>>>>>>> by
>>>> DPDK.
>>>>>>> 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
>>>>>>>
>>>>>>>> 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.
>>>>>>>>
>>>>>>>> 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?
>>>>>
>>>>> 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++)
>>
>> You must not use enum values in the .h, otherwise ABI compatibility is not
>> ensured.
>> Yes you can do a macro, but it must call functions, not using direct values.
>>
>
> [Anoob] Understood. Will do that.
>
> @Ray, @Akhil, you are also in agreement, right?
>
Yes - whether you use the MACRO or not less important.
In order to maintain the ABI ... you need to learn the array size through an API.
@@ -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",
@@ -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;
@@ -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
};
/**