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.
Looking at the "problematic changes", we only have 2-3 application & PMD
changes. For unit test application, we could may be do something like,
- 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.
