[coreboot] msr_set(), msr_clear() and friends

[Peter Stuge]

On Thu, Apr 24, 2008 at 10:09:38AM -0600, Marc Jones wrote:
> Peter Stuge wrote:
> > Maybe add some functions? Or macro?
> >
> > void msr_set(const unsigned index, const msr_t set) {
> > void msr_set_lo(const unsigned index, const unsigned set_lo) {
> > void msr_set_hi(const unsigned index, const unsigned set_hi) {
> > 
> > void msr_clear(const unsigned index, const msr_t clear) {
> > void msr_clear_lo(const unsigned index, const unsigned clear_lo) {
> > void msr_clear_hi(const unsigned index, const unsigned clear_hi) {
> >
> > I can make a patch, msr.h right?
> 
> Personally, I prefer seeing the & and | values inline so I know
> exactly what happened as I read the code. Not having to remember if
> the function ~ the mask,

_clear() would clear bits set both in hardware MSR and the input MSR.

My motivation is to make code easier and thus faster to read and
write by using function names and operations that are easily
understood in a spoken language. (set and clear in this case)

No doubt anyone using them must still understand what &= and |= do,
but there's no point in reading or writing more code than is
neccessary IMO. (Not everyone agrees.)


> BUT I could be convinced that the functions are the right way to
> go.

Great! :)


> I think msr.h would be the correct place.

Hm.


> Would you do a massive search and replace on the entire v2 tree?
> That would be a lot of changes.

But a mighty good one I think. :)


> Maybe start in the v3 tree?

Yep, that's an excellent idea.


On Thu, Apr 24, 2008 at 10:21:53AM -0600, Jordan Crouse wrote:
> I agree with Marc - the readmsr / modify / writemsr cycle inline is
> far easier to understand and debug.

Even though it is 3+ lines per operation and always requires a
local variable in whatever scope one might be when wanting to
touch an MSR? :\

Maybe you're jaded by now(?) but I really like putting common things
like this (and this is quite common, plus I expect it to become even
more common in the future) in a single place.

Another argument is that it eliminates the risk of a missing & or |
causing problems. And did I mention that less typing is needed?


> See the Geode X driver code for multiple examples of MSR functions
> gone silly.

I've never seen the code before, grep -ri msr returns every single
line of code in the driver (no, but many) and {cim/cim,geode}_msr.c
actually looked pretty good to me. Where are the mistakes so I can
learn from them?


On Thu, Apr 24, 2008 at 09:26:36AM -0700, ron minnich wrote:
> I'm with Jordan and Marc on this one.

I seem to be going uphill, but I do think this is, and will be,
important.

If everyone will stand fast I will not waste time, but I would like
the change.

Maybe v3 is the perfect sandbox?


> Also, we might want to stop dropping code in .h files.

Yes, agreed. What is a better place? Create arch/i386/lib/msr.c ?


> Unless you've really measured it, we can not assume that
> inline/macro is somehow always better than a function call.
> People have gone out of control with inlines in other projects :-)

I have believed that the inline keyword means that the compiler will
not make a function of the code but will replicate the function
contents at every call site. Conversely I assumed that functions
were not replicated otherwise.

Seems I am already old school. :p

I prefer functions over macros, but on rare occasions I've done nice
syntax tricks in macros that just were not possible with functions.
Not neccessary here though, so I favor functions and let others
decide if they should be inline or not.


//Peter