Re: [dpdk-dev] [PATCH] qat: addition of optimized content descriptor for AES128-SHA1-HMAC

[Thomas Monjalon <thomas.monjalon@6wind.com>]

Hi,

I'm not used to review crypto patches but I think this patch can
be improved.

2016-06-21 13:55, John Griffin:
> Adding an optimized content descriptor for AES128-SHA1-HMAC to
> improve thoughput performance.

Maybe you can explain how it improves the performance.

> +/*
> + * Function which will return if it's possible to use the
> + * optimised content descriptor.
> + */
> +int qat_crypto_sym_use_optimized_alg(struct qat_session *session)
[...]
> +/*
> + * Function to create an optimised content descriptor for AES128 SHA1.
> + */
> +int qat_crypto_create_optimzed_session(struct qat_session *session,

These function are very specific with a generic name.
Maybe that CBC AES128 SHA1 or something like that must be part of
the function name.
Otherwise you will come with yet another crypto refactoring patch in
few weeks.

>  	case ICP_QAT_FW_LA_CMD_CIPHER:
> -	session = qat_crypto_sym_configure_session_cipher(dev, xform, session);
> +		session = qat_crypto_sym_configure_session_cipher(dev, xform,
> +				session);
>  		break;
>  	case ICP_QAT_FW_LA_CMD_AUTH:
> -	session = qat_crypto_sym_configure_session_auth(dev, xform, session);
> +		session = qat_crypto_sym_configure_session_auth(dev, xform,
> +				session);
>  		break;
>  	case ICP_QAT_FW_LA_CMD_CIPHER_HASH:
> -	session = qat_crypto_sym_configure_session_cipher(dev, xform, session);
> -	session = qat_crypto_sym_configure_session_auth(dev, xform, session);
> -		break;
> +		session = qat_crypto_sym_configure_session_cipher(dev, xform,
> +				session);
> +		session = qat_crypto_sym_configure_session_auth(dev, xform,
> +				session);
> +		if (qat_crypto_sym_use_optimized_alg(session))
> +			qat_crypto_sym_configure_optimized_session(dev, xform,
> +				session);
> +	break;
>  	case ICP_QAT_FW_LA_CMD_HASH_CIPHER:
> -	session = qat_crypto_sym_configure_session_auth(dev, xform, session);
> -	session = qat_crypto_sym_configure_session_cipher(dev, xform, session);
> -		break;
> +		session = qat_crypto_sym_configure_session_auth(dev, xform,
> +				session);
> +		session = qat_crypto_sym_configure_session_cipher(dev, xform,
> +				session);
> +		if (qat_crypto_sym_use_optimized_alg(session))
> +			qat_crypto_sym_configure_optimized_session(dev, xform,
> +				session);
> +	break;

There is a lot of indent fixing mixed with the addition here.
2 patches would make things easier to understand.

> @@ -551,11 +591,11 @@ qat_crypto_sym_configure_session_auth(struct rte_cryptodev *dev,
>  				auth_xform->algo);
>  		goto error_out;
>  	}
> -	cipher_xform = qat_get_cipher_xform(xform);
>  
>  	if ((session->qat_hash_alg == ICP_QAT_HW_AUTH_ALGO_GALOIS_128) ||
>  			(session->qat_hash_alg ==
>  				ICP_QAT_HW_AUTH_ALGO_GALOIS_64))  {
> +		cipher_xform = qat_get_cipher_xform(xform);
>  		if (qat_alg_aead_session_create_content_desc_auth(session,

How this move is related to the patch?