RE: [PATCH v3 1/7] cryptodev: add APIs to get/set event metadata

[Akhil Goyal <gakhil@marvell.com>]

Hi Abhinandan,

Please see inline.
> > +
> > +void *
> > +rte_cryptodev_session_event_mdata_get(struct rte_crypto_op *op) {
> Null check for op?

Null check can be added, but this a datapath dpdk internal API.
We do not normally add checks in datapath.
If you insist, I can add, but before calling this API, PMD/lib would have already
checked for null op.

> > +	if (op->type == RTE_CRYPTO_OP_TYPE_SYMMETRIC &&
> > +			op->sess_type == RTE_CRYPTO_OP_WITH_SESSION)
> > +		return rte_cryptodev_sym_session_get_user_data(op->sym-
> > >session);
> > +	else if (op->type == RTE_CRYPTO_OP_TYPE_ASYMMETRIC &&
> > +			op->sess_type == RTE_CRYPTO_OP_WITH_SESSION)
> > +		return op->asym->session->event_mdata;
> > +	else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
> > +			op->private_data_offset)
> > +		return ((uint8_t *)op + op->private_data_offset);
> > +	else
> > +		return NULL;
> > +}
> > diff --git a/lib/cryptodev/cryptodev_pmd.h b/lib/cryptodev/cryptodev_pmd.h
> > index 2b1ce2da2d..7969944b66 100644
> > --- a/lib/cryptodev/cryptodev_pmd.h
> > +++ b/lib/cryptodev/cryptodev_pmd.h
> > @@ -398,6 +398,25 @@ typedef int
> > (*cryptodev_sym_configure_raw_dp_ctx_t)(
> >  	enum rte_crypto_op_sess_type sess_type,
> >  	union rte_cryptodev_session_ctx session_ctx, uint8_t is_update);
> >
> > +/**
> > + * Typedef that the driver provided to set event crypto meta data.
> > + *
> > + * @param	dev		Crypto device pointer.
> > + * @param	sess		Crypto or security session.
> > + * @param	op_type		Operation type.
> > + * @param	sess_type	Session type.
> > + * @param	ev_mdata	Pointer to the event crypto meta data
> > + *				(aka *union rte_event_crypto_metadata*)
> > + * @return
> > + *   - On success return 0.
> > + *   - On failure return negative integer.
> > + */
> > +typedef int (*cryptodev_session_event_mdata_set_t)(
> > +	struct rte_cryptodev *dev, void *sess,
> > +	enum rte_crypto_op_type op_type,
> > +	enum rte_crypto_op_sess_type sess_type,
> > +	void *ev_mdata);
> > +
> >  /** Crypto device operations function pointer table */  struct
> > rte_cryptodev_ops {
> >  	cryptodev_configure_t dev_configure;	/**< Configure device. */
> > @@ -442,6 +461,8 @@ struct rte_cryptodev_ops {
> >  			/**< Initialize raw data path context data. */
> >  		};
> >  	};
> > +	cryptodev_session_event_mdata_set_t session_ev_mdata_set;
> > +	/**< Set a Crypto or Security session even meta data. */
> >  };
> >
> >
> > @@ -603,6 +624,19 @@ void
> >  cryptodev_fp_ops_set(struct rte_crypto_fp_ops *fp_ops,
> >  		     const struct rte_cryptodev *dev);
> >
> > +/**
> > + * Get session event meta data (aka *union rte_event_crypto_metadata*)
> > + *
> > + * @param	op            pointer to *rte_crypto_op* structure.
> > + *
> > + * @return
> > + *  - On success, pointer to event crypto metadata
> > + *  - On failure, a negative value.
> > + */
> > +__rte_internal
> > +void *
> > +rte_cryptodev_session_event_mdata_get(struct rte_crypto_op *op);
> > +
> >  static inline void *
> >  get_sym_session_private_data(const struct rte_cryptodev_sym_session
> *sess,
> >  		uint8_t driver_id) {
> > @@ -636,6 +670,8 @@ RTE_STD_C11 struct rte_cryptodev_asym_session {
> >  	/**< Size of private data used when creating mempool */
> >  	uint16_t user_data_sz;
> >  	/**< Session user data will be placed after sess_data */
> > +	void *event_mdata;
> > +	/**< Event crypto adapter metadata */
> Add reference to rte_event_crypto_metadata for clarity?

Ok will add the comment.

> >  	uint8_t padding[3];
> >  	uint8_t sess_private_data[0];
> >  };
> > diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
> index
> > 3500a2d470..a070cb2a00 100644
> > --- a/lib/cryptodev/rte_cryptodev.c
> > +++ b/lib/cryptodev/rte_cryptodev.c
> > @@ -2051,6 +2051,9 @@ rte_cryptodev_asym_session_free(uint8_t dev_id,
> > void *sess)
> >
> >  	dev->dev_ops->asym_session_clear(dev, sess);
> >
> > +	if (((struct rte_cryptodev_asym_session *)sess)->event_mdata)
> > +		rte_free(((struct rte_cryptodev_asym_session *)sess)-
> > >event_mdata);
> > +
> Who allocates memory for event_mdata? If this done by application before
> calling
> rte_cryptodev_session_event_mdata_set(), please document it.

It is same as was done before this patch.
The rte_cryptodev_session_event_mdata_set is allocating and making the copy
as it was copied in userdata before. Hence, no update is required.

> >  	/* Return session to mempool */
> >  	sess_mp = rte_mempool_from_obj(sess);
> >  	rte_mempool_put(sess_mp, sess);
> > @@ -2259,6 +2262,44 @@ rte_cryptodev_configure_raw_dp_ctx(uint8_t
> > dev_id, uint16_t qp_id,
> >  			sess_type, session_ctx, is_update);
> >  }
> >
> > +int
> > +rte_cryptodev_session_event_mdata_set(uint8_t dev_id, void *sess,
> > +	enum rte_crypto_op_type op_type,
> > +	enum rte_crypto_op_sess_type sess_type,
> > +	void *ev_mdata,
> > +	uint16_t size)
> > +{
> > +	struct rte_cryptodev *dev;
> > +
> > +	if (!rte_cryptodev_is_valid_dev(dev_id))
> > +		goto skip_pmd_op;
> > +
> > +	dev = rte_cryptodev_pmd_get_dev(dev_id);
> > +	if (dev->dev_ops->session_ev_mdata_set == NULL)
> > +		goto skip_pmd_op;
> > +
> > +	return (*dev->dev_ops->session_ev_mdata_set)(dev, sess, op_type,
> > +			sess_type, ev_mdata);
> > +
> > +skip_pmd_op:
> > +	if (op_type == RTE_CRYPTO_OP_TYPE_SYMMETRIC)
> > +		return rte_cryptodev_sym_session_set_user_data(sess,
> > ev_mdata,
> > +				size);
> > +	else if (op_type == RTE_CRYPTO_OP_TYPE_ASYMMETRIC) {
> > +		struct rte_cryptodev_asym_session *s = sess;
> Null check for sess?

Again, it is a datapath, avoided extra checks.

> > +
> > +		if (s->event_mdata == NULL) {
> > +			s->event_mdata = rte_malloc(NULL, size, 0);
> > +			if (s->event_mdata == NULL)
> > +				return -ENOMEM;
> > +		}
> > +		rte_memcpy(s->event_mdata, ev_mdata, size);
> > +
> > +		return 0;
> > +	} else
> > +		return -ENOTSUP;
> > +}