Re: [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session

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

Hi Konstantin,
Sorry for the delay in reviewing this and thanks for your proposals.

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, August 24, 2018 10:48 AM
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>; Doherty, Declan
> <declan.doherty@intel.com>; Ravi Kumar <ravi1.kumar@amd.com>; Jerin Jacob
> <jerin.jacob@caviumnetworks.com>; Zhang, Roy Fan <roy.fan.zhang@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Tomasz Duszynski <tdu@semihalf.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Natalie Samsonov <nsamsono@marvell.com>; Dmitri Epshtein
> <dima@marvell.com>; Jay Zhou <jianjay.zhou@huawei.com>
> Subject: [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
> 
> This RFC for proposes several changes inside rte_cryptodev_sym_session.
> Note that this is just RFC not a complete patch, so for now
> I modified only the librte_cryptodev itself,
> some cryptodev PMD, test-crypto-perf and ipsec-secgw example.
> Proposed changes means ABI/API breakage inside cryptodev,
> so looking for feedback from crypto-dev lib and crypto-PMD maintainiers.
> Below are details and reasoning for proposed changes.
> 
> 1.rte_cryptodev_sym_session_init()/ rte_cryptodev_sym_session_clear()
>   operate based on cytpodev device id, though inside
>   rte_cryptodev_sym_session device specific data is addressed
>   by driver id (not device id).
>   That creates a problem with current implementation when we have
>   two or more devices with the same driver used by the same session.
>   Consider the following example:
> 
>   struct rte_cryptodev_sym_session *sess;
>   rte_cryptodev_sym_session_init(dev_id=X, sess, ...);
>   rte_cryptodev_sym_session_init(dev_id=Y, sess, ...);
>   rte_cryptodev_sym_session_clear(dev_id=X, sess);
> 
>   After that point if X and Y uses the same driver,
>   then sess can't be used by device Y any more.
>   The reason for that - driver specific (not device specific)
>   data per session, plus there is no information
>   how many device instances use that data.
>   Probably the simplest way to deal with that issue -
>   add a reference counter per each driver data.
[Fiona] Ok, I agree with this issue and proposed fix.
We need to also document that it's user's responsibility
not to call either init() or clear() twice on same device, as
that would break the ref count.
The same should be added to asym_session - though I accept
it'sbe outside of the scope of this patch.


> 2.rte_cryptodev_sym_session_set_user_data() and
>   rte_cryptodev_sym_session_get_user_data() -
>   with current implementation there is no defined way for the user to
>   determine what is the max allowed size of the private data.
>   Even within rte_cryptodev_sym_session_set_user_data() we just blindly
>   copying user provided data without checking memory boundaries violation.
>   To overcome that issue I added 'uint16_t priv_size' into
>   rte_cryptodev_sym_session structure.
[Fiona] I agree, this is needed.
But I propose to call it user_data_sz NOT priv_size. 
See https://patches.dpdk.org/patch/42515/ to understand why.

 
> 3.rte_cryptodev_sym_session contains an array of variable size for
>   driver specific data.
>   Though number of elements in that array is determined by static
>   variable nb_drivers, that could be modified by
>   rte_cryptodev_allocate_driver().
>   That construction seems to work ok so far, as right now users register
>   all their PMDs at startup, though it doesn't mean that it would always
>   remain like that.
>   To make it less error prone I added 'uint16_t nb_drivers' into the
>   rte_cryptodev_sym_session structure.
>   At least that allows related functions to check that provided
>   driver id wouldn't overrun variable array boundaries,
>   again it allows to determine size of already allocated session
>   without accessing global variable.
[Fiona] I agree with both issue and solution. 
The same should be added to asym_session - though again
it's outside of the scope of this patch.

> 4.#2 and #3 above implies that now each struct rte_cryptodev_sym_session
>   would have sort of readonly type data (init once at allocation time,
>   keep unmodified through session life-time).
>   That requires more changes in current cryptodev implementation:
>   Right now inside cryptodev framework both rte_cryptodev_sym_session
>   and driver specific session data are two completely different sctrucures
>   (e.g. struct struct null_crypto_session and struct null_crypto_session).
>   Though current cryptodev implementation implicitly assumes that driver
>   will allocate both of them from within the same mempool.
>   Plus this is done in a manner that they override each other fields
>   (reuse the same space - sort of implicit C union).
>   That's probably not the best programming practice,
>   plus make impossible to have readonly fields inside both of them.
>   So to overcome that situation I changed an API a bit, to allow
>   to use two different mempools for these two distinct data structures.
> 
>  5. Add 'uint64_t userdata' inside struct rte_cryptodev_sym_session.
>    I suppose that self-explanatory, and might be used in a lot of places
>    (would be quite useful for ipsec library we develop).
[Fiona] Seems unnecessary - the set_user_data can be used. Why have 2
separate user data spaces in the session? - it's confusing.
If these is some good reason, then a different name should be used for clarity.
Not private. And not user. Possibly opaque data. Though afaik we had this in the op
and removed it as it was felt appending user_data was enough.


> So the new proposed layout for rte_cryptodev_sym_session:
> struct rte_cryptodev_sym_session {
>         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 */
> };
> 
> 
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
>  app/test-crypto-perf/cperf.h                       |   1 +
>  app/test-crypto-perf/cperf_ops.c                   |  11 +-
>  app/test-crypto-perf/cperf_ops.h                   |   2 +-
>  app/test-crypto-perf/cperf_test_latency.c          |   5 +-
>  app/test-crypto-perf/cperf_test_latency.h          |   1 +
>  app/test-crypto-perf/cperf_test_pmd_cyclecount.c   |   5 +-
>  app/test-crypto-perf/cperf_test_pmd_cyclecount.h   |   1 +
>  app/test-crypto-perf/cperf_test_throughput.c       |   5 +-
>  app/test-crypto-perf/cperf_test_throughput.h       |   1 +
>  app/test-crypto-perf/cperf_test_verify.c           |   5 +-
>  app/test-crypto-perf/cperf_test_verify.h           |   1 +
>  app/test-crypto-perf/main.c                        | 111 +++++++++++------
>  drivers/crypto/aesni_gcm/aesni_gcm_pmd.c           |  10 +-
>  drivers/crypto/aesni_gcm/aesni_gcm_pmd_ops.c       |   5 +-
>  drivers/crypto/aesni_gcm/aesni_gcm_pmd_private.h   |   4 +-
>  drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c         |  10 +-
>  drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c     |   5 +-
>  drivers/crypto/aesni_mb/rte_aesni_mb_pmd_private.h |   4 +-
>  drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c        |   3 +-
>  drivers/crypto/dpaa_sec/dpaa_sec.c                 |   3 +-
>  drivers/crypto/null/null_crypto_pmd.c              |  14 ++-
>  drivers/crypto/null/null_crypto_pmd_ops.c          |   5 +-
>  drivers/crypto/null/null_crypto_pmd_private.h      |   4 +-
>  drivers/crypto/scheduler/scheduler_pmd_ops.c       |   5 +-
>  drivers/crypto/virtio/virtio_cryptodev.c           |   6 +-
>  examples/ipsec-secgw/ipsec-secgw.c                 | 116 ++++++++++++------
>  examples/ipsec-secgw/ipsec.h                       |   2 +
>  lib/librte_cryptodev/rte_cryptodev.c               | 134 ++++++++++++---------
>  lib/librte_cryptodev/rte_cryptodev.h               |  53 ++++++--
>  lib/librte_cryptodev/rte_cryptodev_pmd.h           |  16 ++-
>  30 files changed, 356 insertions(+), 192 deletions(-)
> 
///snip///

>  struct cnt_blk {
> diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
> index 63ae23f00..e25282445 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.c
> +++ b/lib/librte_cryptodev/rte_cryptodev.c
> @@ -943,8 +943,7 @@ rte_cryptodev_close(uint8_t dev_id)
> 
>  int
>  rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
> -		const struct rte_cryptodev_qp_conf *qp_conf, int socket_id,
> -		struct rte_mempool *session_pool)
> +		const struct rte_cryptodev_qp_conf *qp_conf, int socket_id)
> 
>  {
>  	struct rte_cryptodev *dev;
> @@ -954,6 +953,12 @@ rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
>  		return -EINVAL;
>  	}
> 
> +	if (qp_conf == NULL || qp_conf->sess_pool == NULL ||
> +			qp_conf->priv_sess_pool == NULL) {
> +		CDEV_LOG_ERR("Invalid queue_pair config");
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_crypto_devices[dev_id];
>  	if (queue_pair_id >= dev->data->nb_queue_pairs) {
>  		CDEV_LOG_ERR("Invalid queue_pair_id=%d", queue_pair_id);
> @@ -969,7 +974,7 @@ rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->queue_pair_setup, -ENOTSUP);
> 
>  	return (*dev->dev_ops->queue_pair_setup)(dev, queue_pair_id, qp_conf,
> -			socket_id, session_pool);
> +			socket_id);
>  }
> 
> 
> @@ -1146,6 +1151,41 @@ rte_cryptodev_pmd_callback_process(struct rte_cryptodev *dev,
>  	rte_spinlock_unlock(&rte_cryptodev_cb_lock);
>  }
> 
> +static void
> +cryptodev_sym_session_init_elem(__rte_unused struct rte_mempool *pool,
> +	void *arg, void *obj, __rte_unused uint32_t idx)
> +{
> +	struct rte_cryptodev_sym_session *ds;
> +	const struct rte_cryptodev_sym_session *ss;
> +
> +	ds = obj;
> +	ss = arg;
> +
> +	*ds = *ss;
> +	memset(ds->sess_data, 0, rte_cryptodev_sym_session_data_size(ds));
> +}
> +
> +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 s = {
> +		.nb_drivers = nb_drivers,
> +		.priv_size = priv_size,
> +	};
> +
> +	elt_size = rte_cryptodev_sym_session_max_size(priv_size);
> +	mp = rte_mempool_create(name, nb_elts, elt_size, cache_size, 0,
> +		NULL, NULL, cryptodev_sym_session_init_elem, &s,
> +		socket_id, 0);
> +	if (mp == NULL)
> +		CDEV_LOG_ERR("%s(name=%s) failed, rte_errno=%d\n",
> +			__func__, name, rte_errno);
> +	return mp;
> +}
> 
>  int
>  rte_cryptodev_sym_session_init(uint8_t dev_id,
> @@ -1163,12 +1203,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",
> @@ -1177,6 +1220,7 @@ rte_cryptodev_sym_session_init(uint8_t dev_id,
>  		}
>  	}
> 
> +	sess->sess_data[index].refcnt++;
>  	return 0;
>  }
> 
> @@ -1229,8 +1273,7 @@ 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));
> -
> +	memset(sess->sess_data, 0, rte_cryptodev_sym_session_data_size(sess));
>  	return sess;
>  }
> 
> @@ -1258,16 +1301,20 @@ rte_cryptodev_sym_session_clear(uint8_t dev_id,
>  		struct rte_cryptodev_sym_session *sess)
>  {
>  	struct rte_cryptodev *dev;
> +	uint32_t idx;
> 
>  	dev = rte_cryptodev_pmd_get_dev(dev_id);
> 
> -	if (dev == NULL || sess == NULL)
> +	if (dev == NULL || sess == NULL || dev->driver_id > sess->nb_drivers)
>  		return -EINVAL;
> 
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->sym_session_clear, -ENOTSUP);
> 
> -	dev->dev_ops->sym_session_clear(dev, sess);
> +	idx = dev->driver_id;
> +	if (--sess->sess_data[idx].refcnt != 0)
> +		return -EBUSY;
> 
> +	dev->dev_ops->sym_session_clear(dev, sess);
>  	return 0;
>  }
> 
> @@ -1285,7 +1332,6 @@ rte_cryptodev_asym_session_clear(uint8_t dev_id,
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->asym_session_clear, -ENOTSUP);
> 
>  	dev->dev_ops->asym_session_clear(dev, sess);
> -
>  	return 0;
>  }
> 
> @@ -1293,7 +1339,6 @@ 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)
> @@ -1301,8 +1346,7 @@ rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
> 
>  	/* Check that all device private data has been freed */
>  	for (i = 0; i < nb_drivers; i++) {
[Fiona] Use the sess.nb_drivers rather than the global.
Actually maybe name slightly differently to reduce the chance of that mistake happening, e.g. 
rename sess.nb_drivers to sess.num_drivers?

> -		sess_priv = get_sym_session_private_data(sess, i);
> -		if (sess_priv != NULL)
> +		if (sess->sess_data[i].refcnt != 0)
>  			return -EBUSY;
>  	}
> 
> @@ -1313,6 +1357,23 @@ rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
>  	return 0;
>  }
> 
> +unsigned int
> +rte_cryptodev_sym_session_max_data_size(void)
[Fiona] Suggest renaming this 
rte_cryptodev_sym_session_max_array_size()

> +{
> +	struct rte_cryptodev_sym_session *sess = NULL;
> +
> +	return (sizeof(sess->sess_data[0]) * nb_drivers);
> +}
> +
> +size_t
> +rte_cryptodev_sym_session_max_size(uint16_t priv_size)
> +{
> +	struct rte_cryptodev_sym_session *sess = NULL;
> +
> +	return (sizeof(*sess) + priv_size +
> +		rte_cryptodev_sym_session_max_data_size());
> +}
> +
>  int __rte_experimental
>  rte_cryptodev_asym_session_free(struct rte_cryptodev_asym_session *sess)
>  {
> @@ -1337,18 +1398,6 @@ rte_cryptodev_asym_session_free(struct rte_cryptodev_asym_session *sess)
>  	return 0;
>  }
> 
> -
> -unsigned int
> -rte_cryptodev_sym_get_header_session_size(void)
> -{
> -	/*
> -	 * 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));
> -}
> -
>  unsigned int __rte_experimental
>  rte_cryptodev_asym_get_header_session_size(void)
>  {
> @@ -1361,11 +1410,9 @@ rte_cryptodev_asym_get_header_session_size(void)
>  }
> 
>  unsigned int
> -rte_cryptodev_sym_get_private_session_size(uint8_t dev_id)
> +rte_cryptodev_sym_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))
>  		return 0;
> @@ -1375,18 +1422,7 @@ rte_cryptodev_sym_get_private_session_size(uint8_t dev_id)
>  	if (*dev->dev_ops->sym_session_get_size == NULL)
>  		return 0;
> 
> -	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;
> -
> +	return (*dev->dev_ops->sym_session_get_size)(dev);
>  }
> 
>  unsigned int __rte_experimental
> @@ -1409,7 +1445,6 @@ rte_cryptodev_asym_get_private_session_size(uint8_t dev_id)
>  		return header_size;
> 
>  	return priv_sess_size;
> -
>  }
> 
>  int __rte_experimental
> @@ -1418,15 +1453,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;
>  }
> 
> @@ -1434,14 +1464,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 (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 4099823f1..d88454f02 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.h
> +++ b/lib/librte_cryptodev/rte_cryptodev.h
> @@ -495,6 +495,14 @@ enum rte_cryptodev_event_type {
>  /** Crypto device queue pair configuration structure. */
>  struct rte_cryptodev_qp_conf {
>  	uint32_t nb_descriptors; /**< Number of descriptors per queue pair */
> +	struct rte_mempool *sess_pool;
> +	/**< Pointer to crypto sessions mempool,
> +	 * used for session-less operations.
> +	 */
> +	struct rte_mempool *priv_sess_pool;
> +	/**< Pointer to device specific sessions mempool,
> +	 * used for session-less operations.
> +	 */
>  };
> 
>  /**
> @@ -680,17 +688,13 @@ rte_cryptodev_close(uint8_t dev_id);
>   *				*SOCKET_ID_ANY* if there is no NUMA constraint
>   *				for the DMA memory allocated for the receive
>   *				queue pair.
> - * @param	session_pool	Pointer to device session mempool, used
> - *				for session-less operations.
> - *
>   * @return
>   *   - 0: Success, queue pair correctly set up.
>   *   - <0: Queue pair configuration failed
>   */
>  extern int
>  rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
> -		const struct rte_cryptodev_qp_conf *qp_conf, int socket_id,
> -		struct rte_mempool *session_pool);
> +		const struct rte_cryptodev_qp_conf *qp_conf, int socket_id);
> 
>  /**
>   * Get the number of queue pairs on a specific crypto device
> @@ -954,10 +958,43 @@ rte_cryptodev_enqueue_burst(uint8_t dev_id, uint16_t qp_id,
>   * 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 */
>  };
> 
> +static inline size_t
> +rte_cryptodev_sym_session_data_size(const struct rte_cryptodev_sym_session *s)
> +{
> +	return (sizeof(s->sess_data[0]) * s->nb_drivers);
> +}
> +
> +static inline size_t
> +rte_cryptodev_sym_session_size(const struct rte_cryptodev_sym_session *s)
> +{
> +	return (sizeof(*s) + (s)->priv_size +
> +		rte_cryptodev_sym_session_data_size(s));
> +}
> +
[Fiona] Are above 2 fns used? 
Look like duplicates of the "max" fns?


> +unsigned int
> +rte_cryptodev_sym_session_max_data_size(void);
> +
> +size_t
> +rte_cryptodev_sym_session_max_size(uint16_t priv_size);
> +
> +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);
> +
>  /** Cryptodev asymmetric crypto session */
>  struct rte_cryptodev_asym_session {
>  	__extension__ void *sess_private_data[0];
> @@ -1123,7 +1160,7 @@ rte_cryptodev_asym_get_header_session_size(void);
>   *   symmetric session
>   */
>  unsigned int
> -rte_cryptodev_sym_get_private_session_size(uint8_t dev_id);
> +rte_cryptodev_sym_private_session_size(uint8_t dev_id);
> 
>  /**
>   * Get the size of the private data for asymmetric session
> diff --git a/lib/librte_cryptodev/rte_cryptodev_pmd.h b/lib/librte_cryptodev/rte_cryptodev_pmd.h
> index 6ff49d64d..2f98f65d1 100644
> --- a/lib/librte_cryptodev/rte_cryptodev_pmd.h
> +++ b/lib/librte_cryptodev/rte_cryptodev_pmd.h
> @@ -191,13 +191,12 @@ typedef void (*cryptodev_info_get_t)(struct rte_cryptodev *dev,
>   * @param	qp_id		Queue Pair Index
>   * @param	qp_conf		Queue configuration structure
>   * @param	socket_id	Socket Index
> - * @param	session_pool	Pointer to device session mempool
>   *
>   * @return	Returns 0 on success.
>   */
>  typedef int (*cryptodev_queue_pair_setup_t)(struct rte_cryptodev *dev,
>  		uint16_t qp_id,	const struct rte_cryptodev_qp_conf *qp_conf,
> -		int socket_id, struct rte_mempool *session_pool);
> +		int socket_id);
> 
>  /**
>   * Release memory resources allocated by given queue pair.
> @@ -478,20 +477,25 @@ RTE_INIT(init_ ##driver_id)\
> 
>  static inline void *
>  get_sym_session_private_data(const struct rte_cryptodev_sym_session *sess,
> -		uint8_t driver_id) {
> -	return sess->sess_private_data[driver_id];
> +		uint8_t driver_id)
> +{
> +	if (driver_id < sess->nb_drivers)
> +		return sess->sess_data[driver_id].data;
> +	return NULL;
>  }
> 
>  static inline void
>  set_sym_session_private_data(struct rte_cryptodev_sym_session *sess,
>  		uint8_t driver_id, void *private_data)
>  {
> -	sess->sess_private_data[driver_id] = private_data;
> +	if (driver_id < sess->nb_drivers)
> +		sess->sess_data[driver_id].data = private_data;
>  }
> 
>  static inline void *
>  get_asym_session_private_data(const struct rte_cryptodev_asym_session *sess,
> -		uint8_t driver_id) {
> +		uint8_t driver_id)
> +{
>  	return sess->sess_private_data[driver_id];
>  }
> 
> --
> 2.13.6