Patchwork fix normal vs. fallback

login
register
about
Submitter Myles Watson
Date 2010-07-08 17:50:11
Message ID <AANLkTikykS322MfCbUXovXwwrqai3vNXf1plj44Ws2iQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/1593/
State New
Headers show

Comments

Myles Watson - 2010-07-08 17:50:11
BOOTBLOCK_NORMAL allows the user to use CMOS values to select which
image to boot.  This patch:

- makes BOOTBLOCK_NORMAL depend on USE_OPTION_TABLE
- makes compilation of bootblock.inc depend on OPTION_TABLE_H
- removes broken includes

Signed-off-by: Myles Watson <mylesgw@gmail.com>

Thanks,
Myles
Patrick Georgi - 2010-07-08 17:56:38
Am 08.07.2010 19:50, schrieb Myles Watson:
> BOOTBLOCK_NORMAL allows the user to use CMOS values to select which
> image to boot.  This patch:
> 
> - makes BOOTBLOCK_NORMAL depend on USE_OPTION_TABLE
This would prevent the "old" scheme of building a fallback image (which
is built first) with BOOTBLOCK_NORMAL and _no_ USE_OPTION_TABLE (so it
uses the hardcoded defaults) and a normal image with USE_OPTION_TABLE.

> - makes compilation of bootblock.inc depend on OPTION_TABLE_H
How does this interact with BOOTBLOCK_SIMPLE builds without
USE_OPTION_TABLE?


Patrick
Myles Watson - 2010-07-08 18:09:01
On Thu, Jul 8, 2010 at 11:56 AM, Patrick Georgi <patrick@georgi-clan.de> wrote:
> Am 08.07.2010 19:50, schrieb Myles Watson:
>> BOOTBLOCK_NORMAL allows the user to use CMOS values to select which
>> image to boot.  This patch:
>>
>> - makes BOOTBLOCK_NORMAL depend on USE_OPTION_TABLE
> This would prevent the "old" scheme of building a fallback image (which
> is built first) with BOOTBLOCK_NORMAL and _no_ USE_OPTION_TABLE (so it
> uses the hardcoded defaults) and a normal image with USE_OPTION_TABLE.
So I probably went too far.  I should have made it depend on HAVE_OPTION_TABLE.

>> - makes compilation of bootblock.inc depend on OPTION_TABLE_H
> How does this interact with BOOTBLOCK_SIMPLE builds without
> USE_OPTION_TABLE?
It shouldn't make a difference.  OPTION_TABLE_H is empty if there isn't one.

Thanks,
Myles
Myles Watson - 2010-07-08 18:11:43
On Thu, Jul 8, 2010 at 12:09 PM, Myles Watson <mylesgw@gmail.com> wrote:
> On Thu, Jul 8, 2010 at 11:56 AM, Patrick Georgi <patrick@georgi-clan.de> wrote:
>> Am 08.07.2010 19:50, schrieb Myles Watson:
>>> BOOTBLOCK_NORMAL allows the user to use CMOS values to select which
>>> image to boot.  This patch:
>>>
>>> - makes BOOTBLOCK_NORMAL depend on USE_OPTION_TABLE
>> This would prevent the "old" scheme of building a fallback image (which
>> is built first) with BOOTBLOCK_NORMAL and _no_ USE_OPTION_TABLE (so it
>> uses the hardcoded defaults) and a normal image with USE_OPTION_TABLE.
> So I probably went too far.  I should have made it depend on HAVE_OPTION_TABLE.

I admit I've never used it.  I just tried to build it to see if there
were any warnings, and it wouldn't build.  Maybe it would be better if
you fixed it the right way.

Thanks,
Myles
Stefan Reinauer - 2010-07-09 09:02:02
On 7/8/10 8:09 PM, Myles Watson wrote:
> On Thu, Jul 8, 2010 at 11:56 AM, Patrick Georgi <patrick@georgi-clan.de> wrote:
>> Am 08.07.2010 19:50, schrieb Myles Watson:
>>> BOOTBLOCK_NORMAL allows the user to use CMOS values to select which
>>> image to boot.  This patch:
>>>
>>> - makes BOOTBLOCK_NORMAL depend on USE_OPTION_TABLE
>> This would prevent the "old" scheme of building a fallback image (which
>> is built first) with BOOTBLOCK_NORMAL and _no_ USE_OPTION_TABLE (so it
>> uses the hardcoded defaults) and a normal image with USE_OPTION_TABLE.
> So I probably went too far.  I should have made it depend on HAVE_OPTION_TABLE.
Even though the normal/fallback mechanism uses CMOS, it does not require
an option table.
Are there advantages in changing this?

Stefan
Peter Stuge - 2010-07-09 13:10:14
Stefan Reinauer wrote:
> Even though the normal/fallback mechanism uses CMOS, it does not
> require an option table.
> Are there advantages in changing this?

One advantage would be that any use of NVRAM always implies having an
option table, which I think makes sense. Somewhere it needs to be
specified what bit(s) the mechanism uses, better in an option table
than hardcoded IMO.


//Peter
Myles Watson - 2010-07-09 13:36:16
On Fri, Jul 9, 2010 at 7:10 AM, Peter Stuge <peter@stuge.se> wrote:
> Stefan Reinauer wrote:
>> Even though the normal/fallback mechanism uses CMOS, it does not
>> require an option table.
>> Are there advantages in changing this?
>
> One advantage would be that any use of NVRAM always implies having an
> option table, which I think makes sense. Somewhere it needs to be
> specified what bit(s) the mechanism uses, better in an option table
> than hardcoded IMO.

That was my thought.  It should be obvious when we're using/corrupting
values, to minimize surprises.

Thanks,
Myles
Stefan Reinauer - 2010-07-09 15:55:21
On 7/9/10 3:36 PM, Myles Watson wrote:
> On Fri, Jul 9, 2010 at 7:10 AM, Peter Stuge <peter@stuge.se> wrote:
>> Stefan Reinauer wrote:
>>> Even though the normal/fallback mechanism uses CMOS, it does not
>>> require an option table.
>>> Are there advantages in changing this?
>> One advantage would be that any use of NVRAM always implies having an
>> option table, which I think makes sense. Somewhere it needs to be
>> specified what bit(s) the mechanism uses, better in an option table
>> than hardcoded IMO.
> That was my thought.  It should be obvious when we're using/corrupting
> values, to minimize surprises.

the normal/fallback selection and the cmos settings are living in
completely distinct spaces. normal/fallback is not covered by the
checksum. So I don't think this applies here. At least not until we
create this necessity. I think we want to be able to use CMOS options
without normal / fallback and the other way round, so we need to be careful.

Stefan
Myles Watson - 2010-07-09 16:01:15
On Fri, Jul 9, 2010 at 9:55 AM, Stefan Reinauer
<stefan.reinauer@coresystems.de> wrote:
>  On 7/9/10 3:36 PM, Myles Watson wrote:
>> On Fri, Jul 9, 2010 at 7:10 AM, Peter Stuge <peter@stuge.se> wrote:
>>> Stefan Reinauer wrote:
>>>> Even though the normal/fallback mechanism uses CMOS, it does not
>>>> require an option table.
>>>> Are there advantages in changing this?
>>> One advantage would be that any use of NVRAM always implies having an
>>> option table, which I think makes sense. Somewhere it needs to be
>>> specified what bit(s) the mechanism uses, better in an option table
>>> than hardcoded IMO.
>> That was my thought.  It should be obvious when we're using/corrupting
>> values, to minimize surprises.
>
> the normal/fallback selection and the cmos settings are living in
> completely distinct spaces. normal/fallback is not covered by the
> checksum.
Yes.  It could still corrupt values used by the factory BIOS if you're
trying to have them coexist.  For testing, it can be nice to not have
to do BIOS setup every time you boot the factory BIOS.

> So I don't think this applies here. At least not until we
> create this necessity. I think we want to be able to use CMOS options
> without normal / fallback and the other way round, so we need to be careful.

No problem.  It compiles now, so I'm happy.

Thanks,
Myles
Stefan Reinauer - 2010-07-09 16:29:23
On 7/9/10 6:01 PM, Myles Watson wrote:
> On Fri, Jul 9, 2010 at 9:55 AM, Stefan Reinauer
> <stefan.reinauer@coresystems.de> wrote:
>>  On 7/9/10 3:36 PM, Myles Watson wrote:
>>> On Fri, Jul 9, 2010 at 7:10 AM, Peter Stuge <peter@stuge.se> wrote:
>>>> Stefan Reinauer wrote:
>>>>> Even though the normal/fallback mechanism uses CMOS, it does not
>>>>> require an option table.
>>>>> Are there advantages in changing this?
>>>> One advantage would be that any use of NVRAM always implies having an
>>>> option table, which I think makes sense. Somewhere it needs to be
>>>> specified what bit(s) the mechanism uses, better in an option table
>>>> than hardcoded IMO.
>>> That was my thought.  It should be obvious when we're using/corrupting
>>> values, to minimize surprises.
>> the normal/fallback selection and the cmos settings are living in
>> completely distinct spaces. normal/fallback is not covered by the
>> checksum.
> Yes.  It could still corrupt values used by the factory BIOS if you're
> trying to have them coexist.  For testing, it can be nice to not have
> to do BIOS setup every time you boot the factory BIOS.

0. Maybe we should hard code the Normal / Fallback ("BOOT_BYTE") into
the cmos.layout parser tool so anyone who tries to use that byte gets a
decent error.

1. Should Fallback always ignore CMOS? I think it would make more sense
if Normal and Fallback were the same and both would write a decent set
of CMOS defaults in the case of a bad checksum.

2. Ok, so what should happen when bad settings are detected?
2.1 Go back to fallback (the same path like when the normal rom image is
corrupted)?
2.1.1 leave BOOT_BYTE alone ...?
2.1.2 dump a set of cmos default values
2.2 Stay in Normal
2.2.1 leave BOOT_BYTE alone ...?
2.2.2 dump a set of cmos default values

3. Do we want an extra "checksum" for BOOT_BYTE?

Stefan
Myles Watson - 2010-07-09 17:02:33
> 0. Maybe we should hard code the Normal / Fallback ("BOOT_BYTE") into
> the cmos.layout parser tool so anyone who tries to use that byte gets a
> decent error.
It seems more flexible to use the value from cmos.layout unless there isn't
one, then hard code it.  But I agree we don't want it to be part of the
checksum.

> 1. Should Fallback always ignore CMOS? I think it would make more sense
> if Normal and Fallback were the same and both would write a decent set
> of CMOS defaults in the case of a bad checksum.
Agreed.  The way we build each image separately now, you could have a
different set of defaults for Normal and Fallback.

> 2. Ok, so what should happen when bad settings are detected?
> 2.1 Go back to fallback (the same path like when the normal rom image is
> corrupted)?
> 2.1.1 leave BOOT_BYTE alone ...?
> 2.1.2 dump a set of cmos default values
> 2.2 Stay in Normal
> 2.2.1 leave BOOT_BYTE alone ...?
> 2.2.2 dump a set of cmos default values

Stay in Normal, set boot count to maximum, set CMOS to default.

Then if it fails, fallback is tried next.  I guess it would be nice to have
fallback set CMOS to its default values if it gets selected because the boot
count was too high.  Booting fallback with normal's defaults doesn't seem
wise.
 
> 3. Do we want an extra "checksum" for BOOT_BYTE?
Might be good.  One byte seems cheap.

Thanks,
Myles
Peter Stuge - 2010-07-09 18:35:59
Stefan Reinauer wrote:
> 1. Should Fallback always ignore CMOS? I think it would make more sense
> if Normal and Fallback were the same and both would write a decent set
> of CMOS defaults in the case of a bad checksum.

NAK if this means that testing coreboot and later booting factory
BIOS again will throw an error and lose/change the NVRAM contents.


//Peter
Stefan Reinauer - 2010-07-09 18:50:13
On 7/9/10 8:35 PM, Peter Stuge wrote:
> Stefan Reinauer wrote:
>> 1. Should Fallback always ignore CMOS? I think it would make more sense
>> if Normal and Fallback were the same and both would write a decent set
>> of CMOS defaults in the case of a bad checksum.
> NAK if this means that testing coreboot and later booting factory
> BIOS again will throw an error and lose/change the NVRAM contents.
>

I don't understand your concern.

That's always the case in one way or another, unless you disable CMOS
settings in coreboot completely.

Are you saying that coreboot should not corrupt factory bios settings
even if that is required to have settings in coreboot? It sounds if you
need intelligence in that area, you're gonna get it with tools like
nvramtool rather than generally cutting coreboot's feature set. In my
scenarios a machine is never running vendor bioses again after coreboot
is able to boot (that's the whole purpose of coreboot)

Stefan
Peter Stuge - 2010-07-09 19:11:22
Stefan Reinauer wrote:
> >> if Normal and Fallback were the same and both would write a
> >> decent set of CMOS defaults in the case of a bad checksum.
> > NAK if this means that testing coreboot and later booting factory
> > BIOS again will throw an error and lose/change the NVRAM contents.
> 
> I don't understand your concern.
..
> In my scenarios a machine is never running vendor bioses again
> after coreboot is able to boot (that's the whole purpose of coreboot)

And that's the good scenario that we all know and love. But remember
that there are other scenarios up until that point.

I think it's really bad for coreboot to trample NVRAM contents since
it hurts the impression of coreboot when someone is evaluating
coreboot for the very first time.


> That's always the case in one way or another, unless you disable
> CMOS settings in coreboot completely.

This is another way to express what I think is important; a way to
disable NVRAM options that guarantees that coreboot will never write
to NVRAM.


> Are you saying that coreboot should not corrupt factory bios settings
> even if that is required to have settings in coreboot? It sounds if you
> need intelligence in that area, you're gonna get it with tools like
> nvramtool rather than generally cutting coreboot's feature set.

I'm after a more sophisticated feature set in coreboot, I don't want
to cut it down in any way. I totally understand that coreboot in some
areas makes convenient assumptions that hold for some scenarios but
breaks in others, and it may not be entirely trivial to support those
"new" scenarios.


//Peter
Myles Watson - 2010-07-09 19:48:09
> This is another way to express what I think is important; a way to
> disable NVRAM options that guarantees that coreboot will never write
> to NVRAM.

It's implemented: Don't use fallback/normal & set USE_OPTION_TABLE to false.

Thanks,
Myles
Peter Stuge - 2010-07-09 20:30:13
Myles Watson wrote:
> > This is another way to express what I think is important; a way to
> > disable NVRAM options that guarantees that coreboot will never write
> > to NVRAM.
> 
> It's implemented: Don't use fallback/normal & set USE_OPTION_TABLE to
> false.

Fantastic. I probably knew this already, sorry for forgetting. It
would be nice to never mention fallback anywhere unless there is
also a normal, but that's another issue.

If USE_OPTION_TABLE is set during config then I think it's fine to
overwrite any invalid NVRAM contents.


//Peter
Myles Watson - 2010-07-09 20:47:51
> > It's implemented: Don't use fallback/normal & set USE_OPTION_TABLE to
> > false.
> 
> Fantastic. I probably knew this already, sorry for forgetting.
No problem.

> It
> would be nice to never mention fallback anywhere unless there is
> also a normal, but that's another issue.
Yes.  It's always seemed to me like we should always have a normal image,
and let fallback be optional.  I think you can change that via Kconfig, but
I haven't played with it.

> If USE_OPTION_TABLE is set during config then I think it's fine to
> overwrite any invalid NVRAM contents.
Great.  That's how it's supposed to work.

Thanks,
Myles

Patch

Index: svn/src/arch/i386/Kconfig
===================================================================
--- svn.orig/src/arch/i386/Kconfig
+++ svn/src/arch/i386/Kconfig
@@ -63,6 +63,7 @@  config BOOTBLOCK_SIMPLE
 
 config BOOTBLOCK_NORMAL
 	bool "Switch to normal if CMOS says so"
+	depends on USE_OPTION_TABLE
 
 endchoice
 
Index: svn/src/arch/i386/Makefile.bootblock.inc
===================================================================
--- svn.orig/src/arch/i386/Makefile.bootblock.inc
+++ svn/src/arch/i386/Makefile.bootblock.inc
@@ -65,7 +65,7 @@  $(obj)/mainboard/$(MAINBOARDDIR)/bootblo
 	@printf "    CC         $(subst $(obj)/,,$(@))\n"
 	$(CC) -MMD -x assembler-with-cpp -DASSEMBLY -E -I$(src)/include -I$(src)/arch/i386/include -I$(obj) -I$(obj)/bootblock -include $(obj)/config.h -I. -I$(src) $< -o $@
 
-$(obj)/mainboard/$(MAINBOARDDIR)/bootblock.inc: $(src)/arch/i386/init/$(subst ",,$(CONFIG_BOOTBLOCK_SOURCE)) $(objutil)/romcc/romcc
+$(obj)/mainboard/$(MAINBOARDDIR)/bootblock.inc: $(src)/arch/i386/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
Index: svn/src/arch/i386/init/bootblock_normal.c
===================================================================
--- svn.orig/src/arch/i386/init/bootblock_normal.c
+++ svn/src/arch/i386/init/bootblock_normal.c
@@ -1,7 +1,5 @@ 
 #include <bootblock_common.h>
 
-#include <arch/io.h>
-#include "arch/romcc_io.h"
 #include <pc80/mc146818rtc.h>
 
 static void main(unsigned long bist)