[coreboot] [PATCH]Move option table to CBFS
Mathias Krause
mathias.krause at secunet.com
Tue Jan 18 13:52:06 CET 2011
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 at 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
More information about the coreboot
mailing list