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.