On 12/10/2021 15:18, Anoob Joseph wrote:
> Hi Thomas,
>
> Please see inline.
>
> Thanks,
> Anoob
>
>> -----Original Message-----
>> From: Thomas Monjalon <tho...@monjalon.net>
>> Sent: Tuesday, October 12, 2021 7:25 PM
>> To: Kinsella, Ray <m...@ashroe.eu>; Akhil Goyal <gak...@marvell.com>;
>> dev@dpdk.org; Anoob Joseph <ano...@marvell.com>
>> Cc: 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>; bruce.richard...@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 <tho...@monjalon.net>
>>>> 12/10/2021 13:34, Anoob Joseph:
>>>>> From: Kinsella, Ray <m...@ashroe.eu>
>>>>>> On 12/10/2021 11:50, Anoob Joseph wrote:
>>>>>>> From: Akhil Goyal <gak...@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 <gak...@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.
>>
>>
>
