Hi Fan,
>
> This patch changes the symmetric session structure of cryptodev.
> The symmetric session now contains extra information for secure
> access purposes. The patch also includes the updates to the
> PMDs, test applications, and examples to fit the change.
Few more comments from me in-line below.
Also a generic question - docs also might also need to be updated?
Apart from that and naming collisions - looks good overall.
Konstantin
>
> Signed-off-by: Fan Zhang <roy.fan.zh...@intel.com>
> ---
> app/test-crypto-perf/cperf_ops.c | 11 +-
> app/test-crypto-perf/main.c | 91 ++++++++----
> drivers/crypto/aesni_gcm/aesni_gcm_pmd.c | 3 +-
> drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c | 3 +-
> drivers/crypto/armv8/rte_armv8_pmd.c | 3 +-
> drivers/crypto/kasumi/rte_kasumi_pmd.c | 3 +-
> drivers/crypto/openssl/rte_openssl_pmd.c | 3 +-
> drivers/crypto/snow3g/rte_snow3g_pmd.c | 3 +-
> drivers/crypto/zuc/rte_zuc_pmd.c | 3 +-
> examples/fips_validation/main.c | 43 ++++--
> examples/l2fwd-crypto/main.c | 66 ++++++---
> examples/vhost_crypto/main.c | 12 +-
Shouldn't ipsec-secgw also be updated?
> lib/librte_cryptodev/rte_cryptodev.c | 142 +++++++++++++++----
> lib/librte_cryptodev/rte_cryptodev.h | 65 ++++++++-
> lib/librte_cryptodev/rte_cryptodev_pmd.h | 4 +-
> lib/librte_vhost/rte_vhost_crypto.h | 9 +-
> lib/librte_vhost/vhost_crypto.c | 8 +-
> test/test/test_cryptodev.c | 220
> +++++++++++++++++------------
> test/test/test_cryptodev_blockcipher.c | 7 +-
> test/test/test_cryptodev_blockcipher.h | 1 +
> test/test/test_event_crypto_adapter.c | 32 +++--
> 21 files changed, 515 insertions(+), 217 deletions(-)
>
...
> diff --git a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
> b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
> index b0f5c4d67..135a2d8cb 100644
> --- a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
> +++ b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
> @@ -951,7 +951,8 @@ post_process_mb_job(struct aesni_mb_qp *qp, JOB_AES_HMAC
> *job)
> if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS) {
> memset(sess, 0, sizeof(struct aesni_mb_session));
> memset(op->sym->session, 0,
> - rte_cryptodev_sym_get_header_session_size());
> + rte_cryptodev_sym_get_header_session_size(
> + op->sym->session));
> rte_mempool_put(qp->sess_mp_priv, sess);
> rte_mempool_put(qp->sess_mp, op->sym->session);
> op->sym->session = NULL;
As a generic one - all the drivers for session-less sessions assume that
sess_mp is initialized with
DIM(sess_data[]) greater than their driver id.
It is a valid assumption, but should we add extra check in
rte_cryptodev_queue_pair_setup()?
...
> diff --git a/lib/librte_cryptodev/rte_cryptodev.c
> b/lib/librte_cryptodev/rte_cryptodev.c
> index 11776b6ac..920a32cd6 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.c
> +++ b/lib/librte_cryptodev/rte_cryptodev.c
> @@ -1143,7 +1143,6 @@ rte_cryptodev_pmd_callback_process(struct rte_cryptodev
> *dev,
> rte_spinlock_unlock(&rte_cryptodev_cb_lock);
> }
>
> -
> int
> rte_cryptodev_sym_session_init(uint8_t dev_id,
> struct rte_cryptodev_sym_session *sess,
> @@ -1160,12 +1159,15 @@ rte_cryptodev_sym_session_init(uint8_t dev_id,
> return -EINVAL;
>
> index = dev->driver_id;
> + if (index > sess->nb_drivers)
> + return -EINVAL;
>
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->sym_session_configure, -ENOTSUP);
>
> - if (sess->sess_private_data[index] == NULL) {
> + if (sess->sess_data[index].refcnt == 0) {
> ret = dev->dev_ops->sym_session_configure(dev, xforms,
> - sess, mp);
> + sess, mp);
> +
> if (ret < 0) {
> CDEV_LOG_ERR(
> "dev_id %d failed to configure session details",
> @@ -1174,6 +1176,7 @@ rte_cryptodev_sym_session_init(uint8_t dev_id,
> }
> }
>
> + sess->sess_data[index].refcnt++;
> return 0;
> }
>
> @@ -1212,10 +1215,70 @@ rte_cryptodev_asym_session_init(uint8_t dev_id,
> return 0;
> }
>
> +struct rte_cryptodev_sym_session_pool_private_data {
> + uint16_t nb_drivers;
> + /**< number of elements in sess_data array */
> + uint16_t priv_size;
> + /**< session private data will be placed after sess_data */
> +};
> +
> +struct rte_mempool *
> +rte_cryptodev_sym_session_pool_create(const char *name,
> + uint32_t nb_elts, uint32_t cache_size, uint16_t priv_size,
> + int socket_id)
> +{
> + struct rte_mempool *mp;
> + uint32_t elt_size;
> + struct rte_cryptodev_sym_session_pool_private_data *pool_priv;
> +
> + elt_size = (uint32_t)rte_cryptodev_sym_get_header_session_size(NULL) +
> + priv_size;
> + mp = rte_mempool_create(name, nb_elts, elt_size, cache_size,
> + (uint32_t)(sizeof(*pool_priv)),
> + NULL, NULL, NULL, NULL,
> + socket_id, 0);
> + if (mp == NULL)
> + CDEV_LOG_ERR("%s(name=%s) failed, rte_errno=%d\n",
> + __func__, name, rte_errno);
> +
> + pool_priv = rte_mempool_get_priv(mp);
> + if (!pool_priv) {
> + CDEV_LOG_ERR("%s(name=%s) failed to get private data\n",
> + __func__, name);
> + rte_mempool_free(mp);
> + return NULL;
> + }
> +
> + pool_priv->nb_drivers = nb_drivers;
> + pool_priv->priv_size = priv_size;
> +
> + return mp;
> +}
> +
> +static unsigned int
> +rte_cryptodev_sym_session_data_size(struct rte_cryptodev_sym_session *sess)
> +{
> + return (sizeof(sess->sess_data[0]) * sess->nb_drivers) +
> + sess->priv_size;
> +}
> +
> struct rte_cryptodev_sym_session *
> rte_cryptodev_sym_session_create(struct rte_mempool *mp)
> {
> struct rte_cryptodev_sym_session *sess;
> + struct rte_cryptodev_sym_session_pool_private_data *pool_priv;
> +
> + if (!mp) {
> + CDEV_LOG_ERR("Invalid mempool\n");
> + return NULL;
> + }
> +
> + pool_priv = rte_mempool_get_priv(mp);
> +
> + if (!pool_priv || mp->private_data_size < sizeof(*pool_priv)) {
> + CDEV_LOG_ERR("Invalid mempool\n");
> + return NULL;
> + }
>
> /* Allocate a session structure from the session pool */
> if (rte_mempool_get(mp, (void **)&sess)) {
> @@ -1226,7 +1289,12 @@ rte_cryptodev_sym_session_create(struct rte_mempool
> *mp)
> /* Clear device session pointer.
> * Include the flag indicating presence of user data
> */
> - memset(sess, 0, (sizeof(void *) * nb_drivers) + sizeof(uint8_t));
> + sess->nb_drivers = pool_priv->nb_drivers;
> + sess->priv_size = pool_priv->priv_size;
> + sess->userdata = 0;
> +
> + memset(sess->sess_data, 0,
> + rte_cryptodev_sym_session_data_size(sess));
>
> return sess;
> }
> @@ -1255,12 +1323,17 @@ rte_cryptodev_sym_session_clear(uint8_t dev_id,
> struct rte_cryptodev_sym_session *sess)
> {
> struct rte_cryptodev *dev;
> + uint8_t driver_id;
>
> dev = rte_cryptodev_pmd_get_dev(dev_id);
>
> if (dev == NULL || sess == NULL)
> return -EINVAL;
>
> + driver_id = dev->driver_id;
> + if (--sess->sess_data[driver_id].refcnt != 0)
> + return -EBUSY;
> +
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->sym_session_clear, -ENOTSUP);
>
> dev->dev_ops->sym_session_clear(dev, sess);
> @@ -1290,16 +1363,15 @@ int
> rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
> {
> uint8_t i;
> - void *sess_priv;
> struct rte_mempool *sess_mp;
>
> if (sess == NULL)
> return -EINVAL;
>
> /* Check that all device private data has been freed */
> + /* Check that all device private data has been freed */
> for (i = 0; i < nb_drivers; i++) {
As Fiona pointed we have to use sess->nb_drivers here.
> - sess_priv = get_sym_session_private_data(sess, i);
> - if (sess_priv != NULL)
> + if (sess->sess_data[i].refcnt != 0)
> return -EBUSY;
> }
>
> @@ -1336,14 +1408,24 @@ rte_cryptodev_asym_session_free(struct
> rte_cryptodev_asym_session *sess)
>
>
> unsigned int
> -rte_cryptodev_sym_get_header_session_size(void)
> +rte_cryptodev_sym_get_header_session_size(
> + struct rte_cryptodev_sym_session *sess)
That's a change in the public API, np in general,
but I suppose needs to be documented (release notes?).
> {
> /*
> * Header contains pointers to the private data
> * of all registered drivers, and a flag which
> * indicates presence of user data
> */
> - return ((sizeof(void *) * nb_drivers) + sizeof(uint8_t));
> + if (!sess) {
> + struct rte_cryptodev_sym_session s = {0};
> + s.nb_drivers = nb_drivers;
> + s.priv_size = 0;
> +
> + return (unsigned int)(sizeof(s) +
> + rte_cryptodev_sym_session_data_size(&s));
> + } else
> + return (unsigned int)(sizeof(*sess) +
> + rte_cryptodev_sym_session_data_size(sess));
> }
>
> unsigned int __rte_experimental
> @@ -1361,7 +1443,6 @@ unsigned int
> rte_cryptodev_sym_get_private_session_size(uint8_t dev_id)
> {
> struct rte_cryptodev *dev;
> - unsigned int header_size = sizeof(void *) * nb_drivers;
> unsigned int priv_sess_size;
>
> if (!rte_cryptodev_pmd_is_valid_dev(dev_id))
> @@ -1374,19 +1455,27 @@ rte_cryptodev_sym_get_private_session_size(uint8_t
> dev_id)
>
> priv_sess_size = (*dev->dev_ops->sym_session_get_size)(dev);
>
> - /*
> - * If size is less than session header size,
> - * return the latter, as this guarantees that
> - * sessionless operations will work
> - */
> - if (priv_sess_size < header_size)
> - return header_size;
> -
> return priv_sess_size;
>
> }
>
> unsigned int __rte_experimental
> +rte_cryptodev_sym_get_max_private_session_size(void)
> +{
> + unsigned int priv_sess_size = 0;
> + uint8_t dev_id;
> +
> + for (dev_id = 0; dev_id < rte_cryptodev_count(); dev_id++) {
> + unsigned int dev_p_size =
> + rte_cryptodev_sym_get_private_session_size(dev_id);
> +
> + priv_sess_size = RTE_MAX(priv_sess_size, dev_p_size);
> + }
> +
> + return priv_sess_size;
> +}
> +
> +unsigned int __rte_experimental
> rte_cryptodev_asym_get_private_session_size(uint8_t dev_id)
> {
> struct rte_cryptodev *dev;
> @@ -1415,15 +1504,10 @@ rte_cryptodev_sym_session_set_user_data(
> void *data,
> uint16_t size)
> {
> - uint16_t off_set = sizeof(void *) * nb_drivers;
> - uint8_t *user_data_present = (uint8_t *)sess + off_set;
> -
> - if (sess == NULL)
> + if (sess == NULL || sess->priv_size < size)
> return -EINVAL;
>
> - *user_data_present = 1;
> - off_set += sizeof(uint8_t);
> - rte_memcpy((uint8_t *)sess + off_set, data, size);
> + rte_memcpy(sess->sess_data + sess->nb_drivers, data, size);
> return 0;
> }
>
> @@ -1431,14 +1515,10 @@ void * __rte_experimental
> rte_cryptodev_sym_session_get_user_data(
> struct rte_cryptodev_sym_session *sess)
> {
> - uint16_t off_set = sizeof(void *) * nb_drivers;
> - uint8_t *user_data_present = (uint8_t *)sess + off_set;
> -
> - if (sess == NULL || !*user_data_present)
> + if (sess == NULL || sess->priv_size == 0)
> return NULL;
>
> - off_set += sizeof(uint8_t);
> - return (uint8_t *)sess + off_set;
> + return (void *)(sess->sess_data + sess->nb_drivers);
> }
>
> /** Initialise rte_crypto_op mempool element */
> diff --git a/lib/librte_cryptodev/rte_cryptodev.h
> b/lib/librte_cryptodev/rte_cryptodev.h
> index f9e7507da..999253f1f 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.h
> +++ b/lib/librte_cryptodev/rte_cryptodev.h
> @@ -951,14 +951,22 @@ rte_cryptodev_enqueue_burst(uint8_t dev_id, uint16_t
> qp_id,
> dev->data->queue_pairs[qp_id], ops, nb_ops);
> }
>
> -
> /** Cryptodev symmetric crypto session
> * Each session is derived from a fixed xform chain. Therefore each session
> * has a fixed algo, key, op-type, digest_len etc.
> */
> struct rte_cryptodev_sym_session {
> - __extension__ void *sess_private_data[0];
> - /**< Private symmetric session material */
> + uint64_t userdata;
> + /**< Can be used for external metadata */
> + uint16_t nb_drivers;
> + /**< number of elements in sess_data array */
> + uint16_t priv_size;
> + /**< session private data will be placed after sess_data */
> + __extension__ struct {
> + void *data;
> + uint16_t refcnt;
> + } sess_data[0];
> + /**< Driver specific session material, variable size */
> };
>
> /** Cryptodev asymmetric crypto session */
> @@ -968,6 +976,31 @@ struct rte_cryptodev_asym_session {
> };
>
> /**
> + * Create a symmetric session mempool.
> + *
> + * @param name
> + * The unique mempool name.
> + * @param nb_elts
> + * The number of elements in the mempool.
> + * @param cache_size
> + * The number of per-lcore cache elements
> + * @param priv_size
> + * The private data size of each session.
> + * @param socket_id
> + * The *socket_id* argument is the socket identifier in the case of
> + * NUMA. The value can be *SOCKET_ID_ANY* if there is no NUMA
> + * constraint for the reserved zone.
> + *
> + * @return
> + * - On success return size of the session
> + * - On failure returns 0
> + */
> +struct rte_mempool *
> +rte_cryptodev_sym_session_pool_create(const char *name,
> + uint32_t nb_elts, uint32_t cache_size, uint16_t priv_size,
> + int socket_id);
New function - needs experimental tag, I think.
> +
> +/**
> * Create symmetric crypto session header (generic with no private data)
> *
> * @param mempool Symmetric session mempool to allocate session
> @@ -1097,13 +1130,22 @@ rte_cryptodev_asym_session_clear(uint8_t dev_id,
> struct rte_cryptodev_asym_session *sess);
>
> /**
> - * Get the size of the header session, for all registered drivers.
> + * Get the size of the header session, for all registered drivers excluding
> + * the private data size
> + *
> + * @param sess
> + * The sym cryptodev session pointer
> *
> * @return
> - * Size of the symmetric eader session.
> + * - If sess is not NULL, return the size of the header session including
> + * the private data size defined within sess.
> + * - If sess is NULL, return the size of the header session excluded
> + * the private data size. This way can be used for creating the session
> + * mempool but the user has to add its private data to this return
> manually.
> */
> unsigned int
> -rte_cryptodev_sym_get_header_session_size(void);
> +rte_cryptodev_sym_get_header_session_size(
> + struct rte_cryptodev_sym_session *sess);
>
> /**
> * Get the size of the asymmetric session header, for all registered drivers.
> @@ -1129,6 +1171,17 @@ unsigned int
> rte_cryptodev_sym_get_private_session_size(uint8_t dev_id);
>
> /**
> + * Get the maximum size of the private symmetric session data
> + * for all devices.
> + *
> + * @return
> + * - Size of the maximum private data, if successful.
> + * - 0 if there is no device in the system.
> + */
> +unsigned int __rte_experimental
> +rte_cryptodev_sym_get_max_private_session_size(void);
> +
> +/**
> * Get the size of the private data for asymmetric session
> * on device
> *
