[coreboot] inline asm peculiarities in v3

ron minnich 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:
>>
>>> 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




More information about the coreboot mailing list