Hi Fan,
> -----Original Message-----
> From: Zhang, Fan <fanzhang....@gmail.com>
> Sent: Wednesday 21 December 2022 15:21
> To: Power, Ciara <ciara.po...@intel.com>; Akhil Goyal
> <gak...@marvell.com>
> Cc: dev@dpdk.org; Ji, Kai <kai...@intel.com>; fanzhang....@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
