Re: [dpdk-dev] [dpdk-dev v1 1/7] crypro/qat: qat driver refactor skeleton

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

Hi Kai,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Kai Ji
> Sent: Tuesday, October 26, 2021 6:25 PM
> To: dev@dpdk.org
> Cc: Ji, Kai <kai.ji@intel.com>
> Subject: [dpdk-dev] [dpdk-dev v1 1/7] crypro/qat: qat driver refactor
> skeleton
> 
> Add-in enqueue/dequeue op burst and build-request skeleton functions
> for qat crypto driver refactor.
> 
> Signed-off-by: Kai Ji <kai.ji@intel.com>
> ---

...

> +/**
> + * @brief Function prototype to build a QAT request.
> + * @param opaque: an opaque data may be used to store context may be
> useful
> + *    between 2 enqueue operations.
> + **/
The comments for the function prototype can be polished to make it clearer.
Also the formatting shall be inline with the rest of dpdk code. Such as

/**
 * <brief introduction to the function prototype>
 *
 * @param x
 *   Description to the param x.
 * @return
 *   - 0 if the crypto request is built successfully.
*    - error code otherwise.
 */


> +typedef int (*qat_op_build_request_t)(void *in_op, uint8_t *out_msg,
> +		void *op_cookie, uint64_t *opaque, enum qat_device_gen
> dev_gen);
> +
> +typedef int (*qat_op_dequeue_t)(void **op, uint8_t *resp, void
> *op_cookie,
> +		uint64_t *dequeue_err_count __rte_unused);

The above function prototype worth some description too?

> +
> +#define QAT_BUILD_REQUEST_MAX_OPAQUE_SIZE	2
> +
>  struct qat_qp {
>  	void			*mmap_bar_addr;
>  	struct qat_queue	tx_q;
> @@ -44,6 +57,7 @@ struct qat_qp {
>  	struct rte_mempool *op_cookie_pool;
>  	void **op_cookies;
>  	uint32_t nb_descriptors;
> +	uint64_t opaque[QAT_BUILD_REQUEST_MAX_OPAQUE_SIZE];
>  	enum qat_device_gen qat_dev_gen;
>  	enum qat_service_type service_type;
>  	struct qat_pci_device *qat_dev;
> @@ -77,6 +91,14 @@ struct qat_qp_config {
>  	const char *service_str;
>  };
> 
> +uint16_t
> +refactor_qat_enqueue_op_burst(void *qp, qat_op_build_request_t
> op_build_request,
> +		void **ops, uint16_t nb_ops);
> +
> +uint16_t
> +refactor_qat_dequeue_op_burst(void *qp, void **ops,
> +		qat_op_dequeue_t qat_dequeue_process_response,
> uint16_t nb_ops);
> +
>  uint16_t
>  qat_enqueue_op_burst(void *qp, void **ops, uint16_t nb_ops);
> 
> diff --git a/drivers/crypto/qat/qat_asym.c b/drivers/crypto/qat/qat_asym.c
> index 85973812a8..d5b4c66d68 100644
> --- a/drivers/crypto/qat/qat_asym.c
> +++ b/drivers/crypto/qat/qat_asym.c
> @@ -456,6 +456,41 @@ qat_asym_fill_arrays(struct rte_crypto_asym_op
> *asym_op,
>  	return 0;
>  }
> 
> +static __rte_always_inline int
> +refactor_qat_asym_build_request(__rte_unused void *in_op,
> +		__rte_unused uint8_t *out_msg, __rte_unused void
> *op_cookie,
> +		__rte_unused uint64_t *opaque,
> +		__rte_unused enum qat_device_gen dev_gen)
> +{
> +	return 0;
> +}
> +
> +int
> +refactor_qat_asym_process_response(__rte_unused void **op,
> +		__rte_unused uint8_t *resp,
> +		__rte_unused void *op_cookie,
> +		__rte_unused uint64_t *dequeue_err_count)
> +{
> +	return 0;
> +}

I assume the functions refactor_xxx() is removed in future patches right?
Maybe adding some comments to help reviewing.

> +
> +uint16_t
> +qat_asym_crypto_enqueue_op_burst(void *qp, struct rte_crypto_op
> **ops,
> +		uint16_t nb_ops)
> +{
> +	return refactor_qat_enqueue_op_burst(qp,
> +					refactor_qat_asym_build_request,
> +					(void **)ops, nb_ops);
> +}
> +
> +uint16_t
> +qat_asym_crypto_dequeue_op_burst(void *qp, struct rte_crypto_op
> **ops,
> +		uint16_t nb_ops)
> +{
> +	return refactor_qat_dequeue_op_burst(qp, (void **)ops,
> +				refactor_qat_asym_process_response,
> nb_ops);
> +}
> +
>  int
>  qat_asym_build_request(void *in_op,
>  			uint8_t *out_msg,
> diff --git a/drivers/crypto/qat/qat_asym.h b/drivers/crypto/qat/qat_asym.h
> index 308b6b2e0b..50c2641eba 100644
> --- a/drivers/crypto/qat/qat_asym.h
> +++ b/drivers/crypto/qat/qat_asym.h
> @@ -92,4 +92,19 @@ void
>  qat_asym_process_response(void __rte_unused **op, uint8_t *resp,
>  		void *op_cookie);
> 
> +int
> +refactor_qat_asym_process_response(__rte_unused void **op,
> +		__rte_unused uint8_t *resp,
> +		__rte_unused void *op_cookie,
> +		__rte_unused uint64_t *dequeue_err_count);
> +
> +uint16_t
> +qat_asym_crypto_enqueue_op_burst(void *qp, struct rte_crypto_op
> **ops,
> +		uint16_t nb_ops);
> +
> +
> +uint16_t
> +qat_asym_crypto_dequeue_op_burst(void *qp, struct rte_crypto_op
> **ops,
> +		uint16_t nb_ops);
> +
>  #endif /* _QAT_ASYM_H_ */
> diff --git a/drivers/crypto/qat/qat_sym.c b/drivers/crypto/qat/qat_sym.c
> index 93b257522b..a92874cd27 100644
> --- a/drivers/crypto/qat/qat_sym.c
> +++ b/drivers/crypto/qat/qat_sym.c
> @@ -210,6 +210,31 @@ handle_spc_gmac(struct qat_sym_session *ctx,
> struct rte_crypto_op *op,
>  			ICP_QAT_FW_LA_NO_PROTO);
>  }
> 
> +static __rte_always_inline int
> +refactor_qat_sym_build_request(__rte_unused void *in_op,
> +		__rte_unused uint8_t *out_msg, __rte_unused	void
> *op_cookie,
> +		__rte_unused uint64_t *opaque,
> +		__rte_unused enum qat_device_gen dev_gen)
> +{
> +	return 0;
> +}
> +
> +uint16_t
> +refactor_qat_sym_enqueue_burst(void *qp, struct rte_crypto_op **ops,
> +		uint16_t nb_ops)
> +{
> +	return refactor_qat_enqueue_op_burst(qp,
> refactor_qat_sym_build_request,
> +			(void **)ops, nb_ops);
> +}
> +

Same as above, some comments would be helpful and shall remove altogether in
later patches.

> +uint16_t
> +refactor_qat_sym_dequeue_burst(void *qp, struct rte_crypto_op **ops,
> +		uint16_t nb_ops)
> +{
> +	return refactor_qat_dequeue_op_burst(qp, (void **)ops,
> +				refactor_qat_sym_process_response,
> nb_ops);
> +}
> +
>  int
>  qat_sym_build_request(void *in_op, uint8_t *out_msg,
>  		void *op_cookie, enum qat_device_gen qat_dev_gen)
> diff --git a/drivers/crypto/qat/qat_sym.h b/drivers/crypto/qat/qat_sym.h
> index e3ec7f0de4..17b2c871bd 100644
> --- a/drivers/crypto/qat/qat_sym.h
> +++ b/drivers/crypto/qat/qat_sym.h
> @@ -54,6 +54,22 @@ struct qat_sym_op_cookie {
>  	} opt;
>  };
> 
> +static __rte_always_inline int
> +refactor_qat_sym_process_response(__rte_unused void **op,
> +		__rte_unused uint8_t *resp, __rte_unused void *op_cookie,
> +		__rte_unused uint64_t *dequeue_err_count)
> +{
> +	return 0;
> +}
> +
> +uint16_t
> +refactor_qat_sym_enqueue_burst(void *qp, struct rte_crypto_op **ops,
> +		uint16_t nb_ops);
> +
> +uint16_t
> +refactor_qat_sym_dequeue_burst(void *qp, struct rte_crypto_op **ops,
> +		uint16_t nb_ops);
> +
>  int
>  qat_sym_build_request(void *in_op, uint8_t *out_msg,
>  		void *op_cookie, enum qat_device_gen qat_dev_gen);
> diff --git a/drivers/crypto/qat/qat_sym_session.h
> b/drivers/crypto/qat/qat_sym_session.h
> index 6ebc176729..73493ec864 100644
> --- a/drivers/crypto/qat/qat_sym_session.h
> +++ b/drivers/crypto/qat/qat_sym_session.h
> @@ -55,6 +55,11 @@
>  #define QAT_SESSION_IS_SLICE_SET(flags, flag)	\
>  	(!!((flags) & (flag)))
> 
> +struct qat_sym_session;
> +
> +typedef int (*qat_sym_build_request_t)(void *in_op, struct
> qat_sym_session *ctx,
> +		uint8_t *out_msg, void *op_cookie);
> +
>  enum qat_sym_proto_flag {
>  	QAT_CRYPTO_PROTO_FLAG_NONE = 0,
>  	QAT_CRYPTO_PROTO_FLAG_CCM = 1,
> @@ -107,6 +112,7 @@ struct qat_sym_session {
>  	/* Some generations need different setup of counter */
>  	uint32_t slice_types;
>  	enum qat_sym_proto_flag qat_proto_flag;
> +	qat_sym_build_request_t build_request[2];
>  };
> 
>  int
> --
> 2.17.1

All comments are towards the comments - as of the rest
Acked-by: Fan Zhang <roy.fan.zhang@intel.com>