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 12696A0C47; Tue, 12 Oct 2021 13:28:25 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 996094112E; Tue, 12 Oct 2021 13:28:24 +0200 (CEST) Received: from mail-108-mta236.mxroute.com (mail-108-mta236.mxroute.com [136.175.108.236]) by mails.dpdk.org (Postfix) with ESMTP id 4790E40E0F for ; Tue, 12 Oct 2021 13:28:23 +0200 (CEST) Received: from filter004.mxroute.com ([149.28.56.236] filter004.mxroute.com) (Authenticated sender: mN4UYu2MZsgR) by mail-108-mta236.mxroute.com (ZoneMTA) with ESMTPSA id 17c7442b0740000b55.004 for (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES128-GCM-SHA256); Tue, 12 Oct 2021 11:28:20 +0000 X-Zone-Loop: c52b7355ec8a75ace87ef0eebb109af93254ed0c3937 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=zP3FRKlL7rpAQSxoHXj3vpOJu4PnVVsIyNQYtU6mLaM=; b=m5YoEns8ruNS5gELIYopbetJzY Vh2YWMXd9pWelolltFC7v/EDqagmTYmVP6eQCpwSIh/83bg3pvyadmwASjfL7BxhlETZpW3cTPHaG RYeZB31qQ4Ul4elhZ1O0siplln7eRJcOAdI86FNcEkWLBdNO0abliEqi1Kj2zNC0c5SuqXF8o7olg qCQtZRBZ8qg0LPaFytd4Ci9R1SvgbZqK8+lV7roNiPPEtIUwN0DLvy87LCcHRob2CA2SsaBw//TXe 7PnNmVsrU5kzOFpoPJvPpTMZZ3rVjUFplzctfxJE75Jqb56cQFYg/CJF3VJMZt4bax49rjIjFj/AR CkqUmayQ==; To: Anoob Joseph , Akhil Goyal , "dev@dpdk.org" Cc: "thomas@monjalon.net" , "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" References: <20210731181327.660296-1-gakhil@marvell.com> <20211008204516.3497060-1-gakhil@marvell.com> From: "Kinsella, Ray" Message-ID: <84e597ee-ab57-1a2c-889c-68d04e58a12d@ashroe.eu> Date: Tue, 12 Oct 2021 12:28:12 +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 11:50, Anoob Joseph wrote: > Hi Ray, Akhil, > > Please see inline. > > Thanks, > Anoob > >> -----Original Message----- >> From: Akhil Goyal >> Sent: Tuesday, October 12, 2021 3:49 PM >> To: Kinsella, Ray ; dev@dpdk.org >> Cc: thomas@monjalon.net; david.marchand@redhat.com; >> hemant.agrawal@nxp.com; Anoob Joseph ; >> 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 >> 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 >>>> --- >>>> 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. 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. > > - 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. >