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