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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> 
> Hi Akhil and Konstantin,
> 
> > -----Original Message-----
> > From: Akhil Goyal <akhil.goyal@nxp.com>
> > Sent: Tuesday, September 22, 2020 10:06 AM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Zhang, Roy Fan
> > <roy.fan.zhang@intel.com>; dev@dpdk.org; 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 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;
> 
> We will need 2 IV here, one for cipher and one for auth (GMAC for example).

Hmm, why do we need to different IVs for GMAC?
And if so how does it work now with either rte_crypto_op or with rte_crypto_sym_vec?

> 
> > >         /** 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
> > > >