Hi,
I just added a bare-bones implementation of CBFS to SeaBIOS. I thought I would relay my thoughts:
I really feel CBFS' big-endian numbers should be changed to little-endian. Coreboot's primary market is X86, and forcing all CBFS readers to do ntohl() is a pain. Those calls to ntohl() cause terrible code generation (and this can hurt in the limited code size of SeaBIOS). I understand the desire to make the format well-defined on all platforms, but I think little-endian makes much more sense (eg, rename the ntohl() calls to le32_to_cpu() ).
I look forward to seeing the name "romfs" officially replaced with something else (eg, CBFS). SeaBIOS doesn't require coreboot - it works well with emulators like Linux KVM. I'm reluctant to put references to "romfs" in SeaBIOS, because it could lead to confusion with other users not familiar with coreboot.
It appears that romtool is not properly padding filenames - it looks like it is padding the names to 16 bytes. However, because the file names start at an 8 byte offset, this results in the file contents all starting 8 bytes in from 16 byte alignment. The file contents thus aren't 16 byte aligned.
The current romfs_find() function appears to be checking every 16 bytes for a file - it should be jumping past the file contents of previous files it finds.
Some thoughts on improving the CBFS format:
As I stated in an earlier email, I think the type info should be removed from the file header. Lets move the type signature into the payload structures (eg, struct romfs_stage, romfs_payload). Putting the signature there I think is safer (no chance a user mistypes a romtool command) and simpler.
I think the 'offset' field in 'struct romfs_file' should be replaced with a file name length field. The offset can then be constructed by calling ALIGN(file->namelength, header->align). The gain of doing this is that it is then possible to optimize file searches. The majority of searches are done for an exact filename - the scanner could skip the file name string compares of all files that have different file name length. This can reduce the amount of flash read accesses which can speed things up.
It should be possible to reduce the size of 'struct romfs_file' from 24 bytes to 16 bytes. This can be done by removing 'type', shrinking 'offset' (or filename length as above) to two bytes, and stealing 2 bytes from either 'signature' or 'checksum'. Shrinking the header to 16 bytes will reduce flash read accesses during file scans.
Finally, I think it would be simpler if 'struct romfs_stage', 'struct romfs_payload', and 'struct romfs_optionrom' were merged. Just add a 'u64 entry' and 'u32 segment_count' to 'struct romfs_payload'. Then a 'struct romfs_stage' would just be a payload with only one uncompressed segment. A 'struct optionrom' would also be a payload with one segment (optionally compressed). This will consume a few extra bytes of flash for option roms and "stages", but the code shrink for the consumers will more than make up for it (the scanning code wont have to test and branch for the different types).
-Kevin