Name of user not set #1003143 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44669 )
Change subject: tests: Add lib/imd-test test case ......................................................................
Patch Set 3:
(11 comments)
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c File tests/lib/imd-test.c:
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c@9 PS2, Line 9: #include <limits.h>
What do you need this for? I can't see anything that needs it but I may have missed something. […]
Done
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c@13 PS2, Line 13: void *memset(void *ptr, int value, size_t num);
For these kinds of cases I would suggest to centralize them rather than than defining them manually […]
Done
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c@38 PS2, Line 38: } __packed;
Let's not duplicate all of this stuff, let's just factor them out from the code under test into a se […]
Done
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c@43 PS2, Line 43: #define CBMEM_ID_IMD_SMALL 0x53a11439
Don't you get these from cbmem. […]
Done
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c@56 PS2, Line 56: #define CURSOR_SZ sizeof(struct imd_cursor)
Not sure why you have all of these rather than using sizeof() directly in the code? Not sure they re […]
Done
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c@69 PS2, Line 69: uint32_t align_up_pow2(uint32_t x)
You can just use (1 << log2_ceil(x)) from coreboot's <lib.h>.
Done
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c@89 PS2, Line 89: uintptr_t test_inputs[] = {0, 0xDEAA, 0xF0F0F0F0, ((1ULL << 32) + 4),
Consider adding comments for each of these as to why the values were chosen, e.g. […]
Done
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c@155 PS2, Line 155: /* Expect failure, since imd handle is not initialized */
"Expect imd_create_empty to fail, since ...
Done
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c@236 PS2, Line 236: sm_region_size = SM_ROOT_SIZE - RP_SZ - ROOT_SZ;
Can you explain what you're doing here?
Done
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c@299 PS2, Line 299: r = imd.lg.r; : assert_int_equal(2, r->num_entries);
I know it's more verbose, but […]
Done
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c@495 PS2, Line 495: /* Fail when root is locked. */
Consider saying "Cannot add an entry when root is locked" as that explains the specific thing that f […]
Done