Luc Verhaegen.
Luc Verhaegen wrote:
From 261d18b8825a849d6e7b678294cf13ede5f0e8c9 Mon Sep 17 00:00:00 2001 From: Luc Verhaegen libv@skynet.be Date: Fri, 29 May 2009 15:18:33 +0200 Subject: [PATCH] CMOS: Add set_option and rework get_option.
To ease some of my debugging pain on the unichrome, i decided i needed to move FB size selection into cmos, so i could test a size and then reset it to the default after loading this value so that the next reboot uses the (working) default again. This meant implementing set_option in parallel to get_option.
get_option was then found to have inversed argument ordering (like outb) and passing char * and then depending on the cmos layout length, which made me feel quite uncomfortable. Since we either have reserved space (which we shouldn't do anything with in these two functions), an enum or a hexadecimal value, unsigned int seemed like the way to go. So all users of get_option now have their arguments inversed and switched from using ints to unsigned ints now.
The way get_cmos_value was implemented forced us to not overlap byte and to have multibyte values be byte aligned. This logic is now adapted to do a full uint32_t read (when needed) at any offset and any length up to 32, and the shifting all happens inside an uint32_t as well. set_cmos_value was implemented similarly. Both routines have been extensively tested in a quick separate little program as it is not easy to get this stuff right.
build_opt_tbl.c was altered to function correctly within these new parameters. The enum value retrieval has been changed strol(..., NULL, 10) to stroul(..., NULL, 0), so that we not only are able to use unsigned ints now but so that we also interprete hex values correctly. The 32bit limit gets imposed on all entries not marked reserved, an unused "user_data" field that appeared in a lot of cmos.layouts has been changed to reserved as well.
Signed-off-by: Luc Verhaegen libv@skynet.be
Acked-by: Peter Stuge peter@stuge.se
On Fri, May 29, 2009 at 03:48:09PM +0200, Peter Stuge wrote:
Luc Verhaegen wrote:
From 261d18b8825a849d6e7b678294cf13ede5f0e8c9 Mon Sep 17 00:00:00 2001 From: Luc Verhaegen libv@skynet.be Date: Fri, 29 May 2009 15:18:33 +0200 Subject: [PATCH] CMOS: Add set_option and rework get_option.
To ease some of my debugging pain on the unichrome, i decided i needed to move FB size selection into cmos, so i could test a size and then reset it to the default after loading this value so that the next reboot uses the (working) default again. This meant implementing set_option in parallel to get_option.
get_option was then found to have inversed argument ordering (like outb) and passing char * and then depending on the cmos layout length, which made me feel quite uncomfortable. Since we either have reserved space (which we shouldn't do anything with in these two functions), an enum or a hexadecimal value, unsigned int seemed like the way to go. So all users of get_option now have their arguments inversed and switched from using ints to unsigned ints now.
The way get_cmos_value was implemented forced us to not overlap byte and to have multibyte values be byte aligned. This logic is now adapted to do a full uint32_t read (when needed) at any offset and any length up to 32, and the shifting all happens inside an uint32_t as well. set_cmos_value was implemented similarly. Both routines have been extensively tested in a quick separate little program as it is not easy to get this stuff right.
build_opt_tbl.c was altered to function correctly within these new parameters. The enum value retrieval has been changed strol(..., NULL, 10) to stroul(..., NULL, 0), so that we not only are able to use unsigned ints now but so that we also interprete hex values correctly. The 32bit limit gets imposed on all entries not marked reserved, an unused "user_data" field that appeared in a lot of cmos.layouts has been changed to reserved as well.
Signed-off-by: Luc Verhaegen libv@skynet.be
Acked-by: Peter Stuge peter@stuge.se
That was mighty fast :)
Since this changes "known" behaviour of cmos options, i would prefer at least a second Ack though, especially since the above reasoning might not be agreed with fully by all.
One future change that could be considered is to make build_opt_table no longer spew out a blob of chars. This whole char stuff makes me feel very uncomfortable as well, and the state of and code in build_opt_table.c does not exactly help get rid of that feeling.
It would be nice if this program spewed out #define's for options where we now blindly use strings (this makes another issue that i will address later more prominent). It would also be nice if the values of the enums would also be provided in a header file as <OPTION_NAME>_<OPTION_VALUE>, so that we no longer can feed or read invalid data.
Currently, our security of the cmos value being correct with respect to the options table, is the existence of the option name string in the optionstable, and the validity of the checksum. We need more than this, we need a better way of matching cmos data with the options table.
A quick suggestion would be this: * use 2 shorts of for some sort of board id thingie, so that we can match th with the options table. * use 2 shorts as a major and minor revision number. Both are present in cmos and in the options table, and the values are of course defined in cmos.layout.
If the board id differs or the major revision number differs, we should act the same way as if the checksum is invalid: flag the data as invalid to the users of get_option. Minor number should be bumped on additional entries or additional values, so that only newer option tables (from cb tables or from cmos.layout) are allowed by nvram_tool.
Thoughts, suggestions?
Luc Verhaegen.
Luc Verhaegen wrote:
That was mighty fast :)
I read it through and I liked it.
Since this changes "known" behaviour of cmos options, i would prefer at least a second Ack though, especially since the above reasoning might not be agreed with fully by all.
Sure.
A quick suggestion would be this:
Or store a 32 bit CRC in NVRAM, calculated from a canonicalized form of the cmos.layout contents.
Minor number should be bumped on additional entries or additional values, so that only newer option tables (from cb tables or from cmos.layout) are allowed by nvram_tool.
Versioning is nice - but do we need it? I think this nvram stuff is the most infrequent changing part of all of coreboot.
//Peter
On Fri, May 29, 2009 at 04:16:33PM +0200, Peter Stuge wrote:
Luc Verhaegen wrote:
That was mighty fast :)
I read it through and I liked it.
:)
A quick suggestion would be this:
Or store a 32 bit CRC in NVRAM, calculated from a canonicalized form of the cmos.layout contents.
Yeah, that would be a good solution for checking the validity too.
Versioning is nice - but do we need it? I think this nvram stuff is the most infrequent changing part of all of coreboot.
To be absolutely honest, i was just spewing some thoughts, my itch is scratched with this patch, i have what i needed for my board, and i see it less likely that i will implement this than that i will fix up the TODOs in vga_console.c :)
Luc Verhaegen.
Luc Verhaegen wrote:
Or store a 32 bit CRC in NVRAM, calculated from a canonicalized form of the cmos.layout contents.
Yeah, that would be a good solution for checking the validity too.
Only problem is that if the canonicalization ever changes everything is suddenly invalid. That's bad.
//Peter
On 29.05.2009 16:16 Uhr, Peter Stuge wrote:
Minor number should be bumped on additional entries or additional values, so that only newer option tables (from cb tables or from cmos.layout) are allowed by nvram_tool.
Versioning is nice - but do we need it? I think this nvram stuff is the most infrequent changing part of all of coreboot.
No absolutely not. the nvramtool can read the format of the current version and work with that. No use for a version here.
We would have to provide a description for all versions in the coreboot table. please don't even start thinking about this.
On 29.05.2009 16:14 Uhr, Luc Verhaegen wrote:
Currently, our security of the cmos value being correct with respect to the options table, is the existence of the option name string in the optionstable, and the validity of the checksum. We need more than this, we need a better way of matching cmos data with the options table.
Why?
On 30.05.2009 6:26 Uhr, Peter Stuge wrote:
Stefan Reinauer wrote:
we need a better way of matching cmos data with the options table.
Why?
nvramtool can ask user for a decision.
coreboot can more easily ignore invalid data.
I like validation.
Sorry, I think I completely lost you.
What's wrong with our way of handling nvram again? And what's the context of your statements above?
Afaict: - coreboot does validate the nvram before using it. - coreboot should not try to use nvram if it is not valid. - nvramtool never _asks_ the user. It's not an interactive tool. Instead it will fix checksums as soon as you start setting options.
Stefan
On Fri, May 29, 2009 at 03:44:28PM +0200, Luc Verhaegen wrote:
To ease some of my debugging pain on the unichrome, i decided i needed to move FB size selection into cmos, so i could test a size and then reset it to the default after loading this value so that the next reboot uses the (working) default again. This meant implementing set_option in parallel to get_option.
As an aside, I think long-term coreboot should have a config file in flash and use that instead of nvram.
The issues with using nvram:
* it's small and it leads to weird hacks to store data
* the coreboot layout conflicts with the vendor layout and it's a pain when switching between coreboot and factory bios
* the batteries frequently get old and nvram storage becomes flaky
Just my 2 cents. -Kevin
On Sat, May 30, 2009 at 11:59 AM, Kevin O'Connor kevin@koconnor.net wrote:
On Fri, May 29, 2009 at 03:44:28PM +0200, Luc Verhaegen wrote:
To ease some of my debugging pain on the unichrome, i decided i needed to move FB size selection into cmos, so i could test a size and then reset it to the default after loading this value so that the next reboot uses the (working) default again. This meant implementing set_option in parallel to get_option.
As an aside, I think long-term coreboot should have a config file in flash and use that instead of nvram.
The issues with using nvram:
* it's small and it leads to weird hacks to store data
* the coreboot layout conflicts with the vendor layout and it's a pain when switching between coreboot and factory bios
* the batteries frequently get old and nvram storage becomes flaky
the big concern is having to rewrite a 64K flash page to change one bit. However! We could come up with a format for an erased "option page" in flash like this: struct option {u8 type; u32 len; char values[]};
Type would be 0xff to indicate unused, have some non-zero value to indicate a valid type, and 0 to indicate 'deleted' (much as we do on cbfs now). To eliminate/replace an option that is in the FLASH, set type to 0 (on many parts, we can do this without affecting other bytes) and then set the new option at the end of the current options.
Or should we just store options as files in cbfs? That seems simplest.
ron
On Sat, May 30, 2009 at 09:00:42PM -0400, ron minnich wrote:
On Sat, May 30, 2009 at 11:59 AM, Kevin O'Connor kevin@koconnor.net wrote:
The issues with using nvram:
* it's small and it leads to weird hacks to store data
* the coreboot layout conflicts with the vendor layout and it's a pain when switching between coreboot and factory bios
* the batteries frequently get old and nvram storage becomes flaky
the big concern is having to rewrite a 64K flash page to change one bit. However!
It would require dedicating a flash page for the config, and it would require using flashrom (or an equivalent) to change the config. I wouldn't worry about the flash wear - the config wont change frequently.
[...]
Or should we just store options as files in cbfs? That seems simplest.
That's a neat idea. It's not very compact, but it would work well for a handful of options.
If we went with a config file, I propose something very simple - like text lines with "name=value\n".
-Kevin
On Fri, May 29, 2009 at 07:45:18PM +0200, Stefan Reinauer wrote:
On 29.05.2009 16:14 Uhr, Luc Verhaegen wrote:
Currently, our security of the cmos value being correct with respect to the options table, is the existence of the option name string in the optionstable, and the validity of the checksum. We need more than this, we need a better way of matching cmos data with the options table.
Why?
Example:
Run native bios on epia-m. It stops booting, loads the defaults and tells you to go into the config. Alter the config, boot again, and it is happy.
Then plug in a coreboot image. It flags that the checksum is invalid, and therefor most of the users of cmos data use a default, yet it continues booting.
Alter a single value with nvramtool, and suddenly the cmos checksum is correct. No matter what the other values are. Sometimes you're lucky, but in the epia-m case, you lose serial output (as it suddenly is the lowest value, a value which screen wasn't used to) and seabios tries to boot off the network. All one can do is clear the cmos and then boot properly...
What can be done to fix this: 1) track board and version information. 2) add some way to define the default settings in the cmos.layout.
Then there are several situations possible.
* when a legacy bios was run and the rtc is full of garbage (for us), or when a different board is used, or when the major is different. This should be treated the same way as an invalid checksum. Booting refuses to use it and feeds back defaults (which can also be overridden by code depending on the return value of get_option). Nvramtool can catch this nicely and can offer to fill in defaults, or at least refuse to correct the checksum. * when booting, and when a minor difference between cmos and option table is detected, treat the retrieved information as in the previous point. * when a minor difference between rom option table and cmos.layout is detected, then nvramtool can offer to program the default value of the difference, because it can find out the difference. * when the cmos.layout is for a different board, or of a lower version, then nvramtool should basically refuse to accept it.
Also when nvramtool detects invalid values, it can offer to program defaults or at least refuse to validate the checksum. nvramtool should be able to write anything though, through the use of some argument like --force. It should also be able to just set up everything by default.
Before any of this is done, the table generation code should get fixed to output more sensible things (why do we dump this to a list of chars in a table?) with things like defines or enums for the values. And we should be able to do checking at boottime as well.
I know that this all nontrivial to implement, and that therefor it'll take some time for someone to implement it. But i believe it makes sense, and it will definitely avoid a board booting up with invalid settings, and at least go for defaults.
Luc Verhaegen.
On Sat, May 30, 2009 at 11:59:00AM -0400, Kevin O'Connor wrote:
On Fri, May 29, 2009 at 03:44:28PM +0200, Luc Verhaegen wrote:
To ease some of my debugging pain on the unichrome, i decided i needed to move FB size selection into cmos, so i could test a size and then reset it to the default after loading this value so that the next reboot uses the (working) default again. This meant implementing set_option in parallel to get_option.
As an aside, I think long-term coreboot should have a config file in flash and use that instead of nvram.
The issues with using nvram:
- it's small and it leads to weird hacks to store data
You have, iirc, 228 bytes to your disposal.
Even if we would put all current cmos.layouts on byte boundaries, no-one would run out of space anyway.
How would you want to store information, and how would you want to handle parsing of this information? Do you really want to store configuration in xml and include an xml parser in a bios? Do you want to invent your own structured information storage system? If so, what is wrong with what we do now, especially, since we do not have to reprogram part of our flash every 5 seconds?
228 bytes is plenty if you are not storing whole strings.
- the coreboot layout conflicts with the vendor layout and it's a pain when switching between coreboot and factory bios
This is what the board-id-ing and versioning in both cmos and optiom table will solve.
- the batteries frequently get old and nvram storage becomes flaky
In such a case, too bad, get a new battery and program defaults.
CMOS is the best part of 256bytes that's available for everyone to use. It's there, it's free, we know how to access it, we know how to make very efficient use of it. We can write to it almost infinitely and very quickly. Sure, you cannot stuff many kBs in there, but how would you use such space anyway?
Why not make maximal use of infrastructure/hardware that is freely available?
Luc Verhaegen.
On Tue, Jun 02, 2009 at 06:45:28PM +0200, Luc Verhaegen wrote:
On Sat, May 30, 2009 at 11:59:00AM -0400, Kevin O'Connor wrote:
On Fri, May 29, 2009 at 03:44:28PM +0200, Luc Verhaegen wrote:
To ease some of my debugging pain on the unichrome, i decided i needed to move FB size selection into cmos, so i could test a size and then reset it to the default after loading this value so that the next reboot uses the (working) default again. This meant implementing set_option in parallel to get_option.
As an aside, I think long-term coreboot should have a config file in flash and use that instead of nvram.
The issues with using nvram:
- it's small and it leads to weird hacks to store data
You have, iirc, 228 bytes to your disposal.
Even if we would put all current cmos.layouts on byte boundaries, no-one would run out of space anyway.
How would you want to store information, and how would you want to handle parsing of this information? Do you really want to store configuration in xml and include an xml parser in a bios? Do you want to invent your own structured information storage system? If so, what is wrong with what we do now, especially, since we do not have to reprogram part of our flash every 5 seconds?
228 bytes is plenty if you are not storing whole strings.
- the coreboot layout conflicts with the vendor layout and it's a pain when switching between coreboot and factory bios
This is what the board-id-ing and versioning in both cmos and optiom table will solve.
- the batteries frequently get old and nvram storage becomes flaky
In such a case, too bad, get a new battery and program defaults.
CMOS is the best part of 256bytes that's available for everyone to use. It's there, it's free, we know how to access it, we know how to make very efficient use of it. We can write to it almost infinitely and very quickly. Sure, you cannot stuff many kBs in there, but how would you use such space anyway?
Why not make maximal use of infrastructure/hardware that is freely available?
Luc Verhaegen.
I cannot help but feel that this is just trying to be different for the sake of trying to be different.
Luc Verhaegen.
Luc wrote:
Also when nvramtool detects invalid values, it can offer to program defaults or at least refuse to validate the checksum. nvramtool should be able to write anything though, through the use of some argument like --force. It should also be able to just set up everything by default.
I think this is the fix we want. Forget the other stuff.
Stefan Reinauer wrote:
Also when nvramtool detects invalid values, it can offer to program defaults or at least refuse to validate the checksum. nvramtool should be able to write anything though, through the use of some argument like --force. It should also be able to just set up everything by default.
I think this is the fix we want. Forget the other stuff.
I agree it is part of what we want, but I think it is also pretty important for coreboot to gracefully ignore what appears to be a cluttered NVRAM, without making any changes to it.
//Peter
Stefan Reinauer wrote:
Also when nvramtool detects invalid values, it can offer to program defaults or at least refuse to validate the checksum. nvramtool should be able to write anything though, through the use of some argument like --force. It should also be able to just set up everything by default.
I think this is the fix we want. Forget the other stuff.
I agree it is part of what we want, but I think it is also pretty important for coreboot to gracefully ignore what appears to be a cluttered NVRAM, without making any changes to it.
That's a one liner not to clear cmos in case the checksum is wrong.
On Tue, Jun 02, 2009 at 06:49:10PM +0200, Luc Verhaegen wrote:
On Tue, Jun 02, 2009 at 06:45:28PM +0200, Luc Verhaegen wrote:
How would you want to store information, and how would you want to handle parsing of this information? Do you really want to store configuration in xml and include an xml parser in a bios? Do you want to invent your own structured information storage system? If so, what is wrong with what we do now, especially, since we do not have to reprogram part of our flash every 5 seconds?
I would place a configuration file in flash (a simple one). I would not use xml. I do not believe it is realistic that users will change bootup config setting every 5 seconds.
I cannot help but feel that this is just trying to be different for the sake of trying to be different.
Try the following:
- Boot your machine with a factory bios and read the flash into a file.
- poweroff the machine, remove (or add) a pci device, bootup, and read the flash to a new file
- compare the files.
I think you'll find (as I did on my machine) that the two flash images are different.
I'm suggesting to be different, for the sake of being better. The existing factory bios developers ran out of space in cmos and so they use both cmos and flash. I think 200 bytes of storage is not enough space - and having two places to store things is silly. Why use 200 bytes and 250000 bytes when you can just use the 250000 bytes?
-Kevin
On Fri, May 29, 2009 at 04:14:07PM +0200, Luc Verhaegen wrote:
On Fri, May 29, 2009 at 03:48:09PM +0200, Peter Stuge wrote:
Acked-by: Peter Stuge peter@stuge.se
That was mighty fast :)
Since this changes "known" behaviour of cmos options, i would prefer at least a second Ack though, especially since the above reasoning might not be agreed with fully by all.
Since no has provided a secondary ack since friday, i have committed this anyway.
-> r4332.
Luc Verhaegen.
-
Kevin O'Connor -
Luc Verhaegen -
Peter Stuge -
ron minnich -
Stefan Reinauer