Re: [dpdk-dev] [PATCH 1/2] crypto/qat: add chacha poly implementation

["Trahe, Fiona" <fiona.trahe@intel.com>]

Hi Arek,

> -----Original Message-----
> From: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>
> Sent: Friday, December 6, 2019 6:14 PM
> To: dev@dpdk.org
> Cc: akhil.goyal@nxp.com; Trahe, Fiona <fiona.trahe@intel.com>; Kusztal, ArkadiuszX
> <arkadiuszx.kusztal@intel.com>
> Subject: [PATCH 1/2] crypto/qat: add chacha poly implementation
> 
> This patchset adds Chacha20-Poly1305 implementation to Intel
> QuickAssist Technology pmd.
> 
> Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> ---
>  doc/guides/cryptodevs/qat.rst             |  1 +
>  doc/guides/rel_notes/release_20_02.rst    |  4 ++++
>  drivers/common/qat/qat_adf/icp_qat_hw.h   | 17 ++++++++++++++--
>  drivers/crypto/qat/qat_sym_capabilities.h | 32 +++++++++++++++++++++++++++++++
>  drivers/crypto/qat/qat_sym_pmd.c          | 11 ++++++++++-
>  drivers/crypto/qat/qat_sym_session.c      | 20 +++++++++++++++----
>  6 files changed, 78 insertions(+), 7 deletions(-)
[Fiona] The supported algorithm Matrix should be updated. This was missing from the API patch,
But can be added with this patch as this is the first PMD which is enabling this algorithm.


///snip///
> diff --git a/drivers/common/qat/qat_adf/icp_qat_hw.h b/drivers/common/qat/qat_adf/icp_qat_hw.h
> index cef6486..ed04178 100644
> --- a/drivers/common/qat/qat_adf/icp_qat_hw.h
> +++ b/drivers/common/qat/qat_adf/icp_qat_hw.h
> @@ -51,7 +51,10 @@ enum icp_qat_hw_auth_algo {
>  	ICP_QAT_HW_AUTH_ALGO_SHA3_256 = 17,
>  	ICP_QAT_HW_AUTH_RESERVED_3 = 18,
>  	ICP_QAT_HW_AUTH_ALGO_SHA3_512 = 19,
> -	ICP_QAT_HW_AUTH_ALGO_DELIMITER = 20
> +	ICP_QAT_HW_AUTH_ALGO_SHAKE_128 = 20,
> +	ICP_QAT_HW_AUTH_ALGO_SHAKE_256 = 21,
> +	ICP_QAT_HW_AUTH_ALGO_POLY = 22,
[Fiona] I don't see anywhere these are used? Shouldn't they be?

///snip///
> +#define ICP_QAT_HW_CHACHAPOLY_ICV__SZ 16
[Fiona] typo double underscore. Need to update based on latest firmware hdrs

///snip///
> diff --git a/drivers/crypto/qat/qat_sym_session.c b/drivers/crypto/qat/qat_sym_session.c
> index 72290ba..c6ca42c 100644
> --- a/drivers/crypto/qat/qat_sym_session.c
> +++ b/drivers/crypto/qat/qat_sym_session.c
> @@ -519,7 +519,8 @@ qat_sym_session_handle_single_pass(struct qat_sym_dev_private *internals,
>  		session->is_single_pass = 1;
>  		session->min_qat_dev_gen = QAT_GEN3;
>  		session->qat_cmd = ICP_QAT_FW_LA_CMD_CIPHER;
> -		session->qat_mode = ICP_QAT_HW_CIPHER_AEAD_MODE;
> +		if (aead_xform->algo == RTE_CRYPTO_AEAD_AES_GCM)
> +			session->qat_mode = ICP_QAT_HW_CIPHER_AEAD_MODE;
[Fiona] This fn should be reworked as now handling both chacha and gcm - comments only refer to GCM, and condition only checks GCM iv length - which is coincidentally matching chacha length. Several vars (is_single_pass, qat_mode , qat_dev_gen) are set up before for chacha, then rewritten here. Best do his only in fn and make clear what's common and what's different for each algo. 


///snip///
> @@ -746,15 +749,24 @@ qat_sym_session_configure_aead(struct rte_cryptodev *dev,
>  		session->qat_mode = ICP_QAT_HW_CIPHER_CTR_MODE;
>  		session->qat_hash_alg = ICP_QAT_HW_AUTH_ALGO_AES_CBC_MAC;
>  		break;
> +	case RTE_CRYPTO_AEAD_CHACHA20_POLY1305:
> +		if (aead_xform->key.length != ICP_QAT_HW_CHACHAPOLY_KEY_SZ)
> +			return -EINVAL;
> +		session->qat_cipher_alg = ICP_QAT_HW_CIPHER_ALGO_CHACHA20_POLY1305;
> +		session->qat_mode = ICP_QAT_HW_CIPHER_CTR_MODE;
> +		session->min_qat_dev_gen = QAT_GEN3;
> +		session->is_single_pass = 1;
[Fiona] as comment above - combine these with the single-pass fn. 
Maybe move call to handle_single_pass() into this switch statement?

Also,  I don't see qat_hash_alg set anywhere for CHACHA? Is this defaulting to ALGO_NULL or missed?
If default, better set explicitly as several other fns depend on this (get_state1, get_digest_size, get_block_size).