RE: [PATCH] test/crypto: add further ZUC testcases

["Power, Ciara" <ciara.power@intel.com>]

Hi Fan,

> -----Original Message-----
> From: Zhang, Fan <fanzhang.oss@gmail.com>
> Sent: Wednesday 21 December 2022 15:21
> To: Power, Ciara <ciara.power@intel.com>; Akhil Goyal
> <gakhil@marvell.com>
> Cc: dev@dpdk.org; Ji, Kai <kai.ji@intel.com>; fanzhang.oss@gmail.com
> Subject: Re: [PATCH] test/crypto: add further ZUC testcases
> 
> Hi Ciara,
> 
> On 12/21/2022 2:04 PM, Ciara Power wrote:
> > Previously no ZUC decryption only or hash verify testcases existed,
> > only encryption and authentication.
> > This commit adds testcases for ZUC 128 and 256 decryption, and hash
> > verify.
> <snip>
> > +	if (direction == RTE_CRYPTO_CIPHER_OP_ENCRYPT) {
> <snip>
> > +	} else {
> > +		if (ut_params->obuf)
> > +			plaintext = rte_pktmbuf_mtod(ut_params->obuf,
> uint8_t *);
> > +		else
> > +			plaintext = ciphertext;
> > +
> > +		debug_hexdump(stdout, "plaintext:", plaintext,
> ciphertext_len);
> > +
> The below line looks a bit off: bits len = bytes len * 8 right?
[CP] 
Yes think this should be : (tdata->validCipherOffsetInBits.len >> 3)

> > +		const uint8_t *reference_plaintext = tdata->plaintext.data +
> > +				(tdata->validCipherOffsetInBits.len);
> > +
> > +		/* Validate obuf */
> > +		TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT(
> > +				plaintext,
> > +				reference_plaintext,
> > +				tdata->validCipherLenInBits.len,
> > +				"ZUC Plaintext data not as expected");
> <snip>
> >   static int
> > -test_zuc_encryption_sgl(const struct wireless_test_data *tdata)
> > +test_zuc_cipher_sgl(const struct wireless_test_data *tdata,
> > +		enum rte_crypto_cipher_operation direction)
> >   {
> >   	struct crypto_testsuite_params *ts_params = &testsuite_params;
> >   	struct crypto_unittest_params *ut_params = &unittest_params;
> >
> >   	int retval;
> >
> > -	unsigned int plaintext_pad_len;
> > -	unsigned int plaintext_len;
> > -	const uint8_t *ciphertext;
> > -	uint8_t ciphertext_buffer[2048];
> > +	unsigned int plaintext_pad_len, ciphertext_pad_len;
> > +	unsigned int plaintext_len, ciphertext_len;
> > +	const uint8_t *ciphertext, *plaintext;
> 
> Just a piece of advice: Instead of allocating 2 buffers and we may use only
> one in either direction,
> 
>   we may use
> 
> uint8_t buffers[2048];
> 
> uint8_t *ciphertext_buffer = NULL, *plaintext_buffer = NULL;
> 
> And pointing either ciphertext_buffer or plaintext_buffer to buffers based
> on the direction value?
[CP] 
Good idea.
Probably no need to even have individual ct/pt buffer pointers either here

> 
> > +	uint8_t ciphertext_buffer[2048], plaintext_buffer[2048];
> >   	struct rte_cryptodev_info dev_info;
> >
> >   	/* Check if device supports ZUC EEA3 */ @@ -6074,21 +6110,36 @@
> > test_zuc_encryption_sgl(const struct wireless_test_data *tdata)
> >   		return TEST_SKIPPED;
> >   	}
> >
> > -	plaintext_len = ceil_byte_length(tdata->plaintext.len);
> > +	if (direction == RTE_CRYPTO_CIPHER_OP_ENCRYPT) {
> > +		plaintext_len = ceil_byte_length(tdata->plaintext.len);
> >
> > -	/* Append data which is padded to a multiple */
> > -	/* of the algorithms block size */
> > -	plaintext_pad_len = RTE_ALIGN_CEIL(plaintext_len, 8);
> > +		/* Append data which is padded to a multiple */
> > +		/* of the algorithms block size */
> > +		plaintext_pad_len = RTE_ALIGN_CEIL(plaintext_len, 8);
> >
> > -	ut_params->ibuf = create_segmented_mbuf(ts_params-
> >mbuf_pool,
> > -			plaintext_pad_len, 10, 0);
> > +		ut_params->ibuf = create_segmented_mbuf(ts_params-
> >mbuf_pool,
> > +				plaintext_pad_len, 10, 0);
> >
> > -	pktmbuf_write(ut_params->ibuf, 0, plaintext_len,
> > -			tdata->plaintext.data);
> > +		pktmbuf_write(ut_params->ibuf, 0, plaintext_len,
> > +				tdata->plaintext.data);
> > +	} else {
> > +		ciphertext_len = ceil_byte_length(tdata->ciphertext.len);
> > +
> > +		/* Append data which is padded to a multiple */
> > +		/* of the algorithms block size */
> > +		ciphertext_pad_len = RTE_ALIGN_CEIL(ciphertext_len, 8);
> > +
> > +		ut_params->ibuf = create_segmented_mbuf(ts_params-
> >mbuf_pool,
> > +				ciphertext_pad_len, 10, 0);
> > +
> > +		pktmbuf_write(ut_params->ibuf, 0, ciphertext_len,
> > +				tdata->ciphertext.data);
> > +
> > +	}
> >
> >   	/* Create ZUC session */
> >   	retval = create_wireless_algo_cipher_session(ts_params-
> >valid_devs[0],
> > -			RTE_CRYPTO_CIPHER_OP_ENCRYPT,
> > +			direction,
> >   			RTE_CRYPTO_CIPHER_ZUC_EEA3,
> >   			tdata->key.data, tdata->key.len,
> >   			tdata->cipher_iv.len);
> > @@ -6096,8 +6147,10 @@ test_zuc_encryption_sgl(const struct
> wireless_test_data *tdata)
> >   		return retval;
> >
> >   	/* Clear mbuf payload */
> > -
> > -	pktmbuf_write(ut_params->ibuf, 0, plaintext_len, tdata-
> >plaintext.data);
> > +	if (direction == RTE_CRYPTO_CIPHER_OP_ENCRYPT)
> > +		pktmbuf_write(ut_params->ibuf, 0, plaintext_len, tdata-
> >plaintext.data);
> > +	else
> > +		pktmbuf_write(ut_params->ibuf, 0, ciphertext_len,
> > +tdata->ciphertext.data);
> >
> >   	/* Create ZUC operation */
> >   	retval =
> > create_wireless_algo_cipher_operation(tdata->cipher_iv.data,
> > @@ -6115,28 +6168,48 @@ test_zuc_encryption_sgl(const struct
> wireless_test_data *tdata)
> >   	TEST_ASSERT_NOT_NULL(ut_params->op, "failed to retrieve obuf");
> >
> >   	ut_params->obuf = ut_params->op->sym->m_dst;
> > -	if (ut_params->obuf)
> > -		ciphertext = rte_pktmbuf_read(ut_params->obuf,
> > -			0, plaintext_len, ciphertext_buffer);
> > -	else
> > -		ciphertext = rte_pktmbuf_read(ut_params->ibuf,
> > -			0, plaintext_len, ciphertext_buffer);
> >
> > -	/* Validate obuf */
> > -	debug_hexdump(stdout, "ciphertext:", ciphertext, plaintext_len);
> > +	if (direction == RTE_CRYPTO_CIPHER_OP_ENCRYPT) {
> > +		if (ut_params->obuf)
> > +			ciphertext = rte_pktmbuf_read(ut_params->obuf,
> > +				0, plaintext_len, ciphertext_buffer);
> > +		else
> > +			ciphertext = rte_pktmbuf_read(ut_params->ibuf,
> > +				0, plaintext_len, ciphertext_buffer);
> >
> > -	/* Validate obuf */
> > -	TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT(
> > -		ciphertext,
> > -		tdata->ciphertext.data,
> > -		tdata->validCipherLenInBits.len,
> > -		"ZUC Ciphertext data not as expected");
> > +		/* Validate obuf */
> > +		debug_hexdump(stdout, "ciphertext:", ciphertext,
> plaintext_len);
> > +
> > +		/* Validate obuf */
> > +		TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT(
> > +			ciphertext,
> > +			tdata->ciphertext.data,
> > +			tdata->validCipherLenInBits.len,
> > +			"ZUC Ciphertext data not as expected");
> > +	} else {
> > +		if (ut_params->obuf)
> > +			plaintext = rte_pktmbuf_read(ut_params->obuf,
> > +				0, ciphertext_len, plaintext_buffer);
> > +		else
> > +			plaintext = rte_pktmbuf_read(ut_params->ibuf,
> > +				0, ciphertext_len, plaintext_buffer);
> > +
> > +		/* Validate obuf */
> > +		debug_hexdump(stdout, "plaintext:", plaintext,
> ciphertext_len);
> > +
> > +		/* Validate obuf */
> > +		TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT(
> > +			plaintext,
> > +			tdata->plaintext.data,
> > +			tdata->validCipherLenInBits.len,
> > +			"ZUC Plaintext data not as expected");
> > +		}
> >
> >   	return 0;
> >   }
> >
> >   static int
> > -test_zuc_authentication(const struct wireless_test_data *tdata)
> 
> Just a nit,
> 
> we may pass "enum rte_crypto_auth_operation" as parameter below,
> instead of a "verify" for passing.
> 
> This also makes the changes below more readable.
> 
[CP] 
Yes, will make this change, thanks.

> > +test_zuc_authentication(const struct wireless_test_data *tdata,
> > +uint8_t verify)

[CP] 

I missed these suggestions for my v2, which has since been merged.
I will send a small improvement patch to make these changes - thanks!

Ciara