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

[Warren Turkal]

Maybe instead of making it look like a function like:
enable_cache()
it should just look like a normal macro like:
ENABLE_CACHE_ASM

Maybe that would make it more obvious that it's not a function.

Alternatively, we could say that all "function" calls in pre-CAR state
destroy all registers as a kind of pseudo-calling convention.

wt

On Sun, Oct 3, 2010 at 10:17 AM, ron minnich <rminnich at gmail.com> wrote:
> 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
>
> --
> coreboot mailing list: coreboot at coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot
>