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

[Akhil Goyal]

Hi Konstantin,
> Hi lads,
> 
> >
> > Hi Akhil,
> >
> > Thanks again for the review!
> > To summarize, the following places to be changed for v10.
> >
> > 1. Documentation update and reviewed internally in Intel first.
> > 2. Add the missing comments to the structure.
> > 3. Change the name "dp_service" to "raw_dp" to all APIs and documentation.
> > 4. Change the structure
> > struct rte_crypto_sym_vec {
> > 	/** array of SGL vectors */
> > 	struct rte_crypto_sgl *sgl;
> >
> > 	union {
> > 		/** Supposed to be used with CPU crypto API call. */
> > 		struct {
> > 			/** array of pointers to IV */
> > 			void **iv;
> > 			/** array of pointers to AAD */
> > 			void **aad;
> > 			/** array of pointers to digest */
> > 			void **digest;
> > 		} cpu_crypto;
> > 		/** Supposed to be used with HW raw crypto API call. */
> > 		struct {
> > 			/** array of pointers to cipher IV */
> > 			void **cipher_iv_ptr;
> > 			/** array of IOVA addresses to cipher IV */
> > 			rte_iova_t *cipher_iv_iova;
> > 			/** array of pointers to auth IV */
> > 			void **auth_iv_ptr;
> > 			/** array of IOVA addresses to auth IV */
> > 			rte_iova_t *auth_iv_iova;
> > 			/** array of pointers to digest */
> > 			void **digest_ptr;
> > 			/** array of IOVA addresses to digest */
> > 			rte_iova_t *digest_iova;
> > 		} hw_chain;
> > 		/** Supposed to be used with HW raw crypto API call. */
> > 		struct {
> > 			/** array of pointers to AEAD IV */
> > 			void **iv_ptr;
> > 			/** array of IOVA addresses to AEAD IV */
> > 			rte_iova_t *iv_iova;
> > 			/** array of pointers to AAD */
> > 			void **aad_ptr;
> > 			/** array of IOVA addresses to AAD */
> > 			rte_iova_t *aad_iova;
> > 			/** array of pointers to digest */
> > 			void **digest_ptr;
> > 			/** array of IOVA addresses to digest */
> > 			rte_iova_t *digest_iova;
> > 		} hw_aead;
> > 	};
> >
> > 	/**
> > 	 * array of statuses for each operation:
> > 	 *  - 0 on success
> > 	 *  - errno on error
> > 	 */
> > 	int32_t *status;
> > 	/** number of operations to perform */
> > 	uint32_t num;
> > };
> 
> 
> As I understand you just need to add pointers to iova[] for iv, aad and digest,
> correct?
> If so, why not simply:
> 
> struct rte_va_iova_ptr {
> 	void *va;
> 	rte_iova_t *iova;
> };
> 
> struct rte_crypto_sym_vec {
>         /** array of SGL vectors */
>         struct rte_crypto_sgl *sgl;
>         /** array of pointers to IV */
>         struct rte_va_iova_ptr iv;
>         /** array of pointers to AAD */
>         struct rte_va_iova_ptr aad;
>         /** array of pointers to digest */
>         struct rte_va_iova_ptr digest;
>         /**
>          * array of statuses for each operation:
>          *  - 0 on success
>          *  - errno on error
>          */
>         int32_t *status;
>         /** number of operations to perform */
>         uint32_t num;
> };
> 
> BTW, it would be both ABI and API breakage,
> though all functions using this struct are marked as experimental,
> plus it is an LTS release, so it seems to be ok.
> Though I think it needs to be flagged in RN.

This is a good suggestion. This will make some changes in the cpu-crypto support as well
And should be a separate patch.
We can take the API and ABI breakage in this release. That is not an issue.


> 
> Another option obviously - introduce completely new structure for it
> and leave existing one unaffected.
> 
This will create some duplicate code. Would not prefer that.

> >
> > 5. Remove enum rte_crypto_dp_service, let the PMDs using the session private
> data to decide function handler.
> > 6. Remove is_update parameter.
> >
> > The main point that is uncertain is the existance of "submit_single".
> > I am ok to remove "submit_single" function. In VPP we can use
> rte_cryptodev_dp_sym_submit_vec() with vec.num=1 each time to avoid
> > double looping.
> > But we have to put the rte_cryptodev_dp_sym_submit_vec() as an inline
> function - this will cause the API not traced in version map.
> >
> > Any ideas?
> >
> > Regards,
> > Fan
> >