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