Hi Thomas, Please see inline.
Thanks, Anoob > -----Original Message----- > From: Thomas Monjalon <[email protected]> > Sent: Tuesday, October 12, 2021 7:25 PM > To: Kinsella, Ray <[email protected]>; Akhil Goyal <[email protected]>; > [email protected]; Anoob Joseph <[email protected]> > Cc: [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; Nagadheeraj Rottela > <[email protected]>; Ankur Dwivedi <[email protected]>; > [email protected]; Stephen Hemminger <[email protected]>; > Yigit, Ferruh <[email protected]>; [email protected] > 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 <[email protected]> > > > 12/10/2021 13:34, Anoob Joseph: > > > > From: Kinsella, Ray <[email protected]> > > > > > On 12/10/2021 11:50, Anoob Joseph wrote: > > > > > > From: Akhil Goyal <[email protected]> > > > > > >>> 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 <[email protected]> > > > > > >>>> --- > > > > > >>>> - } 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. > >
