Re: [dpdk-dev] [PATCH v2 3/6] crypto/aesni_mb: add support for DOCSIS protocol

["Coyle, David" <david.coyle@intel.com>]

Hi Pablo, thank you for the comments

> -----Original Message-----
> From: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Sent: Tuesday, June 23, 2020 6:57 PM
> 
> > +static inline void
> > +verify_docsis_sec_crc(JOB_AES_HMAC *job, uint16_t crc_len, uint8_t
> > +*status) {
> > +	uint16_t crc_offset;
> > +	uint8_t *crc;
> > +
> > +	if (!job->msg_len_to_hash_in_bytes)
> > +		return;
> > +
> > +	crc_offset = job->hash_start_src_offset_in_bytes +
> > +			job->msg_len_to_hash_in_bytes -
> > +			job->cipher_start_src_offset_in_bytes;
> > +	crc = job->dst + crc_offset;
> > +
> > +	/* Verify CRC (at the end of the message) */
> > +	if (memcmp(job->auth_tag_output, crc, crc_len) != 0)
> 
> I'd say we can use direct RTE_ETHER_CRC_LEN here, as there is no other
> possible case, right?
> It should perform better.

[DC] You are correct - I have changed this to use RTE_ETHER_CRC_LEN.

I had been thinking about removing the crc_size from the rte_security_docsis_xform
and the docsis capabilities completely and your comment here has made me realize
I should do this, as there is only 1 CRC length that can be used for DOCSIS.
So these have been removed.

These changes will be in v3 early next week

> 
> > +		*status = RTE_CRYPTO_OP_STATUS_AUTH_FAILED; } #endif
> > +
> >  static inline void
> >  verify_digest(JOB_AES_HMAC *job, void *digest, uint16_t len, uint8_t
> > *status)  { @@ -1196,9 +1452,27 @@ static inline struct rte_crypto_op
> > * post_process_mb_job(struct aesni_mb_qp *qp, JOB_AES_HMAC *job)  {
> >  	struct rte_crypto_op *op = (struct rte_crypto_op *)job->user_data;
> > -	struct aesni_mb_session *sess = get_sym_session_private_data(
> > -							op->sym->session,
> > -							cryptodev_driver_id);
> > +	struct aesni_mb_session *sess = NULL;
> > +
> > +#ifdef AESNI_MB_DOCSIS_SEC_ENABLED
> > +	struct rte_security_op *sec_op = NULL;
> > +
> > +	if (unlikely(op->type == RTE_CRYPTO_OP_TYPE_SECURITY)) {
> 
> Not sure if this unlikely is actually needed. I don't expect to have multiple
> types enqueued in the same queue, so this or the other branch will always
> be taken.

[DC] That's a fair point - I have removed the unlikely
Again, the change will be available in v3

> 
> > +		/*
> > +		 * Assuming at this point that if it's a security type op, that
> > +		 * this is for DOCSIS
> > +		 */
> > +		sec_op = (struct rte_security_op *)&op->security;
> > +		struct rte_crypto_sym_op *crypto_sym =
> > +						&sec_op-
> >docsis.crypto_sym;
> > +		sess = get_sec_session_private_data(crypto_sym-
> > >sec_session);
> 
> ...
> 
> > -		retval = set_mb_job_params(job, qp, op, &digest_idx);
> > +#ifdef AESNI_MB_DOCSIS_SEC_ENABLED
> > +		if (unlikely(op->type == RTE_CRYPTO_OP_TYPE_SECURITY))
> 
> Same comment as above.
[DC] Same reply as above. :)
> 
> > +			retval = set_sec_mb_job_params(job, qp, op,
> > +						&digest_idx);
> > +		else
> > +#endif