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.kusz...@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&data=02%7C01%7Cakhil.goyal%40nxp. > >> com%7Ce6789fd42a5946c128e508d7e2dffe2f%7C686ea1d3bc2b4c6fa92cd99c > >> 5c301635%7C0%7C0%7C637227323980059545&sdata=50eQJE7WHTME6d > >> qA7Nfk%2B50PVAyJrpKlMw%2BoGtA1%2FTc%3D&reserved=0) > > > > Please include the dependent patches in a single series in your next > > version. > >> > >> lib/librte_cryptodev/rte_cryptodev.c | 143 > >> ++++++++++++++++++++++++- > >> lib/librte_cryptodev/rte_cryptodev.h | 39 ++++++- > >> lib/librte_cryptodev/rte_cryptodev_version.map | 7 ++ > >> 3 files changed, 186 insertions(+), 3 deletions(-) > >> > >> diff --git a/lib/librte_cryptodev/rte_cryptodev.c > >> b/lib/librte_cryptodev/rte_cryptodev.c > >> index 6d1d0e9..b061447 100644 > >> --- a/lib/librte_cryptodev/rte_cryptodev.c > >> +++ b/lib/librte_cryptodev/rte_cryptodev.c > >> @@ -41,6 +41,9 @@ > >> #include "rte_cryptodev.h" > >> #include "rte_cryptodev_pmd.h" > >> > >> +#include <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 > >
