Re: [dpdk-dev] [PATCH 2/2] cryptodev: change symmetric session structure

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

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.zhang@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
>   *