Re: [PATCH v1 1/1] lib/crypto: tests: Add KUnit tests for AES

[Eric Biggers <ebiggers@kernel.org>]

On Thu, Jan 15, 2026 at 07:38:31PM +0100, Holger Dengler wrote:
> Add a KUnit test suite for AES library functions, including KAT and
> benchmarks.
> 
> Signed-off-by: Holger Dengler <dengler@linux.ibm.com>

The cover letter had some more information.  Could you put it in the
commit message directly?  Normally cover letters aren't used for a
single patch: the explanation should just be in the patch itself.

> diff --git a/lib/crypto/tests/aes-testvecs.h b/lib/crypto/tests/aes-testvecs.h
> new file mode 100644
> index 000000000000..dfa528db7f02
> --- /dev/null
> +++ b/lib/crypto/tests/aes-testvecs.h
> @@ -0,0 +1,78 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _AES_TESTVECS_H
> +#define _AES_TESTVECS_H
> +
> +#include <crypto/aes.h>
> +
> +struct buf {
> +	size_t blen;
> +	u8 b[];
> +};

'struct buf' is never used.

> +static const struct aes_testvector aes128_kat = {

Where do these test vectors come from?  All test vectors should have a
documented source.

> +static void benchmark_aes(struct kunit *test, const struct aes_testvector *tv)
> +{
> +	const size_t num_iters = 10000000;

10000000 iterations is too many.  That's 160 MB of data in each
direction per AES key length.  Some CPUs without AES instructions can do
only ~20 MB AES per second.  In that case, this benchmark would take 16
seconds to run per AES key length, for 48 seconds total.

hash-test-template.h and crc_kunit.c use 10000000 / (len + 128)
iterations.  That would be 69444 in this case (considering len=16),
which is less than 1% of the iterations you've used.  Choosing a number
similar to that would seem more appropriate.

Ultimately these are just made-up numbers.  But I think we should aim
for the benchmark test in each KUnit test suite to take less than a
second or so.  The existing tests roughly achieve that, whereas it seems
this one can go over it by quite a bit due to the 10000000 iterations.

> +	kunit_info(test, "enc (iter. %zu, duration %lluns)",
> +		   num_iters, t_enc);
> +	kunit_info(test, "enc (len=%zu): %llu MB/s",
> +		   (size_t)AES_BLOCK_SIZE,
> +		   div64_u64((u64)AES_BLOCK_SIZE * num_iters * NSEC_PER_SEC,
> +			     (t_enc ?: 1) * SZ_1M));
> +
> +	kunit_info(test, "dec (iter. %zu, duration %lluns)",
> +		   num_iters, t_dec);
> +	kunit_info(test, "dec (len=%zu): %llu MB/s",
> +		   (size_t)AES_BLOCK_SIZE,
> +		   div64_u64((u64)AES_BLOCK_SIZE * num_iters * NSEC_PER_SEC,
> +			     (t_dec ?: 1) * SZ_1M));

Maybe delete the first line of each pair, and switch from power-of-2
megabytes to power-of-10?  That would be consistent with how the other
crypto and CRC benchmarks print their output.

> +MODULE_DESCRIPTION("KUnit tests and benchmark aes library");

"aes library" => "for the AES library"

- Eric