From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 327A8A0561; Mon, 20 Apr 2020 19:27:10 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 771811D5AD; Mon, 20 Apr 2020 19:27:09 +0200 (CEST) Received: from qrelay188.mxroute.com (qrelay188.mxroute.com [172.82.139.188]) by dpdk.org (Postfix) with ESMTP id 6BF7E1C1FE for ; Mon, 20 Apr 2020 19:27:08 +0200 (CEST) Received: from filter003.mxroute.com ([168.235.111.26] 168-235-111-26.cloud.ramnode.com) (Authenticated sender: mN4UYu2MZsgR) by qrelay188.mxroute.com (ZoneMTA) with ESMTPA id 17198a10b810000d83.001 for ; Mon, 20 Apr 2020 17:27:03 +0000 X-Zone-Loop: 7142d80746ed93a768b18a41ff1bc8886fad5bce81be X-Originating-IP: [168.235.111.26] Received: from galaxy.mxroute.com (unknown [23.92.70.113]) by filter003.mxroute.com (Postfix) with ESMTPS id 852A260040; Mon, 20 Apr 2020 17:27:02 +0000 (UTC) Received: from irdmzpr01-ext.ir.intel.com ([192.198.151.36]) by galaxy.mxroute.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.91) (envelope-from ) id 1jQZmj-00044A-Le; Mon, 20 Apr 2020 13:00:42 -0400 To: "Trahe, Fiona" , Akhil Goyal , "Kusztal, ArkadiuszX" , "dev@dpdk.org" Cc: Thomas Monjalon References: <20200417145924.2052-1-arkadiuszx.kusztal@intel.com> From: Ray Kinsella Autocrypt: addr=mdr@ashroe.eu; keydata= mQINBFv8B3wBEAC+5ImcgbIvadt3axrTnt7Sxch3FsmWTTomXfB8YiuHT8KL8L/bFRQSL1f6 ASCHu3M89EjYazlY+vJUWLr0BhK5t/YI7bQzrOuYrl9K94vlLwzD19s/zB/g5YGGR5plJr0s JtJsFGEvF9LL3e+FKMRXveQxBB8A51nAHfwG0WSyx53d61DYz7lp4/Y4RagxaJoHp9lakn8j HV2N6rrnF+qt5ukj5SbbKWSzGg5HQF2t0QQ5tzWhCAKTfcPlnP0GymTBfNMGOReWivi3Qqzr S51Xo7hoGujUgNAM41sxpxmhx8xSwcQ5WzmxgAhJ/StNV9cb3HWIoE5StCwQ4uXOLplZNGnS uxNdegvKB95NHZjRVRChg/uMTGpg9PqYbTIFoPXjuk27sxZLRJRrueg4tLbb3HM39CJwSB++ YICcqf2N+GVD48STfcIlpp12/HI+EcDSThzfWFhaHDC0hyirHxJyHXjnZ8bUexI/5zATn/ux TpMbc/vicJxeN+qfaVqPkCbkS71cHKuPluM3jE8aNCIBNQY1/j87k5ELzg3qaesLo2n1krBH bKvFfAmQuUuJT84/IqfdVtrSCTabvDuNBDpYBV0dGbTwaRfE7i+LiJJclUr8lOvHUpJ4Y6a5 0cxEPxm498G12Z3NoY/mP5soItPIPtLR0rA0fage44zSPwp6cQARAQABtBxSYXkgS2luc2Vs bGEgPG1kckBhc2hyb2UuZXU+iQJUBBMBCAA+FiEEcDUDlKDJaDuJlfZfdJdaH/sCCpsFAlv8 B3wCGyMFCQlmAYAFCwkIBwIGFQoJCAsCBBYCAwECHgECF4AACgkQdJdaH/sCCptdtRAAl0oE msa+djBVYLIsax+0f8acidtWg2l9f7kc2hEjp9h9aZCpPchQvhhemtew/nKavik3RSnLTAyn B3C/0GNlmvI1l5PFROOgPZwz4xhJKGN7jOsRrbkJa23a8ly5UXwF3Vqnlny7D3z+7cu1qq/f VRK8qFyWkAb+xgqeZ/hTcbJUWtW+l5Zb+68WGEp8hB7TuJLEWb4+VKgHTpQ4vElYj8H3Z94a 04s2PJMbLIZSgmKDASnyrKY0CzTpPXx5rSJ1q+B1FCsfepHLqt3vKSALa3ld6bJ8fSJtDUJ7 JLiU8dFZrywgDIVme01jPbjJtUScW6jONLvhI8Z2sheR71UoKqGomMHNQpZ03ViVWBEALzEt TcjWgJFn8yAmxqM4nBnZ+hE3LbMo34KCHJD4eg18ojDt3s9VrDLa+V9fNxUHPSib9FD9UX/1 +nGfU/ZABmiTuUDM7WZdXri7HaMpzDRJUKI6b+/uunF8xH/h/MHW16VuMzgI5dkOKKv1LejD dT5mA4R+2zBS+GsM0oa2hUeX9E5WwjaDzXtVDg6kYq8YvEd+m0z3M4e6diFeLS77/sAOgaYL 92UcoKD+Beym/fVuC6/55a0e12ksTmgk5/ZoEdoNQLlVgd2INtvnO+0k5BJcn66ZjKn3GbEC VqFbrnv1GnA58nEInRCTzR1k26h9nmS5Ag0EW/wHfAEQAMth1vHr3fOZkVOPfod3M6DkQir5 xJvUW5EHgYUjYCPIa2qzgIVVuLDqZgSCCinyooG5dUJONVHj3nCbITCpJp4eB3PI84RPfDcC hf/V34N/Gx5mTeoymSZDBmXT8YtvV/uJvn+LvHLO4ZJdvq5ZxmDyxfXFmkm3/lLw0+rrNdK5 pt6OnVlCqEU9tcDBezjUwDtOahyV20XqxtUttN4kQWbDRkhT+HrA9WN9l2HX91yEYC+zmF1S OhBqRoTPLrR6g4sCWgFywqztpvZWhyIicJipnjac7qL/wRS+wrWfsYy6qWLIV80beN7yoa6v ccnuy4pu2uiuhk9/edtlmFE4dNdoRf7843CV9k1yRASTlmPkU59n0TJbw+okTa9fbbQgbIb1 pWsAuicRHyLUIUz4f6kPgdgty2FgTKuPuIzJd1s8s6p2aC1qo+Obm2gnBTduB+/n1Jw+vKpt 07d+CKEKu4CWwvZZ8ktJJLeofi4hMupTYiq+oMzqH+V1k6QgNm0Da489gXllU+3EFC6W1qKj tkvQzg2rYoWeYD1Qn8iXcO4Fpk6wzylclvatBMddVlQ6qrYeTmSbCsk+m2KVrz5vIyja0o5Y yfeN29s9emXnikmNfv/dA5fpi8XCANNnz3zOfA93DOB9DBf0TQ2/OrSPGjB3op7RCfoPBZ7u AjJ9dM7VABEBAAGJAjwEGAEIACYWIQRwNQOUoMloO4mV9l90l1of+wIKmwUCW/wHfAIbDAUJ CWYBgAAKCRB0l1of+wIKm3KlD/9w/LOG5rtgtCUWPl4B3pZvGpNym6XdK8cop9saOnE85zWf u+sKWCrxNgYkYP7aZrYMPwqDvilxhbTsIJl5HhPgpTO1b0i+c0n1Tij3EElj5UCg3q8mEc17 c+5jRrY3oz77g7E3oPftAjaq1ybbXjY4K32o3JHFR6I8wX3m9wJZJe1+Y+UVrrjY65gZFxcA thNVnWKErarVQGjeNgHV4N1uF3pIx3kT1N4GSnxhoz4Bki91kvkbBhUgYfNflGURfZT3wIKK +d50jd7kqRouXUCzTdzmDh7jnYrcEFM4nvyaYu0JjSS5R672d9SK5LVIfWmoUGzqD4AVmUW8 pcv461+PXchuS8+zpltR9zajl72Q3ymlT4BTAQOlCWkD0snBoKNUB5d2EXPNV13nA0qlm4U2 GpROfJMQXjV6fyYRvttKYfM5xYKgRgtP0z5lTAbsjg9WFKq0Fndh7kUlmHjuAIwKIV4Tzo75 QO2zC0/NTaTjmrtiXhP+vkC4pcrOGNsbHuaqvsc/ZZ0siXyYsqbctj/sCd8ka2r94u+c7o4l BGaAm+FtwAfEAkXHu4y5Phuv2IRR+x1wTey1U1RaEPgN8xq0LQ1OitX4t2mQwjdPihZQBCnZ wzOrkbzlJMNrMKJpEgulmxAHmYJKgvZHXZXtLJSejFjR0GdHJcL5rwVOMWB8cg== Message-ID: <075c0e6b-0588-9e83-efd8-a4d7d937438e@ashroe.eu> Date: Mon, 20 Apr 2020 18:26:58 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-AuthUser: mdr@ashroe.eu Subject: Re: [dpdk-dev] [PATCH v3] cryptodev: version cryptodev info get function X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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 20/04/2020 17:54, Trahe, Fiona wrote: > Hi Ray, Akhil, > > >> On 20/04/2020 15:22, Akhil Goyal wrote: >>> >>> >>>> >>>> This patch adds versioned function rte_cryptodev_info_get() >>>> to prevent some issues with ABI policy. >>>> Node v21 works in same way as before, returning driver capabilities >>>> directly to the API caller. These capabilities may include new elements >>>> not part of the v20 ABI. >>>> Node v20 function maintains compatibility with v20 ABI releases >>>> by stripping out elements not supported in v20 ABI. Because >>>> rte_cryptodev_info_get is called by other API functions, >>>> rte_cryptodev_sym_capability_get function is versioned the same way. >>>> >>>> Signed-off-by: Arek Kusztal >>>> --- >>>> v2: >>>> - changed version numbers of symbols to 20.0.2 >>>> v3: >>>> - added v2/v3 informations >>>> - changed version numbers of symbols to 21 >>>> - fixed checkpatch issues >>>> >>>> This patch depends on following patches: >>>> >>>> [1] - "[v3] cryptodev: add chacha20-poly1305 aead algorithm" >>>> (https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatchwor >>>> k.dpdk.org%2Fpatch%2F64549%2F&data=02%7C01%7Cakhil.goyal%40nxp. >>>> com%7Ce6789fd42a5946c128e508d7e2dffe2f%7C686ea1d3bc2b4c6fa92cd99c >>>> 5c301635%7C0%7C0%7C637227323980059545&sdata=50eQJE7WHTME6d >>>> qA7Nfk%2B50PVAyJrpKlMw%2BoGtA1%2FTc%3D&reserved=0) >>> >>> Please include the dependent patches in a single series in your next version. >>>> >>>> lib/librte_cryptodev/rte_cryptodev.c | 143 ++++++++++++++++++++++++- >>>> lib/librte_cryptodev/rte_cryptodev.h | 39 ++++++- >>>> lib/librte_cryptodev/rte_cryptodev_version.map | 7 ++ >>>> 3 files changed, 186 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/lib/librte_cryptodev/rte_cryptodev.c >>>> b/lib/librte_cryptodev/rte_cryptodev.c >>>> index 6d1d0e9..b061447 100644 >>>> --- a/lib/librte_cryptodev/rte_cryptodev.c >>>> +++ b/lib/librte_cryptodev/rte_cryptodev.c >>>> @@ -41,6 +41,9 @@ >>>> #include "rte_cryptodev.h" >>>> #include "rte_cryptodev_pmd.h" >>>> >>>> +#include >>>> +#include >>>> + >>>> static uint8_t nb_drivers; >>>> >>>> static struct rte_cryptodev rte_crypto_devices[RTE_CRYPTO_MAX_DEVS]; >>>> @@ -56,6 +59,14 @@ static struct rte_cryptodev_global cryptodev_globals = { >>>> /* spinlock for crypto device callbacks */ >>>> static rte_spinlock_t rte_cryptodev_cb_lock = RTE_SPINLOCK_INITIALIZER; >>>> >>>> +static const struct rte_cryptodev_capabilities >>>> + cryptodev_undefined_capabilities[] = { >>>> + RTE_CRYPTODEV_END_OF_CAPABILITIES_LIST() >>>> +}; >>>> + >>>> +static struct rte_cryptodev_capabilities >>>> + *capability_copies[RTE_CRYPTO_MAX_DEVS]; >>> >>> Capabilities_copy is a better name as it is copy of many capabilities. > [Fiona] ok > > >>>> const struct rte_cryptodev_symmetric_capability * >>>> -rte_cryptodev_sym_capability_get(uint8_t dev_id, >>>> +rte_cryptodev_sym_capability_get_v20(uint8_t dev_id, >>> >>> __vsym Annotation to be used in a declaration of the internal symbol >>> to signal that it is being used as an implementation of a particular >>> version of symbol. > [Fiona] ok > > >>>> + } >>>> + >>>> + return NULL; >>>> + >>> >>> Extra line > [Fiona] ok > >>> >>>> +} >>>> +VERSION_SYMBOL(rte_cryptodev_sym_capability_get, _v20, 20.0); >>>> + >>>> +const struct rte_cryptodev_symmetric_capability * >>> >>> __vsym annotation > [Fiona] ok > > >>> >>>> +rte_cryptodev_sym_capability_get_v21(uint8_t dev_id, >>>> const struct rte_cryptodev_sym_capability_idx *idx) >>>> { >>>> const struct rte_cryptodev_capabilities *capability; >>>> @@ -313,6 +360,10 @@ rte_cryptodev_sym_capability_get(uint8_t dev_id, >>>> return NULL; >>>> >>>> } >>>> +MAP_STATIC_SYMBOL(const struct rte_cryptodev_symmetric_capability * >>>> + rte_cryptodev_sym_capability_get(uint8_t dev_id, >>>> + const struct rte_cryptodev_sym_capability_idx *idx), >>>> + rte_cryptodev_sym_capability_get_v21); >>>> >>>> static int >>>> param_range_check(uint16_t size, const struct rte_crypto_param_range *range) >>>> @@ -999,6 +1050,13 @@ rte_cryptodev_close(uint8_t dev_id) >>>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_close, -ENOTSUP); >>>> retval = (*dev->dev_ops->dev_close)(dev); >>>> >>>> + >>>> + if (capability_copies[dev_id]) { >>>> + free(capability_copies[dev_id]); >>>> + capability_copies[dev_id] = NULL; >>>> + } >>>> + is_capability_checked[dev_id] = 0; >>>> + >>>> if (retval < 0) >>>> return retval; >>>> >>>> @@ -1111,9 +1169,61 @@ rte_cryptodev_stats_reset(uint8_t dev_id) >>>> (*dev->dev_ops->stats_reset)(dev); >>>> } >>>> >>>> +static void >>>> +get_v20_capabilities(uint8_t dev_id, struct rte_cryptodev_info *dev_info) >>>> +{ >>>> + const struct rte_cryptodev_capabilities *capability; >>>> + uint8_t found_invalid_capa = 0; >>>> + uint8_t counter = 0; >>>> + >>>> + for (capability = dev_info->capabilities; >>>> + capability->op != RTE_CRYPTO_OP_TYPE_UNDEFINED; >>>> + ++capability, ++counter) { >>>> + if (capability->op == RTE_CRYPTO_OP_TYPE_SYMMETRIC && >>>> + capability->sym.xform_type == >>>> + RTE_CRYPTO_SYM_XFORM_AEAD >>>> + && capability->sym.aead.algo >= >>>> + RTE_CRYPTO_AEAD_CHACHA20_POLY1305) { >>>> + found_invalid_capa = 1; >>>> + counter--; >>>> + } >>>> + } >>>> + is_capability_checked[dev_id] = 1; >>>> + if (found_invalid_capa) { >>> >>> Code becomes unreadable due to indentation which can be avoided. > [Fiona] ok > > > >>>> +void >>>> +rte_cryptodev_info_get_v21(uint8_t dev_id, struct rte_cryptodev_info >>>> *dev_info); >>>> +BIND_DEFAULT_SYMBOL(rte_cryptodev_info_get, _v21, 21); >>> >>> I am not sure if we need to bind for _v20 also >>> BIND_DEFAULT_SYMBOL(rte_cryptodev_info_get, _v20, 20); >> >> The correct call to VERSION_SYMBOL is already above. > [Fiona] ok, so won't do this. > > >>> Ray, can you please suggest if it required or not? And what all we need to check? >> >> See below. >> >>> >>> The patch is still showing Incompatibilities >>> NOTICE: ABI may be incompatible, check reports/logs for details. >>> NOTICE: Incompatible list: librte_cryptodev.so >> >> So I looked through the issues it is complaining about, these are here. >> https://travis-ci.com/github/ovsrobot/dpdk/jobs/320526253#L4540 >> >> Basically they all are warnings related to the changes to the enumeration >> rte_crypto_aead_algorithm. >> >> Essentially the new member RTE_CRYPTO_AEAD_CHACHA20_POLY1305. >> The change to the end value RTE_CRYPTO_AEAD_LIST_END. >> Members of this type "enum rte_crypto_aead_algorithm algo" are demeeded to also have changed, >> but they haven't. >> >> With the additional work to create the v20 version of rte_cryptodev_info_get. >> I think all reasonable steps have been been taken here. > [Fiona] Do we need to change the tool or somehow mark as a false positive? Yes, I will take a look at it. > > >>>> /** >>>> * Register a callback function for specific device id. >>>> diff --git a/lib/librte_cryptodev/rte_cryptodev_version.map >>>> b/lib/librte_cryptodev/rte_cryptodev_version.map >>>> index 6e41b4b..512a4a7 100644 >>>> --- a/lib/librte_cryptodev/rte_cryptodev_version.map >>>> +++ b/lib/librte_cryptodev/rte_cryptodev_version.map >>>> @@ -58,6 +58,13 @@ DPDK_20.0 { >>>> local: *; >>>> }; >>>> >>>> +DPDK_21 { >> >> Should be DPDK_21.0 > [Fiona] Can you explain why? > I thought it could go back to a 2-number system with _v21 ABI. > I thought we'd clarified that DPDK_20.0 is only there due to a mistake, that should have been DPDK_20. > > >>>> + global: >>>> + rte_cryptodev_info_get; >>>> + rte_cryptodev_sym_capability_get; >>>> +} DPDK_20.0; >>>> + >>>> + >>>> EXPERIMENTAL { >>>> global: >>>> >>>> -- >>>> 2.1.0 >>>