[coreboot] [PATCH] Factor out common CAR asm snippets

[ron minnich]

OK, I've thought about why this patch makes me so uncomfortable. See
if I make sense.


The patch takes sequences like this:
-	/* Enable cache. */
-	movl	%cr0, %eax
-	andl	$(~((1 << 30) | (1 << 29))), %eax
-	movl	%eax, %cr0

and replaces them with this:

+	enable_cache()

Seems ok, right? Let's think about some rules in C:

int eax, ebx, ecx, edx;
eax = 1;
ebx = 2;
enable_cache();
ecx = eax;

Now, as we all know, in C, that function call won't affect those
variables. ecx will have a value of 2. We have a stack and there are
rules in C. So the variables are safe, and we're all used to thinking
that way. We think that way, in fact, by habit, even when looking at
assembly code. The syntax of the proposed patch makes it look very
much like a side-effect-free function call. That's one worry I have
all ready: it looks like a function, but it's not a function.

In other words, in C, local variables don't affect what goes on in the
function, and the function does not affect what goes on in local
variables. You don't have to go read the code for the function; that's
the whole point of a function. Functions provide a side-effect-free
way to get some operation done.

How different this is from assembly macros!

movl $2, %eax
enable_cache()
movl %eax, %ecx

If enable_cache is the function call it seems to be, %ecx will have 2.
As most of us know, it won't of course; it has some number with lots
of bits set, but it sure does not have 2.

Sure, we all know this; but what about a newcomer in 5 years; will
they know this?

So, what's the value of eax after the "call"? How can you know? Well,
you have to go read that code. You have to find the include file. You
have to have intimate understanding of the function, to make sure it
does not affect your "local variables" -- your registers. That's not a
function. The usage makes it look like a function, but it's not a
function. I think this practice is going to cause trouble.

So what are your options here? For each of the "functions", you can
track down the file, read the code, and make sure you understand it.
This is in my view *worse* than just having assembly code inline; you
have to hop around the tree but, more importantly, you have to
understand how the target is built, to make sure you're understanding
what's going on.

But wait! it gets worse! Just because you understand the file now
doesn't mean it won't change. You have to future-proof your code,
because, 5 or 10 years from now (some of the code is that old!), for
some new machine, somebody might make a change to enable_cache(), and
use one extra register, and break one target, and it might not be
noticed for a year. Yes, this sort of thing happens; it has happened
to me several times on different projects.

But wait! it gets worse! What if you are the person who wrote the
enable_cache() "function"? Then, if you ever need to change it, you
have to find each and every file that uses that function, and make
quite sure you understand the assembly code that surrounds the
"function", and make sure you won't break it. Now that's a lot of
work!

What if the function needs an extra parameter in a register due to
some new architecture? If that ever happens, and you need to add a new
register to the "function", you're going to have to once again find
all uses of it and fix all that code. Is it possible that in that
process errors might creep in? It's actually a certainty. Note that
we're build-testing, not boot-testing, a lot of these changes; that's
what broke one of my VIA boards -- permanently.

There is one way to future-proof the calls to these "functions". The
only safe assumption you can make after that "call" to disable_cache
is that all registers are dead: nothing safely carries across the
"call". Then I think you have less of a chance of a problem. That way,
one can make lots of changes to the functions and not have to track
down all uses to see if any are broken by it. Do you really want to do
this work? And are you sure that 5 years from now, new code authors
will be aware of that rule?

Just my $.015

ron