[coreboot] Notes on CBFS

[Kevin O'Connor]

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