Re: [dpdk-dev] [PATCH] cryptodev: version rte_cryptodev_info_get function

["Trahe, Fiona" <fiona.trahe@intel.com>]

Hi Ray,

We're going to need hep to understand the numbering.
The examples here http://doc.dpdk.org/guides/contributing/abi_versioning.html#major-abi-versions
show only major numbers in the .map file.
Whereas the map files all have major.minor.

The example shows that to add a new version of an existing fn one should use the next major number, 
"When an ABI change is made between major ABI versions to a given library, a new section is added to
 that library’s version map describing the impending new ABI version"
so 
 - Old fn becomes fn_v20()
 - VERSION_SYMBOL(fn, _v20, 20); - maps node 20 to fn_v20() for dynamic linking
 - New fn becomes fn_v21()
 - BIND_DEFAULT_SYMBOL(fn, _v21, 21); - maps new builds to fn_v21 for dynamic linking and makes fn_v21 default
 - MAP_STATIC_SYMBOL(fn_proto, fn_v21); - for static linking
And the map file should have:

DPDK_21 {
	global:
	rte_cryptodev_info_get;
} DPDK_20;

When the ABI version moves to node 21 in DPDK20.11, the _v20 symbols can be removed.

You suggest using DPDK_20.0.2 and _v2002
Can you explain why? 
In 20.0.2 - which number is minor?
And why 3 numbers?

Regards,
Fiona


> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Ray Kinsella
> Sent: Tuesday, April 14, 2020 2:54 PM
> To: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] cryptodev: version rte_cryptodev_info_get function
> 
> 
> 
> On 18/03/2020 20:41, Arek Kusztal wrote:
> > This patch adds versioned function rte_cryptodev_info_get.
> > Node 20.05 function works the same way it was working before.
> > Node 20.0 function strips capability added in 20.05 release
> > to prevent some issues with ABI policy. To do that new capability
> > array is allocated per device and returned to user instead of the
> > original array passed by PMD.
> >
> > Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> > ---
> >  lib/librte_cryptodev/rte_cryptodev.c           | 97 +++++++++++++++++++++++++-
> >  lib/librte_cryptodev/rte_cryptodev.h           | 12 +++-
> >  lib/librte_cryptodev/rte_cryptodev_version.map |  7 ++
> >  3 files changed, 113 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
> > index 6d1d0e9..2d030ba 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,13 @@ 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()
> > +};
> > +
> > +struct rte_cryptodev_capabilities *capability_copies[RTE_CRYPTO_MAX_DEVS];
> > +uint8_t is_capability_checked[RTE_CRYPTO_MAX_DEVS];
> >
> >  /**
> >   * The user application callback description.
> > @@ -999,6 +1009,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,11 +1128,12 @@ rte_cryptodev_stats_reset(uint8_t dev_id)
> >  	(*dev->dev_ops->stats_reset)(dev);
> >  }
> >
> > -
> >  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;
> > +	const struct rte_cryptodev_capabilities *capability;
> > +	uint8_t counter = 0;
> >
> >  	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
> >  		CDEV_LOG_ERR("Invalid dev_id=%d", dev_id);
> > @@ -1129,10 +1147,85 @@ 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]) {
> > +			uint8_t found_invalid_capa = 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];
> > +				}
> > +			}
> > +		}
> > +	} 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_v2005(uint8_t dev_id, struct rte_cryptodev_info *dev_info)
> 
> should be rte_cryptodev_info_get_v2002
> 
> > +{
> > +	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_v2005);
> 
> should be rte_cryptodev_info_get_v2002
> 
> >
> >  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..06ce2f2 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 */
> > @@ -758,7 +761,14 @@ 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_v20(uint8_t dev_id, struct rte_cryptodev_info *dev_info);
> > +
> > +void
> > +rte_cryptodev_info_get_v2005(uint8_t dev_id, struct rte_cryptodev_info *dev_info);
> > +BIND_DEFAULT_SYMBOL(rte_cryptodev_info_get, _v2005, 20.05);
> > +
> > +void
> >  rte_cryptodev_info_get(uint8_t dev_id, struct rte_cryptodev_info *dev_info);
> do we still need this forward declaration?
> 
> >
> >
> > diff --git a/lib/librte_cryptodev/rte_cryptodev_version.map
> b/lib/librte_cryptodev/rte_cryptodev_version.map
> > index 6e41b4b..d6c945a 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_20.05 {
> 
> should be DPDK_20.0.2
> 
> > +	global:
> > +	rte_cryptodev_info_get;
> > +} DPDK_20.0;
> > +
> > +
> >  EXPERIMENTAL {
> >  	global:
> >
> >