| Message ID | 20220704135833.1496303-1-martin.fernandez@eclypsium.com |
|---|---|
| Headers | show |
| Series | x86: Show in sysfs if a memory node is able to do encryption | expand |
On Mon, Jul 4, 2022 at 9:59 PM 'Martin Fernandez' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > Add KUnit tests for the e820_range_* functions. > > Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com> > --- 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@google.com> 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@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20220704135833.1496303-8-martin.fernandez%40eclypsium.com.
On 7/4/22, David Gow <davidgow@google.com> wrote: > On Mon, Jul 4, 2022 at 9:59 PM 'Martin Fernandez' via KUnit > Development <kunit-dev@googlegroups.com> wrote: >> >> Add KUnit tests for the e820_range_* functions. >> >> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com> >> --- > > 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' Yes, that's how I ran it. The new qemu executions are great by the way :) > 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@google.com> > > 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 I don't know what happened, I saw checkpatch warning me about the a help description but since it looked good to me I didn't mind. Now I see the actual error. >> + >> 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. That's true, I didn't think about, because as you said the naming is quite obviuos, but I get that it could be an issue. >> +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@googlegroups.com. >> To view this discussion on the web visit >> https://groups.google.com/d/msgid/kunit-dev/20220704135833.1496303-8-martin.fernandez%40eclypsium.com. >
On 7/4/22, Greg KH <gregkh@linuxfoundation.org> wrote: > On Mon, Jul 04, 2022 at 10:58:33AM -0300, Martin Fernandez wrote: >> Show in each node in sysfs if its memory is able to do be encrypted by >> the CPU; on EFI systems: if all its memory is marked with >> EFI_MEMORY_CPU_CRYPTO in the EFI memory map. >> >> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com> >> --- >> Documentation/ABI/testing/sysfs-devices-node | 10 ++++++++++ >> drivers/base/node.c | 10 ++++++++++ >> 2 files changed, 20 insertions(+) >> create mode 100644 Documentation/ABI/testing/sysfs-devices-node >> >> diff --git a/Documentation/ABI/testing/sysfs-devices-node >> b/Documentation/ABI/testing/sysfs-devices-node >> new file mode 100644 >> index 000000000000..0e95420bd7c5 >> --- /dev/null >> +++ b/Documentation/ABI/testing/sysfs-devices-node >> @@ -0,0 +1,10 @@ >> +What: /sys/devices/system/node/nodeX/crypto_capable >> +Date: April 2022 >> +Contact: Martin Fernandez <martin.fernandez@eclypsium.com> >> +Users: fwupd (https://fwupd.org) >> +Description: >> + This value is 1 if all system memory in this node is >> + capable of being protected with the CPU's memory >> + cryptographic capabilities. It is 0 otherwise. >> + On EFI systems the node will be marked with >> + EFI_MEMORY_CPU_CRYPTO. > > Where will such a node be "marked"? I do not understand this last > sentence, sorry, can you please reword this? What I meant is that if all the memory regions in a given node are flagged with EFI_MEMORY_CPU_CRYPTO then that file will hold a 1. Maybe it's a little confusing if you don't know what EFI_MEMORY_CPU_CRYPTO is. > And why is EFI an issue here at all? Checking for EFI_MEMORY_CPU_CRYPTO is the way to know if a memory region is able to be encrypted by the CPU on EFI platforms. It's not really an issue and it's currently the only implementation for this file. Is it clearer here? This value is 1 if the memory in this node is capable of being protected with the CPU's memory cryptographic capabilities. It is 0 otherwise. On EFI systems this means that all the memory regions of the node have the EFI_MEMORY_CPU_CRYPTO attribute set. > thanks, > > greg k-h >
On Tue, Jul 05, 2022 at 02:35:18PM -0300, Martin Fernandez wrote: > On 7/4/22, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Mon, Jul 04, 2022 at 10:58:33AM -0300, Martin Fernandez wrote: > >> Show in each node in sysfs if its memory is able to do be encrypted by > >> the CPU; on EFI systems: if all its memory is marked with > >> EFI_MEMORY_CPU_CRYPTO in the EFI memory map. > >> > >> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com> > >> --- > >> Documentation/ABI/testing/sysfs-devices-node | 10 ++++++++++ > >> drivers/base/node.c | 10 ++++++++++ > >> 2 files changed, 20 insertions(+) > >> create mode 100644 Documentation/ABI/testing/sysfs-devices-node > >> > >> diff --git a/Documentation/ABI/testing/sysfs-devices-node > >> b/Documentation/ABI/testing/sysfs-devices-node > >> new file mode 100644 > >> index 000000000000..0e95420bd7c5 > >> --- /dev/null > >> +++ b/Documentation/ABI/testing/sysfs-devices-node > >> @@ -0,0 +1,10 @@ > >> +What: /sys/devices/system/node/nodeX/crypto_capable > >> +Date: April 2022 > >> +Contact: Martin Fernandez <martin.fernandez@eclypsium.com> > >> +Users: fwupd (https://fwupd.org) > >> +Description: > >> + This value is 1 if all system memory in this node is > >> + capable of being protected with the CPU's memory > >> + cryptographic capabilities. It is 0 otherwise. > >> + On EFI systems the node will be marked with > >> + EFI_MEMORY_CPU_CRYPTO. > > > > Where will such a node be "marked"? I do not understand this last > > sentence, sorry, can you please reword this? > > What I meant is that if all the memory regions in a given node are > flagged with EFI_MEMORY_CPU_CRYPTO then that file will hold a 1. > > Maybe it's a little confusing if you don't know what > EFI_MEMORY_CPU_CRYPTO is. > > > And why is EFI an issue here at all? > > Checking for EFI_MEMORY_CPU_CRYPTO is the way to know if a memory > region is able to be encrypted by the CPU on EFI platforms. It's not > really an issue and it's currently the only implementation for this > file. > > Is it clearer here? > > This value is 1 if the memory in this node is capable of being > protected with the CPU's memory cryptographic capabilities. It is 0 > otherwise. > On EFI systems this means that all the memory regions of the node > have the EFI_MEMORY_CPU_CRYPTO attribute set. Much better, thanks. greg k-h
On Mon, Jul 04, 2022 at 10:58:24AM -0300, Martin Fernandez wrote: > If all nodes are capable of encryption and if the system have tme/sme > on we can pretty confidently say that the device is actively > encrypting all its memory. Wait, what? If all memory is crypto capable and I boot with mem_encrypt=off, then the device is certainly not encrypting any memory. dhansen says TME cannot be controlled this way and if you turn it off in the BIOS, EFI_MEMORY_CPU_CRYPTO attr should not be set either. But that marking won't work on AMD. You really need to be able to check whether memory encryption is also enabled. And I believe I've said this before but even if encryption is on, it is never "all its memory": the machine can decide to decrypt a page or a bunch of them for whatever reason. And then they're plaintext. > It's planned to make this check part of an specification that can be > passed to people purchasing hardware How is that supposed to work? People would boot a Linux on that hardware and fwupd would tell them whether it can encrypt memory or not? But if that were the only use case, why can't EFI simply say that in its fancy GUI? Because all the kernel seems to be doing here is parrot further EFI_MEMORY_CPU_CRYPTO. And that attribute gets set by EFI so it goes and picks apart whether the underlying hw can encrypt memory. So EFI could report it too. Hmmm?
On 10/13/22, Borislav Petkov <bp@alien8.de> wrote: > On Mon, Jul 04, 2022 at 10:58:24AM -0300, Martin Fernandez wrote: >> If all nodes are capable of encryption and if the system have tme/sme >> on we can pretty confidently say that the device is actively >> encrypting all its memory. > > Wait, what? > > If all memory is crypto capable and I boot with mem_encrypt=off, then > the device is certainly not encrypting any memory. > > dhansen says TME cannot be controlled this way and if you turn it off in > the BIOS, EFI_MEMORY_CPU_CRYPTO attr should not be set either. That's bad, because it would be nice if that attribute only depended on the hardware and not on some setting. The plan of this patch was, as you mentioned just to report EFI_MEMORY_CPU_CRYPTO in a per node level. Now, I think I will need to check for tme/sme and only if those are active then show the file in sysfs, otherwise not show it at all, because it would be misleading. Any other idea? > But that > marking won't work on AMD. You mean that EFI_MEMORY_CPU_CRYPTO means nothing on an AMD system?
On 10/13/22 12:48, Borislav Petkov wrote: >> It's planned to make this check part of an specification that can be >> passed to people purchasing hardware > How is that supposed to work? > > People would boot a Linux on that hardware and fwupd would tell them > whether it can encrypt memory or not? > > But if that were the only use case, why can't EFI simply say that in its > fancy GUI? > > Because all the kernel seems to be doing here is parrot further > EFI_MEMORY_CPU_CRYPTO. > > And that attribute gets set by EFI so it goes and picks apart whether > the underlying hw can encrypt memory. So EFI could report it too. I think the kernel _would_ just be parroting the firmware's info *if* this stuff was all static at boot. It pretty much _is_ static on today's systems. You generally can't hotplug memory (encrypted or not) on any of these fancy memory encryption systems. On the Intel side, I'm thinking mostly of Ice Lake systems. But, that is very much changing once CXL comes on the scene. A system might boot with only DRAM attached right to the CPU and all of it is encryption-capable. Then, some wise guys plugs in a CXL card that doesn't support encryption. That makes the "is everything encrypted" answer dynamic and is essentially unanswerable at boot, other than to give a one-off answer.
On Thu, Oct 13, 2022 at 06:00:58PM -0300, Martin Fernandez wrote: > That's bad, because it would be nice if that attribute only depended > on the hardware and not on some setting. Why would that be bad? You want to be able to disable encryption for whatever reason sometimes. > The plan of this patch was, as you mentioned just to report > EFI_MEMORY_CPU_CRYPTO in a per node level. > > Now, I think I will need to check for tme/sme and only if those are > active then show the file in sysfs, otherwise not show it at all, > because it would be misleading. Any other idea? Well, I still think this is not going to work in all cases. SME/TME can be enabled but the kernel can go - and for whatever reason - map a bunch of memory unencrypted. So I don't know what the goal of this fwupd checking whether users have configured memory encryption properly is. It might end up giving that false sense of security... > You mean that EFI_MEMORY_CPU_CRYPTO means nothing on an AMD system? I mean, you still can disable memory encryption.
On 10/27/22 01:57, Borislav Petkov wrote: > Well, I still think this is not going to work in all cases. SME/TME can > be enabled but the kernel can go - and for whatever reason - map a bunch > of memory unencrypted. For TME on Intel systems, there's no way to make it unencrypted. The memory controller is doing all the encryption behind the back of the OS and even devices that are doing DMA. Nothing outside of the memory controller really knows or cares that encryption is happening.
On Thu, Oct 27, 2022 at 08:21:02AM -0700, Dave Hansen wrote: > On 10/27/22 01:57, Borislav Petkov wrote: > > Well, I still think this is not going to work in all cases. SME/TME can > > be enabled but the kernel can go - and for whatever reason - map a bunch > > of memory unencrypted. > > For TME on Intel systems, there's no way to make it unencrypted. The > memory controller is doing all the encryption behind the back of the OS > and even devices that are doing DMA. Nothing outside of the memory > controller really knows or cares that encryption is happening. Ok, Tom just confirmed that AMD's TSME thing also encrypts all memory. So I guess the code should check for TME or TSME. If those are set, then you can assume that all memory is encrypted.
Show for each node if every memory descriptor in that node has the EFI_MEMORY_CPU_CRYPTO attribute. fwupd project plans to use it as part of a check to see if the users have properly configured memory hardware encryption capabilities. fwupd's people have seen cases where it seems like there is memory encryption because all the hardware is capable of doing it, but on a closer look there is not, either because of system firmware or because some component requires updating to enable the feature. The MKTME/TME spec says that it will only encrypt those memory regions which are flagged with the EFI_MEMORY_CPU_CRYPTO attribute. If all nodes are capable of encryption and if the system have tme/sme on we can pretty confidently say that the device is actively encrypting all its memory. It's planned to make this check part of an specification that can be passed to people purchasing hardware These checks will run at every boot. The specification is called Host Security ID: https://fwupd.github.io/libfwupdplugin/hsi.html. We choosed to do it a per-node basis because although an ABI that shows that the whole system memory is capable of encryption would be useful for the fwupd use case, doing it in a per-node basis would make the path easier to give the capability to the user to target allocations from applications to NUMA nodes which have encryption capabilities in the future. Changes since v8: Add unit tests to e820_range_* functions Changes since v7: Less kerneldocs Less verbosity in the e820 code Changes since v6: Fixes in __e820__handle_range_update Const correctness in e820.c Correct alignment in memblock.h Rework memblock_overlaps_region Changes since v5: Refactor e820__range_{update, remove, set_crypto_capable} in order to avoid code duplication. Warn the user when a node has both encryptable and non-encryptable regions. Check that e820_table has enough size to store both current e820_table and EFI memmap. Changes since v4: Add enum to represent the cryptographic capabilities in e820: e820_crypto_capabilities. Revert __e820__range_update, only adding the new argument for __e820__range_add about crypto capabilities. Add a function __e820__range_update_crypto similar to __e820__range_update but to only update this new field. Changes since v3: Update date in Doc/ABI file. More information about the fwupd usecase and the rationale behind doing it in a per-NUMA-node. Changes since v2: e820__range_mark_crypto -> e820__range_mark_crypto_capable. In e820__range_remove: Create a region with crypto capabilities instead of creating one without it and then mark it. Changes since v1: Modify __e820__range_update to update the crypto capabilities of a range; now this function will change the crypto capability of a range if it's called with the same old_type and new_type. Rework efi_mark_e820_regions_as_crypto_capable based on this. Update do_add_efi_memmap to mark the regions as it creates them. Change the type of crypto_capable in e820_entry from bool to u8. Fix e820__update_table changes. Remove memblock_add_crypto_capable. Now you have to add the region and mark it then. Better place for crypto_capable in pglist_data. Martin Fernandez (9): mm/memblock: Tag memblocks with crypto capabilities mm/mmzone: Tag pg_data_t with crypto capabilities x86/e820: Add infrastructure to refactor e820__range_{update,remove} x86/e820: Refactor __e820__range_update x86/e820: Refactor e820__range_remove x86/e820: Tag e820_entry with crypto capabilities x86/e820: Add unit tests for e820_range_* functions x86/efi: Mark e820_entries as crypto capable from EFI memmap drivers/node: Show in sysfs node's crypto capabilities Documentation/ABI/testing/sysfs-devices-node | 10 + arch/x86/Kconfig.debug | 10 + arch/x86/include/asm/e820/api.h | 1 + arch/x86/include/asm/e820/types.h | 12 +- arch/x86/kernel/e820.c | 393 ++++++++++++++----- arch/x86/kernel/e820_test.c | 249 ++++++++++++ arch/x86/platform/efi/efi.c | 37 ++ drivers/base/node.c | 10 + include/linux/memblock.h | 5 + include/linux/mmzone.h | 3 + mm/memblock.c | 62 +++ mm/page_alloc.c | 1 + 12 files changed, 695 insertions(+), 98 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-devices-node create mode 100644 arch/x86/kernel/e820_test.c