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

["Griffin, John" <john.griffin@intel.com>]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, June 21, 2016 2:50 PM
> To: Griffin, John <john.griffin@intel.com>
> Cc: dev@dpdk.org; Jain, Deepak K <deepak.k.jain@intel.com>; De Lara
> Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] qat: addition of optimized content
> descriptor for AES128-SHA1-HMAC
> 
> 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.
[JFG] Can add some text- it's though a reduction in the amount of the session information which
DMA'ed to the device - helps to reduce PCIe b/w.  I'll add some text.

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

[JFG] 
[JFG] Ok - I might not rename, but I'll make the implementation more generic to all it cope better with 
the potential addition of other algorithms.

> 
> >  	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.

[JFG] 
[JFG] Ok let me remove from this patch.

> 
> > @@ -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?

 [JFG] It's not tbh - I'll remove from this patch.