Hi Ray, Please see inline.
Thanks, Anoob > -----Original Message----- > From: Kinsella, Ray <m...@ashroe.eu> > Sent: Tuesday, October 12, 2021 4:58 PM > To: Anoob Joseph <ano...@marvell.com>; Akhil Goyal <gak...@marvell.com>; > dev@dpdk.org > Cc: tho...@monjalon.net; david.march...@redhat.com; > hemant.agra...@nxp.com; pablo.de.lara.gua...@intel.com; > fiona.tr...@intel.com; declan.dohe...@intel.com; ma...@nvidia.com; > g.si...@nxp.com; roy.fan.zh...@intel.com; jianjay.z...@huawei.com; > asoma...@amd.com; ruifeng.w...@arm.com; konstantin.anan...@intel.com; > radu.nico...@intel.com; ajit.khapa...@broadcom.com; Nagadheeraj Rottela > <rnagadhee...@marvell.com>; Ankur Dwivedi <adwiv...@marvell.com>; > ciara.po...@intel.com; Stephen Hemminger <step...@networkplumber.org>; > Yigit, Ferruh <ferruh.yi...@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 <gak...@marvell.com> > >> Sent: Tuesday, October 12, 2021 3:49 PM > >> To: Kinsella, Ray <m...@ashroe.eu>; dev@dpdk.org > >> Cc: tho...@monjalon.net; david.march...@redhat.com; > >> hemant.agra...@nxp.com; Anoob Joseph <ano...@marvell.com>; > >> pablo.de.lara.gua...@intel.com; fiona.tr...@intel.com; > >> declan.dohe...@intel.com; ma...@nvidia.com; g.si...@nxp.com; > >> roy.fan.zh...@intel.com; jianjay.z...@huawei.com; asoma...@amd.com; > >> ruifeng.w...@arm.com; konstantin.anan...@intel.com; > >> radu.nico...@intel.com; ajit.khapa...@broadcom.com; Nagadheeraj > >> Rottela <rnagadhee...@marvell.com>; Ankur Dwivedi > >> <adwiv...@marvell.com>; ciara.po...@intel.com; Stephen Hemminger > >> <step...@networkplumber.org>; Yigit, Ferruh <ferruh.yi...@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 <gak...@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. > >