Patchwork Move option table to CBFS

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:
> 1. 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 :-)

> 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.
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.

>> 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.
> 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