From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2A244A0C47; Tue, 12 Oct 2021 15:54:57 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0AAB140E0F; Tue, 12 Oct 2021 15:54:57 +0200 (CEST) Received: from new2-smtp.messagingengine.com (new2-smtp.messagingengine.com [66.111.4.224]) by mails.dpdk.org (Postfix) with ESMTP id 596EC40151 for ; Tue, 12 Oct 2021 15:54:55 +0200 (CEST) Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailnew.nyi.internal (Postfix) with ESMTP id AF702580E4E; Tue, 12 Oct 2021 09:54:54 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute3.internal (MEProxy); Tue, 12 Oct 2021 09:54:54 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=fm2; bh= 9JLHSpa4yM9lv7S1noG7SGclm99KMs3xu2WgAZvu6Ws=; b=XHRrIGCm7j8du1U6 3ktF+OIURjdQmgN7wrwsOZI168Hps8NxKh93+JNCc5snnQDH0Z23kE9SUEYVQPf6 xdsL0/zrgf8jLrCjl34avDuw8brO0cInHavHR9kjdsOB4YEz9bhjjXfNUyaKY+lT d7Pd346ISPf9pCNFIJ1kobI/OerkcPN1ShgnaTcks0uK+QleKRXPb4JTOvAvovYI taOfGJGc/LMK0MO27AcezaZjxVV6jrY4j+Rn2oE04EFFe8Wxb6r1jEXUm/AI6nY5 prRmbFPhxym1ewBXIlE7tFE3bpJVWVexNXTeLJ+wYOrQWpRRRhydj8fCso8eUFcx QwS6xg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=9JLHSpa4yM9lv7S1noG7SGclm99KMs3xu2WgAZvu6 Ws=; b=EJ2j2J7iJvejAlyHqpZMnI0lDTOsXldfFt5HFMW3J4lSfve2LQYtqI37W ODduHATSO7cbOkA3XCa/E90GonzII28U/DpFQn4ceGI1/O3obcROjdQDxJQBx4w9 jK/dc7E3zQ5tlihxbrEVByRPhIoi5RI2arRVuGgp6/xpP2yodGb3WD+ww+ayTqNy o7lTCu2G1Hiv+t/u60sJoQ+SHlptZP5kt60zuyUBm3rj/KWQ7elKQst0r+vBtl6O U81kSd6lB0oCIBdhSf9YIZpqCFX7IKLrE9YHZuNpa4Krbk2RooE+a3csQBV7qgHR rlow5rFG+jFcIzuLHym0nfNoiqbbQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrvddtkedgieekucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkfgjfhgggfgtsehtqhertddttdejnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnhepkeethedtieevhfeigeejleegudefjeehkeekteeuveeiuedvveeu tdejveehveetnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrh homhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 12 Oct 2021 09:54:48 -0400 (EDT) From: Thomas Monjalon To: "Kinsella, Ray" , Akhil Goyal , "dev@dpdk.org" , Anoob Joseph Cc: "david.marchand@redhat.com" , "hemant.agrawal@nxp.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 , Ankur Dwivedi , "ciara.power@intel.com" , Stephen Hemminger , "Yigit, Ferruh" , bruce.richardson@intel.com Date: Tue, 12 Oct 2021 15:54:44 +0200 Message-ID: <2012129.4fn6nFIri4@thomas> In-Reply-To: References: <20210731181327.660296-1-gakhil@marvell.com> <2844039.NtWzsPphL5@thomas> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 12/10/2021 15:38, Anoob Joseph: > From: Thomas Monjalon > > 12/10/2021 13:34, Anoob Joseph: > > > From: Kinsella, Ray > > > > On 12/10/2021 11:50, Anoob Joseph wrote: > > > > > From: Akhil Goyal > > > > >>> 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 > > > > >>>> --- > > > > >>>> - } else if (xform->xform_type >=3D > > > > >>> 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. > >=20 > > Indeed, it is not an improvement. > >=20 > > > > >>> 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=E2=80=99t 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 l= ibrary) > > 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 applica= tion) > > to determine iterations of loop. This would solve the problem, right? > >=20 > > The problem is not the application. > > Are you asking the app to define DPDK types? >=20 > [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. Cu= rrently, it does that by looping from 0 to LIST_END which happens to give y= ou the first n features. Now, if we add any new asymmetric feature, LIST_EN= D value would change. Isn't that the very reason why we removed LIST_END fr= om 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 a= pplication will not be in a position to do a loop. My suggestion is, we lis= t the types that are supposed to be tested by the app, and let that array b= e used as feature list. >=20 > 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[] =3D {=20 >=20 > >=20 > > The problem is in DPDK API. We must not suggest a size for enums. >=20 > [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 l= ib binary > > (through a function) when the size increases. >=20 > [Anoob] Precisely my thoughts. The loop with LIST_END done in application= is not correct.=20 > >=20 > >=20 > >=20 > > > > > - for (i =3D 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i+= +) { > > > > > + enum rte_crypto_asym_op_type types[] =3D { > > > > > + 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_GE= NERATE, > > > > > + RTE_CRYPTO_ASYM_OP_PUBLIC_KEY_GEN= ERATE, > > > > > + RTE_CRYPTO_ASYM_OP_SHARED_SECRET_= COMPUTE, > > > > > + }; > > > > > + for (i =3D 0; i <=3D RTE_DIM(types); i++) { > > > > > if (tc.modex.xform_type =3D=3D > > 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 =3D test_c= ryptodev_asym_op( > > > > > &testsuit= e_params, &tc, > > > > > - test_msg,= sessionless, i, > > > > > + test_msg, > > > > > + sessionless, types[i], > > > > > RTE_RSA_K= EY_TYPE_EXP); > > > > > } > > > > > if (status) > > > > > break; > > > > > - if (tc.rsa_data.key_qt &&= (i =3D=3D > > > > > + if (tc.rsa_data.key_qt && > > > > > + (types[i] =3D=3D > > > > > RTE_CRYPT= O_ASYM_OP_DECRYPT || > > > > > - i =3D=3D = RTE_CRYPTO_ASYM_OP_SIGN)) { > > > > > + types[i] > > > > > + =3D=3D > > > > > + RTE_CRYPTO_ASYM_OP_SIGN)) { > > > > > status =3D test_c= ryptodev_asym_op( > > > > > &testsuit= e_params, > > > > > - &tc, test= _msg, sessionless, i, > > > > > + &tc, > > > > > + test_msg, sessionless, types[i], > > > > > RTE_RSA_K= ET_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 =3D=3D x > > > > || alg =3D y || ...) to get the same effect.