From: Thomas Monjalon <thomas@monjalon.net>
To: "Kinsella, Ray" <mdr@ashroe.eu>, Akhil Goyal <gakhil@marvell.com>,
"dev@dpdk.org" <dev@dpdk.org>, Anoob Joseph <anoobj@marvell.com>
Cc: "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>,
bruce.richardson@intel.com
Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators
Date: Tue, 12 Oct 2021 15:54:44 +0200 [thread overview]
Message-ID: <2012129.4fn6nFIri4@thomas> (raw)
In-Reply-To: <PH0PR18MB467239ADF5F76C4EDEA4E5BCDFB69@PH0PR18MB4672.namprd18.prod.outlook.com>
12/10/2021 15:38, Anoob Joseph:
> From: 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?
>
> [Anoob] I didn't understand how you concluded that.
Because you define a specific array in the test app.
> The app is supposed to test "n" asymmetric features supported by DPDK. Currently, it does that by looping from 0 to LIST_END which happens to give you the first n features. Now, if we add any new asymmetric feature, LIST_END value would change. Isn't that the very reason why we removed LIST_END from symmetric library and applications?
Yes
> Now coming to what I proposed, the app is supposed to test "n" asymmetric features. LIST_END helps in doing the loops. If we remove LIST_END, then application will not be in a position to do a loop. My suggestion is, we list the types that are supposed to be tested by the app, and let that array be used as feature list.
>
> PS: Just to reiterate, my proposal is just a local array which would hold DPDK defined RTE enum values for the features that would be tested by this app/function.
I am more concerned by the general case than the test app.
I think a function returning a number is more app-friendly.
> > > > > + enum rte_crypto_asym_op_type types[] = {
>
> >
> > The problem is in DPDK API. We must not suggest a size for enums.
>
> [Anoob] So agreed that LIST_END should be removed?
Yes
> > If we really need a size, then it must be explicit and updated in the lib binary
> > (through a function) when the size increases.
>
> [Anoob] Precisely my thoughts. The loop with LIST_END done in application is not correct.
> >
> >
> >
> > > > > - 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.
next prev parent reply other threads:[~2021-10-12 13:54 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
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 [this message]
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=2012129.4fn6nFIri4@thomas \
--to=thomas@monjalon.net \
--cc=adwivedi@marvell.com \
--cc=ajit.khaparde@broadcom.com \
--cc=anoobj@marvell.com \
--cc=asomalap@amd.com \
--cc=bruce.richardson@intel.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 \
/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).