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

[Akhil Goyal <akhil.goyal@nxp.com>]

> 
> 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.

> +static uint8_t is_capability_checked[RTE_CRYPTO_MAX_DEVS];
> 
>  /**
>   * The user application callback description.
> @@ -280,7 +291,43 @@ rte_crypto_auth_operation_strings[] = {
>  };
> 
>  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.

> +		const struct rte_cryptodev_sym_capability_idx *idx)
> +{
> +	const struct rte_cryptodev_capabilities *capability;
> +	struct rte_cryptodev_info dev_info;
> +	int i = 0;
> +
> +	rte_cryptodev_info_get_v20(dev_id, &dev_info);
> +
> +	while ((capability = &dev_info.capabilities[i++])->op !=
> +			RTE_CRYPTO_OP_TYPE_UNDEFINED) {
> +		if (capability->op != RTE_CRYPTO_OP_TYPE_SYMMETRIC)
> +			continue;
> +
> +		if (capability->sym.xform_type != idx->type)
> +			continue;
> +
> +		if (idx->type == RTE_CRYPTO_SYM_XFORM_AUTH &&
> +			capability->sym.auth.algo == idx->algo.auth)
> +			return &capability->sym;
> +
> +		if (idx->type == RTE_CRYPTO_SYM_XFORM_CIPHER &&
> +			capability->sym.cipher.algo == idx->algo.cipher)
> +			return &capability->sym;
> +
> +		if (idx->type == RTE_CRYPTO_SYM_XFORM_AEAD &&
> +				capability->sym.aead.algo == idx->algo.aead)
> +			return &capability->sym;
> +	}
> +
> +	return NULL;
> +

Extra line

> +}
> +VERSION_SYMBOL(rte_cryptodev_sym_capability_get, _v20, 20.0);
> +
> +const struct rte_cryptodev_symmetric_capability *

__vsym annotation

> +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.

if (!found_invalid_capa)
	return;
capability_copies[dev_id] = malloc(counter * sizeof(struct rte_cryptodev_capabilities));
if (capability_copies[dev_id] == NULL) {
	dev_info->capabilities = cryptodev_undefined_capabilities;
	is_capability_checked[dev_id] = 0;
	return;
}

counter = 0;
for(...) {
...
}
> +		capability_copies[dev_id] = malloc(counter *
> +			sizeof(struct rte_cryptodev_capabilities));
> +		if (capability_copies[dev_id] == NULL) {
> +			 /*
> +			  * error case - no memory to store the trimmed
> +			  * list, so have to return an empty list
> +			  */
> +			dev_info->capabilities =
> +				cryptodev_undefined_capabilities;
> +			is_capability_checked[dev_id] = 0;
> +		} else {
> +			counter = 0;
> +			for (capability = dev_info->capabilities;
> +					capability->op !=
> +					RTE_CRYPTO_OP_TYPE_UNDEFINED;
> +					capability++) {
> +				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)) {
> +					capability_copies[dev_id][counter++] =
> +							*capability;
> +				}
> +			}
> +			dev_info->capabilities =
> +					capability_copies[dev_id];
> +		}
> +	}
> +}
> 
>  void
> -rte_cryptodev_info_get(uint8_t dev_id, struct rte_cryptodev_info *dev_info)
> +rte_cryptodev_info_get_v20(uint8_t dev_id, struct rte_cryptodev_info
> *dev_info)
>  {
>  	struct rte_cryptodev *dev;
> 
> @@ -1129,10 +1239,39 @@ rte_cryptodev_info_get(uint8_t dev_id, struct
> rte_cryptodev_info *dev_info)
>  	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
>  	(*dev->dev_ops->dev_infos_get)(dev, dev_info);
> 
> +	if (capability_copies[dev_id] == NULL) {
> +		if (!is_capability_checked[dev_id])
> +			get_v20_capabilities(dev_id, dev_info);
> +	} else
> +		dev_info->capabilities = capability_copies[dev_id];
> +
>  	dev_info->driver_name = dev->device->driver->name;
>  	dev_info->device = dev->device;
>  }
> +VERSION_SYMBOL(rte_cryptodev_info_get, _v20, 20.0);
> +
> +void
> +rte_cryptodev_info_get_v21(uint8_t dev_id, struct rte_cryptodev_info
> *dev_info)
> +{
> +	struct rte_cryptodev *dev;
> +
> +	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
> +		CDEV_LOG_ERR("Invalid dev_id=%d", dev_id);
> +		return;
> +	}
> +
> +	dev = &rte_crypto_devices[dev_id];
> 
> +	memset(dev_info, 0, sizeof(struct rte_cryptodev_info));
> +
> +	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
> +	(*dev->dev_ops->dev_infos_get)(dev, dev_info);
> +
> +	dev_info->driver_name = dev->device->driver->name;
> +	dev_info->device = dev->device;
> +}
> +MAP_STATIC_SYMBOL(void rte_cryptodev_info_get(uint8_t dev_id,
> +	struct rte_cryptodev_info *dev_info), rte_cryptodev_info_get_v21);
> 
>  int
>  rte_cryptodev_callback_register(uint8_t dev_id,
> diff --git a/lib/librte_cryptodev/rte_cryptodev.h
> b/lib/librte_cryptodev/rte_cryptodev.h
> index 437b8a9..7f1bc90 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.h
> +++ b/lib/librte_cryptodev/rte_cryptodev.h
> @@ -24,6 +24,9 @@ extern "C" {
>  #include <rte_common.h>
>  #include <rte_config.h>
> 
> +#include <rte_compat.h>
> +#include <rte_function_versioning.h>
> +
>  extern const char **rte_cyptodev_names;
> 
>  /* Logging Macros */
> @@ -217,6 +220,15 @@ struct rte_cryptodev_asym_capability_idx {
>   *   - Return NULL if the capability not exist.
>   */
>  const struct rte_cryptodev_symmetric_capability *
> +rte_cryptodev_sym_capability_get_v20(uint8_t dev_id,
> +		const struct rte_cryptodev_sym_capability_idx *idx);
> +
> +const struct rte_cryptodev_symmetric_capability *
> +rte_cryptodev_sym_capability_get_v21(uint8_t dev_id,
> +		const struct rte_cryptodev_sym_capability_idx *idx);
> +BIND_DEFAULT_SYMBOL(rte_cryptodev_sym_capability_get, _v21, 21);
> +
> +const struct rte_cryptodev_symmetric_capability *
>  rte_cryptodev_sym_capability_get(uint8_t dev_id,
>  		const struct rte_cryptodev_sym_capability_idx *idx);
> 
> @@ -758,9 +770,34 @@ rte_cryptodev_stats_reset(uint8_t dev_id);
>   * the last valid element has it's op field set to
>   * RTE_CRYPTO_OP_TYPE_UNDEFINED.
>   */
> -extern void
> +
> +void
>  rte_cryptodev_info_get(uint8_t dev_id, struct rte_cryptodev_info *dev_info);
> 
> +/* An extra element RTE_CRYPTO_AEAD_CHACHA20_POLY1305 is added
> + * to enum rte_crypto_aead_algorithm, also changing the value of
> + *  RTE_CRYPTO_AEAD_LIST_END. To maintain ABI compatibility with
> applications
> + * which linked against earlier versions, preventing them, for example, from
> + * picking up the new value and using it to index into an array sized too small
> + * for it, it is necessary to have two versions of rte_cryptodev_info_get()
> + * The latest version just returns directly the capabilities retrieved from
> + * the device. The compatible version inspects the capabilities retrieved
> + * from the device, but only returns them directly if the new value
> + * is not included. If the new value is included, it allocates space
> + * for a copy of the device capabilities, trims the new value from this
> + * and returns this copy. It only needs to do this once per device.
> + * For the corner case of a corner case when the alloc may fail,
> + * an empty capability list is returned, as there is no mechanism to return
> + * an error and adding such a mechanism would itself be an ABI breakage.
> + * The compatible version can be removed after the next major ABI release.
> + */
> +
> +void
> +rte_cryptodev_info_get_v20(uint8_t dev_id, struct rte_cryptodev_info
> *dev_info);
> +
> +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);

Ray, can you please suggest if it required or not? And what all we need to check?

The patch is still showing Incompatibilities
NOTICE: ABI may be incompatible, check reports/logs for details.
NOTICE: Incompatible list:  librte_cryptodev.so

> 
>  /**
>   * 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 {
> +	global:
> +	rte_cryptodev_info_get;
> +	rte_cryptodev_sym_capability_get;
> +} DPDK_20.0;
> +
> +
>  EXPERIMENTAL {
>  	global:
> 
> --
> 2.1.0