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.
>
> - 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.
>
