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.

Another option obviously - introduce completely new structure for it
and leave existing one unaffected.

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

quoted

-----Original Message-----
From: Akhil Goyal <redacted>
Sent: Monday, September 21, 2020 4:49 PM
To: Zhang, Roy Fan <redacted>; dev@dpdk.org; Ananyev,
Konstantin [off-list ref]; Thomas Monjalon
[off-list ref]
Cc: Trahe, Fiona <redacted>; Kusztal, ArkadiuszX
[off-list ref]; Dybkowski, AdamX
[off-list ref]; Bronowski, PiotrX
[off-list ref]; Anoob Joseph [off-list ref]
Subject: RE: [dpdk-dev v9 1/4] cryptodev: add crypto data-path service APIs

Hi Fan,

quoted

Hi AKhil

...

quoted

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?

quoted

So we have to use something like

struct {
	void **cipher_iv_ptr;

You can even drop _ptr from the name of each of them.

quoted

	rtei_iova_t *cipher_iv_iova;
	...
} hw_chain;
struct {
	void **iv_ptr;
	rte_iova_t *iv_iova;
	...
} hw_aead;

Is it ok?

Regards,
Fan