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.
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.
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?
The K8 CAR disabling has inline asm depending on good compiler behaviour and luck. Patch will be sent separately.
Regards, Carl-Daniel
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.
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.
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.
The K8 CAR disabling has inline asm depending on good compiler behaviour and luck. Patch will be sent separately.
Thank you.
ron
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.
Segher confirmed these findings. I'll send a patch to move everything to the short syntax.
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.
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?
The K8 CAR disabling has inline asm depending on good compiler behaviour and luck. Patch will be sent separately.
Thank you.
You're welcome. See the mail with subject "[PATCH] v3: correct K8 stack preservation asm".
Regards, Carl-Daniel
By the way, we have quite a few asm statements which have incorrect clobber constraints. That could lead to all sorts of mayhem if gcc becomes more clever.
Oh, and at least for geodelx we break gcc ABI expectations by not clearing the direction flag in stage0 asm.
I'll send patches in a few days unless someone else wants to tackle this.
Regards, Carl-Daniel
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
Carl-Daniel Hailfinger wrote:
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?
But it seems not because we know what gcc is doing and its doing the right thing.
-
Carl-Daniel Hailfinger -
ron minnich -
Stefan Reinauer