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 <[email protected]> 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: [email protected] > http://www.coreboot.org/mailman/listinfo/coreboot > -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
