Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44669 )
Change subject: tests: Add lib/imd-test test case ......................................................................
Patch Set 2:
(5 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@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. uintptr_t test_inputs[] = { 0, /* Lowest possible address */ 0xDEAA, /* Fits in 16 bits */ 0xF0F0F0F0, /* Fits in 32 bits */ ((1ULL << 32) + 4), /* Just above 32-bit limit */ ((1ULL << 60) - 100) /* Very large, but still fits in 64-bit address space */ };
as well as noting which ones will get rounded down by LIMIT_ALIGN. And you should have at least one case (other than 0) which does not get rounded down by LIMIT_ALIGN.
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 ...
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?
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 assert_int_equal(2, imd.lg.r->num_entries); would be a little more clear. By using r to point to imd.lg or imd.sm, you're introducing the possibility for error if people add or remove code.
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 fails.