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 11:
(7 comments)
File tests/lib/imd_cbmem-test.c:
https://review.coreboot.org/c/coreboot/+/46458/comment/f484a0e2_628cad8f PS10, Line 431: assert_int_not_equal(0, size);
The maximum amount of small entries should be ((CBMEM_SM_ROOT_SIZE - sizeof(struct imd_root_pointer) […]
Thanks for clarifying. I will add a few small entries and check overall size.
File tests/lib/imd_cbmem-test.c:
https://review.coreboot.org/c/coreboot/+/46458/comment/86262890_d534138b PS11, Line 144: /* Expect clean initialization of cbmem */
This comment confused me... […]
Sure, this comment is confusing. What happens is the imd is initialized but no root and small region is created, so cbmem is not in desired state and has to be initialized using cbmem_initialize_empty*(). Am I right?
https://review.coreboot.org/c/coreboot/+/46458/comment/62902c54_cdbd771e PS11, Line 161: /* Expect clean initialization of cbmem */
(same)
Done
https://review.coreboot.org/c/coreboot/+/46458/comment/93d3c518_6dae7f72 PS11, Line 172: recovers and creates its root entry */
nit: no reason not to use the full line length for comments...
Done
https://review.coreboot.org/c/coreboot/+/46458/comment/563e3047_1b080020 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 […]
So, I will remove this line. It does not segfault, because IMD does not find entry within its entries structure. But, this is tied to current code, not design assumptions and would create a wrong impression of that passing NULL is correct.
https://review.coreboot.org/c/coreboot/+/46458/comment/5e94b137_3fc98c57 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_functio […]
Yes, it should be fine without expect_function_call(). It is more useful with functions in which we do not check arguments.
https://review.coreboot.org/c/coreboot/+/46458/comment/7cc3990c_b5195b1d PS11, Line 543: memcpy(cbmem_buffer, get_cbmem_ptr(), CBMEM_SIZE);
No, this isn't really what I meant. […]
I do not think that storing 32+KiB of raw data in a test file is a good solution. I came up with two simple solutions. The first option I see is to create a new file for this data. The second is to set only non-zero values in the array at the end of a file and create an extern definition at top of the file. And the other issue is data description. Should It be divided by entries and described or just plain data with one comment will be enough?