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