Re: [dpdk-dev] [dpdk-dev v7 1/4] cryptodev: add crypto data-path service APIs

["Zhang, Roy Fan" <roy.fan.zhang@intel.com>]

Hi Arek,


> -----Original Message-----
> From: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>
> Sent: Monday, August 31, 2020 7:24 AM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; dev@dpdk.org
> Cc: akhil.goyal@nxp.com; Trahe, Fiona <fiona.trahe@intel.com>; Dybkowski,
> AdamX <adamx.dybkowski@intel.com>; Bronowski, PiotrX
> <piotrx.bronowski@intel.com>
> Subject: RE: [dpdk-dev v7 1/4] cryptodev: add crypto data-path service APIs
> 
> Hi Fan, Piotrek,
> 
> -----Original Message-----
> From: Zhang, Roy Fan <roy.fan.zhang@intel.com>
> Sent: piątek, 28 sierpnia 2020 14:58
> To: dev@dpdk.org
> Cc: akhil.goyal@nxp.com; Trahe, Fiona <fiona.trahe@intel.com>; Kusztal,
> ArkadiuszX <arkadiuszx.kusztal@intel.com>; Dybkowski, AdamX
> <adamx.dybkowski@intel.com>; Zhang, Roy Fan <roy.fan.zhang@intel.com>;
> Bronowski, PiotrX <piotrx.bronowski@intel.com>
> Subject: [dpdk-dev v7 1/4] cryptodev: add crypto data-path service APIs
> 
> This patch adds data-path service APIs for enqueue and dequeue operations
> to cryptodev. The APIs support flexible user-define enqueue and dequeue
> behaviors and operation mode.
> 
> Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
> Signed-off-by: Piotr Bronowski <piotrx.bronowski@intel.com>
> ---
>  lib/librte_cryptodev/rte_crypto.h             |   9 +
>  lib/librte_cryptodev/rte_crypto_sym.h         |  44 ++-
>  lib/librte_cryptodev/rte_cryptodev.c          |  45 +++
>  lib/librte_cryptodev/rte_cryptodev.h          | 335 +++++++++++++++++-
>  lib/librte_cryptodev/rte_cryptodev_pmd.h      |  47 ++-
>  .../rte_cryptodev_version.map                 |  10 +
>  6 files changed, 481 insertions(+), 9 deletions(-)
> 
> +/** Crypto data-path service types */
> +enum rte_crypto_dp_service {
> +	RTE_CRYPTO_DP_SYM_CIPHER_ONLY = 0,
> +	RTE_CRYPTO_DP_SYM_AUTH_ONLY,
> +	RTE_CRYPTO_DP_SYM_CHAIN,
> [Arek] - if it is auth-cipher/cipher-auth will be decided thanks to
> sym_session/xform?

[Fan] - yes.

> +	RTE_CRYPTO_DP_SYM_AEAD,
> +	RTE_CRYPTO_DP_N_SERVICE


...

> +			|| dev->dev_ops->configure_service == NULL)
> +		return -1;
> +
> [Arek] - Why to change order of arguments between
> rte_cryptodev_dp_configure_service and configure_service pointer? Except
> of dev and dev_id they all are the same.

[Fan] - I will update.
 
> + * Submit a data vector into device queue but the driver will not start
> + * processing until rte_cryptodev_dp_sym_submit_vec() is called.
> [Arek] " until ``rte_cryptodev_dp_submit_done`` function is called " ?

[Fan] - Yes, will update. Sorry for it.
 
> 
> + *
> + * @param	qp		Driver specific queue pair data.
> + * @param	service_data	Driver specific service data.
> + * @param	vec		The array of job vectors.
> + * @param	ofs		Start and stop offsets for auth and cipher
> + *				operations.
> + * @param	opaque		The array of opaque data for
> dequeue.
> + * @return
> + *   - The number of jobs successfully submitted.
> + */
> +typedef uint32_t (*cryptodev_dp_sym_submit_vec_t)(
> +	void *qp, uint8_t *service_data, struct rte_crypto_sym_vec *vec,
> +	union rte_crypto_sym_ofs ofs, void **opaque);
> +
> +/**
> + * Submit single job into device queue but the driver will not start
> + * processing until rte_cryptodev_dp_sym_submit_vec() is called.
> [Arek] until ``rte_cryptodev_dp_submit_done`` function is called " as above.

[Fan] - Yes, will update. Sorry for it.

> + *
> + * @param	qp		Driver specific queue pair data.
> + * @param	service_data	Driver specific service data.
> + * @param	data		The buffer vector.
> + * @param	n_data_vecs	Number of buffer vectors.
> + * @param	ofs		Start and stop offsets for auth and cipher
> + *				operations.
> + * @param	iv		IV data.
> + * @param	digest		Digest data.
> + * @param	aad		AAD data.
> + * @param	opaque		The opaque data for dequeue.
> + * @return
> + *   - On success return 0.
> + *   - On failure return negative integer.
> [Arek] How can we distinguish between malformed packet and full queue?

[Fan] We may have to rely on the application to track the inflight ops. 
We want to avoid use enqueue_burst same as rte_cryptodev_enqueue_burst,
as the caller application and the PMD has to loop the same job burst 2 times to write
and read the job content (cause >5% perf loss). The best option is providing an inline
function to enqueue one job at a time and provides the capability of abandoning the
enqueue if not all expected jobs are enqueued. So the idea is to create a shadow copy
of the device's queue stored in the user application and then have the capability
to abandon the enqueue when not all expected jobs are enqueued. The necessary
check has to be made by the driver when merging the user's local queue data into PMD's
queue data.

> + */
> +typedef int (*cryptodev_dp_submit_single_job_t)(
> +	void *qp, uint8_t *service_data, struct rte_crypto_vec *data,
> +	uint16_t n_data_vecs, union rte_crypto_sym_ofs ofs,
> +	struct rte_crypto_data *iv, struct rte_crypto_data *digest,
> +	struct rte_crypto_data *aad, void *opaque);
> +
> +/**


> +struct rte_crypto_dp_service_ctx {
> +	void *qp_data;
> +
> +	union {
> [Arek] - Why union? It will be extended by other structs in future?

[Fan] - yes, maybe used by asymmetric crypto in the future.
 
> [Arek] - unnamed union and struct, C11 extension.
[Fan] Will correct it.


> +		/* Supposed to be used for symmetric crypto service */
> +		struct {
> +			cryptodev_dp_submit_single_job_t
> submit_single_job;
> +			cryptodev_dp_sym_submit_vec_t submit_vec;
> +			cryptodev_dp_sym_operation_done_t submit_done;
> +			cryptodev_dp_sym_dequeue_t dequeue_opaque;
> +			cryptodev_dp_sym_dequeue_single_job_t
> dequeue_single;
> +			cryptodev_dp_sym_operation_done_t
> dequeue_done;
> +		};
> +	};
> +
> +	/* Driver specific service data */
> +	uint8_t drv_service_data[];
> [Arek] - flexible array, C99 so again extension
[Fan] Will correct it.
> +};
> +
> +/**
> + * Configure one DP service context data. Calling this function for the
> +first
> + * time the user should unset the *is_update* parameter and the driver
> +will
> + * fill necessary operation data into ctx buffer. Only when
> + * rte_cryptodev_dp_submit_done() is called the data stored in the ctx
> +buffer
> + * will not be effective.
> + *
> + * @param	dev_id		The device identifier.
> + * @param	qp_id		The index of the queue pair from which to
> + *				retrieve processed packets. The value must
> be
> + *				in the range [0, nb_queue_pair - 1]
> previously
> + *				supplied to rte_cryptodev_configure().
> + * @param	service_type	Type of the service requested.
> + * @param	sess_type	session type.
> + * @param	session_ctx	Session context data.
> + * @param	ctx		The data-path service context data.
> + * @param	is_update	Set 1 if ctx is pre-initialized but need
> + *				update to different service type or session,
> + *				but the rest driver data remains the same.
> [Arek] - if user will call it only once with is_update == 1 will there be any error
> shown about ctx not set when started processing or is it undefined behavior?
 [Fan] - the driver will not know the difference so we may have to rely on the user
to  set this correctly
> +	rte_cryptodev_dp_configure_service;
> +	rte_cryptodev_get_dp_service_ctx_data_size;
> 
> 
> [Arek] - I know its experimental but following 6 functions are not only static
> but __always_inline__ too, I doubt there will be any symbol generated,
> should they be placed inside map file then?


[Fan] the symbols are not generated. I also have the same question - do we need
to place them in the map file?

> [Arek] - Another thing since symbol is not created these funcs are outside of
> abidifftool scope of interest either I think.
> 
> +	rte_cryptodev_dp_submit_single_job;
> +	rte_cryptodev_dp_sym_submit_vec;
> +	rte_cryptodev_dp_submit_done;
> +	rte_cryptodev_dp_sym_dequeue;
> +	rte_cryptodev_dp_sym_dequeue_single_job;
> +	rte_cryptodev_dp_dequeue_done;
> };
> 
> [Arek] - Two small things:
> 	- rte_crypto_data - there is many thing in asymmetric/symmetric
> crypto that can be called like that, and this is basically symmetric
> encrypted/authenticated data pointer, maybe some more specific name
> could be better.
> 	- can iv, digest, aad be grouped into one? All share the same features,
> and there would less arguments to functions.
[Fan] - answered in the last email. Grouping is a good idea but may limit the usage.
We need a structure that does not necessarily contains the length info for all
data pre-defined when creating the sessions.
> --
> 2.20.1