Hi,
currently the option table (which contains the metadata necessary to parse CMOS data properly) is stored in ramstage. This patch moves it to CBFS, making it available for inspection by utilities.
The idea is to allow nvramtool to configure cmos defaults (which are stored in CBFS) by using the table (also stored in CBFS). This requires some changes to nvramtool, but this is the only necessary change in coreboot to allow this.
The downside is that cmos_layout.bin is not compressed, while ramstage (and thus the layout information it contained) usually is. With layout data being 2kb or less that shouldn't be much of an issue.
Right now, the table is copied into cbtable by coreboot, like it used to. It would be possible to just link it to the end of cbtable, making the chain run into flash space, but I didn't do that to minimize the impact of this patch. I'm not sure if reducing cbtable's RAM use by 2kb is worth the potential trouble.
Signed-off-by: Patrick Georgi patrick.georgi@secunet.com
* Patrick Georgi Patrick.Georgi@secunet.com [110114 11:39]:
Hi,
currently the option table (which contains the metadata necessary to parse CMOS data properly) is stored in ramstage. This patch moves it to CBFS, making it available for inspection by utilities.
The idea is to allow nvramtool to configure cmos defaults (which are stored in CBFS) by using the table (also stored in CBFS). This requires some changes to nvramtool, but this is the only necessary change in coreboot to allow this.
The downside is that cmos_layout.bin is not compressed, while ramstage (and thus the layout information it contained) usually is. With layout data being 2kb or less that shouldn't be much of an issue.
Right now, the table is copied into cbtable by coreboot, like it used to. It would be possible to just link it to the end of cbtable, making the chain run into flash space, but I didn't do that to minimize the impact of this patch. I'm not sure if reducing cbtable's RAM use by 2kb is worth the potential trouble.
Signed-off-by: Patrick Georgi patrick.georgi@secunet.com
struct cmos_option_table option_table = cbfs_find_file("cmos_layout.bin", 0x1aa);
Make sure to also teach the magic value to cbfstool (and possibly add a magic value to some coreboot include file and use that instead) Those file types are increasingly hard to keep track of.
Acked-by: Stefan Reinauer stepan@coreboot.org
Stefan
On 14.01.2011 11:39, Patrick Georgi wrote:
Hi,
currently the option table (which contains the metadata necessary to parse CMOS data properly) is stored in ramstage. This patch moves it to CBFS, making it available for inspection by utilities.
The idea is to allow nvramtool to configure cmos defaults (which are stored in CBFS) by using the table (also stored in CBFS). This requires some changes to nvramtool, but this is the only necessary change in coreboot to allow this.
The downside is that cmos_layout.bin is not compressed, while ramstage (and thus the layout information it contained) usually is. With layout data being 2kb or less that shouldn't be much of an issue.
Right now, the table is copied into cbtable by coreboot, like it used to. It would be possible to just link it to the end of cbtable, making the chain run into flash space, but I didn't do that to minimize the impact of this patch. I'm not sure if reducing cbtable's RAM use by 2kb is worth the potential trouble.
Signed-off-by: Patrick Georgi patrick.georgi@secunet.com
Index: coreboot/src/arch/x86/Makefile.inc
--- coreboot.orig/src/arch/x86/Makefile.inc +++ coreboot/src/arch/x86/Makefile.inc @@ -27,7 +27,10 @@ subdirs-y += smp
OPTION_TABLE_H:= ifeq ($(CONFIG_HAVE_OPTION_TABLE),y) -ramstage-srcs += $(obj)/option_table.c +cbfs-files-y += $(obj)/cmos_layout.bin +$(obj)/cmos_layout.bin-name = cmos_layout.bin +$(obj)/cmos_layout.bin-type = 0x01aa
OPTION_TABLE_H:=$(obj)/option_table.h endif
@@ -64,7 +67,7 @@ prebuild-files = \ $(CBFSTOOL) $@ add $(call extract_nth,1,$(file)) $(call extract_nth,2,$(file)) $(call extract_nth,3,$(file)) $(call extract_nth,4,$(file)); ) prebuilt-files = $(foreach file,$(cbfs-files), $(call extract_nth,1,$(file)))
-$(obj)/coreboot.pre1: $(obj)/coreboot.bootblock $(prebuilt-files) $(CBFSTOOL) +$(obj)/coreboot.pre1: $(obj)/coreboot.bootblock $$(prebuilt-files) $(CBFSTOOL) rm -f $@ $(CBFSTOOL) $@ create $(CONFIG_COREBOOT_ROMSIZE_KB)K $(obj)/coreboot.bootblock $(prebuild-files) @@ -121,9 +124,9 @@ $(OPTION_TABLE_H): $(objutil)/options/bu @printf " OPTION $(subst $(obj)/,,$(@))\n" $(objutil)/options/build_opt_tbl --config $(top)/src/mainboard/$(MAINBOARDDIR)/cmos.layout --header $@
-$(obj)/option_table.c: $(objutil)/options/build_opt_tbl $(top)/src/mainboard/$(MAINBOARDDIR)/cmos.layout +$(obj)/cmos_layout.bin: $(objutil)/options/build_opt_tbl $(top)/src/mainboard/$(MAINBOARDDIR)/cmos.layout @printf " OPTION $(subst $(obj)/,,$(@))\n"
- $(objutil)/options/build_opt_tbl --config $(top)/src/mainboard/$(MAINBOARDDIR)/cmos.layout --option $@
- $(objutil)/options/build_opt_tbl --config $(top)/src/mainboard/$(MAINBOARDDIR)/cmos.layout --binary $@
$(objutil)/options/build_opt_tbl: $(top)/util/options/build_opt_tbl.c $(top)/src/include/pc80/mc146818rtc.h $(top)/src/include/boot/coreboot_tables.h @printf " HOSTCC $(subst $(obj)/,,$(@))\n" Index: coreboot/src/arch/x86/boot/coreboot_table.c =================================================================== --- coreboot.orig/src/arch/x86/boot/coreboot_table.c +++ coreboot/src/arch/x86/boot/coreboot_table.c @@ -542,11 +542,14 @@ unsigned long write_coreboot_table(
#if (CONFIG_USE_OPTION_TABLE == 1) {
struct lb_record *rec_dest = lb_new_record(head);/* Copy the option config table, it's already a lb_record... */memcpy(rec_dest, &option_table, option_table.size);/* Create cmos checksum entry in coreboot table */lb_cmos_checksum(head);
struct cmos_option_table option_table = cbfs_find_file("cmos_layout.bin", 0x1aa);if (option_table) {struct lb_record *rec_dest = lb_new_record(head);/* Copy the option config table, it's already a lb_record... */memcpy(rec_dest, &option_table, option_table.size);/* Create cmos checksum entry in coreboot table */lb_cmos_checksum(head);}
Maybe emit a warning when no layout file was found, dispite there should have been one.
} #endif /* Record where RAM is located */ Index: coreboot/src/arch/x86/include/arch/coreboot_tables.h =================================================================== --- coreboot.orig/src/arch/x86/include/arch/coreboot_tables.h +++ coreboot/src/arch/x86/include/arch/coreboot_tables.h @@ -16,8 +16,6 @@ void lb_memory_range(struct lb_memory *m */ struct lb_memory *get_lb_mem(void);
-extern struct cmos_option_table option_table;
/* defined by mainboard.c if the mainboard requires extra resources */ int add_mainboard_resources(struct lb_memory *mem); int add_northbridge_resources(struct lb_memory *mem); Index: coreboot/src/pc80/mc146818rtc.c =================================================================== --- coreboot.orig/src/pc80/mc146818rtc.c +++ coreboot/src/pc80/mc146818rtc.c @@ -4,6 +4,7 @@ #include <string.h> #if CONFIG_USE_OPTION_TABLE #include "option_table.h" +#include <cbfs.h> #endif
/* control registers - Moto names @@ -217,7 +218,6 @@ static int get_cmos_value(unsigned long
int get_option(void *dest, const char *name) {
- extern struct cmos_option_table option_table; struct cmos_option_table *ct; struct cmos_entries *ce; size_t namelen;
@@ -227,7 +227,7 @@ int get_option(void *dest, const char *n namelen = strnlen(name, CMOS_MAX_NAME_LENGTH);
/* find the requested entry record */
- ct=&option_table;
- ct=cbfs_find_file("cmos_layout.bin", 0x1aa); ce=(struct cmos_entries*)((unsigned char *)ct + ct->header_length); for(;ce->tag==LB_TAG_OPTION; ce=(struct cmos_entries*)((unsigned char *)ce + ce->size)) {
Index: coreboot/util/options/build_opt_tbl.c
--- coreboot.orig/util/options/build_opt_tbl.c +++ coreboot/util/options/build_opt_tbl.c @@ -142,8 +142,9 @@ static void display_usage(char *name) printf(" [--option filename]\n"); printf(" [--header filename]\n\n"); printf("--config = Build the definitions table from the given file.\n");
- printf("--bin = Output a binary file with the definitions.\n");
^^^^^^^ that should be "--binary", I guess.
printf("--option = Output a C source file with the definitions.\n");
- printf("--header = Ouput a C header file with the definitions.\n");
- printf("--header = Output a C header file with the definitions.\n"); exit(1);
}
@@ -253,6 +254,7 @@ int main(int argc, char **argv) { int i; char *config=0;
- char *binary=0; char *option=0; char *header=0; FILE *fp;
@@ -288,6 +290,12 @@ int main(int argc, char **argv) } config=argv[++i]; break;
case 'b': /* Emit a binary file */if(strcmp(&argv[i][2],"binary")) {display_usage(argv[0]);}binary=argv[++i];break; case 'o': /* use a cmos definitions table file */ if(strcmp(&argv[i][2],"option")) { display_usage(argv[0]);@@ -563,6 +571,41 @@ int main(int argc, char **argv) perror(NULL); unlink(tempfilename); exit(1);
}- }
- /* See if we also want to output a binary file */
- if(binary) {
int err=0;strncpy(tempfilename, dirname(strdup(binary)), TMPFILE_LEN);strncat(tempfilename, TMPFILE_TEMPLATE, TMPFILE_LEN);
This looks odd. Three problems here in only two lines of code: 1. The memory allocated by strdup() gets leaked. Maybe that's okay because it's only a few bytes. 2. The strncpy() isn't guaranteed to null-terminate the buffer, so strncat() may start writing way beyond the end of the buffer. 3. The length argument to strncat() should be TMPFILE_LEN - strlen(tempfilename) otherwise strncat is allowed to write strlen(tempfilename) bytes beyond the end of the buffer.
Albeit looking around in the file this pattern can also be found in two other places. So maybe this should be fixed in a follow up patch.
Not related to you patch, but noticed while reviewing it: Additionally TMPFILE_LEN looks like to be choosen a little bit too small (only 256). Why isn't it something more reasonable like PATH_MAX? Maybe this should be fixed in a follow up patch, too.
tempfile = mkstemp(tempfilename);if(tempfile == -1) {perror("Error - Could not create temporary file");exit(1);}if((fp=fdopen(tempfile,"wb"))==NULL){perror("Error - Could not open temporary file");unlink(tempfilename);exit(1);}/* write the array values */if(!fwrite(cmos_table, (int)(ct->size-1), 1, fp)) {
It would be better to check that all bytes have been written not only some, so do something like:
if(fwrite(cmos_table, (int)(ct->size-1), 1, fp) != ct->size-1) {
perror("Error - Could not write image file");fclose(fp);unlink(tempfilename);exit(1);}fclose(fp);UNLINK_IF_NECESSARY(binary);if (rename(tempfilename, binary)) {fprintf(stderr, "Error - Could not write %s: ", binary);perror(NULL);unlink(tempfilename);exit(1);
Regards, Mathias
Am Dienstag, den 18.01.2011, 13:52 +0100 schrieb Mathias Krause:
if (option_table) {struct lb_record *rec_dest = lb_new_record(head);/* Copy the option config table, it's already a lb_record... */memcpy(rec_dest, &option_table, option_table.size);/* Create cmos checksum entry in coreboot table */lb_cmos_checksum(head);}Maybe emit a warning when no layout file was found, dispite there should have been one.
That would be a broken build (as soon as USE_OPTION_TABLE is in, cmos_layout.bin is generated and added, or should be). I'd rather make that a build time test, at runtime it's too late for the user to do something sensible about it.
- printf("--bin = Output a binary file with the definitions.\n");
^^^^^^^ that should be "--binary", I guess.
Uh, right.
- if(binary) {
int err=0;strncpy(tempfilename, dirname(strdup(binary)), TMPFILE_LEN);strncat(tempfilename, TMPFILE_TEMPLATE, TMPFILE_LEN);This looks odd. Three problems here in only two lines of code:
- The memory allocated by strdup() gets leaked. Maybe that's okay because it's only a few bytes.
And the tool is short lived. A few bytes wasted for 0.02 seconds? pff :-)
- The strncpy() isn't guaranteed to null-terminate the buffer, so strncat() may start writing way beyond the end of the buffer.
- The length argument to strncat() should be TMPFILE_LEN - strlen(tempfilename) otherwise strncat is allowed to write strlen(tempfilename) bytes beyond the end of the buffer.
Albeit looking around in the file this pattern can also be found in two other places. So maybe this should be fixed in a follow up patch.
Not related to you patch, but noticed while reviewing it: Additionally TMPFILE_LEN looks like to be choosen a little bit too small (only 256). Why isn't it something more reasonable like PATH_MAX? Maybe this should be fixed in a follow up patch, too.
PATH_MAX isn't that good an idea either, see http://www.gnu.org/software/hurd/community/gsoc/project_ideas/maxpath.html and the blog article they link to. I guess TMPFILE_LEN used to work because it usually ends up as a short name in /tmp/, 20 characters or less. But you're right, this needs fixing.
if(!fwrite(cmos_table, (int)(ct->size-1), 1, fp)) {It would be better to check that all bytes have been written not only some, so do something like:
if(fwrite(cmos_table, (int)(ct->size-1), 1, fp) != ct->size-1) {
fwrite returns the number of successful elements, not bytes. Given that there's only 1 element to write (of size ct->size-1), I'd expect it to return a number <= 1 (1 on success, 0 or less on error)
Patrick
Am Dienstag, den 18.01.2011, 14:17 +0100 schrieb Patrick Georgi:
I guess TMPFILE_LEN used to work because it usually ends up as a short name in /tmp/, 20 characters or less. But you're right, this needs fixing.
Actually I'm mistaken on that - it's a relative path (but outside /tmp), which should save us in most situations, but it's usually more than 20 bytes.
I simply give it 25600 bytes now, which should be enough for everyone[tm]. Also moved to using snprintf to prevent the string from exceeding its assigned space (and other nastiness).
Committed as r6268, every other problem can be fixed on top of that :)
Thanks for the review!
Patrick
On 18.01.2011 14:17, Patrick Georgi wrote:
Am Dienstag, den 18.01.2011, 13:52 +0100 schrieb Mathias Krause:
if (option_table) {struct lb_record *rec_dest = lb_new_record(head);/* Copy the option config table, it's already a lb_record... */memcpy(rec_dest, &option_table, option_table.size);/* Create cmos checksum entry in coreboot table */lb_cmos_checksum(head);}Maybe emit a warning when no layout file was found, dispite there should have been one.
That would be a broken build (as soon as USE_OPTION_TABLE is in, cmos_layout.bin is generated and added, or should be). I'd rather make that a build time test, at runtime it's too late for the user to do something sensible about it.
Well, but after the build the CBFS could still be modified (intentional or not) in such a way that it would no longer contain a file named "cmos_layout.bin". So at least to aid debugging it would be nice to see a warning here instead of wondering why the CMOS layout is missing in the LB table.
- The strncpy() isn't guaranteed to null-terminate the buffer, so strncat() may start writing way beyond the end of the buffer.
- The length argument to strncat() should be TMPFILE_LEN - strlen(tempfilename) otherwise strncat is allowed to write strlen(tempfilename) bytes beyond the end of the buffer.
Albeit looking around in the file this pattern can also be found in two other places. So maybe this should be fixed in a follow up patch.
Not related to you patch, but noticed while reviewing it: Additionally TMPFILE_LEN looks like to be choosen a little bit too small (only 256). Why isn't it something more reasonable like PATH_MAX? Maybe this should be fixed in a follow up patch, too.
PATH_MAX isn't that good an idea either, see http://www.gnu.org/software/hurd/community/gsoc/project_ideas/maxpath.html and the blog article they link to. I guess TMPFILE_LEN used to work because it usually ends up as a short name in /tmp/, 20 characters or less. But you're right, this needs fixing.
Well, it's POSIX. But to support GNU Hurd I guess it's save to use an arbitrary, but sane value for TMPFILE_LEN instead of PATH_MAX.
if(!fwrite(cmos_table, (int)(ct->size-1), 1, fp)) {It would be better to check that all bytes have been written not only some, so do something like:
if(fwrite(cmos_table, (int)(ct->size-1), 1, fp) != ct->size-1) {
fwrite returns the number of successful elements, not bytes. Given that there's only 1 element to write (of size ct->size-1), I'd expect it to return a number <= 1 (1 on success, 0 or less on error)
I guess you assume it'll return either 1 (good case) or 0 (bad case) not -1 - which is the case btw. But you're right. I mixed up the size and count parameters of the fwrite() call. So you check is fine the way it is.
Mathias
Am Dienstag, den 18.01.2011, 15:22 +0100 schrieb Mathias Krause:
Well, but after the build the CBFS could still be modified (intentional or not) in such a way that it would no longer contain a file named "cmos_layout.bin". So at least to aid debugging it would be nice to see a warning here instead of wondering why the CMOS layout is missing in the LB table.
I added some reporting in r6269.
I guess you assume it'll return either 1 (good case) or 0 (bad case) not -1 - which is the case btw. But you're right. I mixed up the size and count parameters of the fwrite() call. So you check is fine the way it is.
r6270 makes this more explicit for all calls (so return -E* bails out, too).
Patrick
Patrick Georgi wrote:
if(!fwrite(cmos_table, (int)(ct->size-1), 1, fp)) {It would be better to check that all bytes have been written not only some, so do something like:
if(fwrite(cmos_table, (int)(ct->size-1), 1, fp) != ct->size-1) {
fwrite returns the number of successful elements, not bytes. Given that there's only 1 element to write (of size ct->size-1),
I have had very bad experience with this, where things did not work unless element size=1 and number of elements=bytestowrote, so I continue to write all code that way.
Unfortunately I can't say what the circumstances were, but when I saw it I decided better be safe. And a smart libc could coalesce anyway.
//Peter
-
Mathias Krause -
Patrick Georgi -
Peter Stuge -
Stefan Reinauer