Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

["Kinsella, Ray" <mdr@ashroe.eu>]

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