Hi Ray,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Kinsella, Ray <m...@ashroe.eu>
> Sent: Tuesday, October 12, 2021 4:58 PM
> To: Anoob Joseph <ano...@marvell.com>; Akhil Goyal <gak...@marvell.com>;
> dev@dpdk.org
> Cc: tho...@monjalon.net; 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>
> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END
> enumerators
> 
> 
> 
> On 12/10/2021 11:50, Anoob Joseph wrote:
> > Hi Ray, Akhil,
> >
> > Please see inline.
> >
> > Thanks,
> > Anoob
> >
> >> -----Original Message-----
> >> From: Akhil Goyal <gak...@marvell.com>
> >> Sent: Tuesday, October 12, 2021 3:49 PM
> >> To: Kinsella, Ray <m...@ashroe.eu>; dev@dpdk.org
> >> Cc: tho...@monjalon.net; david.march...@redhat.com;
> >> hemant.agra...@nxp.com; Anoob Joseph <ano...@marvell.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>
> >> Subject: RE: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove
> >> LIST_END enumerators
> >>
> >>>
> >>> 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>
> >>>> ---
> >>>> v2: no change
> >>>>
> >>>>  app/test/test_cryptodev_asym.c  | 4 ++--
> >>>>  drivers/crypto/qat/qat_asym.c   | 2 +-
> >>>>  lib/cryptodev/rte_crypto_asym.h | 4 ----
> >>>>  3 files changed, 3 insertions(+), 7 deletions(-)
> >>>>
> >>>> diff --git a/app/test/test_cryptodev_asym.c
> >>> b/app/test/test_cryptodev_asym.c
> >>>> index 9d19a6d6d9..603b2e4609 100644
> >>>> --- a/app/test/test_cryptodev_asym.c
> >>>> +++ b/app/test/test_cryptodev_asym.c
> >>>> @@ -541,7 +541,7 @@ test_one_case(const void *test_case, int
> >>> sessionless)
> >>>>                  printf("  %u) TestCase %s %s\n", test_index++,
> >>>>                          tc.modex.description, test_msg);
> >>>>          } else {
> >>>> -                for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> >>>> +                for (i = 0; i <=
> >>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE; i++) {
> >>>>                          if (tc.modex.xform_type ==
> >>> RTE_CRYPTO_ASYM_XFORM_RSA) {
> >>>>                                  if (tc.rsa_data.op_type_flags & (1 << 
> >>>> i)) {
> >>>>                                          if (tc.rsa_data.key_exp) {
> >>>> @@ -1027,7 +1027,7 @@ static inline void print_asym_capa(
> >>>>                          
> >>>> rte_crypto_asym_xform_strings[capa->xform_type]);
> >>>>          printf("operation supported -");
> >>>>
> >>>> -        for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> >>>> +        for (i = 0; i <= RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE;
> >>> i++) {
> >>>>                  /* check supported operations */
> >>>>                  if
> >>> (rte_cryptodev_asym_xform_capability_check_optype(capa, i))
> >>>>                          printf(" %s",
> >>>> diff --git a/drivers/crypto/qat/qat_asym.c
> >>>> b/drivers/crypto/qat/qat_asym.c index 85973812a8..026625a4d2 100644
> >>>> --- a/drivers/crypto/qat/qat_asym.c
> >>>> +++ b/drivers/crypto/qat/qat_asym.c
> >>>> @@ -742,7 +742,7 @@ qat_asym_session_configure(struct rte_cryptodev
> >>> *dev,
> >>>>                          err = -EINVAL;
> >>>>                          goto error;
> >>>>                  }
> >>>> -        } else if (xform->xform_type >=
> >>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> >>>> +        } else if (xform->xform_type > RTE_CRYPTO_ASYM_XFORM_ECPM
> >>>>                          || xform->xform_type <=
> >>> RTE_CRYPTO_ASYM_XFORM_NONE) {
> >>>>                  QAT_LOG(ERR, "Invalid asymmetric crypto xform");
> >>>>                  err = -EINVAL;
> >>>> diff --git a/lib/cryptodev/rte_crypto_asym.h
> >>> b/lib/cryptodev/rte_crypto_asym.h
> >>>> index 9c866f553f..5edf658572 100644
> >>>> --- a/lib/cryptodev/rte_crypto_asym.h
> >>>> +++ b/lib/cryptodev/rte_crypto_asym.h
> >>>> @@ -94,8 +94,6 @@ enum rte_crypto_asym_xform_type {
> >>>>           */
> >>>>          RTE_CRYPTO_ASYM_XFORM_ECPM,
> >>>>          /**< Elliptic Curve Point Multiplication */
> >>>> -        RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> >>>> -        /**< End of list */
> >>>>  };
> >>>>
> >>>>  /**
> >>>> @@ -116,7 +114,6 @@ enum rte_crypto_asym_op_type {
> >>>>          /**< DH Public Key generation operation */
> >>>>          RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
> >>>>          /**< DH Shared Secret compute operation */
> >>>> -        RTE_CRYPTO_ASYM_OP_LIST_END
> >>>>  };
> >>>>
> >>>>  /**
> >>>> @@ -133,7 +130,6 @@ enum rte_crypto_rsa_padding_type {
> >>>>          /**< RSA PKCS#1 OAEP padding scheme */
> >>>>          RTE_CRYPTO_RSA_PADDING_PSS,
> >>>>          /**< RSA PKCS#1 PSS padding scheme */
> >>>> -        RTE_CRYPTO_RSA_PADDING_TYPE_LIST_END
> >>>>  };
> >>>>
> >>>>  /**
> >>>
> >>> So I am not sure that this is 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?
 
> 
> >
> > -               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.
> >

Reply via email to