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@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@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot