[coreboot] [Patch] CMOS: Add set_option and rework get_option.

Luc Verhaegen libv at skynet.be
Fri May 29 16:14:07 CEST 2009


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 at 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 at skynet.be>
> 
> Acked-by: Peter Stuge <peter at 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.




More information about the coreboot mailing list