Attention is currently required from: Jakub Czapiga, Jan Dabros.
9 comments:
File tests/lib/bootmem-test.c:
Patch Set #3, 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:
Patch Set #5, 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).
Patch Set #5, 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.
Patch Set #5, 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.
Patch Set #5, Line 175: int os_bootmem_walk_cnt = 0;
nit: I think it would be cleaner to initialize these explicitly in test_bootmem_walk().
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().
Patch Set #5, 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).
Patch Set #5, 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).
Would be nice to test that sizes get aligned up to 4K, too.
To view, visit change 43510. To unsubscribe, or for help writing mail filters, visit settings.