Attention is currently required from: Julius Werner. jacz@semihalf.com 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)
File tests/lib/imd_cbmem-test.c:
https://review.coreboot.org/c/coreboot/+/46458/comment/77375e4e_bd0401d0 PS9, Line 284:
...did you? Sorry, I don't see it...
Now it is done. I'm sorry, I marked it as done, and forgot to add it to patch.
File tests/lib/imd_cbmem-test.c:
https://review.coreboot.org/c/coreboot/+/46458/comment/ec79aeeb_04bd77aa 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 […]
Well, I should have thought about this. Thank you for bringing it up. It also made me rethink some test cases.
https://review.coreboot.org/c/coreboot/+/46458/comment/b0b2e0ce_25486a3a PS10, Line 96: // Check that cbmem has only root, large and small entry.
/* Single line comment style is this. […]
Done
https://review.coreboot.org/c/coreboot/+/46458/comment/b0c85a4f_32f7d104 PS10, Line 113: /* Check that cbmem has root, large, small entries
. […]
Done
https://review.coreboot.org/c/coreboot/+/46458/comment/e38cfc78_e14e7b09 PS10, Line 187: prepare_cbmem_for_recovery();
This file in general could use some more comments about what specifically you're trying to test. […]
More comments with description surely will come in handy for someone that sees this file for the first time (I could tell the same about files like imd_cbmem.c). I did not want to add too many comments, but I ended up not adding them at all in places, where they should be.
https://review.coreboot.org/c/coreboot/+/46458/comment/b62fc1be_7983f516 PS10, Line 194: clear_cbmem();
Why is this here? If anything, this should be part of the teardown, not the test itself.
Right. I moved it to test teardown.
https://review.coreboot.org/c/coreboot/+/46458/comment/18d40ba8_58c2bade PS10, Line 202:
It would be good to also call reset_imd() here to ensure the data is recovered from scratch only fro […]
I added reset_imd() and clear_cbmem() to prepare_cbmem_for_recovery(), so cbmem will always be in the same state after function was executed.
https://review.coreboot.org/c/coreboot/+/46458/comment/4702c039_5905ac52 PS10, Line 240: cbmem_entry_add(id2, CBMEM_ROOT_SIZE);
Why not test the return value of cbmem_entry_add() as well?
Good point. Comparing returned pointer of cbmem_entry_add() with one returned by cbmem_entry_find() might help spot errors related to their behavior.
https://review.coreboot.org/c/coreboot/+/46458/comment/dd22a51e_bec8b829 PS10, Line 254: assert_ptr_equal(entry2, entry3);
This one in particular would make more sense with return values. […]
Sure, this is not a point of this test case. As you mentioned, we want to test if cbmem_entry_add() returns the same entry when creating it again. So this will be changed to more appropriate form.
https://review.coreboot.org/c/coreboot/+/46458/comment/613264b0_111d59e6 PS10, Line 268: cbmem_add(id2, entry2_size);
(same here)
Done
https://review.coreboot.org/c/coreboot/+/46458/comment/bc6ae970_1e289812 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. […]
After looking at this code once again I see, that checking alignment of added entry address is not correct. Mentioned const/define will come in handy. Hardcoded integer looked out of place.
https://review.coreboot.org/c/coreboot/+/46458/comment/2de44d90_e4c27e71 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. […]
Sure, I should write tests using API, not module internal implementation. Thank you for the hint. While changing this test case I noticed that I did not check cbmem_entry_add with size of 0.
https://review.coreboot.org/c/coreboot/+/46458/comment/191a745f_bb6fb9f8 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(cbme […]
You are right. Non-NULL check is not enough to check if this function works correctly. I think that small and large entries also should be tested.
https://review.coreboot.org/c/coreboot/+/46458/comment/de69ad44_455bffac 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 hap […]
So, I should just check arguments and if function was called, right? I think there is no need to mock this function, because it would check if the wrapper function was called. And, there is no original function if it is not compiled, so it can be re-implemented.
https://review.coreboot.org/c/coreboot/+/46458/comment/a8d7af49_2466e89a PS10, Line 425: assert_int_equal(0, size);
This tests undefined behavior so it's probably not that useful or a good idea. […]
I see it right now. Testing undefined behavior is not right, because it might cause random errors. I will remove this for now.
https://review.coreboot.org/c/coreboot/+/46458/comment/955d8fd7_fdc1c51e 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 […]
I added multiple big (same size) entries, accumulated their size and, with overhead size and root entry size, compared with value returned by cbmem_get_region(). I tried to add small entries testing, but I do not know, how to calculate the maximum number of entries that fit in a small region. I tried to do this mathematically and using simple iteration. Do you see any way to do this? I think that a test like this would be helpful if someone tries to change, how entries are allocated.
https://review.coreboot.org/c/coreboot/+/46458/comment/592a71c3_e29a130f PS10, Line 437: reset_imd();
This should probably also call clear_cbmem() to ensure your tests are hermetic?
Yes, this should call clear_cbmem(). I do not know, why I missed something that important.