Attention is currently required from: Julius Werner, Jan Dabros. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56813 )
Change subject: tests: Add lib/cbfs-lookup-test test case ......................................................................
Patch Set 5:
(13 comments)
File tests/lib/cbfs-lookup-test.c:
https://review.coreboot.org/c/coreboot/+/56813/comment/9e5af27d_66192792 PS4, Line 12: #define HEADER_INITIALIZER(ftype, attr_len, file_len) { \
If you're reusing components of this CBFS mocking stuff in multiple tests, please factor them out in […]
Ok. Now it is in previous patch (libcbfs-verification-test).
https://review.coreboot.org/c/coreboot/+/56813/comment/5fdde253_e20c1e42 PS4, Line 115: BE32(CBFS_FILE_ATTR_TAG_UNUSED),
What's this? This isn't valid. […]
So what about CBFS_FILE_ATTR_TAG_UNUSED and CBFS_FILE_ATTR_TAG_UNUSED2? They are here only for cbfstool?
https://review.coreboot.org/c/coreboot/+/56813/comment/e26fe911_d71d14e0 PS4, Line 139: }, Wow, that's a lot of cases. Thank you.
doesn't all need to be in the same CL
You know, how it is usually - "I'll do it later" - and it is postponed indefinitely.
- CBFS file header at a non-CBFS_ALIGNMENT boundary
Now all tests are executed for aligned and unaligned buffer (in terms of address alignment, not CBFS_ALIGNMENT)
- CBFS files where either len, attributes_offset or offset are (uint32_t)-1 or something like that
I think, that these cases are covered by those:
- CBFS file where the data length goes beyond the end of the rdev
- CBFS file where attributes_offset is larger than offset
- CBFS file where attributes_offset is smaller than sizeof(struct cbfs_file)
But I aded one case anyway - for .len = cpu_to_be32((uint32_t)-1);
If you see that I missed/didn't understand any of listed cases, then please, tell me.
https://review.coreboot.org/c/coreboot/+/56813/comment/0d535063_0f05c8c3 PS4, Line 148: __aligned(CBFS_ALIGNMENT);
nit: technically, I don't see why this should need to be aligned? The only access function is cbfs_d […]
Done. However, I see that CBFS "aligns" files by CBFS_ALIGNMENT, but does not use ALIGN_UP/DOWN. Is it intended? Won't it cause problems in the future?
https://review.coreboot.org/c/coreboot/+/56813/comment/ab906777_73047b77 PS4, Line 153: __weak extern u8 _ecbfs_cache[];
Why not just #include <symbols. […]
Done
https://review.coreboot.org/c/coreboot/+/56813/comment/134d828c_da07f741 PS4, Line 154: TEST_REGION(cbfs_cache, TEST_CBFS_CACHE_SIZE);
If you're planning to link mem_pool and have a real cbfs_cache in here, I'd suggest doing the pass-t […]
Done
https://review.coreboot.org/c/coreboot/+/56813/comment/13b1765c_289b9860 PS4, Line 159: int create_cbfs(const struct cbfs_test_file **files, size_t nfiles)
One of the lesser-tested properties of CBFS is that there may be an arbitrary amount of space behind […]
Done in test_cbfs_map() and test_cbfs_cbmem_alloc()
https://review.coreboot.org/c/coreboot/+/56813/comment/d751c247_1e93630e PS4, Line 167: if (&data_ptr[file_size] > &cbfs_buffer[ARRAY_SIZE(cbfs_buffer) - 1])
nit: this is checking your test data, not the code under test, so should probably just assert()?
Until it is used inside test code, not in any other place (like setup/teardown functions) it will be ok.
https://review.coreboot.org/c/coreboot/+/56813/comment/332b33dd_46deaf9b PS4, Line 173: if ((uintptr_t)data_ptr % CBFS_ALIGNMENT != 0)
This would of course need to check for the alignment of (data_ptr - cbfs_buffer), rather than data_p […]
Done
https://review.coreboot.org/c/coreboot/+/56813/comment/a221927a_01ba3e6e PS4, Line 189: function_called();
nit: I think technically you don't really need function_called() when you already have a check_expec […]
True. Checking arguments is enough here. For now.
https://review.coreboot.org/c/coreboot/+/56813/comment/3c9cb918_e9e6bdf0 PS4, Line 240: cbd.mcache_size = TEST_MCACHE_SIZE;
This isn't actually testing the mcache? If you want to test with mcache, you have to cbfs_mcache_bui […]
Each test calls cbfs_init_boot_device() after constructing CBFS. cbfs_init_boot_device() calls cbfs_mcache_build() if required. then, mocks are configured depending on presence or absence of MCache.
https://review.coreboot.org/c/coreboot/+/56813/comment/b4a657b1_cfdb7565 PS4, Line 253: extern uintptr_t _cbmem_top_ptr;
I'd honestly consider just mocking out the whole CBMEM part here, its internals are completely isola […]
Removing CBMEM implementation from this test is a good idea. Checking id and size should suffice.
https://review.coreboot.org/c/coreboot/+/56813/comment/8566a92c_179215e1 PS4, Line 308: size_out
Please also test cbfs_map("filename", NULL) somewhere.
Done in: - test_cbfs_map() -> TEST_DATA_INT_2 - test_cbfs_cbmem_alloc() -> TEST_DATA_2