| Submitter | Stefan Reinauer |
|---|---|
| Date | 2010-02-27 11:51:31 |
| Message ID | <4B890743.4000608@coresystems.de> |
| Download | mbox | patch |
| Permalink | /patch/985/ |
| State | Superseded |
| Headers | show |
Comments
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
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 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
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
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 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
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. > >
Patch
Index: src/include/cbfs.h =================================================================== --- src/include/cbfs.h (revision 5166) +++ src/include/cbfs.h (working copy) @@ -63,9 +63,13 @@ Users are welcome to use any other value for their components */ -#define CBFS_TYPE_STAGE 0x10 -#define CBFS_TYPE_PAYLOAD 0x20 -#define CBFS_TYPE_OPTIONROM 0x30 +#define CBFS_TYPE_STAGE 0x10 +#define CBFS_TYPE_PAYLOAD 0x20 +#define CBFS_TYPE_OPTIONROM 0x30 +#define CBFS_TYPE_BOOTSPLASH 0x40 +#define CBFS_TYPE_RAW 0x50 +#define CBFS_TYPE_VSA 0x51 +#define CBFS_TYPE_MBI 0x52 /** this is the master cbfs header - it need to be located somewhere in the bootblock. Where it @@ -164,11 +168,7 @@ void * cbfs_get_file(const char *name); void *cbfs_load_optionrom(u16 vendor, u16 device, void * dest); int run_address(void *f); -int cbfs_decompress(int algo, void *src, void *dst, int len); -struct cbfs_stage *cbfs_find_file(const char *name, int type); -int cbfs_check_magic(struct cbfs_file *file); -struct cbfs_header *cbfs_master_header(void); -struct cbfs_file *cbfs_find(const char *name); +void *cbfs_find_file(const char *name, int type); void cbfs_and_run_core(const char *filename, unsigned int ebp); #endif Index: src/lib/cbfs.c =================================================================== --- src/lib/cbfs.c (revision 5166) +++ src/lib/cbfs.c (working copy) @@ -24,7 +24,16 @@ #include <lib.h> #include <arch/byteorder.h> -int cbfs_decompress(int algo, void *src, void *dst, int len) + +/** + * Decompression wrapper for CBFS + * @param algo + * @param src + * @param dst + * @param len + * @return 0 on success, -1 on failure + */ +static int cbfs_decompress(int algo, void *src, void *dst, int len) { switch(algo) { case CBFS_COMPRESS_NONE: @@ -44,12 +53,12 @@ } } -int cbfs_check_magic(struct cbfs_file *file) +static int cbfs_check_magic(struct cbfs_file *file) { return !strcmp(file->magic, CBFS_FILE_MAGIC) ? 1 : 0; } -struct cbfs_header *cbfs_master_header(void) +static struct cbfs_header *cbfs_master_header(void) { struct cbfs_header *header; @@ -71,7 +80,7 @@ return header; } -struct cbfs_file *cbfs_find(const char *name) +static struct cbfs_file *cbfs_find(const char *name) { struct cbfs_header *header = cbfs_master_header(); unsigned long offset; @@ -103,7 +112,7 @@ } } -struct cbfs_stage *cbfs_find_file(const char *name, int type) +void *cbfs_find_file(const char *name, int type) { struct cbfs_file *file = cbfs_find(name); @@ -123,7 +132,7 @@ return (void *) CBFS_SUBHEADER(file); } -static int tohex4(unsigned int c) +static inline int tohex4(unsigned int c) { return (c<=9)?(c+'0'):(c-10+'a'); } @@ -205,11 +214,6 @@ return (void *) entry; } -void * cbfs_get_file(const char *name) -{ - return (void *) cbfs_find(name); -} - int cbfs_execute_stage(const char *name) { struct cbfs_stage *stage = (struct cbfs_stage *) @@ -233,7 +237,7 @@ * run_address is passed the address of a function taking no parameters and * jumps to it, returning the result. * @param f the address to call as a function. - * returns value returned by the function. + * @return value returned by the function. */ int run_address(void *f) Index: util/x86emu/yabel/vbe.c =================================================================== --- util/x86emu/yabel/vbe.c (revision 5166) +++ util/x86emu/yabel/vbe.c (working copy) @@ -795,12 +795,11 @@ * cares. */ int imagesize = 1024*768*2; - struct cbfs_file *file = cbfs_find("bootsplash.jpg"); - if (!file) { + unsigned char *jpeg = cbfs_find_file("bootsplash.jpg", CBFS_TYPE_BOOTSPLASH); + if (!jpeg) { DEBUG_PRINTF_VBE("Could not find bootsplash.jpg\n"); return; } - unsigned char *jpeg = ((unsigned char *)file) + ntohl(file->offset); DEBUG_PRINTF_VBE("Splash at %08x ...\n", jpeg); dump(jpeg, 64); Index: util/cbfstool/cbfs.h =================================================================== --- util/cbfstool/cbfs.h (revision 5166) +++ util/cbfstool/cbfs.h (working copy) @@ -68,9 +68,13 @@ Users are welcome to use any other value for their components */ -#define CBFS_COMPONENT_STAGE 0x10 -#define CBFS_COMPONENT_PAYLOAD 0x20 -#define CBFS_COMPONENT_OPTIONROM 0x30 +#define CBFS_COMPONENT_STAGE 0x10 +#define CBFS_COMPONENT_PAYLOAD 0x20 +#define CBFS_COMPONENT_OPTIONROM 0x30 +#define CBFS_COMPONENT_BOOTSPLASH 0x40 +#define CBFS_COMPONENT_RAW 0x50 +#define CBFS_COMPONENT_VSA 0x51 +#define CBFS_COMPONENT_MBI 0x52 /* The deleted type is chosen to be a value * that can be written in a FLASH from all other Index: util/cbfstool/common.c =================================================================== --- util/cbfstool/common.c (revision 5166) +++ util/cbfstool/common.c (working copy) @@ -128,6 +128,10 @@ {CBFS_COMPONENT_STAGE, "stage"}, {CBFS_COMPONENT_PAYLOAD, "payload"}, {CBFS_COMPONENT_OPTIONROM, "optionrom"}, + {CBFS_COMPONENT_BOOTSPLASH, "bootsplash"}, + {CBFS_COMPONENT_RAW, "raw"}, + {CBFS_COMPONENT_VSA, "vsa"}, + {CBFS_COMPONENT_MBI, "mbi"}, {CBFS_COMPONENT_DELETED, "deleted"}, {CBFS_COMPONENT_NULL, "null"} };
See patch - make some functions static because they're only used within cbfs.c - add new types for bootsplash, RAW, VSA, MBI - drop unused cbfs_get_file() Signed-off-by: Stefan Reinauer <stepan@coresystems.de>