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

[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?

The problem is in DPDK API. We must not suggest a size for enums.
If we really need a size, then it must be explicit and updated in the lib binary
(through a function) when the size increases.



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