On Mon, Jul 4, 2022 at 9:59 PM 'Martin Fernandez' via KUnit
Development <kunit-dev@xxxxxxxxxxxxxxxx> wrote:
>
> Add KUnit tests for the e820_range_* functions.
>
> Signed-off-by: Martin Fernandez <martin.fernandez@xxxxxxxxxxxxx>
> ---
This looks good to me from a KUnit point of view. I've tested it on
both 32- and 64- bit x86 under qemu with the following:
./tools/testing/kunit/kunit.py run --arch=i386 'e820'
./tools/testing/kunit/kunit.py run --arch=x86_64 'e820'
Two notes inline below:
- An indentation error in the Kconfig entry, which stops it from compiling.
- Some minor pontificating about how KUnit wants to name macros in
general. (No action required: just making a note that this is probably
okay.)
With the indentation issue fixed, this is:
Reviewed-by: David Gow <davidgow@xxxxxxxxxx>
Cheers,
-- David
> arch/x86/Kconfig.debug | 10 ++
> arch/x86/kernel/e820.c | 5 +
> arch/x86/kernel/e820_test.c | 249 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 264 insertions(+)
> create mode 100644 arch/x86/kernel/e820_test.c
>
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index d872a7522e55..b5040d345fb4 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -225,6 +225,16 @@ config PUNIT_ATOM_DEBUG
> The current power state can be read from
> /sys/kernel/debug/punit_atom/dev_power_state
>
> +config E820_KUNIT_TEST
> + tristate "Tests for E820" if !KUNIT_ALL_TESTS
> + depends on KUNIT=y
> + default KUNIT_ALL_TESTS
> + help
> + This option enables unit tests for the e820.c code. It
> + should be enabled only in development environments.
> +
> + If unsure, say N.
The indentation here seems to be one space off, leading to errors building it:
arch/x86/Kconfig.debug:236: syntax error
arch/x86/Kconfig.debug:235:warning: ignoring unsupported character ','
arch/x86/Kconfig.debug:235:warning: ignoring unsupported character '.'
arch/x86/Kconfig.debug:235: unknown statement "If"
make[2]: *** [../scripts/kconfig/Makefile:77: olddefconfig] Error 1
> +
> choice
> prompt "Choose kernel unwinder"
> default UNWINDER_ORC if X86_64
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index dade59758b9f..a6ced3e306dd 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -1546,3 +1546,8 @@ void __init e820__memblock_setup(void)
>
> memblock_dump_all();
> }
> +
> +#ifdef CONFIG_E820_KUNIT_TEST
> +/* Let e820_test have access the static functions in this file */
> +#include "e820_test.c"
> +#endif
> diff --git a/arch/x86/kernel/e820_test.c b/arch/x86/kernel/e820_test.c
> new file mode 100644
> index 000000000000..6b28ea131380
> --- /dev/null
> +++ b/arch/x86/kernel/e820_test.c
> @@ -0,0 +1,249 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <kunit/test.h>
> +
> +#include <asm/e820/api.h>
> +#include <asm/setup.h>
> +
> +#define KUNIT_EXPECT_E820_ENTRY_EQ(_test, _entry, _addr, _size, _type, \
> + _crypto_capable) \
> + do { \
> + KUNIT_EXPECT_EQ((_test), (_entry).addr, (_addr)); \
> + KUNIT_EXPECT_EQ((_test), (_entry).size, (_size)); \
> + KUNIT_EXPECT_EQ((_test), (_entry).type, (_type)); \
> + KUNIT_EXPECT_EQ((_test), (_entry).crypto_capable, \
> + (_crypto_capable)); \
> + } while (0)
> +
I'm not 100% sure we ever came to a decision about tests naming their
own expect macros KUNIT_EXPECT_*. I know KASAN is doing it, though the
thought there was that other tests might have sensible reasons to
expect given memory accesses, so it might not be limited to the one
test.
Personally, I don't mind it, particularly since it's obvious that this
is specific to the e820 test.
> +struct e820_table test_table __initdata;
> +
> +static void __init test_e820_range_add(struct kunit *test)
> +{
> + u32 full = ARRAY_SIZE(test_table.entries);
> + /* Add last entry. */
> + test_table.nr_entries = full - 1;
> + __e820__range_add(&test_table, 0, 15, 0, 0);
> + KUNIT_EXPECT_EQ(test, test_table.nr_entries, full);
> + /* Skip new entry when full. */
> + __e820__range_add(&test_table, 0, 15, 0, 0);
> + KUNIT_EXPECT_EQ(test, test_table.nr_entries, full);
> +}
> +
> +static void __init test_e820_range_update(struct kunit *test)
> +{
> + u64 entry_size = 15;
> + u64 updated_size = 0;
> + /* Initialize table */
> + test_table.nr_entries = 0;
> + __e820__range_add(&test_table, 0, entry_size, E820_TYPE_RAM,
> + E820_NOT_CRYPTO_CAPABLE);
> + __e820__range_add(&test_table, entry_size, entry_size, E820_TYPE_RAM,
> + E820_NOT_CRYPTO_CAPABLE);
> + __e820__range_add(&test_table, entry_size * 2, entry_size,
> + E820_TYPE_ACPI, E820_NOT_CRYPTO_CAPABLE);
> +
> + updated_size = __e820__range_update(&test_table, 0, entry_size * 2,
> + E820_TYPE_RAM, E820_TYPE_RESERVED);
> +
> + /* The first 2 regions were updated */
> + KUNIT_EXPECT_EQ(test, updated_size, entry_size * 2);
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[0], 0, entry_size,
> + E820_TYPE_RESERVED, E820_NOT_CRYPTO_CAPABLE);
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[1], entry_size,
> + entry_size, E820_TYPE_RESERVED,
> + E820_NOT_CRYPTO_CAPABLE);
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[2], entry_size * 2,
> + entry_size, E820_TYPE_ACPI,
> + E820_NOT_CRYPTO_CAPABLE);
> +
> + updated_size = __e820__range_update(&test_table, 0, entry_size * 3,
> + E820_TYPE_RESERVED, E820_TYPE_RAM);
> +
> + /*
> + * Only the first 2 regions were updated,
> + * since E820_TYPE_ACPI > E820_TYPE_RESERVED
> + */
> + KUNIT_EXPECT_EQ(test, updated_size, entry_size * 2);
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[0], 0, entry_size,
> + E820_TYPE_RAM, E820_NOT_CRYPTO_CAPABLE);
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[1], entry_size,
> + entry_size, E820_TYPE_RAM,
> + E820_NOT_CRYPTO_CAPABLE);
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[2], entry_size * 2,
> + entry_size, E820_TYPE_ACPI,
> + E820_NOT_CRYPTO_CAPABLE);
> +}
> +
> +static void __init test_e820_range_remove(struct kunit *test)
> +{
> + u64 entry_size = 15;
> + u64 removed_size = 0;
> +
> + struct e820_entry_updater updater = { .should_update =
> + remover__should_update,
> + .update = remover__update,
> + .new = NULL };
> +
> + struct e820_remover_data data = { .check_type = true,
> + .old_type = E820_TYPE_RAM };
> +
> + /* Initialize table */
> + test_table.nr_entries = 0;
> + __e820__range_add(&test_table, 0, entry_size, E820_TYPE_RAM,
> + E820_NOT_CRYPTO_CAPABLE);
> + __e820__range_add(&test_table, entry_size, entry_size, E820_TYPE_RAM,
> + E820_NOT_CRYPTO_CAPABLE);
> + __e820__range_add(&test_table, entry_size * 2, entry_size,
> + E820_TYPE_ACPI, E820_NOT_CRYPTO_CAPABLE);
> +
> + /*
> + * Need to use __e820__handle_range_update because
> + * e820__range_remove doesn't ask for the table
> + */
> + removed_size = __e820__handle_range_update(&test_table,
> + 0, entry_size * 2,
> + &updater, &data);
> +
> + /* The first two regions were removed */
> + KUNIT_EXPECT_EQ(test, removed_size, entry_size * 2);
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[0], 0, 0, 0, 0);
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[1], 0, 0, 0, 0);
> +
> + removed_size = __e820__handle_range_update(&test_table,
> + 0, entry_size * 3,
> + &updater, &data);
> +
> + /* Nothing was removed, since nothing matched the target type */
> + KUNIT_EXPECT_EQ(test, removed_size, 0);
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[0], 0, 0, 0, 0);
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[1], 0, 0, 0, 0);
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[2], entry_size * 2,
> + entry_size, E820_TYPE_ACPI,
> + E820_NOT_CRYPTO_CAPABLE);
> +}
> +
> +static void __init test_e820_range_crypto_update(struct kunit *test)
> +{
> + u64 entry_size = 15;
> + u64 updated_size = 0;
> + /* Initialize table */
> + test_table.nr_entries = 0;
> + __e820__range_add(&test_table, 0, entry_size, E820_TYPE_RAM,
> + E820_CRYPTO_CAPABLE);
> + __e820__range_add(&test_table, entry_size, entry_size, E820_TYPE_RAM,
> + E820_NOT_CRYPTO_CAPABLE);
> + __e820__range_add(&test_table, entry_size * 2, entry_size,
> + E820_TYPE_RAM, E820_CRYPTO_CAPABLE);
> +
> + updated_size = __e820__range_update_crypto(&test_table,
> + 0, entry_size * 3,
> + E820_CRYPTO_CAPABLE);
> +
> + /* Only the region in the middle was updated */
> + KUNIT_EXPECT_EQ(test, updated_size, entry_size);
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[0], 0, entry_size,
> + E820_TYPE_RAM, E820_CRYPTO_CAPABLE);
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[1], entry_size,
> + entry_size, E820_TYPE_RAM,
> + E820_CRYPTO_CAPABLE);
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[2], entry_size * 2,
> + entry_size, E820_TYPE_RAM,
> + E820_CRYPTO_CAPABLE);
> +}
> +
> +static void __init test_e820_handle_range_update_intersection(struct kunit *test)
> +{
> + struct e820_entry_updater updater = {
> + .should_update = type_updater__should_update,
> + .update = type_updater__update,
> + .new = type_updater__new
> + };
> +
> + struct e820_type_updater_data data = { .old_type = E820_TYPE_RAM,
> + .new_type = E820_TYPE_RESERVED };
> +
> + u64 entry_size = 15;
> + u64 intersection_size = 2;
> + u64 updated_size = 0;
> + /* Initialize table */
> + test_table.nr_entries = 0;
> + __e820__range_add(&test_table, 0, entry_size, E820_TYPE_RAM,
> + E820_NOT_CRYPTO_CAPABLE);
> +
> + updated_size =
> + __e820__handle_range_update(&test_table, 0,
> + entry_size - intersection_size,
> + &updater, &data);
> +
> + KUNIT_EXPECT_EQ(test, updated_size, entry_size - intersection_size);
> +
> + /* There is a new entry */
> + KUNIT_EXPECT_EQ(test, test_table.nr_entries, intersection_size);
> +
> + /* The original entry now is moved */
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[0],
> + entry_size - intersection_size,
> + intersection_size, E820_TYPE_RAM,
> + E820_NOT_CRYPTO_CAPABLE);
> +
> + /* The new entry has the correct values */
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[1], 0,
> + entry_size - intersection_size,
> + E820_TYPE_RESERVED, E820_NOT_CRYPTO_CAPABLE);
> +}
> +
> +static void __init test_e820_handle_range_update_inside(struct kunit *test)
> +{
> + struct e820_entry_updater updater = {
> + .should_update = type_updater__should_update,
> + .update = type_updater__update,
> + .new = type_updater__new
> + };
> +
> + struct e820_type_updater_data data = { .old_type = E820_TYPE_RAM,
> + .new_type = E820_TYPE_RESERVED };
> +
> + u64 entry_size = 15;
> + u64 updated_size = 0;
> + /* Initialize table */
> + test_table.nr_entries = 0;
> + __e820__range_add(&test_table, 0, entry_size, E820_TYPE_RAM,
> + E820_NOT_CRYPTO_CAPABLE);
> +
> + updated_size = __e820__handle_range_update(&test_table, 5,
> + entry_size - 10,
> + &updater, &data);
> +
> + KUNIT_EXPECT_EQ(test, updated_size, entry_size - 10);
> +
> + /* There are two new entrie */
> + KUNIT_EXPECT_EQ(test, test_table.nr_entries, 3);
> +
> + /* The original entry now shrunk */
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[0], 0, 5,
> + E820_TYPE_RAM, E820_NOT_CRYPTO_CAPABLE);
> +
> + /* The new entries have the correct values */
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[1], 5,
> + entry_size - 10, E820_TYPE_RESERVED,
> + E820_NOT_CRYPTO_CAPABLE);
> + /* Left over of the original region */
> + KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[2], entry_size - 5,
> + 5, E820_TYPE_RAM, E820_NOT_CRYPTO_CAPABLE);
> +}
> +
> +static struct kunit_case e820_test_cases[] __initdata = {
> + KUNIT_CASE(test_e820_range_add),
> + KUNIT_CASE(test_e820_range_update),
> + KUNIT_CASE(test_e820_range_remove),
> + KUNIT_CASE(test_e820_range_crypto_update),
> + KUNIT_CASE(test_e820_handle_range_update_intersection),
> + KUNIT_CASE(test_e820_handle_range_update_inside),
> + {}
> +};
> +
> +static struct kunit_suite e820_test_suite __initdata = {
> + .name = "e820",
> + .test_cases = e820_test_cases,
> +};
> +
> +kunit_test_init_section_suite(e820_test_suite);
> --
> 2.30.2
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@xxxxxxxxxxxxxxxx.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20220704135833.1496303-8-martin.fernandez%40eclypsium.com.
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
