Jan Dabros has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
tests: Add lib/bootmem-test test case
Signed-off-by: Jan Dabros jsd@semihalf.com Change-Id: Ic1e539061ee5051d4158712a8a981a475ea7458a --- M tests/lib/Makefile.inc A tests/lib/bootmem-test.c A tests/lib/bootmem-test.ld 3 files changed, 296 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/43510/1
diff --git a/tests/lib/Makefile.inc b/tests/lib/Makefile.inc index dae406a..69e3fd4 100644 --- a/tests/lib/Makefile.inc +++ b/tests/lib/Makefile.inc @@ -4,6 +4,7 @@ tests-y += b64_decode-test tests-y += hexstrtobin-test tests-y += memrange-test +tests-y += bootmem-test
string-test-srcs += tests/lib/string-test.c string-test-srcs += src/lib/string.c @@ -19,3 +20,10 @@ memrange-test-srcs += src/lib/memrange.c memrange-test-srcs += tests/stubs/console.c memrange-test-srcs += tests/mocks/device/device_util.c + +bootmem-test-srcs += tests/lib/bootmem-test.c +bootmem-test-srcs += src/lib/bootmem.c +bootmem-test-srcs += src/lib/memrange.c +bootmem-test-srcs += tests/stubs/console.c +bootmem-test-srcs += tests/mocks/device/device_util.c +bootmem-test-cflags += -Wl,--script=tests/lib/bootmem-test.ld diff --git a/tests/lib/bootmem-test.c b/tests/lib/bootmem-test.c new file mode 100644 index 0000000..8082720 --- /dev/null +++ b/tests/lib/bootmem-test.c @@ -0,0 +1,272 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <tests/test.h> +#include <stdlib.h> + +#include <bootmem.h> +#include <commonlib/coreboot_tables.h> +#include <device/device.h> +#include <device/resource.h> +#include <memrange.h> +#include <symbols.h> + +/* Stubs defined to satisfy linker dependencies */ +void cbmem_add_bootmem(void) +{ +} + +void bootmem_arch_add_ranges(void) +{ +} + +struct bootmem_ranges_t { + uint64_t start; + uint64_t size; + uint32_t type; +}; + +struct bootmem_ranges_t *os_ranges; +struct bootmem_ranges_t *ranges; + +/* See tests/lib/bootmem-test.ld for boundaries definitions */ +extern u8 _ramstage_size[]; +#define PROGRAM_START ((uintptr_t)_program) +#define RAMSTAGE_SIZE ((uintptr_t)_ramstage_size) +#define CACHEABLE_START PROGRAM_START +#define CACHEABLE_SIZE (1ULL << 32) +#define CACHEABLE_END (CACHEABLE_START + CACHEABLE_SIZE) +#define RESERVED_START (1ULL << 32) +#define RESERVED_SIZE 0x100000 +#define RESERVED_END (RESERVED_START + RESERVED_SIZE) +#define FIRST (RESERVED_END - CACHEABLE_START) + +/* Note that second region overlaps first */ +struct resource res_mock[] = { + { .base = CACHEABLE_START, .size = CACHEABLE_SIZE, .next = &res_mock[1], + .flags = IORESOURCE_CACHEABLE | IORESOURCE_MEM }, + { .base = RESERVED_START, .size = RESERVED_SIZE, .next = NULL, + .flags = IORESOURCE_RESERVE | IORESOURCE_MEM } +}; + +/* Device simulating RAM */ +struct device mem_device_mock = { + .enabled = 1, + .resource_list = res_mock, + .next = NULL +}; + +/* Simplified version for the purpose of tests */ +static uint32_t bootmem_to_lb_tag(const enum bootmem_type tag) +{ + switch (tag) { + case BM_MEM_RAM: + return LB_MEM_RAM; + case BM_MEM_RESERVED: + return LB_MEM_RESERVED; + default: + return LB_MEM_RESERVED; + } +} + +/* Bootmem layout for bootmem tests + * + * Regions marked with asteriks (***) are not visible for OS + * + * +-------+----CACHEABLE_MEMORY---------+-+ <-0x10000000 + * | | ***PROGRAM*** | | + * | +-----------------------------+ | <-0x10040000 + * | | ***STACK*** | | + * | +-----------------------------+ | <-0x10041000 + * | | + * | | + * | | + * | +-------RESERVED_MEMORY-------+ | <-0x100000000 + * | | | | + * | | | | + * | | | | + * | +-----------------------------+ | <-0x100100000 + * | | + * | | + * +---------------------------------------+ <-0x110000000 + * + */ +static int test_basic_setup(void **state) +{ + os_ranges = (struct bootmem_ranges_t *)malloc(3 * sizeof(*os_ranges)); + + if (!os_ranges) + return -1; + + os_ranges[0].start = CACHEABLE_START; + os_ranges[0].size = RESERVED_START - CACHEABLE_START; + os_ranges[0].type = BM_MEM_RAM; + + os_ranges[1].start = RESERVED_START; + os_ranges[1].size = RESERVED_SIZE; + os_ranges[1].type = BM_MEM_RESERVED; + + os_ranges[2].start = RESERVED_END; + os_ranges[2].size = CACHEABLE_END - RESERVED_END; + os_ranges[2].type = BM_MEM_RAM; + + return 0; +} + +static int test_basic_teardown(void **state) +{ + free(os_ranges); + + return 0; +} + +/* This test need to be run first, in order to use bootmem library API */ +static void test_bootmem_write_mem_table(void **state) +{ + int i; + struct lb_memory *lb_mem; + + will_return_always(search_global_resources, &mem_device_mock); + + /* Allocate space for 5 lb_mem entries to be safe */ + lb_mem = malloc(sizeof(*lb_mem) + 5 * sizeof(struct lb_memory_range)); + + bootmem_write_memory_table(lb_mem); + + /* There should be only three entries visible in coreboot table */ + assert_int_equal(lb_mem->size, 3 * sizeof(struct lb_memory_range)); + + for (i = 0; i < lb_mem->size / sizeof(struct lb_memory_range); i++) { + assert_int_equal(unpack_lb64(lb_mem->map[i].start), os_ranges[i].start); + assert_int_equal(unpack_lb64(lb_mem->map[i].size), os_ranges[i].size); + assert_int_equal(lb_mem->map[i].type, bootmem_to_lb_tag(os_ranges[i].type)); + } + + free(lb_mem); +} + +int os_bootmem_walk_cnt; +int bootmem_walk_cnt; + +static bool verify_os_bootmem_walk(const struct range_entry *r, void *arg) +{ + assert_int_equal(range_entry_base(r), os_ranges[os_bootmem_walk_cnt].start); + assert_int_equal(range_entry_size(r), os_ranges[os_bootmem_walk_cnt].size); + assert_int_equal(range_entry_tag(r), os_ranges[os_bootmem_walk_cnt].type); + + os_bootmem_walk_cnt++; + + return true; +} + +static bool verify_bootmem_walk(const struct range_entry *r, void *arg) +{ + assert_int_equal(range_entry_base(r), ranges[bootmem_walk_cnt].start); + assert_int_equal(range_entry_size(r), ranges[bootmem_walk_cnt].size); + assert_int_equal(range_entry_tag(r), ranges[bootmem_walk_cnt].type); + + bootmem_walk_cnt++; + + return true; +} + +static int test_bootmem_walk_setup(void **state) +{ + if (test_basic_setup(state) < 0) + return -1; + + /* program and stack regions should be merged since they are neighbors */ + ranges = (struct bootmem_ranges_t *)malloc(4 * sizeof(*os_ranges)); + + ranges[0].start = PROGRAM_START; + ranges[0].size = RAMSTAGE_SIZE; + ranges[0].type = BM_MEM_RAMSTAGE; + + ranges[1].start = CACHEABLE_START + RAMSTAGE_SIZE; + ranges[1].size = RESERVED_START - ranges[1].start; + ranges[1].type = BM_MEM_RAM; + + ranges[2].start = RESERVED_START; + ranges[2].size = RESERVED_SIZE; + ranges[2].type = BM_MEM_RESERVED; + + ranges[3].start = RESERVED_END; + ranges[3].size = CACHEABLE_END - RESERVED_END; + ranges[3].type = BM_MEM_RAM; + + os_bootmem_walk_cnt = 0; + bootmem_walk_cnt = 0; + + return 0; +} + +static int test_bootmem_walk_teardown(void **state) +{ + test_basic_teardown(state); + + free(ranges); + + return 0; +} + +static void test_bootmem_walk(void **state) +{ + bootmem_walk_os_mem(verify_os_bootmem_walk, NULL); + bootmem_walk(verify_bootmem_walk, NULL); + + assert_int_equal(os_bootmem_walk_cnt, 3); + assert_int_equal(bootmem_walk_cnt, 4); +} + +static void test_bootmem_region_targets_type(void **state) +{ + int ret; + + ret = bootmem_region_targets_type(PROGRAM_START, RAMSTAGE_SIZE, BM_MEM_RAMSTAGE); + assert_int_equal(ret, 1); + + /* Below range covers two differently tagged regions */ + ret = bootmem_region_targets_type(PROGRAM_START, RAMSTAGE_SIZE, BM_MEM_RAMSTAGE + 1); + assert_int_equal(ret, 0); +} + +static void test_bootmem_allocate_buffer(void **state) +{ + void *buf; + + /* All allocated buffers should be below 32bit boundary */ + buf = bootmem_allocate_buffer((1ULL << 32)); + assert_null(buf); + + /* Try too big size for our BM_MEM_RAM range below 32bit boundary */ + buf = bootmem_allocate_buffer(RESERVED_START - PROGRAM_START); + assert_null(buf); + + /* Two working cases */ + buf = bootmem_allocate_buffer(0xE0000000); + assert_non_null(buf); + assert_in_range((uintptr_t)buf, CACHEABLE_START + RAMSTAGE_SIZE, RESERVED_START); + + buf = bootmem_allocate_buffer(0xF000000); + assert_non_null(buf); + assert_in_range((uintptr_t)buf, CACHEABLE_START + RAMSTAGE_SIZE, RESERVED_START); + + /* Run out of memory for new allocations */ + buf = bootmem_allocate_buffer(0x1000000); + assert_null(buf); +} + +int main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test_setup_teardown(test_bootmem_write_mem_table, + test_basic_setup, + test_basic_teardown), + cmocka_unit_test_setup_teardown(test_bootmem_walk, + test_bootmem_walk_setup, + test_bootmem_walk_teardown), + cmocka_unit_test(test_bootmem_allocate_buffer), + cmocka_unit_test(test_bootmem_region_targets_type) + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +} diff --git a/tests/lib/bootmem-test.ld b/tests/lib/bootmem-test.ld new file mode 100644 index 0000000..eefef63 --- /dev/null +++ b/tests/lib/bootmem-test.ld @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +/* + * Below values needs to be in sync with tests/lib/bootmem-test.c code, + * since there are some assumptions about overlapping regions, neighboring + * ones etc. + */ +SECTIONS { + _program = 0x10000000; + _eprogram = _program + 0x40000; + _stack = _eprogram; + _estack = _stack + 0x1000; + _ramstage_size = _estack - _program; +} +/* Below instruction is just for the default script not being overwritten by this helper */ +INSERT AFTER .rodata;
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43510/1/tests/lib/bootmem-test.c File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/1/tests/lib/bootmem-test.c@93 PS1, Line 93: test_ Since this isn't actually a test, I don't think it should have a test_ prefix. Same comment goes for test_basic_teardown, test_bootmem_walk_setup and test_bootmem_walk_teardown.
https://review.coreboot.org/c/coreboot/+/43510/1/tests/lib/bootmem-test.c@12... PS1, Line 122: /* This test need to be run first, in order to use bootmem library API */ Requiring tests to be run in a specific order is usually a sign that the tests have hidden dependencies. It would be better to figure out and fix the problems. However, if you're going to leave it like this, then the comment should also be copied into the block in main() where the order of the tests is actually specified.
https://review.coreboot.org/c/coreboot/+/43510/1/tests/lib/bootmem-test.c@15... PS1, Line 150: bool These functions only get called once, and their return value is not used. Either declare them as void, or assert_true on the return value.
Jan Dabros has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43510/1/tests/lib/bootmem-test.c File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/1/tests/lib/bootmem-test.c@93 PS1, Line 93: test_
Since this isn't actually a test, I don't think it should have a test_ prefix. […]
Hmm.. while it is not actually a test, it is tightly coupled with a particular test. It is easy to identify setup and teardown functions for every test case. Don't you think that _setup and _teardown suffixes are enough to tell that it is not a real test but rather a helper? That being said, removing test_ prefix still will allow developers to match helpers and test method, so I can go with your proposal.
The main problem is probably that I'm not using the scheme <test_name>_setup and <test_name>_teardown consistently throughout the module (since test_basic_setup() is for test_bootmem_write_mem_table()).
https://review.coreboot.org/c/coreboot/+/43510/1/tests/lib/bootmem-test.c@12... PS1, Line 122: /* This test need to be run first, in order to use bootmem library API */
Requiring tests to be run in a specific order is usually a sign that the tests have hidden dependenc […]
You are right, I'm making use of one memory table across all the tests. However I can simply call bootmem_write_memory_table() in every test and simply ignore its output.
https://review.coreboot.org/c/coreboot/+/43510/1/tests/lib/bootmem-test.c@15... PS1, Line 150: bool
These functions only get called once, and their return value is not used. […]
These functions are callbacks defined as typedef bool (*range_action_t)(const struct range_entry *r, void *arg);
, so I need to keep this format. "The caller has to return false to break out of the loop any time, or return true to continue". I want to iterate through all entries, that's why true is always returned.
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43510/1/tests/lib/bootmem-test.c File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/1/tests/lib/bootmem-test.c@93 PS1, Line 93: test_
Hmm.. while it is not actually a test, it is tightly coupled with a particular test. […]
How about test_bootmem_write_mem_table, setup_bootmem_write_mem_table, and teardown_bootmem_write_mem_table? Similarly, test_bootmem_walk, setup_bootmem_walk, and teardown_bootmem_walk.
"There are only two hard things in Computer Science: cache invalidation and naming things." -- Phil Karlton :-D
https://review.coreboot.org/c/coreboot/+/43510/1/tests/lib/bootmem-test.c@15... PS1, Line 150: bool
These functions are callbacks defined as […]
Ack
Hello build bot (Jenkins), Anna Karaś, Patrick Georgi, Martin Roth, Paul Menzel, Paul Fagerburg, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43510
to look at the new patch set (#2).
Change subject: tests: Add lib/bootmem-test test case ......................................................................
tests: Add lib/bootmem-test test case
Signed-off-by: Jan Dabros jsd@semihalf.com Change-Id: Ic1e539061ee5051d4158712a8a981a475ea7458a --- M tests/lib/Makefile.inc A tests/lib/bootmem-test.c A tests/lib/bootmem-test.ld 3 files changed, 326 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/43510/2
Jan Dabros has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 2:
In v2 beside addressing review comments, I also fixed an issue with unit-test failing on Jenkins. It turns out, that newer (at least newer than the version I'm using on my machine) gcc versions are enabling PIE by default. This, combined with the fact that ASLR is applied for all position independent executables, makes my linker script with constant addresses for symbols useless.
In order to overcome above, I've added -no-pie option for bootmem-test compilation.
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43510/1/tests/lib/bootmem-test.c File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/1/tests/lib/bootmem-test.c@93 PS1, Line 93: test_
How about test_bootmem_write_mem_table, setup_bootmem_write_mem_table, and teardown_bootmem_write_me […]
Done
https://review.coreboot.org/c/coreboot/+/43510/1/tests/lib/bootmem-test.c@12... PS1, Line 122: /* This test need to be run first, in order to use bootmem library API */
You are right, I'm making use of one memory table across all the tests. […]
Done
https://review.coreboot.org/c/coreboot/+/43510/2/tests/lib/bootmem-test.c File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/2/tests/lib/bootmem-test.c@21... PS2, Line 212: struct lb_memory *lb_mem; : : will_return_always(search_global_resources, &mem_device_mock); : : /* Allocate space for 5 lb_mem entries to be safe */ : lb_mem = malloc(sizeof(*lb_mem) + 5 * sizeof(struct lb_memory_range)); : : /* We need to call this only to initialize library */ : bootmem_write_memory_table(lb_mem); : free(lb_mem); : Rather than copy/pasting this code into three tests, put it in a function (call it something like "init_memory_table_library"), add comments to the function about why it needs to be called, and then call the function from each of these tests.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/43510/2/tests/lib/bootmem-test.c File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/2/tests/lib/bootmem-test.c@17... PS2, Line 171: static int setup_bootmem_walk(void **state) I'm somewhat confused why you always need the setup and teardown, can't you just make 'ranges' and 'os_ranges' global, statically initialized arrays?
https://review.coreboot.org/c/coreboot/+/43510/2/tests/lib/bootmem-test.c@24... PS2, Line 247: /* Below range covers two differently tagged regions */ This comment sounds like you meant to add +1 to the size, but you actually added it to the tag.
https://review.coreboot.org/c/coreboot/+/43510/2/tests/lib/bootmem-test.c@24... PS2, Line 249: assert_int_equal(ret, 0); I think we'd want a lot more here in general to really cover the intended functionality with these tests, e.g. test only a single region by itself, test a subsection of a region (the boundaries to bootmem_region_targets_type() don't have to match the actual boundaries of the region, you can check the type of a subset), test subsections that border either edge and those that are right in the middle, test stuff that touches 0, etc. I don't know how far you want to take the first iteration of these tests so I'm not saying you have to add all of that right now, just that that's what I would consider a "complete" test set for this function.
https://review.coreboot.org/c/coreboot/+/43510/2/tests/lib/bootmem-test.c@26... PS2, Line 267: ( double parens?
https://review.coreboot.org/c/coreboot/+/43510/2/tests/lib/bootmem-test.ld File tests/lib/bootmem-test.ld:
https://review.coreboot.org/c/coreboot/+/43510/2/tests/lib/bootmem-test.ld@3 PS2, Line 3: /* There's a lot of coreboot code accessing linker script symbols and regions, so we gotta come up with a good general way to mock them. Defining a custom linker script every time seems cumbersome.
I think there are two cases we may want to mock: defining a symbol with a hardcoded, static address (like you're doing here) and defining a fake memlayout region (with the usual _myregion/_emyregion symbols) that's backed by a real global buffer in the program (in case you actually want to be able to manipulate memory inside it, with the trade-off that you then can't control the exact address anymore). I think we could add some clever macros to <tests/test.h> (or some new header specifically for this if you prefer) to allow tests to easily cover both of those:
#define TEST_SYMBOL(symbol, address) asm ( ".set " #symbol ", " #address "\n\t.globl " #symbol ) #define TEST_REGION(region, size) uint8_t _##region[size]; TEST_SYMBOL(_e##region, _##region + size)
Then rather than this script you would just put
TEST_SYMBOL(_program, 0x10000000); TEST_SYMBOL(_eprogram, _program + 0x40000);
in bootmem-test.c. If you instead wanted a writeable region in your program, for example because you wanted to test src/arch/arm64/armv8/mmu.c (which accesses _ttb), you can just do
TEST_REGION(ttb, 64*KiB);
What do you think?
Jan Dabros has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/c/coreboot/+/43510/2/tests/lib/bootmem-test.c File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/2/tests/lib/bootmem-test.c@17... PS2, Line 171: static int setup_bootmem_walk(void **state)
I'm somewhat confused why you always need the setup and teardown, can't you just make 'ranges' and ' […]
This is because in my concept I would need to use non-constant initializers (linker symbols), which is not allowed. My idea was to somehow bind values in linker script with this used here (e.g. CACHEABLE_START == _program). This may be no longer valid once we follow your approach with using "asm()" instructions.
https://review.coreboot.org/c/coreboot/+/43510/2/tests/lib/bootmem-test.c@21... PS2, Line 212: struct lb_memory *lb_mem; : : will_return_always(search_global_resources, &mem_device_mock); : : /* Allocate space for 5 lb_mem entries to be safe */ : lb_mem = malloc(sizeof(*lb_mem) + 5 * sizeof(struct lb_memory_range)); : : /* We need to call this only to initialize library */ : bootmem_write_memory_table(lb_mem); : free(lb_mem); :
Rather than copy/pasting this code into three tests, put it in a function (call it something like "i […]
Makes sense.
https://review.coreboot.org/c/coreboot/+/43510/2/tests/lib/bootmem-test.c@24... PS2, Line 247: /* Below range covers two differently tagged regions */
This comment sounds like you meant to add +1 to the size, but you actually added it to the tag.
Oops, you are right.
https://review.coreboot.org/c/coreboot/+/43510/2/tests/lib/bootmem-test.c@24... PS2, Line 249: assert_int_equal(ret, 0);
I think we'd want a lot more here in general to really cover the intended functionality with these t […]
As with memrange patchset - I want this to be pretty complete. Let me add more cases for next version.
https://review.coreboot.org/c/coreboot/+/43510/2/tests/lib/bootmem-test.c@26... PS2, Line 267: (
double parens?
To be fixed.
https://review.coreboot.org/c/coreboot/+/43510/2/tests/lib/bootmem-test.ld File tests/lib/bootmem-test.ld:
https://review.coreboot.org/c/coreboot/+/43510/2/tests/lib/bootmem-test.ld@3 PS2, Line 3: /*
There's a lot of coreboot code accessing linker script symbols and regions, so we gotta come up with […]
Nice trick with using asm() here. Keeping symbols definition within the same file as test harness sounds like a much better approach. Let my try whether this work nice for my case, if so, then let's go this way.
Attention is currently required from: Jakub Czapiga. Jakub Czapiga has uploaded a new patch set (#3) to the change originally created by Jan Dabros. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
tests: Add lib/bootmem-test test case
Signed-off-by: Jan Dabros jsd@semihalf.com Signed-off-by: Jakub Czapiga jacz@semihalf.com Change-Id: Ic1e539061ee5051d4158712a8a981a475ea7458a --- M tests/lib/Makefile.inc A tests/lib/bootmem-test.c 2 files changed, 287 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/43510/3
Attention is currently required from: Jakub Czapiga, Jan Dabros. Jakub Czapiga has uploaded a new patch set (#4) to the change originally created by Jan Dabros. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
tests: Add lib/bootmem-test test case
Signed-off-by: Jan Dabros jsd@semihalf.com Signed-off-by: Jakub Czapiga jacz@semihalf.com Change-Id: Ic1e539061ee5051d4158712a8a981a475ea7458a --- M tests/lib/Makefile.inc A tests/lib/bootmem-test.c 2 files changed, 269 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/43510/4
Attention is currently required from: Paul Fagerburg, Julius Werner, Jan Dabros. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 4:
(4 comments)
File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/comment/7e74a71f_17f90771 PS2, Line 171: static int setup_bootmem_walk(void **state)
This is because in my concept I would need to use non-constant initializers (linker symbols), which […]
Done. Only required regions symbols are set now from inside of the test source.
https://review.coreboot.org/c/coreboot/+/43510/comment/513c40dc_23b12874 PS2, Line 212: struct lb_memory *lb_mem; : : will_return_always(search_global_resources, &mem_device_mock); : : /* Allocate space for 5 lb_mem entries to be safe */ : lb_mem = malloc(sizeof(*lb_mem) + 5 * sizeof(struct lb_memory_range)); : : /* We need to call this only to initialize library */ : bootmem_write_memory_table(lb_mem); : free(lb_mem); :
Makes sense.
Done. Less code, more readability.
https://review.coreboot.org/c/coreboot/+/43510/comment/e417f85e_fe72104e PS2, Line 267: (
To be fixed.
Done
File tests/lib/bootmem-test.ld:
https://review.coreboot.org/c/coreboot/+/43510/comment/d28c498a_7b86875c PS2, Line 3: /*
Nice trick with using asm() here. […]
File removed. Symbol setup moved to the test source file.
Attention is currently required from: Paul Fagerburg, Julius Werner, Jan Dabros. Jakub Czapiga has uploaded a new patch set (#5) to the change originally created by Jan Dabros. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
tests: Add lib/bootmem-test test case
Signed-off-by: Jan Dabros jsd@semihalf.com Signed-off-by: Jakub Czapiga jacz@semihalf.com Change-Id: Ic1e539061ee5051d4158712a8a981a475ea7458a --- M tests/lib/Makefile.inc A tests/lib/bootmem-test.c 2 files changed, 308 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/43510/5
Attention is currently required from: Paul Fagerburg, Julius Werner, Jan Dabros. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 5:
(2 comments)
File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/comment/7c3806e3_07f87f0e PS2, Line 247: /* Below range covers two differently tagged regions */
Oops, you are right.
Done.
https://review.coreboot.org/c/coreboot/+/43510/comment/b212795a_a6037a30 PS2, Line 249: assert_int_equal(ret, 0);
As with memrange patchset - I want this to be pretty complete. […]
I extended this test by adding a few more cases that Julius was talking about. I also added a region starting at 0x0 to check correct handling of zero address.
Attention is currently required from: Jakub Czapiga, Julius Werner, Jan Dabros. Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 5: Code-Review+1
Attention is currently required from: Jakub Czapiga, Jan Dabros. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 5:
(9 comments)
File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/comment/6fd15d18_eb60e9f4 PS3, Line 33: asm(".set _eprogram, _program + _program_size\n\t.globl _eprogram"); Why not
TEST_SYMBOL(_eprogram, _program + _program_size)?
Please in general try to not spray random inline assembly all over the test code. Try to encapsulate what you need into as few well-defined operations as possible, encapsulate them in a macro, and put them in some common place (e.g. <tests/test.h>) with sufficient documentation of what they do so people can understand them without unpacking the asm. In this case, I think the existing TEST_SYMBOL() should work well enough, but feel free to wrap this all into a TEST_REGION_UNALLOCATED() or something if you want.
File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/comment/9e46e00d_e48d15cb PS5, Line 29: to avoid problems with compile-time expressions containing symbols. */ What problems did you have with this? Ideally we can solve them rather than needing to duplicate so many definitions.
For the sizes, please try to use coreboot's existing REGION_SIZE() accessor. This also makes things easier if we ever have to change how sizes are stored (again).
https://review.coreboot.org/c/coreboot/+/43510/comment/5436dfde_328ea990 PS5, Line 36: TEST_SYMBOL(_eprogram, _program + _program_size); Looking at this again I really think we should introduce another wrapper macro for this use case, e.g. TEST_REGION_UNALLOCATED(start, size). It can rely on TEST_SYMBOL() under the hood. Encapsulating things like these at a higher level really helps when the underlying implementation needs to change at some point.
https://review.coreboot.org/c/coreboot/+/43510/comment/ded58485_124bf82a PS5, Line 159: lb_mem = malloc(sizeof(*lb_mem) + 10 * sizeof(struct lb_memory_range)); nit: always useful to memset() a sentinel value and check that it didn't write beyond the end for these kinds of things, too.
https://review.coreboot.org/c/coreboot/+/43510/comment/8db34e2f_85e301ee PS5, Line 175: int os_bootmem_walk_cnt = 0; nit: I think it would be cleaner to initialize these explicitly in test_bootmem_walk().
https://review.coreboot.org/c/coreboot/+/43510/comment/53eacd32_e898aaca PS5, Line 213: Would be nice to have an explicit test for bootmem_add_range() after bootmem_write_memoy_table() too. Should test the difference between bootmem types above and below BM_MEM_OS_CUTOFF -- types below can no longer be added after the memory table was written, types above *can* be added but only affect the bootmem_walk() table, not bootmem_os_walk().
https://review.coreboot.org/c/coreboot/+/43510/comment/c737bb75_f3997ee0 PS5, Line 241: assert_int_equal(ret, 0); Should also check for success when overlapping multiple ranges but all covered ranges have the required type (e.g. checking an area that starts somewhere in _program and ends somewhere in _stack for MEM_RAMSTAGE).
https://review.coreboot.org/c/coreboot/+/43510/comment/e377b1d5_97e74a7e PS5, Line 283: buf = bootmem_allocate_buffer(0xF000000); This should ensure that it's not overlapping the first buffer (in fact, you should also check explicitly after every allocation that the returned area has been changed to MEM_PAYLOAD).
https://review.coreboot.org/c/coreboot/+/43510/comment/38abc2d6_5fc2bd6b PS5, Line 286: Would be nice to test that sizes get aligned up to 4K, too.
Attention is currently required from: Jakub Czapiga, Jan Dabros. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 5:
(1 comment)
File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/comment/aba5ce84_8af4b478 PS3, Line 33: asm(".set _eprogram, _program + _program_size\n\t.globl _eprogram");
Why not […]
Sorry, this was an old comment left over from what I eventually put in an email instead. Please ignore.
Attention is currently required from: Julius Werner, Jan Dabros. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 5:
(2 comments)
File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/comment/e35f656d_10fe276e PS5, Line 29: to avoid problems with compile-time expressions containing symbols. */
What problems did you have with this? Ideally we can solve them rather than needing to duplicate so […]
I had problems with `#define STACK_END_TO_RESERVED_START_SIZE (RESERVED_START - STACK_END)` and similar cases. This macro contains expresion with linker symbol value and constant value (#define or const int, does not matter). If you try to do this you will get an error telling that it cannot be put in global structure, because value is not computable at load-time.
I was thinking about defining separate symbols with this value but there are problems with symbol size. Default size is 32-bit and I do not want to use compiler flags like `-mcmodel=large` you suggested, because it would give an impression that it is required to make lib/bootmem work correctly.
https://review.coreboot.org/c/coreboot/+/43510/comment/e6cd709b_2aa42022 PS5, Line 36: TEST_SYMBOL(_eprogram, _program + _program_size);
Looking at this again I really think we should introduce another wrapper macro for this use case, e. […]
Yes, macro like that would make tests code more readable. I will introduce it in separate change. :)
Attention is currently required from: Jakub Czapiga, Jan Dabros. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 5:
(1 comment)
File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/comment/5abfa13d_b506cc0d PS5, Line 29: to avoid problems with compile-time expressions containing symbols. */
I had problems with `#define STACK_END_TO_RESERVED_START_SIZE (RESERVED_START - STACK_END)` and simi […]
So using `<constant> - <symbol>` in a static initializer also fails? I understand why `<symbol> - <symbol>` would fail (and that's how you had it at first... did you try it again after changing the RESERVED region to just use constants?), but I think the other one should be perfectly doable using a relocation with an offset. Maybe GCC isn't as smart as I was hoping it would be there...
If it's not possible I'd just make one special constant for the stack end to use in that case (with a comment why it's needed), but not for every other symbol you have.
Attention is currently required from: Jakub Czapiga, Jan Dabros. Jakub Czapiga has uploaded a new patch set (#6) to the change originally created by Jan Dabros. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
tests: Add lib/bootmem-test test case
Signed-off-by: Jan Dabros jsd@semihalf.com Signed-off-by: Jakub Czapiga jacz@semihalf.com Change-Id: Ic1e539061ee5051d4158712a8a981a475ea7458a --- M tests/lib/Makefile.inc A tests/lib/bootmem-test.c 2 files changed, 409 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/43510/6
Attention is currently required from: Julius Werner, Jan Dabros. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 6:
(8 comments)
File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/comment/d84a9398_07d06687 PS5, Line 29: to avoid problems with compile-time expressions containing symbols. */
So using `<constant> - <symbol>` in a static initializer also fails? I understand why `<symbol> - <s […]
Ok. So I removed unused constants and added comment, why it is like that. I hope this is correct now.
https://review.coreboot.org/c/coreboot/+/43510/comment/53155b4d_b4908559 PS5, Line 36: TEST_SYMBOL(_eprogram, _program + _program_size);
Yes, macro like that would make tests code more readable. I will introduce it in separate change. […]
Please look at: https://review.coreboot.org/c/coreboot/+/51769
https://review.coreboot.org/c/coreboot/+/43510/comment/31e5d370_315b80a0 PS5, Line 159: lb_mem = malloc(sizeof(*lb_mem) + 10 * sizeof(struct lb_memory_range));
nit: always useful to memset() a sentinel value and check that it didn't write beyond the end for th […]
Done. I also changed raw value in lb_mem->size assert to ARRAY_SIZE(os_ranges_mock). Now one can clearly see what this assert depends on.
https://review.coreboot.org/c/coreboot/+/43510/comment/30a86e8e_d0cebec6 PS5, Line 175: int os_bootmem_walk_cnt = 0;
nit: I think it would be cleaner to initialize these explicitly in test_bootmem_walk().
Yes, it looks better when in test function.
https://review.coreboot.org/c/coreboot/+/43510/comment/dca23d6d_71500552 PS5, Line 213:
Would be nice to have an explicit test for bootmem_add_range() after bootmem_write_memoy_table() too […]
Done. I think. I had to change test to white-box by including assert.h, redefining assert() and including bootmem.c source. This way I could check assert failures, because assert.h does not provide any way to replace assert macro.
https://review.coreboot.org/c/coreboot/+/43510/comment/14a02fb1_9b8615cc PS5, Line 241: assert_int_equal(ret, 0);
Should also check for success when overlapping multiple ranges but all covered ranges have the requi […]
Sure. There are so many test cases that it might not be possible to cover them all, but one more is better than lack of it :)
https://review.coreboot.org/c/coreboot/+/43510/comment/1e49be26_85afe227 PS5, Line 283: buf = bootmem_allocate_buffer(0xF000000);
This should ensure that it's not overlapping the first buffer (in fact, you should also check explic […]
Done. I used bootmem_region_targets_type(). As this function is tested, I think I can use it there.
https://review.coreboot.org/c/coreboot/+/43510/comment/6f0bd75e_952585ce PS5, Line 286:
Would be nice to test that sizes get aligned up to 4K, too.
Done with bootmem_walk() and custom action function. It checks alignment of base and size of all ranges with BM_MEM_PAYLOAD tag.
Attention is currently required from: Jakub Czapiga, Jan Dabros. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 6:
(4 comments)
File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/comment/8703713e_3ddc4839 PS6, Line 9: #define assert(statement) mock_assert(statement, #statement, __FILE__, __LINE__) (Sorry for holding this patch up again but you added yet another new thing that requires discussion now. ;) )
Having a mechanism to mock asserts is great, but let's not hack it individually at the top of every test. We need something generic to do this that other tests can reuse. I think the only good option there is to put this into coreboot's <assert.h>.
So first, we need a way to distinguish building for tests from building normally. You should add a -D__TEST__ to TEST_CFLAGS, and then you can define an ENV_TEST in <rules.h> based on that (similar to what we have for stages). Then you can use that in <assert.h> to define different behavior for tests.
https://review.coreboot.org/c/coreboot/+/43510/comment/1910c188_5c0c9f6d PS6, Line 13: #include <string.h> nit: alphabetize?
https://review.coreboot.org/c/coreboot/+/43510/comment/c60a76f6_45d406ac PS6, Line 167: assert_memory_equal(sentinel_value_buffer, nit: I've seen this construct (malloc() and memset() something just to compare it) in a couple of tests now, maybe we should have a global assert_memory_filled_with() helper for that.
https://review.coreboot.org/c/coreboot/+/43510/comment/b4942849_0b514b61 PS6, Line 381: 0xE0000000 0xF0000000
Attention is currently required from: Jakub Czapiga, Jan Dabros. Jakub Czapiga has uploaded a new patch set (#7) to the change originally created by Jan Dabros. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
tests: Add lib/bootmem-test test case
Signed-off-by: Jan Dabros jsd@semihalf.com Signed-off-by: Jakub Czapiga jacz@semihalf.com Change-Id: Ic1e539061ee5051d4158712a8a981a475ea7458a --- M tests/lib/Makefile.inc A tests/lib/bootmem-test.c 2 files changed, 417 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/43510/7
Attention is currently required from: Julius Werner, Jan Dabros. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 7:
(4 comments)
File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/comment/e7f1d6ab_e023d6fe PS6, Line 9: #define assert(statement) mock_assert(statement, #statement, __FILE__, __LINE__)
(Sorry for holding this patch up again but you added yet another new thing that requires discussion […]
Don't worry about holding this patch. It's better to submit a good one later than a problematic one right now. :)
Your idea looks great. I will divide it into two changes: - Add __TEST__ and ENV_TEST - Adapt assert.h to use mock_assert instead of default assert for ENV_TEST targets
Regarding ENV_TEST: I think that structure similar to ENV_TIMELESS definition will be sufficient.
Please see: https://review.coreboot.org/c/coreboot/+/51803 https://review.coreboot.org/c/coreboot/+/51804
https://review.coreboot.org/c/coreboot/+/43510/comment/d77273ef_ddbbb020 PS6, Line 13: #include <string.h>
nit: alphabetize?
Done
https://review.coreboot.org/c/coreboot/+/43510/comment/1645e0b0_b408d95a PS6, Line 167: assert_memory_equal(sentinel_value_buffer,
nit: I've seen this construct (malloc() and memset() something just to compare it) in a couple of te […]
I will have that in my mind. TODO as next patch :)
https://review.coreboot.org/c/coreboot/+/43510/comment/70712484_eb59d2dc PS6, Line 381: 0xE0000000
0xF0000000
Done
Attention is currently required from: Jakub Czapiga, Julius Werner. Jakub Czapiga has uploaded a new patch set (#8) to the change originally created by Jan Dabros. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
tests: Add lib/bootmem-test test case
Signed-off-by: Jan Dabros jsd@semihalf.com Signed-off-by: Jakub Czapiga jacz@semihalf.com Change-Id: Ic1e539061ee5051d4158712a8a981a475ea7458a --- M tests/lib/Makefile.inc A tests/lib/bootmem-test.c 2 files changed, 418 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/43510/8
Attention is currently required from: Jakub Czapiga, Julius Werner, Jan Dabros. Jakub Czapiga has uploaded a new patch set (#10) to the change originally created by Jan Dabros. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
tests: Add lib/bootmem-test test case
Signed-off-by: Jan Dabros jsd@semihalf.com Signed-off-by: Jakub Czapiga jacz@semihalf.com Change-Id: Ic1e539061ee5051d4158712a8a981a475ea7458a --- M tests/lib/Makefile.inc A tests/lib/bootmem-test.c 2 files changed, 418 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/43510/10
Attention is currently required from: Jakub Czapiga, Julius Werner, Jan Dabros. Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 10: Code-Review+1
Attention is currently required from: Jakub Czapiga, Jan Dabros. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 10: Code-Review+2
Attention is currently required from: Jakub Czapiga, Jan Dabros. Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 10:
(1 comment)
File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/comment/38e0bc8f_064ed548 PS6, Line 9: #define assert(statement) mock_assert(statement, #statement, __FILE__, __LINE__)
Don't worry about holding this patch. […]
Done
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
tests: Add lib/bootmem-test test case
Signed-off-by: Jan Dabros jsd@semihalf.com Signed-off-by: Jakub Czapiga jacz@semihalf.com Change-Id: Ic1e539061ee5051d4158712a8a981a475ea7458a Reviewed-on: https://review.coreboot.org/c/coreboot/+/43510 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Paul Fagerburg pfagerburg@chromium.org Reviewed-by: Julius Werner jwerner@chromium.org --- M tests/lib/Makefile.inc A tests/lib/bootmem-test.c 2 files changed, 418 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Paul Fagerburg: Looks good to me, but someone else must approve
diff --git a/tests/lib/Makefile.inc b/tests/lib/Makefile.inc index cba4361..3a43a64 100644 --- a/tests/lib/Makefile.inc +++ b/tests/lib/Makefile.inc @@ -24,6 +24,7 @@ tests-y += compute_ip_checksum-test tests-y += memrange-test tests-y += uuid-test +tests-y += bootmem-test
string-test-srcs += tests/lib/string-test.c string-test-srcs += src/lib/string.c @@ -116,3 +117,10 @@ uuid-test-srcs += tests/lib/uuid-test.c uuid-test-srcs += src/lib/hexstrtobin.c uuid-test-srcs += src/lib/uuid.c + +bootmem-test-srcs += tests/lib/bootmem-test.c +bootmem-test-srcs += tests/stubs/console.c +bootmem-test-srcs += src/device/device_util.c +bootmem-test-srcs += src/lib/bootmem.c +bootmem-test-srcs += src/lib/memrange.c + diff --git a/tests/lib/bootmem-test.c b/tests/lib/bootmem-test.c new file mode 100644 index 0000000..85a0136 --- /dev/null +++ b/tests/lib/bootmem-test.c @@ -0,0 +1,410 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <bootmem.h> +#include <commonlib/coreboot_tables.h> +#include <device/device.h> +#include <device/resource.h> +#include <memrange.h> +#include <stdlib.h> +#include <string.h> +#include <symbols.h> +#include <tests/test.h> + +/* Stubs defined to satisfy linker dependencies */ +void cbmem_add_bootmem(void) +{ +} + +void bootmem_arch_add_ranges(void) +{ +} + +struct bootmem_ranges_t { + uint64_t start; + uint64_t size; + uint32_t type; +}; + +/* Define symbols for regions required by bootmem. + Define constants for regions that do not need to be defined in the executable. + There is no need for region memory, just start, end and size symbols are required. + Only used values are defined. */ +#define ZERO_REGION_START ((uintptr_t)0x0) +#define ZERO_REGION_SIZE ((uintptr_t)0x10000) + +TEST_REGION_UNALLOCATED(program, 0x10000000, 0x40000); +#define PROGRAM_START ((uintptr_t)_program) +#define PROGRAM_SIZE REGION_SIZE(program) + +#define CACHEABLE_START ((uintptr_t)0x10000000ULL) +#define CACHEABLE_SIZE ((uintptr_t)0x100000000ULL) +#define CACHEABLE_END ((uintptr_t)(CACHEABLE_START + CACHEABLE_SIZE)) + +/* Stack region end address is hardcoded because `<const> - <symbol>` does not work in GCC */ +TEST_REGION_UNALLOCATED(stack, 0x10040000, 0x1000); +#define STACK_START ((uintptr_t)_stack) +#define STACK_SIZE REGION_SIZE(stack) +#define STACK_END ((uintptr_t)(0x10040000 + 0x1000)) + +#define RESERVED_START ((uintptr_t)0x100000000ULL) +#define RESERVED_SIZE ((uintptr_t)0x100000) +#define RESERVED_END ((uintptr_t)(RESERVED_START + RESERVED_SIZE)) + +TEST_REGION_UNALLOCATED(ramstage, 0x10000000, 0x41000); +#define RAMSTAGE_START ((uintptr_t)_ramstage) +#define RAMSTAGE_SIZE REGION_SIZE(ramstage) + +#define CACHEABLE_START_TO_RESERVED_START_SIZE (RESERVED_START - CACHEABLE_START) +#define RESERVED_END_TO_CACHEABLE_END_SIZE (CACHEABLE_END - RESERVED_END) +#define STACK_END_TO_RESERVED_START_SIZE (RESERVED_START - STACK_END) + + +/* Bootmem layout for tests + * + * Regions marked with asterisks (***) are not visible for OS + * + * +------------------ZERO-----------------+ <-0x0 + * | | + * +---------------------------------------+ <-0x10000 + * + * +-------+----CACHEABLE_MEMORY---------+-+ <-0x10000000 + * | | ***PROGRAM*** | | + * | +-----------------------------+ | <-0x10040000 + * | | ***STACK*** | | + * | +-----------------------------+ | <-0x10041000 + * | | + * | | + * | | + * | +-------RESERVED_MEMORY-------+ | <-0x100000000 + * | | | | + * | | | | + * | | | | + * | +-----------------------------+ | <-0x100100000 + * | | + * | | + * +---------------------------------------+ <-0x110000000 + * + * Ramstage covers PROGRAM and STACK regions. + */ +struct bootmem_ranges_t os_ranges_mock[] = { + [0] = { .start = ZERO_REGION_START, .size = ZERO_REGION_SIZE, + .type = BM_MEM_RAM}, + [1] = { .start = CACHEABLE_START, .size = CACHEABLE_START_TO_RESERVED_START_SIZE, + .type = BM_MEM_RAM }, + [2] = { .start = RESERVED_START, .size = RESERVED_SIZE, + .type = BM_MEM_RESERVED }, + [3] = { .start = RESERVED_END, .size = RESERVED_END_TO_CACHEABLE_END_SIZE, + .type = BM_MEM_RAM }, +}; + +struct bootmem_ranges_t ranges_mock[] = { + [0] = { .start = ZERO_REGION_START, .size = ZERO_REGION_SIZE, + .type = BM_MEM_RAM }, + [1] = { .start = RAMSTAGE_START, .size = RAMSTAGE_SIZE, + .type = BM_MEM_RAMSTAGE }, + [2] = { .start = STACK_END, .size = STACK_END_TO_RESERVED_START_SIZE, + .type = BM_MEM_RAM }, + [3] = { .start = RESERVED_START, .size = RESERVED_SIZE, + .type = BM_MEM_RESERVED }, + [4] = { .start = RESERVED_END, .size = RESERVED_END_TO_CACHEABLE_END_SIZE, + .type = BM_MEM_RAM }, +}; + +struct bootmem_ranges_t *os_ranges = os_ranges_mock; +struct bootmem_ranges_t *ranges = ranges_mock; + +/* Note that second region overlaps first */ +struct resource res_mock[] = { + { .base = ZERO_REGION_START, .size = ZERO_REGION_SIZE, .next = &res_mock[1], + .flags = IORESOURCE_CACHEABLE | IORESOURCE_MEM }, + { .base = CACHEABLE_START, .size = CACHEABLE_SIZE, .next = &res_mock[2], + .flags = IORESOURCE_CACHEABLE | IORESOURCE_MEM }, + { .base = RESERVED_START, .size = RESERVED_SIZE, .next = NULL, + .flags = IORESOURCE_RESERVE | IORESOURCE_MEM } +}; + +/* Device simulating RAM */ +struct device mem_device_mock = { + .enabled = 1, + .resource_list = res_mock, + .next = NULL +}; + +struct device *all_devices = &mem_device_mock; + +/* Simplified version for the purpose of tests */ +static uint32_t bootmem_to_lb_tag(const enum bootmem_type tag) +{ + switch (tag) { + case BM_MEM_RAM: + return LB_MEM_RAM; + case BM_MEM_RESERVED: + return LB_MEM_RESERVED; + default: + return LB_MEM_RESERVED; + } +} + +static void test_bootmem_write_mem_table(void **state) +{ + /* Space for 10 lb_mem entries to be safe */ + const size_t lb_mem_max_size = sizeof(struct lb_memory) + + 10 * sizeof(struct lb_memory_range); + const size_t expected_allocation_size = + (sizeof(struct lb_memory) + + ARRAY_SIZE(os_ranges_mock) * sizeof(struct lb_memory_range)); + const size_t required_unused_space_size = lb_mem_max_size - expected_allocation_size; + int i; + struct lb_memory *lb_mem; + /* Allocate buffer and fill it. Use it to ensure correct size of space used + by bootmem_write_memory_table() */ + u8 sentinel_value_buffer[required_unused_space_size]; + memset(sentinel_value_buffer, 0x77, required_unused_space_size); + + lb_mem = malloc(lb_mem_max_size); + /* Fill rest of buffer with sentinel value */ + memset(((u8 *)lb_mem) + expected_allocation_size, 0x77, required_unused_space_size); + + bootmem_write_memory_table(lb_mem); + + /* There should be only `os_ranges_mock` entries visible in coreboot table */ + assert_int_equal(lb_mem->size, + ARRAY_SIZE(os_ranges_mock) * sizeof(struct lb_memory_range)); + assert_memory_equal(sentinel_value_buffer, + ((u8 *)lb_mem) + expected_allocation_size, + required_unused_space_size); + + for (i = 0; i < lb_mem->size / sizeof(struct lb_memory_range); i++) { + assert_int_equal(unpack_lb64(lb_mem->map[i].start), os_ranges[i].start); + assert_int_equal(unpack_lb64(lb_mem->map[i].size), os_ranges[i].size); + assert_int_equal(lb_mem->map[i].type, bootmem_to_lb_tag(os_ranges[i].type)); + } + + free(lb_mem); +} + +int os_bootmem_walk_cnt; +int bootmem_walk_cnt; + +static bool verify_os_bootmem_walk(const struct range_entry *r, void *arg) +{ + assert_int_equal(range_entry_base(r), os_ranges[os_bootmem_walk_cnt].start); + assert_int_equal(range_entry_size(r), os_ranges[os_bootmem_walk_cnt].size); + assert_int_equal(range_entry_tag(r), os_ranges[os_bootmem_walk_cnt].type); + + os_bootmem_walk_cnt++; + + return true; +} + +static bool verify_bootmem_walk(const struct range_entry *r, void *arg) +{ + assert_int_equal(range_entry_base(r), ranges[bootmem_walk_cnt].start); + assert_int_equal(range_entry_size(r), ranges[bootmem_walk_cnt].size); + assert_int_equal(range_entry_tag(r), ranges[bootmem_walk_cnt].type); + + bootmem_walk_cnt++; + + return true; +} + +static bool count_entries_os_bootmem_walk(const struct range_entry *r, void *arg) +{ + os_bootmem_walk_cnt++; + + return true; +} + +static bool count_entries_bootmem_walk(const struct range_entry *r, void *arg) +{ + bootmem_walk_cnt++; + + return true; +} + +/* This function initializes bootmem using bootmem_write_memory_table(). + bootmem_init() is not accessible directly because it is static. */ +static void init_memory_table_library(void) +{ + struct lb_memory *lb_mem; + + /* Allocate space for 10 lb_mem entries to be safe */ + lb_mem = malloc(sizeof(*lb_mem) + 10 * sizeof(struct lb_memory_range)); + + /* We need to call this only to initialize library */ + bootmem_write_memory_table(lb_mem); + free(lb_mem); +} + +static void test_bootmem_add_range(void **state) +{ + init_memory_table_library(); + + os_bootmem_walk_cnt = 0; + bootmem_walk_os_mem(count_entries_os_bootmem_walk, NULL); + assert_int_equal(os_bootmem_walk_cnt, 4); + + bootmem_walk_cnt = 0; + bootmem_walk(count_entries_bootmem_walk, NULL); + assert_int_equal(bootmem_walk_cnt, 5); + + expect_assert_failure( + bootmem_add_range(ALIGN_UP(PROGRAM_START, 4096), + ALIGN_DOWN(PROGRAM_SIZE / 2, 4096), + BM_MEM_ACPI) + ); + + os_bootmem_walk_cnt = 0; + bootmem_walk_os_mem(count_entries_os_bootmem_walk, NULL); + assert_int_equal(os_bootmem_walk_cnt, 4); + + bootmem_walk_cnt = 0; + bootmem_walk(count_entries_bootmem_walk, NULL); + assert_int_equal(bootmem_walk_cnt, 6); + + /* Do not expect assert failure as BM_MEM_RAMSTAGE should not be added to os_bootmem */ + bootmem_add_range(ALIGN_UP(STACK_END + 4096, 4096), + ALIGN_DOWN(STACK_END_TO_RESERVED_START_SIZE / 2, 4096), + BM_MEM_RAMSTAGE); + + os_bootmem_walk_cnt = 0; + bootmem_walk_os_mem(count_entries_os_bootmem_walk, NULL); + assert_int_equal(os_bootmem_walk_cnt, 4); + + /* Two entries are added because added range is in middle of another */ + bootmem_walk_cnt = 0; + bootmem_walk(count_entries_bootmem_walk, NULL); + assert_int_equal(bootmem_walk_cnt, 8); +} + +static void test_bootmem_walk(void **state) +{ + init_memory_table_library(); + + os_bootmem_walk_cnt = 0; + bootmem_walk_os_mem(verify_os_bootmem_walk, NULL); + assert_int_equal(os_bootmem_walk_cnt, 4); + + bootmem_walk_cnt = 0; + bootmem_walk(verify_bootmem_walk, NULL); + assert_int_equal(bootmem_walk_cnt, 5); +} + +static void test_bootmem_region_targets_type(void **state) +{ + int ret; + u64 subregion_start; + u64 subregion_size; + + init_memory_table_library(); + + /* Single whole region */ + ret = bootmem_region_targets_type(RAMSTAGE_START, RAMSTAGE_SIZE, BM_MEM_RAMSTAGE); + assert_int_equal(ret, 1); + + /* Expect fail because of incorrect bootmem_type */ + ret = bootmem_region_targets_type(RAMSTAGE_START, RAMSTAGE_SIZE, BM_MEM_RESERVED); + assert_int_equal(ret, 0); + + /* Range covering one more byte than one region*/ + ret = bootmem_region_targets_type(RAMSTAGE_START, RAMSTAGE_SIZE + 1, BM_MEM_RAMSTAGE); + assert_int_equal(ret, 0); + + /* Expect success for subregion of ramstage stretching from point in program range + to point in stack range. */ + subregion_start = PROGRAM_START + PROGRAM_SIZE / 4; + subregion_size = STACK_END - STACK_SIZE / 4 - subregion_start; + ret = bootmem_region_targets_type(subregion_start, subregion_size, BM_MEM_RAMSTAGE); + assert_int_equal(ret, 1); + + /* Expect fail for range covering more than one tag as there is no BM_MEM_CACHEABLE */ + subregion_start = STACK_START + STACK_SIZE / 2; + subregion_size = RESERVED_START + RESERVED_SIZE / 4 * 3 - subregion_start; + ret = bootmem_region_targets_type(subregion_start, subregion_size, BM_MEM_RAM); + assert_int_equal(ret, 0); + + /* Middle of range should not fail */ + ret = bootmem_region_targets_type(RESERVED_START + RESERVED_SIZE / 4, + RESERVED_SIZE / 2, BM_MEM_RESERVED); + assert_int_equal(ret, 1); + + /* Subsection of range bordering end edge */ + ret = bootmem_region_targets_type(RESERVED_END + RESERVED_END_TO_CACHEABLE_END_SIZE / 2, + RESERVED_END_TO_CACHEABLE_END_SIZE / 2, BM_MEM_RAM); + assert_int_equal(ret, 1); + + /* Region touching zero */ + ret = bootmem_region_targets_type(ZERO_REGION_START, ZERO_REGION_SIZE, BM_MEM_RAM); + assert_int_equal(ret, 1); + + /* Expect failure when passing zero as size. */ + ret = bootmem_region_targets_type(ZERO_REGION_START, 0, BM_MEM_RAM); + assert_int_equal(ret, 0); + ret = bootmem_region_targets_type(RESERVED_START, 0, BM_MEM_RESERVED); + assert_int_equal(ret, 0); +} + +/* Action function used to check alignment of size and base of allocated ranges */ +static bool verify_bootmem_allocate_buffer(const struct range_entry *r, void *arg) +{ + if (range_entry_tag(r) == BM_MEM_PAYLOAD) { + assert_true(IS_ALIGNED(range_entry_base(r), 4096)); + assert_true(IS_ALIGNED(range_entry_size(r), 4096)); + } + + return true; +} + + +static void test_bootmem_allocate_buffer(void **state) +{ + void *buf; + void *prev; + + init_memory_table_library(); + + /* All allocated buffers should be below 32bit boundary */ + buf = bootmem_allocate_buffer(1ULL << 32); + assert_null(buf); + + /* Try too big size for our BM_MEM_RAM range below 32bit boundary */ + buf = bootmem_allocate_buffer(RESERVED_START - PROGRAM_START); + assert_null(buf); + + /* Two working cases */ + buf = bootmem_allocate_buffer(0xE0000000); + assert_non_null(buf); + assert_int_equal(1, bootmem_region_targets_type((uintptr_t)buf, + 0xE0000000, BM_MEM_PAYLOAD)); + assert_in_range((uintptr_t)buf, CACHEABLE_START + RAMSTAGE_SIZE, RESERVED_START); + /* Check if allocated (payload) ranges have their base and size aligned */ + bootmem_walk(verify_bootmem_allocate_buffer, NULL); + + prev = buf; + buf = bootmem_allocate_buffer(0xF000000); + assert_non_null(buf); + assert_int_equal(1, bootmem_region_targets_type((uintptr_t)buf, + 0xF000000, BM_MEM_PAYLOAD)); + assert_in_range((uintptr_t)buf, CACHEABLE_START + RAMSTAGE_SIZE, RESERVED_START); + /* Check if newly allocated buffer does not overlap with previously allocated range */ + assert_not_in_range((uintptr_t)buf, (uintptr_t)prev, (uintptr_t)prev + 0xE0000000); + /* Check if allocated (payload) ranges have their base and size aligned */ + bootmem_walk(verify_bootmem_allocate_buffer, NULL); + + /* Run out of memory for new allocations */ + buf = bootmem_allocate_buffer(0x1000000); + assert_null(buf); +} + +int main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test(test_bootmem_write_mem_table), + cmocka_unit_test(test_bootmem_add_range), + cmocka_unit_test(test_bootmem_walk), + cmocka_unit_test(test_bootmem_allocate_buffer), + cmocka_unit_test(test_bootmem_region_targets_type) + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +}
Attention is currently required from: Jan Dabros. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 11:
(1 comment)
File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/comment/6a0cb1ee_491bb914 PS11, Line 232: lb_mem = malloc(sizeof(*lb_mem) + 10 * sizeof(struct lb_memory_range)); Found by Coverity: you should initialize lb_mem->size (and ideally check that the function modified it as expected).
Attention is currently required from: Jan Dabros. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43510 )
Change subject: tests: Add lib/bootmem-test test case ......................................................................
Patch Set 11:
(1 comment)
File tests/lib/bootmem-test.c:
https://review.coreboot.org/c/coreboot/+/43510/comment/bf7eb1ea_bfcf32a6 PS11, Line 232: lb_mem = malloc(sizeof(*lb_mem) + 10 * sizeof(struct lb_memory_range));
Found by Coverity: you should initialize lb_mem->size (and ideally check that the function modified […]
Size (and tag) initialization is missing, you are right. But size is present in assert in test_bootmem_write_mem_table(). I think, that it is not necessary to add assert here as it will be checked in mentioned test. I just have to include initial size in assertion :)
Please look at: https://review.coreboot.org/c/coreboot/+/52263
-
Jakub Czapiga (Code Review) -
Jan Dabros (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Fagerburg (Code Review)