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.