On Thu, Sep 4, 2008 at 6:50 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
On 04.09.2008 17:46, ron minnich wrote:
On Thu, Sep 4, 2008 at 8:32 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Hi,
I decided to look at all cases of inline asm in our v3 code and I found a few bits which either work by accident or need better documentation.
We use __asm__ and asm, __volatile__ and volatile. Can we please decide which one we want, then I'll switch the tree over.
somebody who knows better than me the implications can comment.
The gcc info page states that __asm__ and asm are equivalent, but if you use -ansi or -fno-asm or -fno-gnu-keywords in your CFLAGS only __asm__ will be recognized. asm is shorter, so I'd prefer asm. __volatile__ vs. volatile is not explained in the gcc docs at all, but various places on the net state that they're identical.
you definitely want a volatile in there, else the various tools agree to helpfully optimize the code. I've seen it completely eliminate __asm__ that was not protect with a __volatile__. As to the use of __ vs no __, I'm not picky. We're probably locked into gcc so this particular gcc-ism is not going to cause me to sleep badly :-)
northbridge/amd/geodelx/vsmsetup.c:143
__asm__(".text\n" "real_mode_switch_end:\n"); extern char real_mode_switch_end[];
AFAICS the compiler and linker are free to place the resulting code anywhere in the binary independent of each other.
Not a problem. The "extern" is simply making the label available to C. They don't depend on any trickiness.
My point was about the label. Sorry for being unclear. That label can be placed anywhere, so I can't see it serving any purpose.
I don't remember why it is done the way it is. But we can test a change easily, so no problem to try a change.
include/arch/x86/amd/k8/k8.h:746
static void disable_cache_as_ram_bsp(void) { __asm__ volatile ( // "pushl %eax\n\t" "pushl %edx\n\t" "pushl %ecx\n\t" );
disable_cache_as_ram(); __asm__ volatile ( "popl %ecx\n\t" "popl %edx\n\t"// "popl %eax\n\t" ); }
The pushl and popl instructions seem to serve no real purpose. Kill them?
My rule with the Clever v2 code is not to modify it until I understand it. So I did not touch these because I don't know what function they serve. That's one reason that I am bringing code over and leaving it ugly to start. Let's leave this alone until K8 is working.
But can I add a comment that the current code seems nonsensical?
absolutely. See all the #warnings you get when you build k8 :-)
ron