Hi Anoob, Please see my commit inline below.
Thanks Kai > -----Original Message----- > From: Anoob Joseph <ano...@marvell.com> > Sent: Monday, November 8, 2021 4:32 AM > To: Anoob Joseph <ano...@marvell.com>; Ji, Kai <kai...@intel.com>; > dev@dpdk.org; Akhil Goyal <gak...@marvell.com> > Cc: De Lara Guarch, Pablo <pablo.de.lara.gua...@intel.com>; > adamx.dybkow...@intel.com; Zhang, Roy Fan <roy.fan.zh...@intel.com> > Subject: RE: [dpdk-dev] [EXT] [dpdk-dev v1] test/cryptodev: fix incomplete > data length > > Hi Kai, > > Also, couple of nits. Please check inline. > > Thanks, > Anoob > > > -----Original Message----- > > From: dev <dev-boun...@dpdk.org> On Behalf Of Anoob Joseph > > Sent: Monday, November 8, 2021 9:36 AM > > To: Kai Ji <kai...@intel.com>; dev@dpdk.org; Akhil Goyal > > <gak...@marvell.com> > > Cc: pablo.de.lara.gua...@intel.com; adamx.dybkow...@intel.com; > > roy.fan.zh...@intel.com > > Subject: Re: [dpdk-dev] [EXT] [dpdk-dev v1] test/cryptodev: fix > > incomplete data length > > > > Hi Kai, > > > > Patch looks good. Wondering if we need same fix in functions such as " > > test_zuc_auth_cipher()". > > > > We were also hitting this issue when we enabled few additional > > features in Marvell PMDs. Upon investigation, we realized that this > > issue would come up for certain packet size combinations if the padded > > lengths are not same. We observed the issue only with > > test_mixed_auth_cipher(), which is getting addressed with this patch. > > Just wondering if you have checked whether other places also would need > a fix. > > [Kai] Yes, this fix should apply to test_zuc_auth_cipher(). However, I don't see any zuc test failed from my side as there is no zuc test vector to cover the OOP partial digest case. I think I will add this fix into the zuc test anyway. > > Thanks, > > Anoob > > > > > -----Original Message----- > > > From: dev <dev-boun...@dpdk.org> On Behalf Of Kai Ji > > > Sent: Friday, November 5, 2021 9:12 PM > > > To: dev@dpdk.org > > > Cc: Kai Ji <kai...@intel.com>; pablo.de.lara.gua...@intel.com; > > > adamx.dybkow...@intel.com > > > Subject: [EXT] [dpdk-dev] [dpdk-dev v1] test/cryptodev: fix > > > incomplete data length > > > > > > External Email > > > > > > -------------------------------------------------------------------- > > > -- This patch fixes incorrect data lengths computation in cryptodev > > > unit test. > > > Previously some data lengths were incorrectly set, which was > > > insensitive for crypto op unit tets but is critical for raw data > > > path API unit tests. The patch addressed the issue by setting the > > > correct data > > lengths for some tests. > > > > > > Fixes: 681f540da52b ("cryptodev: do not use AAD in wireless > > > algorithms") > > > Cc: pablo.de.lara.gua...@intel.com > > > > > > Fixes: e847fc512817 ("test/crypto: add encrypted digest case for > > > AES-CTR- > > > CMAC") > > > Cc: adamx.dybkow...@intel.com > > > > > > Signed-off-by: Kai Ji <kai...@intel.com> > > > --- <snip> > > > @@ -7335,6 +7335,7 @@ test_mixed_auth_cipher(const struct > > > mixed_cipher_auth_test_data *tdata, > > > unsigned int plaintext_len; > > > unsigned int ciphertext_pad_len; > > > unsigned int ciphertext_len; > > > + unsigned int data_len; > > > > > > struct rte_cryptodev_info dev_info; > > > struct rte_crypto_op *op; > > > @@ -7395,21 +7396,22 @@ test_mixed_auth_cipher(const struct > > > mixed_cipher_auth_test_data *tdata, > > > plaintext_len = ceil_byte_length(tdata->plaintext.len_bits); > > > ciphertext_pad_len = RTE_ALIGN_CEIL(ciphertext_len, 16); > > > plaintext_pad_len = RTE_ALIGN_CEIL(plaintext_len, 16); > > > + data_len = RTE_MAX(ciphertext_pad_len, plaintext_pad_len); > > [Anoob] Isn't ciphertext_pad_len guaranteed to be the larger one of the two? > Do we need another variable and the RTE_MAX? [Kai] the ciphertext_pad_len should be always bigger than plaintext_pad_len, code changed in v2. > > > > > > > if (verify) { > > > ciphertext = (uint8_t *)rte_pktmbuf_append(ut_params- > > > >ibuf, > > > - ciphertext_pad_len); > > > + data_len); > > > memcpy(ciphertext, tdata->ciphertext.data, ciphertext_len); > > > if (op_mode == OUT_OF_PLACE) > > > - rte_pktmbuf_append(ut_params->obuf, > > > ciphertext_pad_len); > > > + rte_pktmbuf_append(ut_params->obuf, data_len); > > > debug_hexdump(stdout, "ciphertext:", ciphertext, > > > ciphertext_len); > > > } else { > > > plaintext = (uint8_t *)rte_pktmbuf_append(ut_params- > > > >ibuf, > > > - plaintext_pad_len); > > > + data_len); > > > memcpy(plaintext, tdata->plaintext.data, plaintext_len); > > > if (op_mode == OUT_OF_PLACE) > > > - rte_pktmbuf_append(ut_params->obuf, > > > plaintext_pad_len); > > > + rte_pktmbuf_append(ut_params->obuf, data_len); > > [Anoob] Now that more things are common across the branches, can we > move out some bits outside the if condition? Like, the above line is > definitely > same and be kept outside condition. The append call prior to this can also be > kept common if we can rename the local variable. [Kai] code changed in v2 > > > > debug_hexdump(stdout, "plaintext:", plaintext, > > plaintext_len); > > > } > > > > > > -- > > > 2.17.1