Patchwork cld before call

login
register
about
Submitter Robert Millan
Date 2009-07-28 22:36:27
Message ID <20090728223627.GA4592@thorin>
Download mbox | patch
Permalink /patch/82/
State Accepted
Headers show

Comments

Robert Millan - 2009-07-28 22:36:27
Hi,

The Multiboot spec is a bit too permissive about this, as it doesn't
specify the state of direction flag when payload is called.  Some payloads
(we found this in ReactOS FreeLDR) assume it is cleared, and fail otherwise.

We adjusted GRUB to be sure it's always cleared.  I think coreboot should do
the same.  Here's a patch for v3.
Robert Millan - 2009-07-28 22:40:07
On Wed, Jul 29, 2009 at 12:36:27AM +0200, Robert Millan wrote:
> Here's a patch for v3.

I just checked, v2 does this already.
Carl-Daniel Hailfinger - 2009-07-29 00:21:38
On 29.07.2009 00:36, Robert Millan wrote:
> The Multiboot spec is a bit too permissive about this, as it doesn't
> specify the state of direction flag when payload is called.  Some payloads
> (we found this in ReactOS FreeLDR) assume it is cleared, and fail otherwise.
>
> We adjusted GRUB to be sure it's always cleared.  I think coreboot should do
> the same.  Here's a patch for v3.
>   

AFAIK this patch is unneeded. The GCC ABI on x86 and x86_64 states that
the direction flag must always be cleared before running any gcc
compiled code, so if you need to execute cld in the middle of C code, it
means that all C code before that cld had undefined behaviour.

The correct place for cld is early asm startup code before any C code
gets executed.

Regards,
Carl-Daniel
Robert Millan - 2009-07-29 00:44:52
On Wed, Jul 29, 2009 at 02:21:38AM +0200, Carl-Daniel Hailfinger wrote:
> On 29.07.2009 00:36, Robert Millan wrote:
> > The Multiboot spec is a bit too permissive about this, as it doesn't
> > specify the state of direction flag when payload is called.  Some payloads
> > (we found this in ReactOS FreeLDR) assume it is cleared, and fail otherwise.
> >
> > We adjusted GRUB to be sure it's always cleared.  I think coreboot should do
> > the same.  Here's a patch for v3.
> >   
> 
> AFAIK this patch is unneeded. The GCC ABI on x86 and x86_64 states that
> the direction flag must always be cleared before running any gcc
> compiled code, so if you need to execute cld in the middle of C code, it
> means that all C code before that cld had undefined behaviour.
> 
> The correct place for cld is early asm startup code before any C code
> gets executed.

Isn't C code allowed to set it back?
ron minnich - 2009-07-29 03:24:58
On Tue, Jul 28, 2009 at 5:44 PM, Robert Millan<rmh@aybabtu.com> wrote:

> Isn't C code allowed to set it back?

my understanding of all this is:
1. if you want it set a certain way, you have to set it
2. don't ever assume it has any particular value.

ron
Carl-Daniel Hailfinger - 2009-07-29 10:51:16
Hi Segher,

a small gcc related question for x86/x86_64 if I may.

On 29.07.2009 05:24, ron minnich wrote:
> On Tue, Jul 28, 2009 at 5:44 PM, Robert Millan<rmh@aybabtu.com> wrote:
>
>   
>> Isn't C code allowed to set it back?
>>     
>
> my understanding of all this is:
> 1. if you want it set a certain way, you have to set it
> 2. don't ever assume it has any particular value.
>   

There is an excellent LWN.net article about this:
http://lwn.net/Articles/272048/

"GCC relies on the ABI mandate that the direction flag is cleared before
entry to a function"

That means if you call the payload via a pointer, gcc will make sure the
direction flag is cleared (either by clearing it directly before the
call or by never changing it after the start of the program).

AFAIK calling a function via inline asm is not something gcc can see, so
this special case might be an exception. Then again, it's pretty
unlikely that gcc sets the direction flag for some operations and defers
clearing to the last possible moment.

Regards,
Carl-Daniel
Carl-Daniel Hailfinger - 2009-07-29 11:08:55
On 29.07.2009 12:51, Carl-Daniel Hailfinger wrote:
> On 29.07.2009 05:24, ron minnich wrote:
>   
>> my understanding of all this is:
>> 1. if you want it set a certain way, you have to set it
>> 2. don't ever assume it has any particular value.
>>   
>>     
>
> There is an excellent LWN.net article about this:
> http://lwn.net/Articles/272048/
>
> "GCC relies on the ABI mandate that the direction flag is cleared before
> entry to a function"
>
> That means if you call the payload via a pointer, gcc will make sure the
> direction flag is cleared (either by clearing it directly before the
> call or by never changing it after the start of the program).
>
> AFAIK calling a function via inline asm is not something gcc can see, so
> this special case might be an exception. Then again, it's pretty
> unlikely that gcc sets the direction flag for some operations and defers
> clearing to the last possible moment.
>   

Turns out gcc developers disagreed with each other about direction flag
guarantees for inline asm. The following is an attempt at summarizing
the discussion on the gcc mailing list linked from the lwn article.

Michael Matz said there are no guarantees for inline asm, but he also
said that std is not emitted lazily, so having cld inside inline asm
will not break any code afterwards, i.e. code after an inline asm does
never depend on the DF being set.
Chris Lattner said it's reasonable for inline asm to expect the DF to be
cleared.
Richard Guenther said inline asm shouldn't make assumptions about DF.
Jan Hubicka said gcc does not emit std at the moment.

Anyone up for reposting that question to the gcc list?

Regards,
Carl-Daniel
Segher Boessenkool - 2009-07-30 01:58:33
Okay, I understand the question now, I hope.

>>> my understanding of all this is:
>>> 1. if you want it set a certain way, you have to set it

Yes.

>>> 2. don't ever assume it has any particular value.

For inline asm?  Yes.

The ABI says that DF=0 whenever a function is called.  For GCC, this
means that it will make sure it is like that whenever it calls some
(external) function; and (in newer GCC versions) it assumes all  
functions
are called with DF=0, so it doesn't have to use the cld insn that often,
which is a good thing because it is quite expensive.

Now, _within_ a function GCC can do as it bloody well pleases.  This
includes functions that are inlined etc.; the ABI only applies to
externally visible functions.

>> "GCC relies on the ABI mandate that the direction flag is cleared  
>> before
>> entry to a function"
>>
>> That means if you call the payload via a pointer, gcc will make  
>> sure the
>> direction flag is cleared (either by clearing it directly before the
>> call or by never changing it after the start of the program).

Yes.

>> AFAIK calling a function via inline asm is not something gcc can see,

Indeed.  It usually isn't such a great thing to do either; you will have
to take care of saving all regs etc. yourself, keeping the stack  
balanced,
all that.  Often you get much nicer code if you write this in actual
(not inline) asm, that you call with the usual calling conventions.

>> so
>> this special case might be an exception. Then again, it's pretty
>> unlikely that gcc sets the direction flag for some operations and  
>> defers
>> clearing to the last possible moment.

You do not want to rely on what GCC does if it doesn't guarantee that
behaviour.

> Turns out gcc developers disagreed with each other about direction  
> flag
> guarantees for inline asm.

Not really.  There are two things: what GCC does right now, and what
should be the guaranteed behaviour (if any).

> Michael Matz said there are no guarantees for inline asm, but he also
> said that std is not emitted lazily, so having cld inside inline asm
> will not break any code afterwards, i.e. code after an inline asm does
> never depend on the DF being set.
> Chris Lattner said it's reasonable for inline asm to expect the DF  
> to be
> cleared.
> Richard Guenther said inline asm shouldn't make assumptions about DF.
> Jan Hubicka said gcc does not emit std at the moment.

The safest way to write inline asm that changes the direction flag is
to push eflags, change the flag, do your thing, and finally pop eflags.
This should work no matter what, and you are crazy if you care about
the performance or tiny code size of this ;-)

Or, write an assembler stub for this, and all your problems magically
go away.


Segher
Carl-Daniel Hailfinger - 2009-08-01 01:55:20
On 29.07.2009 00:36, Robert Millan wrote:
> The Multiboot spec is a bit too permissive about this, as it doesn't
> specify the state of direction flag when payload is called.  Some payloads
> (we found this in ReactOS FreeLDR) assume it is cleared, and fail otherwise.
>
> We adjusted GRUB to be sure it's always cleared.  I think coreboot should do
> the same.  Here's a patch for v3.
>
> Signed-off-by:  Robert Millan <rmh.coreboot@aybabtu.com>
>   

Thanks to the explanation provided by Segher, I now know your patch is
the right thing to do.
Robert, thanks for digging this up.

Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>

Regards,
Carl-Daniel
Robert Millan - 2009-08-01 14:14:12
On Sat, Aug 01, 2009 at 03:55:20AM +0200, Carl-Daniel Hailfinger wrote:
> On 29.07.2009 00:36, Robert Millan wrote:
> > The Multiboot spec is a bit too permissive about this, as it doesn't
> > specify the state of direction flag when payload is called.  Some payloads
> > (we found this in ReactOS FreeLDR) assume it is cleared, and fail otherwise.
> >
> > We adjusted GRUB to be sure it's always cleared.  I think coreboot should do
> > the same.  Here's a patch for v3.
> >
> > Signed-off-by:  Robert Millan <rmh.coreboot@aybabtu.com>
> >   
> 
> Thanks to the explanation provided by Segher, I now know your patch is
> the right thing to do.
> Robert, thanks for digging this up.

You're welcome.

Patch


Signed-off-by:  Robert Millan <rmh.coreboot@aybabtu.com>

Index: arch/x86/stage1.c
===================================================================
--- arch/x86/stage1.c	(revision 1174)
+++ arch/x86/stage1.c	(working copy)
@@ -172,7 +172,7 @@ 
 static int run_address_multiboot(void *f, struct multiboot_info *mbi)
 {
 	int ret, dummy;
-	__asm__ __volatile__ ("call *%4" : "=a" (ret), "=c" (dummy) : "a" (MB_MAGIC2), "b" (mbi), "c" (f) : "edx", "memory");
+	__asm__ __volatile__ ("cld; call *%4" : "=a" (ret), "=c" (dummy) : "a" (MB_MAGIC2), "b" (mbi), "c" (f) : "edx", "memory");
 	return ret;
 }