Re: [dpdk-dev] [PATCH v3 2/5] test/crypto: add combined mode tests

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

Hi Anoob,


>-----Original Message-----
>From: Anoob Joseph <anoobj@marvell.com>
>Sent: Friday 3 September 2021 11:05
>To: Power, Ciara <ciara.power@intel.com>; Akhil Goyal <gakhil@marvell.com>;
>Doherty, Declan <declan.doherty@intel.com>; Zhang, Roy Fan
><roy.fan.zhang@intel.com>; Ananyev, Konstantin
><konstantin.ananyev@intel.com>
>Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Archana Muniganti
><marchana@marvell.com>; Tejasree Kondoj <ktejasree@marvell.com>; Hemant
>Agrawal <hemant.agrawal@nxp.com>; Nicolau, Radu <radu.nicolau@intel.com>;
>Gagandeep Singh <g.singh@nxp.com>; dev@dpdk.org
>Subject: RE: [PATCH v3 2/5] test/crypto: add combined mode tests
>
>Hi Ciara,
>
>Please see inline.
>
>Thanks,
>Anoob
>
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> Hi Anoob,
>>
>> >-----Original Message-----
>> >From: Anoob Joseph <anoobj@marvell.com>
>> >Sent: Friday 3 September 2021 05:47
>> >To: Akhil Goyal <gakhil@marvell.com>; Doherty, Declan
>> ><declan.doherty@intel.com>; Zhang, Roy Fan <roy.fan.zhang@intel.com>;
>> >Ananyev, Konstantin <konstantin.ananyev@intel.com>
>> >Cc: Anoob Joseph <anoobj@marvell.com>; Jerin Jacob
>> ><jerinj@marvell.com>; Archana Muniganti <marchana@marvell.com>;
>> >Tejasree Kondoj <ktejasree@marvell.com>; Hemant Agrawal
>> ><hemant.agrawal@nxp.com>; Nicolau, Radu <radu.nicolau@intel.com>;
>> >Power, Ciara <ciara.power@intel.com>; Gagandeep Singh
>> ><g.singh@nxp.com>; dev@dpdk.org
>> >Subject: [PATCH v3 2/5] test/crypto: add combined mode tests
>> >
>> >Add framework to test IPsec features with all supported combinations
>> >of
>> ciphers.
>> >
>> >Signed-off-by: Anoob Joseph <anoobj@marvell.com>
>> >Signed-off-by: Tejasree Kondoj <ktejasree@marvell.com>
>> >---
>> <snip>
>>
>> >+static int
>> >+test_ipsec_proto_all(const struct ipsec_test_flags *flags) {
>> >+	struct ipsec_test_data td_outb[IPSEC_TEST_PACKETS_MAX];
>> >+	struct ipsec_test_data td_inb[IPSEC_TEST_PACKETS_MAX];
>> >+	unsigned int i, nb_pkts = 1, pass_cnt = 0;
>> >+	int ret;
>> >+
>>
>> Is this testcase actually running multiple testcases under the hood?
>> I wonder could it be suited to use a sub-testsuite structure to bring
>> the testcase results up to the top level, as done with cryptodev blockcipher
>tests.
>> Have you considered this approach?
>
>[Anoob] The idea behind this framework is to test an IPsec feature (like UDP
>encapsulation) without tying it to any specific algorithm. So what this does is, it
>loops over a list of possible combinations and then runs the test for each
>combination. The test would be like this,
>
>1. Do outbound processing to generate encrypted packet 2. Basic checks or
>validation as required for the test (for example, with UDP encapsulation, we
>would validate UDP hdr in the processed packet).
>3. Any manipulations required (like for ICV corruption negative test) 4. Do
>inbound processing to get decrypted packet 5. Validate results based on the type
>of test (ICV corruption would give expect an error while normal tests would have
>the operation return original plain text packet)
>
>It's actually the array (aead_list) and this loop which initiates the test to be run for
>all algos. And, since we are not having static vectors for each test case, this
>approach seemed more straightforward. Do you think sub-testsuite makes more
>sense here?
>

Thanks for the explanation.
I still think having each test reporting its individual result to the top level provides more clarity when running tests,
rather than having multiple tests being run under the disguise of one, and reporting one result.

Even without the sub-testsuite approach, I wonder could something such as the following be more descriptive when looking at results?

static struct unit_test_suite ipsec_proto_testsuite  = {
	.suite_name = "IPsec Proto Unit Test Suite",
	.setup = ipsec_proto_testsuite_setup,
	.unit_test_cases = {
		< Inbound known vector test cases as before >
		
		TEST_CASE_NAMED_WITH_DATA(
			"Combination test (AES-GCM 128)",
			ut_setup_security, ut_teardown,
			test_ipsec_proto_display_list, &aead_list[0]),
		TEST_CASE_NAMED_WITH_DATA(
			"Combination test (AES-GCM 192)",
			ut_setup_security, ut_teardown,
			test_ipsec_proto_display_list, &aead_list[1]),
		TEST_CASE_NAMED_WITH_DATA(
			"Combination test (AES-GCM 256)",
			ut_setup_security, ut_teardown,
			test_ipsec_proto_display_list, &aead_list[2]),

		TEST_CASE_NAMED_WITH_DATA(
			"IV Generation (AES-GCM 128)",
			ut_setup_security, ut_teardown,
			test_ipsec_proto_iv_gen, &aead_list[0]),

		etc.
}

static int
test_ipsec_proto_display_list(const void *data)
{
	struct ipsec_test_flags flags;

	memset(&flags, 0, sizeof(flags));

	flags.display_alg = true;

	return test_ipsec_proto(&flags, (const struct crypto_param *)data);
}

static int
test_ipsec_proto(const struct ipsec_test_flags *flags, const struct crypto_param *data)
{
	struct ipsec_test_data td_outb[IPSEC_TEST_PACKETS_MAX];
	struct ipsec_test_data td_inb[IPSEC_TEST_PACKETS_MAX];
	unsigned int i, nb_pkts = 1, pass_cnt = 0;
	int ret;

	if (flags->iv_gen)
		nb_pkts = IPSEC_TEST_PACKETS_MAX;

	
	test_ipsec_td_prepare(&data,
				      NULL,
				      flags,
				      td_outb,
				      nb_pkts);

< the rest of the function as before but without the loop, using data instead of looping aead values >


Thanks,
Ciara

>>
>> Thanks,
>> Ciara
>>
>> >+	for (i = 0; i < RTE_DIM(aead_list); i++) {
>> >+		test_ipsec_td_prepare(&aead_list[i],
>> >+				      NULL,
>> >+				      flags,
>> >+				      td_outb,
>> >+				      nb_pkts);
>> >+
>> >+		ret = test_ipsec_proto_process(td_outb, td_inb, nb_pkts,
>> true,
>> >+					       flags);
>> >+		if (ret == TEST_SKIPPED)
>> >+			continue;
>> >+
>> >+		if (ret == TEST_FAILED)
>> >+			return TEST_FAILED;
>> >+
>> >+		test_ipsec_td_update(td_inb, td_outb, nb_pkts, flags);
>> >+
>> >+		ret = test_ipsec_proto_process(td_inb, NULL, nb_pkts, true,
>> >+					       flags);
>> >+		if (ret == TEST_SKIPPED)
>> >+			continue;
>> >+
>> >+		if (ret == TEST_FAILED)
>> >+			return TEST_FAILED;
>> >+
>> >+		if (flags->display_alg)
>> >+			test_ipsec_display_alg(&aead_list[i], NULL);
>> >+
>> >+		pass_cnt++;
>> >+	}
>> >+
>> >+	if (pass_cnt > 0)
>> >+		return TEST_SUCCESS;
>> >+	else
>> >+		return TEST_SKIPPED;
>> >+}
>> >+
>> <snip>