See patch
Stefan Reinauer wrote:
- make some functions static because they're only used within cbfs.c
- drop unused cbfs_get_file()
Great. These hunks are
Acked-by: Peter Stuge peter@stuge.se
- add new types for bootsplash, RAW, VSA, MBI
Um..? Oh, ok, they aren't all bootsplash types. But, I think there should be just one meaningful parameter for each file. My point is that a bootsplash type is redundant, since a specific filename must already be used for bootsplashes. The same is true for VSA.
What is MBI?
How could this be handled better? Maybe we should change things around so that the filename is insignificant, and make the type more important? That makes user error much less obvious however, the filename concept is already well known by everyone.
Do we need so specific types at all?
//Peter
On 2/27/10 2:57 PM, Peter Stuge wrote:
Um..? Oh, ok, they aren't all bootsplash types. But, I think there should be just one meaningful parameter for each file. My point is that a bootsplash type is redundant, since a specific filename must already be used for bootsplashes. The same is true for VSA.
Since we only do name based matching in coreboot anyways, do you suggest we drop the type field?
I figured, the names might be changed or varied in the future, while the type is an additional consistency check. Also it makes sense to specify this so cbfstool might be able to use reasonable defaults for compression or placement of the files in the future. (Like, bootsplash jpg files should never be lzma compressed)
What is MBI?
Some Intel add-on for SMI/oprom communication.
How could this be handled better? Maybe we should change things around so that the filename is insignificant, and make the type more important? That makes user error much less obvious however, the filename concept is already well known by everyone.
Also, I think it should be possible to add files to the cbfs that we don't have to know how to parse. Like encryption public keys, or license files for oproms, or, ...
Do we need so specific types at all?
Well we have them, and I think it's better to use them unless/until we decide to drop them. Payloads may or may not make use of them.
I even forgot a type: cpu microcode.
Am 27.02.2010 15:41, schrieb Stefan Reinauer:
On 2/27/10 2:57 PM, Peter Stuge wrote:
Um..? Oh, ok, they aren't all bootsplash types. But, I think there should be just one meaningful parameter for each file. My point is that a bootsplash type is redundant, since a specific filename must already be used for bootsplashes. The same is true for VSA.
Since we only do name based matching in coreboot anyways, do you suggest we drop the type field?
I intend to add a header for option roms "real soon now", which contains compression type and vendor/device ids. That way, the filename can be the official filename (with version number and other information, etc) by the vendor, if available.
The remaining question is how we handle multiple boot paths (fallback/normal, for example) then. Still look for prefixes?
I even forgot a type: cpu microcode.
Sure would be nice to have the microcode uploader just fetch it from CBFS, instead of compiling it in.
Patrick
On 2/27/10 3:48 PM, Patrick Georgi wrote:
The remaining question is how we handle multiple boot paths (fallback/normal, for example) then. Still look for prefixes?
Do we need sub images?
Sure would be nice to have the microcode uploader just fetch it from CBFS, instead of compiling it in.
It would certainly make some people (including me) sleep better.
Stefan
new version
Am 28.02.2010 01:51, schrieb Stefan Reinauer:
new version
I'm not sure what to do with the CBFS type numbers, but I guess we still have them, so we can as well put them to use. This doesn't add any headers or anything similarily controversial, so it won't be in the way should type numbers be removed at some point.
Acked-by: Patrick Georgi patrick.georgi@coresystems.de
On Sat, Feb 27, 2010 at 03:48:57PM +0100, Patrick Georgi wrote:
I intend to add a header for option roms "real soon now", which contains compression type and vendor/device ids. That way, the filename can be the official filename (with version number and other information, etc) by the vendor, if available.
I fear this would be a step backwards. Yes, one would be able to store the vendor's filename, but then users would lose the ability to easily know which devices that rom ran on.
The remaining question is how we handle multiple boot paths (fallback/normal, for example) then. Still look for prefixes?
I'd prefer to use prefixes. Using "directories" is a well understood concept by users.
On Sun, Feb 28, 2010 at 08:33:02AM +0100, Patrick Georgi wrote:
Am 28.02.2010 03:04, schrieb Carl-Daniel Hailfinger:
compatible. Your patch might be backwards compatible, but some of the proposed extensions (option ROM naming and separate PCI ID storage) are not.
The only other user of option roms (SeaBIOS) uses a different type number to prevent issues once the optionrom subheader appears (which is more or less announced in the source). It's also doing its _own_ thing to handle compression (namely, more filename magic).
The filename magic for compression in SeaBIOS is 9 lines of code - 3 lines in cbfs_finddatafile and the 6 line function cbfs_iscomp:
http://git.linuxtogo.org/?p=kevin/seabios.git;a=blob;f=src/coreboot.c;h=9887...
I think adding a ".lzma" externsion to an lzma compressed file is intuitive to users.
So there's no risk in changing the format used with the "official" type number, and there's the chance that SeaBIOS picks up the change, so we're able to work with the same set of images.
-Kevin
Stefan Reinauer wrote:
Since we only do name based matching in coreboot anyways, do you suggest we drop the type field?
Well, yes, I think I am..
I know there are cases when it's handy to inspect the type, but unless the type is the _only_ thing that matters it isn't so intuitive to have one at all.
What do you think?
//Peter
Patrick Georgi wrote:
I intend to add a header for option roms "real soon now",
Peter Stuge wrote:
I know there are cases when it's handy to inspect the type, but unless the type is the _only_ thing that matters it isn't so intuitive to have one at all.
So maybe we should restrict CBFS types to a few that actually matters for consumers and not use CBFS types to describe the purpose of the file?
Ie. vsa, bootsplash and microcode are all blobs, while payloads may not be if they are stored with each segment as an individual file.
//Peter
Jordan, what do you think? Would it make sense to drop either name or type from CBFS? I am hesitating, but maybe you have some reasons to definitely keep it?
On 2/27/10 3:51 PM, Peter Stuge wrote:
Stefan Reinauer wrote:
Since we only do name based matching in coreboot anyways, do you suggest we drop the type field?
Well, yes, I think I am..
I know there are cases when it's handy to inspect the type, but unless the type is the _only_ thing that matters it isn't so intuitive to have one at all.
What do you think?
* Payloads may want to optimize their walking using the type. * in case of some file types it may be interesting to load all of a type from cbfs (ie. public crypto keys) * I think Kevin might not like that idea. He's using the type in SeaBIOS. * Maybe SeaBIOS can be changed? Who will do that? * Maybe we should keep the type on the cbfstool command line so we can keep the additional error checking it allows us, but keep it out of the coreboot file format.
So I think we should keep it for now and keep the possibility to drop it in mind.
On 27.02.2010 16:09, Stefan Reinauer wrote:
On 2/27/10 3:51 PM, Peter Stuge wrote:
Stefan Reinauer wrote:
Since we only do name based matching in coreboot anyways, do you suggest we drop the type field?
Well, yes, I think I am..
I know there are cases when it's handy to inspect the type, but unless the type is the _only_ thing that matters it isn't so intuitive to have one at all.
What do you think?
- I think Kevin might not like that idea. He's using the type in SeaBIOS.
- Maybe SeaBIOS can be changed? Who will do that?
[...] So I think we should keep it for now and keep the possibility to drop it in mind.
IMHO the time to change anything in CBFS is over. It is too widely used to change the in-ROM format in a way that is not 100% backwards compatible. Your patch might be backwards compatible, but some of the proposed extensions (option ROM naming and separate PCI ID storage) are not.
OTOH, if we change the in-ROM format, we might as well fix the design shortcomings I mentioned back in the LAR+SELF debate. AFAIK modern CBFS still is a stripped down LAR+SELF.
My sincere apologies if I missed some important development or misunderstood the proposed changes.
Regards, Carl-Daniel
Just to clarify: My comments are not intended to downplay the work that has been done on CBFS. v2/v4 is a lot more developer/user friendly since CBFS was merged.
Regards, Carl-Daniel
On 2/28/10 3:04 AM, Carl-Daniel Hailfinger wrote:
IMHO the time to change anything in CBFS is over. It is too widely used to change the in-ROM format in a way that is not 100% backwards compatible. Your patch might be backwards compatible, but some of the proposed extensions (option ROM naming and separate PCI ID storage) are not.
There is no way to do partly flash updates of CBFS _or_ LAR formatted coreboot images, so how widely is used just does not really matter. At this time a flash update always updates the complete coreboot image. Until that changes, we don't break anything.
OTOH, if we change the in-ROM format, we might as well fix the design shortcomings I mentioned back in the LAR+SELF debate. AFAIK modern CBFS still is a stripped down LAR+SELF.
What's missing in your opinion?
Stefan
On 28.02.2010 03:23, Stefan Reinauer wrote:
On 2/28/10 3:04 AM, Carl-Daniel Hailfinger wrote:
IMHO the time to change anything in CBFS is over. It is too widely used to change the in-ROM format in a way that is not 100% backwards compatible. Your patch might be backwards compatible, but some of the proposed extensions (option ROM naming and separate PCI ID storage) are not.
There is no way to do partly flash updates of CBFS _or_ LAR formatted coreboot images, so how widely is used just does not really matter. At this time a flash update always updates the complete coreboot image. Until that changes, we don't break anything.
Actually, partial flash updates work just fine with LAR and if someone is interested, I'll gladly demonstrate this.
OTOH, if we change the in-ROM format, we might as well fix the design shortcomings I mentioned back in the LAR+SELF debate. AFAIK modern CBFS still is a stripped down LAR+SELF.
What's missing in your opinion?
I didn't know that CBFS doesn't support partial flash updates. Let's just add that to the list of things I'd like to change. Back then I wrote up a detailed review of LAR+SELF/CBFS, and it may even have been in the wiki, but I couldn't find it during a quick search right now. Anyway, I do not want to limit progress in any way, so I'll wait how this develops, and will probably send a patch for LAR2 in the coming months.
Regards, Carl-Daniel
Am 28.02.2010 03:04, schrieb Carl-Daniel Hailfinger:
compatible. Your patch might be backwards compatible, but some of the proposed extensions (option ROM naming and separate PCI ID storage) are not.
The only other user of option roms (SeaBIOS) uses a different type number to prevent issues once the optionrom subheader appears (which is more or less announced in the source). It's also doing its _own_ thing to handle compression (namely, more filename magic).
So there's no risk in changing the format used with the "official" type number, and there's the chance that SeaBIOS picks up the change, so we're able to work with the same set of images.
Patrick
On Sat, Feb 27, 2010 at 04:09:50PM +0100, Stefan Reinauer wrote:
Jordan, what do you think? Would it make sense to drop either name or type from CBFS? I am hesitating, but maybe you have some reasons to definitely keep it?
On 2/27/10 3:51 PM, Peter Stuge wrote:
Stefan Reinauer wrote:
Since we only do name based matching in coreboot anyways, do you suggest we drop the type field?
Well, yes, I think I am..
I know there are cases when it's handy to inspect the type, but unless the type is the _only_ thing that matters it isn't so intuitive to have one at all.
What do you think?
- Payloads may want to optimize their walking using the type.
- in case of some file types it may be interesting to load all of a type
from cbfs (ie. public crypto keys)
- I think Kevin might not like that idea. He's using the type in SeaBIOS.
I would like to see the type field dropped from CBFS. I think storing a type is unintuitive as filenames are both more powerful and better understood. As Peter mentions, the filename is already the determining factor to loading a rom.
- Maybe SeaBIOS can be changed? Who will do that?
SeaBIOS doesn't look at the type field. There is no reason to.
-Kevin
Stefan Reinauer wrote:
Jordan, what do you think? Would it make sense to drop either name or type from CBFS? I am hesitating, but maybe you have some reasons to definitely keep it?
I feel silly speaking like I'm a guru that somebody climbed a mountain to consult with, but here we go.
I think it is okay to just use name matching. My intent with the type was to have an integer that could be quickly parsed in a ROM for Bayou - "Give me all the payloads" or some such. I realize that isn't as flexible as the names, so if everybody can agree on standard extensions and the extra processing time to parse the string, then carry on.
Please leave a banana in the bowl on your way out.
Jordan
On 2/27/10 3:51 PM, Peter Stuge wrote:
Stefan Reinauer wrote:
Since we only do name based matching in coreboot anyways, do you suggest we drop the type field?
Well, yes, I think I am..
I know there are cases when it's handy to inspect the type, but unless the type is the _only_ thing that matters it isn't so intuitive to have one at all.
What do you think?
- Payloads may want to optimize their walking using the type.
- in case of some file types it may be interesting to load all of a type
from cbfs (ie. public crypto keys)
- I think Kevin might not like that idea. He's using the type in SeaBIOS.
- Maybe SeaBIOS can be changed? Who will do that?
- Maybe we should keep the type on the cbfstool command line so we can
keep the additional error checking it allows us, but keep it out of the coreboot file format.
So I think we should keep it for now and keep the possibility to drop it in mind.
-
Carl-Daniel Hailfinger -
Jordan Crouse -
Kevin O'Connor -
Patrick Georgi -
Patrick Georgi -
Peter Stuge -
Stefan Reinauer