Hi guys
I was playing with CBFS and I found an strange behavior. There are actually 4 types of files in a CBFS rom : stage, payload, option rom and NULL. The latest is used when we don't care of the component type.
I guess if we want to embed a logo, payload specific data or other stuffs, we must use this NULL type. The problem is that the free space in the rom also has a NULL type. In the fs.c file, rom_alloc function, the code searches for a cbfs_file with type == CBFS_COMPONENT_NULL, and if it is large enough, it will store the file in it. Otherwise, it continues until finding a large enough NULL type file.
If you want to add more than one NULL type file, it will work as long as you insert them from the smallest to the largest. Otherwise the NULL type file you are adding will overwrite an existing one.
Example to reproduce the behavior : - create a dummy bootblock dd if=/dev/urandom of=bootblock.rom bs=64k count=1 - create a test cbfs archive ./cbfstool test.rom create 524288 65536 ./bootblock.rom - create 2 dummy files (64 and 128 KB) dd if=/dev/urandom of=dummy64K bs=64k count=1 dd if=/dev/urandom of=dummy128K bs=64k count=2
Now, if I add my files from the smallest to the largest :
./cbfstool test.rom add dummy64K dummy64K free ./cbfstool test.rom add dummy128K dummy128K free ./cbfstool test.rom print test.rom: 512 kB, bootblocksize 65536, romsize 524288, offset 0x0 Alignment: 16 bytes
Name Offset Type Size dummy64K 0x0 free 65536 dummy128K 0x10030 free 131072 0x30060 free 261976
Everything works fine. But if I had the 128K file first, then the 64K file :
./cbfstool test.rom add dummy128K dummy128K free ./cbfstool test.rom add dummy64K dummy64K free ./cbfstool test.rom print test.rom: 512 kB, bootblocksize 65536, romsize 524288, offset 0x0 Alignment: 16 bytes
Name Offset Type Size dummy64K 0x0 free 65536 0x10030 free 65496 0x20030 free 327560
I think it is required to make the distinction between a user or custom type for embedded data, and the free space.
What do you think ?
Regards, Thomas
Bonus question : shouldn't the cbfs code (to parse, find a file...) be added to the libpayload ?
On Sun, 09 Aug 2009 18:55:38 +0200, Thomas Jourdan tjourdan@neuf.fr wrote:
I was playing with CBFS and I found an strange behavior. There are actually 4 types of files in a CBFS rom : stage, payload, option rom and NULL. The latest is used when we don't care of the component type.
No, the latest is used for free memory regions. You can define 256 different types, of which 4 are predefined, and there is some rough partitioning of the file type space - that you can honor, or not.
I guess if we want to embed a logo, payload specific data or other stuffs, we must use this NULL type. The problem is that the free space
As said, NULL is not suitable for that. So your conclusions are wrong.
What do you think ?
Define whatever types you want, expect problems as the numbers might be reused at some point. CBFS isn't finished yet, and it isn't even clean enough at this point to define stable contracts about it (eg. file types beyond what we already have)
Bonus question : shouldn't the cbfs code (to parse, find a file...) be added to the libpayload ?
Yes, at some point. Properly licensed, and properly designed, once CBFS itself is stable enough. There are only 24 hours in a day (that also applies to CBFS development).
CBFS is already an improvement over the old layout in its current state. You're moving beyond what CBFS can provide right now if you put any data into it that isn't supported by the coreboot build system. Future proof designs and implementations welcome.
Patrick
Definitely don't use NULL for anything -- it's a free space marker as patrick points out.
The type is actually unsigned int type;
I would make the following proposal: we reserve the range 0-255 for cbfs "system" (i.e. coreboot) use and open up the rest of the range for any other use. Then we allow people to register types on the wiki in some way.
I can't believe coreboot will ever need more than 256 types, and I can't believe the community will ever need more than 16M.
ron
Hi guys
Thanks for those clarifications. When I read the cbfs documentation, I understood that for custom types, NULL was the correct type to use. The documentation isn't really clear on this :
"There is a 4th component type ,defined as NULL (0xFFFFFFFF). This is the "don't care" component type. This can be used when the component type is not necessary (such as when the name of the component is unique. i.e. option_table). It is recommended that all components be assigned a unique type, but NULL can be used when the type does not matter."
I thought that the type parameter you enter when adding a component in the cbfs file shall be one of the predefined string. I didn't know that it could be any integer number (except the predefined ones).
Indeed, it works a lot better when I use a custom type.
Regards, Thomas
Le dimanche 09 août 2009 à 11:16 -0700, ron minnich a écrit :
Definitely don't use NULL for anything -- it's a free space marker as patrick points out.
The type is actually unsigned int type;
I would make the following proposal: we reserve the range 0-255 for cbfs "system" (i.e. coreboot) use and open up the rest of the range for any other use. Then we allow people to register types on the wiki in some way.
I can't believe coreboot will ever need more than 256 types, and I can't believe the community will ever need more than 16M.
ron
On Sun, Aug 9, 2009 at 3:02 PM, Thomas Jourdantjourdan@neuf.fr wrote:
"There is a 4th component type ,defined as NULL (0xFFFFFFFF). This is the "don't care" component type. This can be used when the component type is not necessary (such as when the name of the component is unique. i.e. option_table). It is recommended that all components be assigned a unique type, but NULL can be used when the type does not matter."
Nice catch! My apologies -- turns out we needed NULL for empty space (this really cleaned things up) and this part of the doco is obsolete. I'll take a patch to fix it.
ron
On Sun, Aug 09, 2009 at 11:16:49AM -0700, ron minnich wrote:
Definitely don't use NULL for anything -- it's a free space marker as patrick points out.
The type is actually unsigned int type;
I would make the following proposal: we reserve the range 0-255 for cbfs "system" (i.e. coreboot) use and open up the rest of the range for any other use. Then we allow people to register types on the wiki in some way.
I can't believe coreboot will ever need more than 256 types, and I can't believe the community will ever need more than 16M.
I believe the community doesn't need any types. I fear we're creating work that doesn't need to be done. (Why maintain a wiki and/or build a standard around a feature that isn't needed?)
Of the four types defined:
null - can be implemented by setting the first character of the filename to 0 on deleted files. This is actually easier for file finding code because then it can just do a filename search and not have to worry about matching deleted files.
stage - instead of having a type in the cbfs header, a signature can be added to cbfs_stage. The same validity check can then be done there.
payload - same as 'stage' - add signature to cbfs_payload.
optionrom - isn't actually in use. Both SeaBIOS and coreboot expect to find option roms without any special header.
-Kevin
-
Kevin O'Connor -
Patrick Georgi -
ron minnich -
Thomas Jourdan