Attention is currently required from: jacz@semihalf.com. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46458 )
Change subject: tests: Add lib/imd_cbmem-test test case ......................................................................
Patch Set 11:
(7 comments)
File tests/lib/imd_cbmem-test.c:
https://review.coreboot.org/c/coreboot/+/46458/comment/03ed984f_2d685226 PS10, Line 431: assert_int_not_equal(0, size);
I added multiple big (same size) entries, accumulated their size and, with overhead size and root en […]
The maximum amount of small entries should be ((CBMEM_SM_ROOT_SIZE - sizeof(struct imd_root_pointer) - sizeof(struct imd_root)) / sizeof(struct imd_entry)) == ((1024 - 8 - 20) / 16) == 62 (see root_num_entries()). But if you add small entries larger than 32 bytes they will fill up more of that space at once (the decision whether to add an entry to small or large region is the (size <= r->entry_align || size <= imd_root_data_left(r) / 4) in imd_entry_add(), so it changes dynamically based on how much of the area is still free).
I don't think we need to test details about small vs. large allocation here, maybe you could add another whitebox test for the lower level IMD APIs for that if you want, but here I think we only care that cbmem_get_region() returns a value that *roughly* corresponds to the amount of data we have thrown into it. Maybe add one or two small entries and check that the overall size doesn't change, but I wouldn't make it more complicated than that.
File tests/lib/imd_cbmem-test.c:
https://review.coreboot.org/c/coreboot/+/46458/comment/a62e735c_f370eac8 PS11, Line 144: /* Expect clean initialization of cbmem */ This comment confused me... what really happens here is that the call fails because there's no previous CBMEM to recover. But you make it sound like it would then get reinitialized from scratch, which is not actually what cbmem_initialize() does.
https://review.coreboot.org/c/coreboot/+/46458/comment/4a322da5_e553d84d PS11, Line 161: /* Expect clean initialization of cbmem */ (same)
https://review.coreboot.org/c/coreboot/+/46458/comment/b5a4dfd4_a3df1d97 PS11, Line 172: recovers and creates its root entry */ nit: no reason not to use the full line length for comments...
https://review.coreboot.org/c/coreboot/+/46458/comment/7fcc38d5_8568f359 PS11, Line 460: assert_null(cbmem_entry_start(NULL)); I would say it's illegal to call this with NULL, you just get lucky that the implementation happens to not segfault on this. It certainly doesn't look intended. Let's leave this out. (Generally most of our APIs are not intended to support calling with NULL unless they explicitly say they are.)
https://review.coreboot.org/c/coreboot/+/46458/comment/e70939fa_74c9d191 PS11, Line 494: expect_function_call(bootmem_add_range); nit: if you're expecting arguments already I don't think you really need the explicit expect_function_call() as well, the arguments you're adding already imply how often the function will be called (and cmocka should check that no arguments are left on the expectation stack at the end of the test).
https://review.coreboot.org/c/coreboot/+/46458/comment/2abccd27_970b58af PS11, Line 543: memcpy(cbmem_buffer, get_cbmem_ptr(), CBMEM_SIZE); No, this isn't really what I meant. You've just comparing that two invocations of prepare_simple_cbmem() create the same memory layout now. What I meant was to hexdump how it looks after prepare_simple_cbmem() and then hardcode those numbers as literals in a raw uint8_t array in this file, and compare the contents of that with what prepare_simple_cbmem() produces. That way the test can detect whether future patches change anything about the CBMEM data format.