[coreboot] inline asm peculiarities in v3
rminnich at gmail.com
Fri Sep 5 05:33:22 CEST 2008
On Thu, Sep 4, 2008 at 6:50 PM, Carl-Daniel Hailfinger
<c-d.hailfinger.devel.2006 at 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 at gmx.net> wrote:
>>> 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 :-)
>>>> __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.
>>>> static void disable_cache_as_ram_bsp(void)
>>>> __asm__ volatile (
>>>> // "pushl %eax\n\t"
>>>> "pushl %edx\n\t"
>>>> "pushl %ecx\n\t"
>>>> __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 :-)
More information about the coreboot