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 A3DBAA0C47; Tue, 12 Oct 2021 16:47:59 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 25D094113A; Tue, 12 Oct 2021 16:47:59 +0200 (CEST) Received: from mail-108-mta145.mxroute.com (mail-108-mta145.mxroute.com [136.175.108.145]) by mails.dpdk.org (Postfix) with ESMTP id 306514111D for ; Tue, 12 Oct 2021 16:47:58 +0200 (CEST) Received: from filter004.mxroute.com ([149.28.56.236] filter004.mxroute.com) (Authenticated sender: mN4UYu2MZsgR) by mail-108-mta145.mxroute.com (ZoneMTA) with ESMTPSA id 17c74f95f670000b55.004 for (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES128-GCM-SHA256); Tue, 12 Oct 2021 14:47:52 +0000 X-Zone-Loop: 2b15bb04195d73b613e9cd775384c3825652e5460a02 X-Originating-IP: [149.28.56.236] DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=ashroe.eu; s=x; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:MIME-Version:Date: Message-ID:From:References:Cc:To:Subject:Sender:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=Ys2pUWAhZFVr14wJfyTdDwBh6nSkq3LqvD9cjKaE63M=; b=AtqlQamNaehvua5gkVUO3Sd473 MZsf5TarDglUAHzZmntnFA9wO+GjgpZ3dWO4xTcl2X786gmJgOeYUQq/ukZwEyxCnPy0GqKu4bwGt j1iS2Xg3RfW4PnzRSrlrNNvjuMel4VYir0p2YYYEGw5ZClte1x9cwk9dN748OMSCqivoo7wA5Ee7c LcQPH/IiXVKqPCfzHA8VeZft6x7AveuFbAONv6W5NtytcHAM79ptQcnPyAXF6PJ7BsWd0IvlNRthu 5aZYGluc3ekgNHDjKqigLI/lvZqFSpCVIqe79kh6fgRd6S5LqC/ZjIP0E1St6EHAzyjYX9mBnXZM4 0MQxE03Q==; To: Anoob Joseph , Thomas Monjalon , Akhil Goyal , "dev@dpdk.org" 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" References: <20210731181327.660296-1-gakhil@marvell.com> <2844039.NtWzsPphL5@thomas> <2012129.4fn6nFIri4@thomas> From: "Kinsella, Ray" Message-ID: <805733d0-38d7-3e51-e317-b3e4ce7ed165@ashroe.eu> Date: Tue, 12 Oct 2021 15:47:45 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-AuthUser: mdr@ashroe.eu X-Zone-Spam-Resolution: no action X-Zone-Spam-Status: No, score=-0.1, required=15, tests=[ARC_NA=0, TO_DN_EQ_ADDR_SOME=0, RCPT_COUNT_TWELVE=0, FROM_HAS_DN=0, TO_DN_SOME=0, MIME_GOOD=-0.1, FROM_EQ_ENVFROM=0, MIME_TRACE=0, RCVD_COUNT_ZERO=0, NEURAL_SPAM=0, MID_RHS_MATCH_FROM=0] 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" On 12/10/2021 15:18, Anoob Joseph wrote: > Hi Thomas, > > Please see inline. > > Thanks, > Anoob > >> -----Original Message----- >> From: Thomas Monjalon >> Sent: Tuesday, October 12, 2021 7:25 PM >> 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 >> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END >> enumerators >> >> 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 >= >>>>>>>>> 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. > > [Anoob] Indeed. But there are 3 LIST_ENDs removed with this patch. Do you propose 3 new APIs to just get max number? 1 API returning a single "info" structure perhaps - as being the most extensible? > >> >>>>>>> + 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. >> >> >