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

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

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;
};

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

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Monday, September 21, 2020 4:49 PM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; dev@dpdk.org; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: Trahe, Fiona <fiona.trahe@intel.com>; Kusztal, ArkadiuszX
> <arkadiuszx.kusztal@intel.com>; Dybkowski, AdamX
> <adamx.dybkowski@intel.com>; Bronowski, PiotrX
> <piotrx.bronowski@intel.com>; Anoob Joseph <anoobj@marvell.com>
> Subject: RE: [dpdk-dev v9 1/4] cryptodev: add crypto data-path service APIs
> 
> Hi Fan,
> > Hi AKhil
> >
> > ...
> > > IMO, the following union can clarify all doubts.
> > > @Ananyev, Konstantin: Any suggestions from your side?
> > >
> > > /** IV and aad information for various use cases. */
> > > 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;  < or any other useful name>
> > >         /* Supposed to be used with HW raw crypto API call. */
> > >         struct {
> > >                 void *cipher_iv_ptr;
> > >                 rte_iova_t cipher_iv_iova;
> > >                 void *auth_iv_ptr;
> > >                 rte_iova_t auth_iv_iova;
> > >                 void *digest_ptr;
> > >                 rte_iova_t digest_iova;
> > >         } hw_chain;
> > >         /* Supposed to be used with HW raw crypto API call. */
> > >         struct {
> > >                 void *iv_ptr;
> > >                 rte_iova_t iv_iova;
> > >                 void *digest_ptr;
> > >                 rte_iova_t digest_iova;
> > >                 void *aad_ptr;
> > >                 rte_iova_t aad_iova;
> > >         } hw_aead;
> > > };
> > >
> > >
> >
> > The above structure cannot support the array of multiple jobs but a single
> job.
> 
> So was your previous structure. Was it not tested before?
> 
> > So we have to use something like
> >
> > struct {
> > 	void **cipher_iv_ptr;
> 
> You can even drop _ptr from the name of each of them.
> 
> > 	rtei_iova_t *cipher_iv_iova;
> > 	...
> > } hw_chain;
> > struct {
> > 	void **iv_ptr;
> > 	rte_iova_t *iv_iova;
> > 	...
> > } hw_aead;
> >
> > Is it ok?
> >
> > Regards,
> > Fan