Hi Arek,
> -----Original Message-----
> From: Kusztal, ArkadiuszX <arkadiuszx.kusz...@intel.com>
> Sent: Monday, August 31, 2020 7:24 AM
> To: Zhang, Roy Fan <roy.fan.zh...@intel.com>; dev@dpdk.org
> Cc: akhil.go...@nxp.com; Trahe, Fiona <fiona.tr...@intel.com>; Dybkowski,
> AdamX <adamx.dybkow...@intel.com>; Bronowski, PiotrX
> <piotrx.bronow...@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.zh...@intel.com>
> Sent: piątek, 28 sierpnia 2020 14:58
> To: dev@dpdk.org
> Cc: akhil.go...@nxp.com; Trahe, Fiona <fiona.tr...@intel.com>; Kusztal,
> ArkadiuszX <arkadiuszx.kusz...@intel.com>; Dybkowski, AdamX
> <adamx.dybkow...@intel.com>; Zhang, Roy Fan <roy.fan.zh...@intel.com>;
> Bronowski, PiotrX <piotrx.bronow...@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.zh...@intel.com>
> Signed-off-by: Piotr Bronowski <piotrx.bronow...@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
