Re: [dpdk-dev] [PATCH v3] cryptodev: version cryptodev info get function

[Ray Kinsella <mdr@ashroe.eu>]

On 20/04/2020 18:26, Ray Kinsella wrote:
> 
> 
> 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 <arkadiuszx.kusztal@intel.com>
>>>>> ---
>>>>> 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&amp;data=02%7C01%7Cakhil.goyal%40nxp.
>>>>> com%7Ce6789fd42a5946c128e508d7e2dffe2f%7C686ea1d3bc2b4c6fa92cd99c
>>>>> 5c301635%7C0%7C0%7C637227323980059545&amp;sdata=50eQJE7WHTME6d
>>>>> qA7Nfk%2B50PVAyJrpKlMw%2BoGtA1%2FTc%3D&amp;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 <rte_compat.h>
>>>>> +#include <rte_function_versioning.h>
>>>>> +
>>>>>  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. 

So take a look at devtools/libabigail.abignore.
Suppressions on 'enum rte_crypto_aead_algorithm' & 'rte_crypto_aead_algorithm_strings',
_should_ take care of it.

Please check with test_build.sh & test_meson_build.sh with DPDK_ABI_REF_VERSION=v19.11

> 
>>
>>
>>>>>  /**
>>>>>   * 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
>>>>