Patchwork Move cmos.default handling to bootblock

login
register
about
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

Patrick Georgi - 2011-02-01 10:50:17
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(-)
Patrick Georgi - 2011-02-17 12:15:24
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?
Peter Stuge - 2011-02-18 02:55:15
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
Patrick Georgi - 2011-02-18 07:09:07
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
Peter Stuge - 2011-02-18 11:15:38
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
Patrick Georgi - 2011-02-18 11:28:23
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
Patrick Georgi - 2011-03-04 07:15:17
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
Stefan Reinauer - 2011-03-04 08:32:33
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
Patrick Georgi - 2011-03-04 20:07:30
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.