| Submitter | Patrick Georgi |
|---|---|
| Date | 2011-02-01 10:50:17 |
| Message ID | <1296566674.6489.78.camel@linux-0a8x.site> |
| Download | mbox | patch |
| Permalink | /patch/2592/ |
| State | Accepted |
| Headers | show |
Comments
On Di, 2011-02-01 at 11:50 +0100, Patrick Georgi wrote: > The cmos.default code wasn't actually used so far, due to an oversight > when forward-porting this feature from an old branch. ping?
Patrick Georgi wrote: > > The cmos.default code wasn't actually used so far, due to an oversight > when forward-porting this feature from an old branch. > > - Extend walkcbfs' use by factoring out the stage handling into C code. > - New sanitize_cmos() function that looks if CMOS data is invalid and > cmos.default exists and if so overwrites CMOS with cmos.default data. > - Use sanitize_cmos() in both bootblock implementations. > - Drop the need to reboot after writing CMOS: CMOS wasn't used so far, > so we can go on without a reboot. > - Remove the restriction that cmos.default only works on CAR boards. > - Always build in cmos.default support on boards that USE_OPTION_TABLE. Is there a Kconfig option for enabling the NVRAM write? I would like that very much. I'd also like it to be off by default. Maybe we should consider some coreboot "profiles" for Kconfig, that would autoselect options, which could be manually overridden by experts? //Peter
Am Freitag, den 18.02.2011, 03:55 +0100 schrieb Peter Stuge: > Patrick Georgi wrote: > > > > The cmos.default code wasn't actually used so far, due to an oversight > > when forward-porting this feature from an old branch. > > > > - Extend walkcbfs' use by factoring out the stage handling into C code. > > - New sanitize_cmos() function that looks if CMOS data is invalid and > > cmos.default exists and if so overwrites CMOS with cmos.default data. > > - Use sanitize_cmos() in both bootblock implementations. > > - Drop the need to reboot after writing CMOS: CMOS wasn't used so far, > > so we can go on without a reboot. > > - Remove the restriction that cmos.default only works on CAR boards. > > - Always build in cmos.default support on boards that USE_OPTION_TABLE. > > Is there a Kconfig option for enabling the NVRAM write? I would like > that very much. I'd also like it to be off by default. HAVE_CMOS_DEFAULT. Without this, cmos.default isn't put into CBFS (by default), and without that file, no write happens. > Maybe we should consider some coreboot "profiles" for Kconfig, that > would autoselect options, which could be manually overridden by > experts? "profiles"? We have those: mainboard defaults. Patrick
Georgi, Patrick wrote: > > Is there a Kconfig option for enabling the NVRAM write? I would like > > that very much. I'd also like it to be off by default. > > HAVE_CMOS_DEFAULT. Without this, cmos.default isn't put into CBFS > (by default), and without that file, no write happens. But it's a mainboard knob, not a user knob, right? > > Maybe we should consider some coreboot "profiles" for Kconfig, that > > would autoselect options, which could be manually overridden by > > experts? > > "profiles"? We have those: mainboard defaults. Call them profiles or maybe personalities. Yes there is one set of options from the developer creating the original port, but I'm thinking more of bird's view variations; * Zero impact coreboot (never writes to NVRAM) * Normal mode (will fix NVRAM with defaults) * Strict mode (requires NVRAM to always be correct, or will fail to boot. maybe also other extra checks in the code) There could be other personalities. The point is that they are very high level (ie. easy for every user to decide on, because they fit or do not fit) and might control more than one Kconfig option. //Peter
Am Freitag, den 18.02.2011, 12:15 +0100 schrieb Peter Stuge: > > HAVE_CMOS_DEFAULT. Without this, cmos.default isn't put into CBFS > > (by default), and without that file, no write happens. > But it's a mainboard knob, not a user knob, right? We don't deliver cmos.default files, so this is a user setting at this time. This might change, and then we should reevaluate how we handle CMOS. But USE_OPTION_TABLE should disable it _all_ anyway. So you already have: - no CMOS at all - CMOS support, but no cmos.default - CMOS support with cmos.default > Call them profiles or maybe personalities. Yes there is one set of > options from the developer creating the original port, but I'm > thinking more of bird's view variations; We already stretch Kconfig beyond its limits. Let's not do it any further or it will break apart. > * Strict mode (requires NVRAM to always be correct, or will fail to > boot. maybe also other extra checks in the code) "Fail to boot"? I wonder about its use. Patrick
Am Freitag, den 18.02.2011, 12:28 +0100 schrieb Georgi, Patrick: > Am Freitag, den 18.02.2011, 12:15 +0100 schrieb Peter Stuge: > > > HAVE_CMOS_DEFAULT. Without this, cmos.default isn't put into CBFS > > > (by default), and without that file, no write happens. > > But it's a mainboard knob, not a user knob, right? > We don't deliver cmos.default files, so this is a user setting at this > time. > This might change, and then we should reevaluate how we handle CMOS. > But USE_OPTION_TABLE should disable it _all_ anyway. > > So you already have: > - no CMOS at all > - CMOS support, but no cmos.default > - CMOS support with cmos.default > > * Strict mode (requires NVRAM to always be correct, or will fail to > > boot. maybe also other extra checks in the code) > "Fail to boot"? I wonder about its use. Ping again? What's missing except for new features (eg. more fine-grained control that allows users to build images that won't boot in certain circumstances)? Patrick
On 2/1/11 2:50 AM, Patrick Georgi wrote: > The cmos.default code wasn't actually used so far, due to an oversight > when forward-porting this feature from an old branch. > > - Extend walkcbfs' use by factoring out the stage handling into C code. > - New sanitize_cmos() function that looks if CMOS data is invalid and > cmos.default exists and if so overwrites CMOS with cmos.default data. > - Use sanitize_cmos() in both bootblock implementations. > - Drop the need to reboot after writing CMOS: CMOS wasn't used so far, > so we can go on without a reboot. > - Remove the restriction that cmos.default only works on CAR boards. > - Always build in cmos.default support on boards that USE_OPTION_TABLE. > > Signed-off-by: Patrick Georgi<patrick.georgi@secunet.com> > Acked-by: Stefan Reinauer <stefan.reinauer@coreboot.org> with some optional comments below > index 895a185..a808cec 100644 > --- a/src/arch/x86/include/bootblock_common.h > +++ b/src/arch/x86/include/bootblock_common.h > @@ -17,17 +17,45 @@ static void bootblock_northbridge_init(void) { } > static void bootblock_southbridge_init(void) { } > #endif > > -static unsigned long findstage(char* target) > +static void *walkcbfs(char *target) > { > - unsigned long entry; > + void *entry; > asm volatile ( > "mov $1f, %%esp\n\t" > - "jmp walkcbfs\n\t" > + "jmp walkcbfs_asm\n\t" maybe just call it _walkcbfs ? > +/* just enough to support findstage. copied because the original version doesn't easily pass through romcc */ > +struct cbfs_stage { > + unsigned long compression; > + unsigned long entry; // this is really 64bit, but properly endianized Would it make sense to add an unsigned long entry_high after this, in this case? Or use a union or uint64_t for entry? > +#if CONFIG_USE_OPTION_TABLE > +#include<pc80/mc146818rtc.h> Since you start using cmos in the bootblock, you might have to consider enabling RCBA and the upper 128 bytes of CMOS in some Intel southbridges' bootblock.c > +static void sanitize_cmos(void) > +{ > + if (cmos_error() || !cmos_chksum_valid()) { Is this reliably working on hardware? I remember cmos_error being flaky on ICH7 early on at some point. > diff --git a/src/pc80/mc146818rtc_early.c b/src/pc80/mc146818rtc_early.c > index 920deda..d09d6b9 100644 > --- a/src/pc80/mc146818rtc_early.c > +++ b/src/pc80/mc146818rtc_early.c > @@ -11,15 +11,6 @@ > #error "CONFIG_MAX_REBOOT_CNT too high" > #endif > > -#if CONFIG_USE_CMOS_RECOVERY > -#include<cbfs.h> > -#include<console/loglevel.h> > - > -int do_printk(int msg_level, const char *fmt, ...) __attribute__((format(printf, 2, 3))); > -#define printk_warning(fmt, arg...) do_printk(BIOS_WARNING ,fmt, ##arg) > -#define printk_debug(fmt, arg...) do_printk(BIOS_DEBUG ,fmt, ##arg) > -#endif > - > static int cmos_error(void) > { > unsigned char reg_d; > @@ -63,25 +54,6 @@ static inline int do_normal_boot(void) > unsigned char byte; > > if (cmos_error() || !cmos_chksum_valid()) { > -#if CONFIG_USE_CMOS_RECOVERY > - char *cmos_default = cbfs_find_file("cmos.default", CBFS_COMPONENT_CMOS_DEFAULT); > - if (cmos_default) { > - int i; > - printk_warning("WARNING - CMOS CORRUPTED. RESTORING DEFAULTS.\n"); > - /* First 14 bytes are reserved for > - RTC and ignored by nvramtool, too. > - Only 128 bytes: 128+ requires cmos configuration and > - contains only suspend-to-ram data, which isn't part > - of the recovery procedure. */ > - for (i = 14; i< 128; i++) { > - cmos_write(cmos_default[i], i); > - } > - /* Now reboot to run with default cmos. */ > - outb(0x06, 0xcf9); > - for (;;) asm("hlt"); /* Wait for reset! */ > - } > -#endif > - How does cmos recovery behave on non-CAR systems if this is removed? It would be nice to if we could make sure it works everywhere
Am 04.03.2011 09:32, schrieb Stefan Reinauer: >> index 895a185..a808cec 100644 >> --- a/src/arch/x86/include/bootblock_common.h >> +++ b/src/arch/x86/include/bootblock_common.h >> @@ -17,17 +17,45 @@ static void bootblock_northbridge_init(void) { } >> static void bootblock_southbridge_init(void) { } >> #endif >> >> -static unsigned long findstage(char* target) >> +static void *walkcbfs(char *target) >> { >> - unsigned long entry; >> + void *entry; >> asm volatile ( >> "mov $1f, %%esp\n\t" >> - "jmp walkcbfs\n\t" >> + "jmp walkcbfs_asm\n\t" > maybe just call it _walkcbfs ? I thought about it, but I wanted to avoid confusion in case some compiler uses _ prefixes (in which case that would be __walkcbfs). >> +/* just enough to support findstage. copied because the original version doesn't easily pass through romcc */ >> +struct cbfs_stage { >> + unsigned long compression; >> + unsigned long entry; // this is really 64bit, but properly endianized > Would it make sense to add an unsigned long entry_high after this, in > this case? Or use a union or uint64_t for entry? unsigned long entry_high; is better (romcc and all that) >> +#if CONFIG_USE_OPTION_TABLE >> +#include <pc80/mc146818rtc.h> > Since you start using cmos in the bootblock, you might have to consider > enabling RCBA and the upper 128 bytes of CMOS in some Intel > southbridges' bootblock.c The recovery code explicitely covers the first 128 bytes only - so far the upper 128 bytes are used for suspend-to-ram data only, so this isn't urgent yet. RCBA, yes.. I'll take a look, but I think it worked when I tested it. >> +static void sanitize_cmos(void) >> +{ >> + if (cmos_error() || !cmos_chksum_valid()) { > Is this reliably working on hardware? I remember cmos_error being flaky > on ICH7 early on at some point. And I vaguely remember that we managed to stabilize this by tweaking registers. > How does cmos recovery behave on non-CAR systems if this is removed? It > would be nice to if we could make sure it works everywhere The only caller of this function is bootblock_normal - so by default, this code wasn't used at all. The patch works on qemu, which is a "non-CAR" system, right? Patrick
Patch
diff --git a/src/arch/x86/Kconfig b/src/arch/x86/Kconfig index 3684c69..aacc098 100644 --- a/src/arch/x86/Kconfig +++ b/src/arch/x86/Kconfig @@ -91,14 +91,8 @@ config PC80_SYSTEM config BOOTBLOCK_NORTHBRIDGE_INIT string -config USE_CMOS_RECOVERY - bool - default n if ROMCC - default y - config HAVE_CMOS_DEFAULT def_bool n - depends on USE_CMOS_RECOVERY config CMOS_DEFAULT_FILE string diff --git a/src/arch/x86/Makefile.bootblock.inc b/src/arch/x86/Makefile.bootblock.inc index 31db224..4958a77 100644 --- a/src/arch/x86/Makefile.bootblock.inc +++ b/src/arch/x86/Makefile.bootblock.inc @@ -57,7 +57,7 @@ $(obj)/mainboard/$(MAINBOARDDIR)/bootblock.s: $(obj)/bootblock/bootblock.S @printf " CC $(subst $(obj)/,,$(@))\n" $(CC) -MMD -DASSEMBLY -E -I$(src)/include -I$(src)/arch/x86/include -I$(obj) -I$(obj)/bootblock -include $(obj)/config.h -I. -I$(src) $< -o $@ -$(obj)/mainboard/$(MAINBOARDDIR)/bootblock.inc: $(src)/arch/x86/init/$(subst ",,$(CONFIG_BOOTBLOCK_SOURCE)) $(objutil)/romcc/romcc +$(obj)/mainboard/$(MAINBOARDDIR)/bootblock.inc: $(src)/arch/x86/init/$(subst ",,$(CONFIG_BOOTBLOCK_SOURCE)) $(objutil)/romcc/romcc $(OPTION_TABLE_H) @printf " ROMCC $(subst $(obj)/,,$(@))\n" $(CC) -MM -MT$(obj)/mainboard/$(MAINBOARDDIR)/bootblock.inc \ $< > $(obj)/mainboard/$(MAINBOARDDIR)/bootblock.inc.d diff --git a/src/arch/x86/include/bootblock_common.h b/src/arch/x86/include/bootblock_common.h index 895a185..a808cec 100644 --- a/src/arch/x86/include/bootblock_common.h +++ b/src/arch/x86/include/bootblock_common.h @@ -17,17 +17,45 @@ static void bootblock_northbridge_init(void) { } static void bootblock_southbridge_init(void) { } #endif -static unsigned long findstage(char* target) +static void *walkcbfs(char *target) { - unsigned long entry; + void *entry; asm volatile ( "mov $1f, %%esp\n\t" - "jmp walkcbfs\n\t" + "jmp walkcbfs_asm\n\t" "1:\n\t" : "=a" (entry) : "S" (target) : "ebx", "ecx", "edi", "esp"); return entry; } +/* just enough to support findstage. copied because the original version doesn't easily pass through romcc */ +struct cbfs_stage { + unsigned long compression; + unsigned long entry; // this is really 64bit, but properly endianized +}; + +static unsigned long findstage(char* target) +{ + return ((struct cbfs_stage*)walkcbfs(target))->entry; +} + static void call(unsigned long addr, unsigned long bist) { asm volatile ("jmp *%0\n\t" : : "r" (addr), "a" (bist)); } + +#if CONFIG_USE_OPTION_TABLE +#include <pc80/mc146818rtc.h> + +static void sanitize_cmos(void) +{ + if (cmos_error() || !cmos_chksum_valid()) { + unsigned char *cmos_default = (unsigned char*)walkcbfs("cmos.default"); + if (cmos_default) { + int i; + for (i = 14; i < 128; i++) { + cmos_write(cmos_default[i], i); + } + } + } +} +#endif diff --git a/src/arch/x86/init/bootblock_normal.c b/src/arch/x86/init/bootblock_normal.c index 08651c3..9955148 100644 --- a/src/arch/x86/init/bootblock_normal.c +++ b/src/arch/x86/init/bootblock_normal.c @@ -8,6 +8,10 @@ static void main(unsigned long bist) bootblock_southbridge_init(); } +#if CONFIG_USE_OPTION_TABLE + sanitize_cmos(); +#endif + unsigned long entry; if (do_normal_boot()) entry = findstage("normal/romstage"); diff --git a/src/arch/x86/init/bootblock_simple.c b/src/arch/x86/init/bootblock_simple.c index e8994ee..3887b97 100644 --- a/src/arch/x86/init/bootblock_simple.c +++ b/src/arch/x86/init/bootblock_simple.c @@ -6,6 +6,11 @@ static void main(unsigned long bist) bootblock_northbridge_init(); bootblock_southbridge_init(); } + +#if CONFIG_USE_OPTION_TABLE + sanitize_cmos(); +#endif + const char* target1 = "fallback/romstage"; unsigned long entry; entry = findstage(target1); diff --git a/src/arch/x86/lib/walkcbfs.S b/src/arch/x86/lib/walkcbfs.S index 395c46e..27d82ae 100644 --- a/src/arch/x86/lib/walkcbfs.S +++ b/src/arch/x86/lib/walkcbfs.S @@ -15,19 +15,13 @@ #define CBFS_FILE_STRUCTSIZE (CBFS_FILE_OFFSET + 4) -#define CBFS_STAGE_COMPRESSION 0 -#define CBFS_STAGE_ENTRY (CBFS_STAGE_COMPRESSION + 4) -#define CBFS_STAGE_LOAD (CBFS_STAGE_ENTRY + 8) -#define CBFS_STAGE_LEN (CBFS_STAGE_LOAD + 8) -#define CBFS_STAGE_MEMLEN (CBFS_STAGE_LEN + 4) - /* input %esi: filename input %esp: return address (not pointer to return address!) output %eax: entry point clobbers %ebx, %ecx, %edi */ -walkcbfs: +walkcbfs_asm: cld mov CBFS_HEADER_PTR, %eax @@ -67,8 +61,6 @@ walker: mov CBFS_FILE_OFFSET(%ebx), %eax bswap %eax add %ebx, %eax - add $CBFS_STAGE_ENTRY, %eax /* eax = ((cbfs_stage* (cbfs_file* ebx)->offset)->entry) */ - mov 0(%eax), %eax jmp *%esp tryharder: diff --git a/src/pc80/mc146818rtc_early.c b/src/pc80/mc146818rtc_early.c index 920deda..d09d6b9 100644 --- a/src/pc80/mc146818rtc_early.c +++ b/src/pc80/mc146818rtc_early.c @@ -11,15 +11,6 @@ #error "CONFIG_MAX_REBOOT_CNT too high" #endif -#if CONFIG_USE_CMOS_RECOVERY -#include <cbfs.h> -#include <console/loglevel.h> - -int do_printk(int msg_level, const char *fmt, ...) __attribute__((format(printf, 2, 3))); -#define printk_warning(fmt, arg...) do_printk(BIOS_WARNING ,fmt, ##arg) -#define printk_debug(fmt, arg...) do_printk(BIOS_DEBUG ,fmt, ##arg) -#endif - static int cmos_error(void) { unsigned char reg_d; @@ -63,25 +54,6 @@ static inline int do_normal_boot(void) unsigned char byte; if (cmos_error() || !cmos_chksum_valid()) { -#if CONFIG_USE_CMOS_RECOVERY - char *cmos_default = cbfs_find_file("cmos.default", CBFS_COMPONENT_CMOS_DEFAULT); - if (cmos_default) { - int i; - printk_warning("WARNING - CMOS CORRUPTED. RESTORING DEFAULTS.\n"); - /* First 14 bytes are reserved for - RTC and ignored by nvramtool, too. - Only 128 bytes: 128+ requires cmos configuration and - contains only suspend-to-ram data, which isn't part - of the recovery procedure. */ - for (i = 14; i < 128; i++) { - cmos_write(cmos_default[i], i); - } - /* Now reboot to run with default cmos. */ - outb(0x06, 0xcf9); - for (;;) asm("hlt"); /* Wait for reset! */ - } -#endif - /* There are no impossible values, no checksums so just * trust whatever value we have in the the cmos, * but clear the fallback bit.
The cmos.default code wasn't actually used so far, due to an oversight when forward-porting this feature from an old branch. - Extend walkcbfs' use by factoring out the stage handling into C code. - New sanitize_cmos() function that looks if CMOS data is invalid and cmos.default exists and if so overwrites CMOS with cmos.default data. - Use sanitize_cmos() in both bootblock implementations. - Drop the need to reboot after writing CMOS: CMOS wasn't used so far, so we can go on without a reboot. - Remove the restriction that cmos.default only works on CAR boards. - Always build in cmos.default support on boards that USE_OPTION_TABLE. Signed-off-by: Patrick Georgi <patrick.georgi@secunet.com> It's abuild tested and boot tested with emulation/qemu-x86. With this patch CMOS recovery only works on bootblock enabled boards/chipsets. Consider this an incentive to implement bootblock support where necessary :-) --- src/arch/x86/Kconfig | 6 ----- src/arch/x86/Makefile.bootblock.inc | 2 +- src/arch/x86/include/bootblock_common.h | 34 ++++++++++++++++++++++++++++-- src/arch/x86/init/bootblock_normal.c | 4 +++ src/arch/x86/init/bootblock_simple.c | 5 ++++ src/arch/x86/lib/walkcbfs.S | 10 +-------- src/pc80/mc146818rtc_early.c | 28 ------------------------- 7 files changed, 42 insertions(+), 47 deletions(-)