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

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

[Arek Kusztal]

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"
(http://patchwork.dpdk.org/patch/64549/)

 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];
+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,
+		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;
+
+}
+VERSION_SYMBOL(rte_cryptodev_sym_capability_get, _v20, 20.0);
+
+const struct rte_cryptodev_symmetric_capability *
+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) {
+		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);
 
 /**
  * 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

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

[Akhil Goyal]

Hi Arek,

Could you please also resend the dependent patches also?
I see that it was archived and there were patches for unit tests as well.

Regards,
Akhil

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

[Akhil Goyal]

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

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

[Ray Kinsella]

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

The correct call to VERSION_SYMBOL is already above.

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

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

>> +	global:
>> +	rte_cryptodev_info_get;
>> +	rte_cryptodev_sym_capability_get;
>> +} DPDK_20.0;
>> +
>> +
>>  EXPERIMENTAL {
>>  	global:
>>
>> --
>> 2.1.0
> 

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

[Trahe, Fiona]

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?


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

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

[Ray Kinsella]

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

True.

So I thought we should keep the DPDK_NN.0 numbering scheme at least consistent with v19.11
To reduce any further confusion caused by this issue. 

I did so, in a similar in a seperate patch to remove maps file references to DPDK_20.0.1.
We will need to a wholesale update of the map files in any case at 20.11, to v21. 

However you are 100% correct on this.
Go direct to DPDK_21?

>  
> 
>>>> +	global:
>>>> +	rte_cryptodev_info_get;
>>>> +	rte_cryptodev_sym_capability_get;
>>>> +} DPDK_20.0;
>>>> +
>>>> +
>>>>  EXPERIMENTAL {
>>>>  	global:
>>>>
>>>> --
>>>> 2.1.0
>>>

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

[Ray Kinsella]

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. 

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

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

[Ray Kinsella]

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