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 10:
(17 comments)
https://review.coreboot.org/c/coreboot/+/46458/9/tests/lib/imd_cbmem-test.c File tests/lib/imd_cbmem-test.c:
https://review.coreboot.org/c/coreboot/+/46458/9/tests/lib/imd_cbmem-test.c@... PS9, Line 284:
Done
...did you? Sorry, I don't see it...
https://review.coreboot.org/c/coreboot/+/46458/10/tests/lib/imd_cbmem-test.c File tests/lib/imd_cbmem-test.c:
https://review.coreboot.org/c/coreboot/+/46458/10/tests/lib/imd_cbmem-test.c... PS10, Line 48: void cbmem_run_init_hooks(int is_recovery) Might be nice to actually mock this function and use expect calls to ensure it is run when it should.
https://review.coreboot.org/c/coreboot/+/46458/10/tests/lib/imd_cbmem-test.c... PS10, Line 96: // Check that cbmem has only root, large and small entry. /* Single line comment style is this. */
https://review.coreboot.org/c/coreboot/+/46458/10/tests/lib/imd_cbmem-test.c... PS10, Line 113: /* Check that cbmem has root, large, small entries .
/* Multi-line comment style is this for short comments (note lack of asterisk on continuation line) */
/* * Or this for * longer comments * with more lines. */
https://review.coreboot.org/c/coreboot/+/46458/10/tests/lib/imd_cbmem-test.c... PS10, Line 187: prepare_cbmem_for_recovery(); This file in general could use some more comments about what specifically you're trying to test. For example, what does this have to do with testing cbmem_initialize_id_size()? I guess you're trying to test that the lockdown flag is cleared if cbmem_initialize_empty() is called again? That's okay if you want to do that, but it should really be explained somewhere because it's not very obvious what's going on here. (Basically, imagine you're writing this file for the person who's patch triggers a test error on one of these lines in the future, and they have to figure out which assumption their test broke without ever having seen this file before.)
https://review.coreboot.org/c/coreboot/+/46458/10/tests/lib/imd_cbmem-test.c... PS10, Line 194: clear_cbmem(); Why is this here? If anything, this should be part of the teardown, not the test itself.
https://review.coreboot.org/c/coreboot/+/46458/10/tests/lib/imd_cbmem-test.c... PS10, Line 202: It would be good to also call reset_imd() here to ensure the data is recovered from scratch only from the CBMEM memory area, and not the imd global variables (which wouldn't survive an actual suspend/resume or stage transition).
https://review.coreboot.org/c/coreboot/+/46458/10/tests/lib/imd_cbmem-test.c... PS10, Line 240: cbmem_entry_add(id2, CBMEM_ROOT_SIZE); Why not test the return value of cbmem_entry_add() as well?
https://review.coreboot.org/c/coreboot/+/46458/10/tests/lib/imd_cbmem-test.c... PS10, Line 254: assert_ptr_equal(entry2, entry3); This one in particular would make more sense with return values. Right now you're just testing that calling cbmem_entry_find() with the same parameter twice returns the same result.
https://review.coreboot.org/c/coreboot/+/46458/10/tests/lib/imd_cbmem-test.c... PS10, Line 268: cbmem_add(id2, entry2_size); (same here)
https://review.coreboot.org/c/coreboot/+/46458/10/tests/lib/imd_cbmem-test.c... PS10, Line 305: assert_int_equal(0, (uintptr_t)cbm_e2 % 4); The minimum CBMEM alignment is CBMEM_SM_ALIGN (i.e. 32), not 4. Note that the alignment applies to the allocated memory area itself, not the cbmem_entry structure. So you'd want to check the alignment of cbmem_entry_start(cbm_e2), not cbm_e2 itself.
https://review.coreboot.org/c/coreboot/+/46458/10/tests/lib/imd_cbmem-test.c... PS10, Line 360: assert_int_equal(0, cbmem_entry_size(cbm_e)); This is just testing deep inner implementation details that are intentionally hidden from the API. I think the more interesting test is that after you do cbmem_add(id, size), cbmem_entry_size(cbmem_entry_find(id)) will return size (especially when size is something not divisible by 4).
https://review.coreboot.org/c/coreboot/+/46458/10/tests/lib/imd_cbmem-test.c... PS10, Line 371: assert_non_null(cbmem_entry_start(cbm_e)); I think a good test here would be to check that cbmem_find(id) is the same as cbmem_entry_start(cbmem_entry_find(id)).
https://review.coreboot.org/c/coreboot/+/46458/10/tests/lib/imd_cbmem-test.c... PS10, Line 385: __real_bootmem_add_range(start, size, tag); Why are you calling this? This is a throwaway function for you, your test never cares about what happens to this again (just that it was called). So you shouldn't actually call the underlying, and shouldn't compile in bootmem.c (or memrange.c) for this test.
https://review.coreboot.org/c/coreboot/+/46458/10/tests/lib/imd_cbmem-test.c... PS10, Line 425: assert_int_equal(0, size); This tests undefined behavior so it's probably not that useful or a good idea. cbmem_get_region() should never be called before CBMEM is initialized. (In fact, might be worth adding an assert() to the coreboot code there to make sure imd_region_used() returned 0. Or it should explicitly catch that and cleanly return NULL/0.)
https://review.coreboot.org/c/coreboot/+/46458/10/tests/lib/imd_cbmem-test.c... PS10, Line 431: assert_int_not_equal(0, size); This is a somewhat boring test, can we make it more interesting? For base_ptr I *think* in practice it should always be exactly _cbmem_top_ptr - size, right? For size, it would be nice if we can test that it "roughly" matches the total amount of memory needed for our allocations without hardcoding too many implementation details. Maybe keep a running counter of total added size of entries while you add a couple of big ones, and then check after each one that the returned size from this is between your running total and the total plus cbmem_overhead_size()?
https://review.coreboot.org/c/coreboot/+/46458/10/tests/lib/imd_cbmem-test.c... PS10, Line 437: reset_imd(); This should probably also call clear_cbmem() to ensure your tests are hermetic?