[PATCH] Move cmos.default handling to bootblock

[Patrick Georgi]

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

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(-)

[Georgi, Patrick]

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]

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

[Georgi, Patrick]

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]

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

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

[Georgi, 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

[Stefan Reinauer]

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

Acked-by: Stefan Reinauer

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]

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