DPDK patches and discussions
 help / color / mirror / Atom feed
From: Anoob Joseph <anoobj@marvell.com>
To: Akhil Goyal <gakhil@marvell.com>, "Kinsella, Ray" <mdr@ashroe.eu>,
	"dev@dpdk.org" <dev@dpdk.org>
Cc: "thomas@monjalon.net" <thomas@monjalon.net>,
	"david.marchand@redhat.com" <david.marchand@redhat.com>,
	"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
	"pablo.de.lara.guarch@intel.com" <pablo.de.lara.guarch@intel.com>,
	"fiona.trahe@intel.com" <fiona.trahe@intel.com>,
	"declan.doherty@intel.com" <declan.doherty@intel.com>,
	"matan@nvidia.com" <matan@nvidia.com>,
	"g.singh@nxp.com" <g.singh@nxp.com>,
	"roy.fan.zhang@intel.com" <roy.fan.zhang@intel.com>,
	"jianjay.zhou@huawei.com" <jianjay.zhou@huawei.com>,
	"asomalap@amd.com" <asomalap@amd.com>,
	"ruifeng.wang@arm.com" <ruifeng.wang@arm.com>,
	"konstantin.ananyev@intel.com" <konstantin.ananyev@intel.com>,
	"radu.nicolau@intel.com" <radu.nicolau@intel.com>,
	"ajit.khaparde@broadcom.com" <ajit.khaparde@broadcom.com>,
	Nagadheeraj Rottela <rnagadheeraj@marvell.com>,
	Ankur Dwivedi <adwivedi@marvell.com>,
	"ciara.power@intel.com" <ciara.power@intel.com>,
	Stephen Hemminger <stephen@networkplumber.org>,
	"Yigit, Ferruh" <ferruh.yigit@intel.com>
Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators
Date: Tue, 12 Oct 2021 10:50:31 +0000	[thread overview]
Message-ID: <PH0PR18MB4672ECCDD02A55E3E80554F7DFB69@PH0PR18MB4672.namprd18.prod.outlook.com> (raw)
In-Reply-To: <CO6PR18MB448451C3CA8C1DCEC79CEEA7D8B69@CO6PR18MB4484.namprd18.prod.outlook.com>

Hi Ray, Akhil,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Tuesday, October 12, 2021 3:49 PM
> To: Kinsella, Ray <mdr@ashroe.eu>; dev@dpdk.org
> Cc: thomas@monjalon.net; david.marchand@redhat.com;
> hemant.agrawal@nxp.com; Anoob Joseph <anoobj@marvell.com>;
> pablo.de.lara.guarch@intel.com; fiona.trahe@intel.com;
> declan.doherty@intel.com; matan@nvidia.com; g.singh@nxp.com;
> roy.fan.zhang@intel.com; jianjay.zhou@huawei.com; asomalap@amd.com;
> ruifeng.wang@arm.com; konstantin.ananyev@intel.com;
> radu.nicolau@intel.com; ajit.khaparde@broadcom.com; Nagadheeraj Rottela
> <rnagadheeraj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>;
> ciara.power@intel.com; Stephen Hemminger <stephen@networkplumber.org>;
> Yigit, Ferruh <ferruh.yigit@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 <gakhil@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. 


  reply	other threads:[~2021-10-12 10:50 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-31 18:13 [dpdk-dev] [PATCH 0/4] cryptodev and security ABI improvements Akhil Goyal
2021-07-31 18:13 ` [dpdk-dev] [PATCH 1/4] cryptodev: remove LIST_END enumerators Akhil Goyal
2021-07-31 18:13 ` [dpdk-dev] [PATCH 2/4] cryptodev: promote asym APIs to stable Akhil Goyal
2021-08-30 15:49   ` Kusztal, ArkadiuszX
2021-09-03 15:17     ` Akhil Goyal
2021-09-07 11:42       ` Kusztal, ArkadiuszX
2021-09-07 11:45         ` Akhil Goyal
2021-09-08 12:37           ` Kinsella, Ray
2023-02-02 10:49             ` [EXT] " Akhil Goyal
2023-02-02 11:02               ` Hemant Agrawal
2023-02-14 18:05               ` Kusztal, ArkadiuszX
2021-07-31 18:13 ` [dpdk-dev] [PATCH 3/4] security: hide internal API Akhil Goyal
2021-09-15 15:54   ` Ananyev, Konstantin
2021-07-31 18:13 ` [dpdk-dev] [PATCH 4/4] security: add reserved bitfields Akhil Goyal
2021-09-15 15:55   ` Ananyev, Konstantin
2021-09-15 16:43   ` Stephen Hemminger
2021-07-31 18:17 ` [dpdk-dev] [PATCH 0/4] cryptodev and security ABI improvements Akhil Goyal
2021-10-08 20:45 ` [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END enumerators Akhil Goyal
2021-10-08 20:45   ` [dpdk-dev] [PATCH v2 2/3] security: hide internal API Akhil Goyal
2021-10-12  8:50     ` Kinsella, Ray
2021-10-08 20:45   ` [dpdk-dev] [PATCH v2 3/3] security: add reserved bitfields Akhil Goyal
2021-10-11  8:31     ` Thomas Monjalon
2021-10-11 16:58       ` [dpdk-dev] [EXT] " Akhil Goyal
2021-10-11 22:15         ` Stephen Hemminger
2021-10-12  8:31           ` Kinsella, Ray
2021-10-12  6:59         ` Thomas Monjalon
2021-10-12  8:53           ` Kinsella, Ray
2021-10-12  8:50     ` [dpdk-dev] " Kinsella, Ray
2021-10-11 10:46   ` [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END enumerators Zhang, Roy Fan
2021-10-12  9:55   ` Kinsella, Ray
2021-10-12 10:19     ` [dpdk-dev] [EXT] " Akhil Goyal
2021-10-12 10:50       ` Anoob Joseph [this message]
2021-10-12 11:28         ` Kinsella, Ray
2021-10-12 11:34           ` Anoob Joseph
2021-10-12 11:52             ` Thomas Monjalon
2021-10-12 13:38               ` Anoob Joseph
2021-10-12 13:54                 ` Thomas Monjalon
2021-10-12 14:18                   ` Anoob Joseph
2021-10-12 14:47                     ` Kinsella, Ray
2021-10-12 15:06                       ` Thomas Monjalon
2021-10-13  5:36                         ` Anoob Joseph
2021-10-13  7:02                           ` Thomas Monjalon
2021-10-13  7:04                             ` Anoob Joseph
2021-10-13  8:39                               ` Kinsella, Ray
2021-10-18  5:22   ` [dpdk-dev] [PATCH v3 1/2] security: hide internal API Akhil Goyal
2021-10-18  5:22     ` [dpdk-dev] [PATCH v3 2/2] security: add reserved bitfields Akhil Goyal
2021-10-18 15:39       ` Akhil Goyal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=PH0PR18MB4672ECCDD02A55E3E80554F7DFB69@PH0PR18MB4672.namprd18.prod.outlook.com \
    --to=anoobj@marvell.com \
    --cc=adwivedi@marvell.com \
    --cc=ajit.khaparde@broadcom.com \
    --cc=asomalap@amd.com \
    --cc=ciara.power@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=declan.doherty@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=fiona.trahe@intel.com \
    --cc=g.singh@nxp.com \
    --cc=gakhil@marvell.com \
    --cc=hemant.agrawal@nxp.com \
    --cc=jianjay.zhou@huawei.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=matan@nvidia.com \
    --cc=mdr@ashroe.eu \
    --cc=pablo.de.lara.guarch@intel.com \
    --cc=radu.nicolau@intel.com \
    --cc=rnagadheeraj@marvell.com \
    --cc=roy.fan.zhang@intel.com \
    --cc=ruifeng.wang@arm.com \
    --cc=stephen@networkplumber.org \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).