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.