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?