See patch.
abuilt-tested.
Uwe.
Uwe Hermann wrote:
Factor out common CAR asm snippets.
This makes the CAR implementations a lot more readable, shorter and easier to follow, and also reduces the amount of uselessly duplicated code.
For example there are more than 12 open-coded "enable cache" instances spread all over the place (and 12 "disable cache" ones), multiple "enable mtrr", "save BIST", "restore BIST", etc. etc.
Signed-off-by: Uwe Hermann uwe@hermann-uwe.de
Nice! I would kill some blank lines too, but this is a really nice improvement either way!
Acked-by: Peter Stuge peter@stuge.se
This was surprising:
+ * Copyright (C) 2000,2007 Ronald G. Minnich rminnich@gmail.com + * Copyright (C) 2007-2008 coresystems GmbH
The macros are well named. I'd prefer including comments only for ones that need an explanation.
+/* Disable cache. */ +#define disable_cache() \ + movl %cr0, %eax; \ + orl $(1 << 30), %eax; \ + movl %eax, %cr0 +
Thanks, Myles
On Fri, Oct 01, 2010 at 03:46:31PM -0600, Myles Watson wrote:
This was surprising:
- Copyright (C) 2000,2007 Ronald G. Minnich rminnich@gmail.com
- Copyright (C) 2007-2008 coresystems GmbH
Yeah, I copied most of the stuff from model_6ex or something like that, hence the (C) lines, but it's all generic simple stuff, maybe I should have made it (C) Uwe Hermann uwe@hermann-uwe.de ?
The macros are well named. I'd prefer including comments only for ones that need an explanation.
I'll extend the comments a bit more later, wanted to leave that for another patch.
Uwe.
On Fri, Oct 01, 2010 at 03:46:31PM -0600, Myles Watson wrote:
This was surprising:
- Copyright (C) 2000,2007 Ronald G. Minnich rminnich@gmail.com
- Copyright (C) 2007-2008 coresystems GmbH
Yeah, I copied most of the stuff from model_6ex or something like that, hence the (C) lines, but it's all generic simple stuff, maybe I should have made it (C) Uwe Hermann uwe@hermann-uwe.de ?
That would work for me. I also wouldn't have been surprised to see
(C) 2000 Ronald G. Minnich rminnich@gmail.com, or whoever the original author of the CAR code was.
I doubt those snippets have changed a lot over the years.
Thanks, Myles
On Fri, Oct 1, 2010 at 3:34 PM, Myles Watson mylesgw@gmail.com wrote:
(C) 2000 Ronald G. Minnich rminnich@gmail.com, or whoever the original author of the CAR code was.
Would that I were that smart :-)
the real guy who gets the credit is Eswar and it would be nice to see his name preserved in there somehow. He really did us a huge favor when he showed us How It Is Done.
ron
On 10/1/10 11:27 PM, Uwe Hermann wrote:
See patch.
abuilt-tested.
Uwe.
IMHO the patch makes it really hard to actually see what the code does. I think Idwer was recently running into a problem where the use of post_code would trash %eax were hard to comprehend. Now we seem have a lot more code that comes from somewhere else and touches registers without it being obvious to the reader of the code.
Just my 2ct.
Stefan
IMHO the patch makes it really hard to actually see what the code does. I think Idwer was recently running into a problem where the use of post_code would trash %eax were hard to comprehend. Now we seem have a lot more code that comes from somewhere else and touches registers without it being obvious to the reader of the code.
I agree for Save/Restore BIST, since it's only one line. I like the macros for longer snippets, though. Would a comment like
/* Clobbers %eax, %edx, ... */
be the right way to fix it? Would it be better to have the clobber list in the name?
disable_sse_with_eax()?
Thanks, Myles
Stefan Reinauer wrote:
IMHO the patch makes it really hard to actually see what the code does.
I think this is the same argument as the one against pci_cfg8_set() and similar functions which would gather read+modify+write into one step.
I disagree that it's a problem. I think that anyone looking at this code should be able to remember e.g. that the bist is saved in ebp, or that enabling the cache clobbers eax..
I think it is far more valuable if this code can be slightly more high level, shorter, and much more heterogenous.
Also, apropos shorter, this code should not have to keep a lot of state across many operations, which IMO reduces the problem.
Now we seem have a lot more code that comes from somewhere else and touches registers without it being obvious to the reader of the code.
We could make a strict policy that all macros used in any CAR code must come from car.h.
//Peter
On Sat, Oct 02, 2010 at 12:09:31AM +0200, Stefan Reinauer wrote:
On 10/1/10 11:27 PM, Uwe Hermann wrote:
See patch.
abuilt-tested.
Uwe.
IMHO the patch makes it really hard to actually see what the code does. I think Idwer was recently running into a problem where the use of post_code would trash %eax were hard to comprehend. Now we seem have a lot more code that comes from somewhere else and touches registers without it being obvious to the reader of the code.
Just my 2ct.
Well, lets make it 4cts.
I think the code is easier to read when it is all right there. The CAR code is doing something very complex - trying to abstract out a few minor operations I think could further obscure what is occurring.
-Kevin
On Sat, Oct 2, 2010 at 9:43 AM, Kevin O'Connor kevin@koconnor.net wrote:
I think the code is easier to read when it is all right there. The CAR code is doing something very complex - trying to abstract out a few minor operations I think could further obscure what is occurring.
I agree with Kevin on this one. The CAR code is basically write-once. It changes almost never. Rules about common code don't really apply quite so much.
I find the non-abstracted version much easier to understand.
ron
ron minnich wrote:
I find the non-abstracted version much easier to understand.
FWIW I think write-once makes it even nicer to have high-level operations in source code. Since it will change less often (ie. be less well known) it's a good thing to not need to see every detail the next time it is looked at.
It seems me and Uwe just can not win this. Too bad.
//Peter
I think that making arguments against this code on grounds that it doesn't feel like other common code is a little shortsighted. When bugs are fixed in one bit of code, we should be able to take advantage of those in other parts of code. For instance, I just had to copy/paste some CPP logic to fix the via/vt8454c CAR init due to a tiny bootblock. That would have already been fixed had the via and amd logic been unified since they are basically identical bits of code.
Thanks, wt
On Sat, Oct 2, 2010 at 12:10 PM, Peter Stuge peter@stuge.se wrote:
ron minnich wrote:
I find the non-abstracted version much easier to understand.
FWIW I think write-once makes it even nicer to have high-level operations in source code. Since it will change less often (ie. be less well known) it's a good thing to not need to see every detail the next time it is looked at.
It seems me and Uwe just can not win this. Too bad.
//Peter
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
On Sat, Oct 2, 2010 at 12:27 PM, Warren Turkal wt@penguintechs.org wrote:
I think that making arguments against this code on grounds that it doesn't feel like other common code is a little shortsighted. When bugs are fixed in one bit of code, we should be able to take advantage of those in other parts of code. For instance, I just had to copy/paste some CPP logic to fix the via/vt8454c CAR init due to a tiny bootblock. That would have already been fixed had the via and amd logic been unified since they are basically identical bits of code.
well, you have to be careful. We've had more than one problem with "common" code that was not so common. CAR is so tricky, and one platform changes may break another platform in ways we don't expect. CAR, in fact, scares me. It's fragile. I'm particularly concerned about the idea of unifying via and amd code. The changes some well-intentioned people made years ago to some via northbridge code to "clean it up" -- it broke many things -- are still fresh in my mind.
I contribute very little code at this point, really zero in the last year, so I don't believe my opinions should be taken that seriously :-) I'm really more than anything recommending you all to be very careful. Anything you do to CAR needs to be extensively tested.
So let's call it my .01, not my .02 :-)
ron
On Sat, Oct 02, 2010 at 12:42:51PM -0700, ron minnich wrote:
On Sat, Oct 2, 2010 at 12:27 PM, Warren Turkal wt@penguintechs.org wrote:
I think that making arguments against this code on grounds that it doesn't feel like other common code is a little shortsighted. When bugs are fixed in one bit of code, we should be able to take advantage of those in other parts of code. For instance, I just had to copy/paste some CPP logic to fix the via/vt8454c CAR init due to a tiny bootblock. That would have already been fixed had the via and amd logic been unified since they are basically identical bits of code.
well, you have to be careful. We've had more than one problem with "common" code that was not so common. CAR is so tricky, and one platform changes may break another platform in ways we don't expect. CAR, in fact, scares me.
Yes, it scares me too (well, sort of :) It's one of the most complex and non-obvious pieces of code we have in coreboot. Which makes it even more important to make this code as clean and easily readable and understandable as we can. Every little bit helps here, e.g. not open-coding various asm snippets (in 3 or more slightly different variants) all over the place is one very good measure to make it less confusing for people trying to read the code. Less lines in cache_as_ram.inc is good. Readable macros such as "enable_l2_cache" instead of some open-coded, harder to understand assembler instructions is good.
High-level "look, here we enable cache"-style code is much better than "look, here we set bit 2 in CR0, and clear bits 5-6 in some MSR and write magic value 456 to magic address 123, now please go find the right datasheet and look up what it actually does".
Of course I'm exagerrating a bit here, but I'm very convinced that readability is of utmost importance in coreboot code, especially so in CAR code.
about the idea of unifying via and amd code.
Yeah, that should be considered very carefully indeed, and only be done after sufficient testing on hardware (if it's feasible and makes sense to do it al all, i.e., if the two implementations are >= 90% similar).
But the general idea of unifying similar code (especially if it stems from copy-pasted code from elsewhere with only minimal customizations) is very good and very necessary indeed. Yeah, things may break, that's life. Someone will test it, we'll fix it, problem solved. No reason to _not_ unify code which can be reasonably unified.
I'm actually planning to unify model_6ex, model_6fx, and model_106cx CAR implementations myself, but that can be done relatively easily, as the diff between all three files is just 2-3 simple lines.
Uwe.
On Sat, Oct 02, 2010 at 10:21:38PM +0200, Uwe Hermann wrote:
Yes, it scares me too (well, sort of :) It's one of the most complex and non-obvious pieces of code we have in coreboot. Which makes it even more important to make this code as clean and easily readable and understandable as we can. Every little bit helps here, e.g. not open-coding various asm snippets (in 3 or more slightly different variants) all over the place is one very good measure to make it less confusing for people trying to read the code. Less lines in cache_as_ram.inc is good. Readable macros such as "enable_l2_cache" instead of some open-coded, harder to understand assembler instructions is good.
I think the three lines of assembler is easier to understand than "enable_l2_cache". Assembler isn't C - the macros defined aren't free abstractions. (In particular, it's not clear they clobber %eax.)
Again, just my 2ct.
High-level "look, here we enable cache"-style code is much better than "look, here we set bit 2 in CR0, and clear bits 5-6 in some MSR and write magic value 456 to magic address 123, now please go find the right datasheet and look up what it actually does".
I think the bit definitions, msr addresses, port numbers, and special addresses should use definitions. For an example of this from seabios, see:
http://git.linuxtogo.org/?p=kevin/seabios.git;a=blob;f=src/romlayout.S;h=a46...
-Kevin
On Sat, Oct 2, 2010 at 4:07 PM, Kevin O'Connor kevin@koconnor.net wrote:
I think the three lines of assembler is easier to understand than "enable_l2_cache". Assembler isn't C - the macros defined aren't free abstractions. (In particular, it's not clear they clobber %eax.)
I definitely agree that clobbering eax is not apparent from the macros. Maybe we need a documented calling convention that says the eax register can be destroyed at this point in the coreboot code?
I think the bit definitions, msr addresses, port numbers, and special addresses should use definitions. For an example of this from seabios, see:
This seems reasonable.
wt
2010/10/2 Stefan Reinauer stefan.reinauer@coresystems.de
On 10/1/10 11:27 PM, Uwe Hermann wrote:
See patch.
abuilt-tested.
Uwe.
IMHO the patch makes it really hard to actually see what the code does. I think Idwer was recently running into a problem where the use of post_code would trash %eax were hard to comprehend. Now we seem have a lot more code that comes from somewhere else and touches registers without it being obvious to the reader of the code.
Uwe pointed me to this errata: http://www.intel.com/Assets/PDF/specupdate/253176.pdf
Is processor init (with or without CAR but preferably with) still possible ?
The CPU that's used on the board is either a "N = Intel ® Pentium ® 4 processor" or a "R = Intel ® Pentium ® 4 processor on 90 nm process", cpuid is 0xf29.
Just my 2ct.
Stefan
Idwer
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
For line 31 and 37 of src/include/x86/car.h, does cache mean L1 cache? If so, it might be nice to make the macros say l1 explicitly. Also, I agree with Myles on not having redundant comments.
It might also be nice for the comments before each macro to indicate which registers get trashed within the macro. Maybe even make the register name a parameter of the macro, though I'm not sure if that would make sense.
Also, {en,dis}able mtrr should document which mtrr is getting {en,dis}abled as it looks like a specific one and not all of them. The name of those macros should probably reflect which mtrr as well.
Thanks, wt
On Fri, Oct 1, 2010 at 2:27 PM, Uwe Hermann uwe@hermann-uwe.de wrote:
See patch.
abuilt-tested.
Uwe.
http://hermann-uwe.de | http://sigrok.org http://randomprojects.org | http://unmaintained-free-software.org
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
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
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
Alternatively, we could say that all "function" calls in pre-CAR state destroy all registers as a kind of pseudo-calling convention.
You cant you need to save BIST somewhere.
Please can we leave the only assembly code in coreboot alone? I think there are other places which needs to get improved.
Like for example review the SB700 code and check if everything is ok. I started to fix the Windows7 boot and immediately lot of skeletons fall off the closet (yes the little bug in SB700 mentioned in another thread is small example) I don't want to hijack this thread with details but a lot of time could be invested to do the code review with datasheet at hand to see if the code is doing what is written in the comments and programming requirements. Which I think is more needed than fixing code which works.
Small examples:
/* Features Enable */ pci_write_config32(dev, 0x64, 0x829E79BF); /* bit10: Enables the HPET interrupt. */
Oh yeah we enable SMI for USB and USB IR12 stuff strange huh?
->/* Don't rename APIC ID */ <------>/* TODO: We should call setup_ioapic() here. But kernel hangs if cpu is K8. <------> * We need to check out why and change back. */ <------>clear_ioapic(ioapic_base);
it talks for itself...
<------>/* IRQ0From8254 */ <------>byte = pci_read_config8(dev, 0x41); <------>byte &= ~(1 << 7); <------>pci_write_config8(dev, 0x41, byte);
Actually anyone idea about this? Not found in datasheet.
---->/* rrg:K8 INTR Enable (BIOS should set this bit after PIC initialization) */ Oh yeah we should init i8259....
Please note that this is just little drop while I started to LOOK into this, probably it is much worse. Lets get to something more productive shall we?
Thanks, Rudolf
-
Idwer Vollering -
Kevin O'Connor -
Myles Watson -
Peter Stuge -
ron minnich -
Rudolf Marek -
Stefan Reinauer -
Uwe Hermann -
Warren Turkal