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

[Akhil Goyal <akhil.goyal@nxp.com>]

Hi Fan,

> +
> +/**
> + * Asynchronous operation job descriptor.
> + * Used by HW crypto devices direct API call that supports such activity
> + **/
> +struct rte_crypto_sym_job {
> +	union {
> +		/**
> +		 * When RTE_CRYPTO_HW_ENQ_FLAG_IS_SGL bit is set in flags,
> sgl
> +		 * field is used as input data. Otherwise data_iova is
> +		 * used.
> +		 **/
> +		rte_iova_t data_iova;
> +		struct rte_crypto_sgl *sgl;
> +	};
> +	union {
> +		/**
> +		 * Different than cryptodev ops, all ofs and len fields have
> +		 * the unit of bytes (including Snow3G/Kasumi/Zuc.
> +		 **/
> +		struct {
> +			uint32_t cipher_ofs;
> +			uint32_t cipher_len;
> +		} cipher_only;
> +		struct {
> +			uint32_t auth_ofs;
> +			uint32_t auth_len;
> +			rte_iova_t digest_iova;
> +		} auth_only;
> +		struct {
> +			uint32_t aead_ofs;
> +			uint32_t aead_len;
> +			rte_iova_t tag_iova;
> +			uint8_t *aad;
> +			rte_iova_t aad_iova;
> +		} aead;
> +		struct {
> +			uint32_t cipher_ofs;
> +			uint32_t cipher_len;
> +			uint32_t auth_ofs;
> +			uint32_t auth_len;
> +			rte_iova_t digest_iova;
> +		} chain;
> +	};
> +	uint8_t *iv;
> +	rte_iova_t iv_iova;
> +};

NACK,
Why do you need this structure definitions again when you have similar ones
(the ones used in CPU crypto) available for the same purpose.
In case of CPU crypto, there were 2 main requirements
- synchronous API instead of enq +deq
- raw buffers.

Now for this patchset, the requirement is 
- raw buffers 
- asynchronous APIs

The data structure for raw buffers and crypto related offsets are already defined
So they should be reused.
And I believe with some changes in rte_crypto_op  and rte_crypto_sym_op,
We can support raw buffers with the same APIs.
Instead of m_src and m_dst, raw buffer data structures can be combined in a
Union and some of the fields in the rte_crypto_op can be left NULL in case of raw buffers.


> +/* Struct for user to perform HW specific enqueue/dequeue function calls */
> +struct rte_crypto_hw_ops {
> +	/* Driver written queue pair data pointer, should NOT be alterred by
> +	 * the user.
> +	 */
> +	void *qp;
> +	/* Function handler to enqueue AEAD job */
> +	rte_crypto_hw_enq_cb_fn enqueue_aead;
> +	/* Function handler to enqueue cipher only job */
> +	rte_crypto_hw_enq_cb_fn enqueue_cipher;
> +	/* Function handler to enqueue auth only job */
> +	rte_crypto_hw_enq_cb_fn enqueue_auth;
> +	/* Function handler to enqueue cipher + hash chaining job */
> +	rte_crypto_hw_enq_cb_fn enqueue_chain;
> +	/* Function handler to query processed jobs */
> +	rte_crypto_hw_query_processed query_processed;
> +	/* Function handler to dequeue one job and return opaque data stored
> */
> +	rte_crypto_hw_deq_one_cb_fn dequeue_one;
> +	/* Function handler to dequeue many jobs */
> +	rte_crypto_hw_deq_many_cb_fn dequeue_many;
> +	/* Reserved */
> +	void *reserved[8];
> +};

Why do we need such callbacks in the library?
These should be inside the drivers, or else we do the same for
Legacy case as well. The pain of finding the correct enq function for 
Appropriate crypto operation is already handled by all the drivers
And we can reuse that or else we modify it there as well.

We should not add a lot of data paths for the user, otherwise the
APIs will become centric to a particular vendor and it will be very difficult
For the user to migrate from one vendor to another and would defeat the
Purpose of DPDK which provide uniform abstraction layer for all the hardware
Vendors.

Adding other vendors to comment.

Regards,
Akhil