[PATCH 1/1] Speedfreq-SMI call clobbers ECX

[PATCH 1/1] Speedfreq-SMI call clobbers ECX

[Stephan Diestelhorst]

Dear Dave,
  I have found that using SMI to change the cpu's frequency on my DELL 
Latitude L400 clobbers the ECX register in speedstep_set_state, causing 
unneccessary retries because the "state" variable has changed silently (GCC 
assumes it is still present in ECX).

The patch is simple: Introduce temporary output "clobber_ecx" operand that 
consumes the clobbered value.

Cheers,
  Stephan

Signed-off: Stephan Diestelhorst <stephan.diestelhorst@gmail.com>

--

--- linux-2.6.24.3/arch/x86/kernel/cpu/cpufreq/speedstep-smi.c.orig	2008-02-26 
01:20:20.000000000 +0100
+++ linux-2.6.24.3/arch/x86/kernel/cpu/cpufreq/speedstep-smi.c	2008-03-05 
15:56:42.000000000 +0100
@@ -160,7 +160,7 @@ static int speedstep_get_state (void)
  */
 static void speedstep_set_state (unsigned int state)
 {
-	unsigned int result = 0, command, new_state;
+	unsigned int result = 0, command, new_state, ecx_clobber;
 	unsigned long flags;
 	unsigned int function=SET_SPEEDSTEP_STATE;
 	unsigned int retry = 0;
@@ -184,7 +184,7 @@ static void speedstep_set_state (unsigne
 		__asm__ __volatile__(
 			"movl $0, %%edi\n"
 			"out %%al, (%%dx)\n"
-			: "=b" (new_state), "=D" (result)
+			: "=b" (new_state), "=D" (result), "=c" (ecx_clobber)
 			: "a" (command), "b" (function), "c" (state), "d" (smi_port), "S" (0)
 			);
 	} while ((new_state != state) && (retry <= SMI_TRIES));
@@ -195,7 +195,7 @@ static void speedstep_set_state (unsigne
 	if (new_state == state) {
 		dprintk("change to %u MHz succeeded after %u tries with result %u\n", 
(speedstep_freqs[new_state].frequency / 1000), retry, result);
 	} else {
-		printk(KERN_ERR "cpufreq: change failed with new_state %u and result %u\n", 
new_state, result);
+		printk(KERN_ERR "cpufreq: change to state %u failed with new_state %u and 
result %u\n", state, new_state, result);
 	}
 
 	return;

Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

[Ingo Molnar]

* Stephan Diestelhorst <langer_mann@web.de> wrote:

> @@ -184,7 +184,7 @@ static void speedstep_set_state (unsigne
>  		__asm__ __volatile__(
>  			"movl $0, %%edi\n"
>  			"out %%al, (%%dx)\n"
> -			: "=b" (new_state), "=D" (result)
> +			: "=b" (new_state), "=D" (result), "=c" (ecx_clobber)
>  			: "a" (command), "b" (function), "c" (state), "d" (smi_port), "S" (0)
>  			);

stupid suggestion: why not do a pusha/popa around those instructions, to 
make sure everything is restored? This isnt a fastpath and being 
conservative about SMI side-effects cannot hurt ...

	Ingo

Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

[H. Peter Anvin]

Ingo Molnar wrote:
> * Stephan Diestelhorst <langer_mann@web.de> wrote:
> 
>> @@ -184,7 +184,7 @@ static void speedstep_set_state (unsigne
>>  		__asm__ __volatile__(
>>  			"movl $0, %%edi\n"
>>  			"out %%al, (%%dx)\n"
>> -			: "=b" (new_state), "=D" (result)
>> +			: "=b" (new_state), "=D" (result), "=c" (ecx_clobber)
>>  			: "a" (command), "b" (function), "c" (state), "d" (smi_port), "S" (0)
>>  			);
> 
> stupid suggestion: why not do a pusha/popa around those instructions, to 
> make sure everything is restored? This isnt a fastpath and being 
> conservative about SMI side-effects cannot hurt ...
> 

You can't pusha/popa if you expect a result.  You can, of course, push 
and pop individual registers.

It's also kind of odd to do "movl $0,%%edi" instead of just setting EDI 
as an input.

	-hpa

Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

[Stephan Diestelhorst]

On, March 5th 2008 16:35:20 Ingo Molnar wrote:
> * Stephan Diestelhorst <langer_mann@web.de> wrote:
> > @@ -184,7 +184,7 @@ static void speedstep_set_state (unsigne
> >  		__asm__ __volatile__(
> >  			"movl $0, %%edi\n"
> >  			"out %%al, (%%dx)\n"
> > -			: "=b" (new_state), "=D" (result)
> > +			: "=b" (new_state), "=D" (result), "=c" (ecx_clobber)
> >
> >  			: "a" (command), "b" (function), "c" (state), "d" (smi_port),
> >  			: "S" (0)
> >
> >  			);
>
> stupid suggestion: why not do a pusha/popa around those
> instructions, to make sure everything is restored? This isnt a
> fastpath and being conservative about SMI side-effects cannot hurt

That sounds like a sane thing to do to me. Should I provide a 'patch'? 
Or leave that (and the decision about it) to the maintainer?

Regards,
  Stephan

Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

[Stephan Diestelhorst]

> On, March 5th 2008 16:35:20 Ingo Molnar wrote:
> > * Stephan Diestelhorst <langer_mann@web.de> wrote:
> > > @@ -184,7 +184,7 @@ static void speedstep_set_state (unsigne
> > >  		__asm__ __volatile__(
> > >  			"movl $0, %%edi\n"
> > >  			"out %%al, (%%dx)\n"
> > > -			: "=b" (new_state), "=D" (result)
> > > +			: "=b" (new_state), "=D" (result), "=c" (ecx_clobber)
> > >
> > >  			: "a" (command), "b" (function), "c" (state), "d"
> > >  			: (smi_port), "S" (0)
> > >
> > >  			);
> >
> > stupid suggestion: why not do a pusha/popa around those
> > instructions, to make sure everything is restored? This isnt a
> > fastpath and being conservative about SMI side-effects cannot
> > hurt

Stephan Diestelhorst wrote:
> That sounds like a sane thing to do to me. Should I provide a
> 'patch'? Or leave that (and the decision about it) to the
> maintainer?

H. Peter Anvin wrote: 
> You can't pusha/popa if you expect a result.  You can, of course,
> push and pop individual registers.
>
> It's also kind of odd to do "movl $0,%%edi" instead of just setting
> EDI as an input.

Whoops, HPA is correct, of course. Manually pushing / popping the 
registers is ugly, how about a larger clobber-list? Let the compiler 
figure out what it wants to save/restore. Only thing to worry about 
is EBP then.

Again, should I provide these patches? This thing just annoyed me for 
a while as I have been patching it in my personal kernels for too 
long.

Regards,
  Stephan

PS: I'm not on LKML, please CC me at your discretion.

Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

[Ingo Molnar]

* Stephan Diestelhorst <langer_mann@web.de> wrote:

> Again, should I provide these patches? This thing just annoyed me for 
> a while as I have been patching it in my personal kernels for too 
> long.

yes, please do keep sending them (and any other patches you might have) 
- it's a real issue on real hardware so we want this fix upstream.

	Ingo

Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

[Stephan Diestelhorst]

Ingo Molnar wrote:
> > Again, should I provide these patches? This thing just annoyed me for 
> > a while as I have been patching it in my personal kernels for too 
> > long.
> 
> yes, please do keep sending them (and any other patches you might have) 
> - it's a real issue on real hardware so we want this fix upstream.

New attempt with full clobbers, note that I deliberatly did not change
the order of the output registers. Real output operands still preceede
outputs used for potential clobbering.

I'm not too sure about the EBP push/pop frame, but as folks pointed
out already, we should not trust the SMI code too much.

Regards,
  Stephan

--
Signed-off by: <Stephan.Diestelhorst@gmail.com>

--- linux-2.6.24.3/arch/x86/kernel/cpu/cpufreq/speedstep-smi.c.orig	2008-02-26 01:20:20.000000000 +0100
+++ linux-2.6.24.3/arch/x86/kernel/cpu/cpufreq/speedstep-smi.c	2008-03-10 16:02:51.000000000 +0100
@@ -63,7 +63,7 @@ static struct cpufreq_frequency_table sp
  */
 static int speedstep_smi_ownership (void)
 {
-	u32 command, result, magic;
+	u32 command, result, magic, dummy;
 	u32 function = GET_SPEEDSTEP_OWNER;
 	unsigned char magic_data[] = "Copyright (c) 1999 Intel Corporation";
 
@@ -73,8 +73,11 @@ static int speedstep_smi_ownership (void
 	dprintk("trying to obtain ownership with command %x at port %x\n", command, smi_port);
 
 	__asm__ __volatile__(
+		"push %%ebp\n"
 		"out %%al, (%%dx)\n"
-		: "=D" (result)
+		"pop %%ebp\n"
+		: "=D" (result), "=a" (dummy), "=b" (dummy),"=c" (dummy),"=d" (dummy),
+			"=S" (dummy)
 		: "a" (command), "b" (function), "c" (0), "d" (smi_port),
 			"D" (0), "S" (magic)
 		: "memory"
@@ -96,7 +99,7 @@ static int speedstep_smi_ownership (void
  */
 static int speedstep_smi_get_freqs (unsigned int *low, unsigned int *high)
 {
-	u32 command, result = 0, edi, high_mhz, low_mhz;
+	u32 command, result = 0, edi, high_mhz, low_mhz, dummy;
 	u32 state=0;
 	u32 function = GET_SPEEDSTEP_FREQS;
 
@@ -109,10 +112,12 @@ static int speedstep_smi_get_freqs (unsi
 
 	dprintk("trying to determine frequencies with command %x at port %x\n", command, smi_port);
 
-	__asm__ __volatile__("movl $0, %%edi\n"
+	__asm__ __volatile__(
+		"push %%ebp\n"
 		"out %%al, (%%dx)\n"
-		: "=a" (result), "=b" (high_mhz), "=c" (low_mhz), "=d" (state), "=D" (edi)
-		: "a" (command), "b" (function), "c" (state), "d" (smi_port), "S" (0)
+		"pop %%ebp"
+		: "=a" (result), "=b" (high_mhz), "=c" (low_mhz), "=d" (state), "=D" (edi), "=S" (dummy)
+		: "a" (command), "b" (function), "c" (state), "d" (smi_port), "S" (0), "D" (0)
 	);
 
 	dprintk("result %x, low_freq %u, high_freq %u\n", result, low_mhz, high_mhz);
@@ -135,16 +140,18 @@ static int speedstep_smi_get_freqs (unsi
 static int speedstep_get_state (void)
 {
 	u32 function=GET_SPEEDSTEP_STATE;
-	u32 result, state, edi, command;
+	u32 result, state, edi, command, dummy;
 
 	command = (smi_sig & 0xffffff00) | (smi_cmd & 0xff);
 
 	dprintk("trying to determine current setting with command %x at port %x\n", command, smi_port);
 
-	__asm__ __volatile__("movl $0, %%edi\n"
+	__asm__ __volatile__(
+		"push %%ebp\n"
 		"out %%al, (%%dx)\n"
-		: "=a" (result), "=b" (state), "=D" (edi)
-		: "a" (command), "b" (function), "c" (0), "d" (smi_port), "S" (0)
+		"pop %%ebp\n"
+		: "=a" (result), "=b" (state), "=D" (edi), "=c" (dummy), "=d" (dummy), "=S" (dummy)
+		: "a" (command), "b" (function), "c" (0), "d" (smi_port), "S" (0), "D" (0)
 	);
 
 	dprintk("state is %x, result is %x\n", state, result);
@@ -160,7 +167,7 @@ static int speedstep_get_state (void)
  */
 static void speedstep_set_state (unsigned int state)
 {
-	unsigned int result = 0, command, new_state;
+	unsigned int result = 0, command, new_state, dummy;
 	unsigned long flags;
 	unsigned int function=SET_SPEEDSTEP_STATE;
 	unsigned int retry = 0;
@@ -182,10 +189,12 @@ static void speedstep_set_state (unsigne
 		}
 		retry++;
 		__asm__ __volatile__(
-			"movl $0, %%edi\n"
+			"push %%ebp\n"
 			"out %%al, (%%dx)\n"
-			: "=b" (new_state), "=D" (result)
-			: "a" (command), "b" (function), "c" (state), "d" (smi_port), "S" (0)
+			"pop %%ebp"
+			: "=b" (new_state), "=D" (result), "=c" (dummy), "=a" (dummy),
+				"=d" (dummy), "=S" (dummy)
+			: "a" (command), "b" (function), "c" (state), "d" (smi_port), "S" (0), "D" (0)
 			);
 	} while ((new_state != state) && (retry <= SMI_TRIES));
 
@@ -195,7 +204,7 @@ static void speedstep_set_state (unsigne
 	if (new_state == state) {
 		dprintk("change to %u MHz succeeded after %u tries with result %u\n", (speedstep_freqs[new_state].frequency / 1000), retry, result);
 	} else {
-		printk(KERN_ERR "cpufreq: change failed with new_state %u and result %u\n", new_state, result);
+		printk(KERN_ERR "cpufreq: change to state %u failed with new_state %u and result %u\n", state, new_state, result);
 	}
 
 	return;

Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

[Andi Kleen]

Stephan Diestelhorst <langer_mann@web.de> writes:
> 
> New attempt with full clobbers, note that I deliberatly did not change
> the order of the output registers. Real output operands still preceede
> outputs used for potential clobbering.
> 
> I'm not too sure about the EBP push/pop frame, but as folks pointed
> out already, we should not trust the SMI code too much.

Be careful -- older gcc versions tend to abort for inline asm
that clobbers too many registers. Especially when the register
is already used (like ebp in a frame pointer enabled kernel) 

Make sure it at least works on the oldest supported gcc version 
(gcc 3.2) and with frame pointer on.

For asms with so many clobbers explicit push/pop is usually safer.

-Andi

Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

[Stephan Diestelhorst]

Andi Kleen wrote:
> Stephan Diestelhorst <langer_mann@web.de> writes:
> > 
> > New attempt with full clobbers, note that I deliberatly did not change
> > the order of the output registers. Real output operands still preceede
> > outputs used for potential clobbering.
> > 
> > I'm not too sure about the EBP push/pop frame, but as folks pointed
> > out already, we should not trust the SMI code too much.
> 
> Be careful -- older gcc versions tend to abort for inline asm
> that clobbers too many registers. Especially when the register
> is already used (like ebp in a frame pointer enabled kernel) 

AFAIK clobbering ebp is silently ignored on the GCCs I've tried it
on (regardles of frame-pointer ommission). Hence there is a hard-coded
push & pop for that register.

Please also note that these are not clobbers in the strict inline asm
syntax, but rather dummy output values that correspond to actual
input parameters. I'd consider this a serious compiler flaw, if not
bug, if these would not work. But let's not get into GCC vs.
Fancy-inline-asm-hacker flames, like the mplayer folks do ;-)

> Make sure it at least works on the oldest supported gcc version 
> (gcc 3.2) and with frame pointer on.

As I've said, I do not expect this to be problematic, but will test,
just to be sure! 

> For asms with so many clobbers explicit push/pop is usually safer.

Yes, but it is unneccessary overhead as the compiler can (should!)
find more elegant ways to get those registers back if needed.

Regards,
  Stephan

Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

[Andi Kleen]

> Please also note that these are not clobbers in the strict inline asm
> syntax, but rather dummy output values that correspond to actual

Outputs/Inputs are as bad as clobbers in triggering gcc reload ICEs.

> input parameters. I'd consider this a serious compiler flaw, if not
> bug, if these would not work. But let's not get into GCC vs.

It is, and it's (mostly) fixed in newer versions, but we're stuck with 
supporting the older compiler versions unfortunately.

> > For asms with so many clobbers explicit push/pop is usually safer.
> 
> Yes, but it is unneccessary overhead as the compiler can (should!)
> find more elegant ways to get those registers back if needed.

This code is not performance critical at all. Safer beats faster
any time in such cases.

-Andi

Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

[Stephan Diestelhorst]

Stephan Diestelhorst wrote:
> Andi Kleen wrote:
> > Stephan Diestelhorst <langer_mann@web.de> writes:
> > > 
> > > New attempt with full clobbers, note that I deliberatly did not change
> > > the order of the output registers. Real output operands still preceede
> > > outputs used for potential clobbering.
> > > 
> > > I'm not too sure about the EBP push/pop frame, but as folks pointed
> > > out already, we should not trust the SMI code too much.
> > 
> > Be careful -- older gcc versions tend to abort for inline asm
> > that clobbers too many registers. Especially when the register
> > is already used (like ebp in a frame pointer enabled kernel) 
> 
> > Make sure it at least works on the oldest supported gcc version 
> > (gcc 3.2) and with frame pointer on.
> 
> As I've said, I do not expect this to be problematic, but will test,
> just to be sure! 

I've tried it on the following GCCs: 3.3, 3.4, 4.0, 4.1, all with and
without frame-pointer ommission.
Result: As expected. Worked w/o problems, warnings anything.

Apologies for not testing gcc-3.2, but compiling it from source did
not work with libtool complaining about tags in libmath. I'd be
grateful if someone with working gcc-3.2 could try this out.

Cheers,
  Stephan

Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

[Ingo Molnar]

* Stephan Diestelhorst <langer_mann@web.de> wrote:

> Please also note that these are not clobbers in the strict inline asm 
> syntax, but rather dummy output values that correspond to actual input 
> parameters. I'd consider this a serious compiler flaw, if not bug, if 
> these would not work. But let's not get into GCC vs. 
> Fancy-inline-asm-hacker flames, like the mplayer folks do ;-)

yep, that's my experience as well - output constraints are pretty robust 
in general, but if one tries the same effect via the clobber list it's 
easy to run into gcc internal errors. I've applied your patch to 
x86.git. Do you think it's 2.6.25 material? I'm inclined to have it in 
the 2.6.26 bucket. We could do your first, minimal fix in 2.6.25 and do 
this second, full fix in 2.6.26.

	Ingo

Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

[Stephan Diestelhorst]

Ingo Molnar wrote:
> 
> I've applied your patch to  x86.git. 

Many thanks!

> Do you think it's 2.6.25 material? I'm inclined to have it in  
> the 2.6.26 bucket. We could do your first, minimal fix in 2.6.25 and do 
> this second, full fix in 2.6.26.

I am not familiar with the policies regarding the time-frame for
inclusion. So I guess the decision is up to you. I feel that the
changes are fairly small (at least for the first patch) so perhaps
splitting the release as you suggested allows people to get more
confidence in the larger patches with the clobbers and manual ebp
push/pop.

Doing this one step after the other has the additional benefit of
havinga fix for the core problem in fairly soon, so I can continue
using prebuilt distro kernels again, soon. :-) ( I know. I'm just lazy
sometimes. )

Apart from that: Feel free to do as you like, after all this code is
GPL.

Kind regards,
  Stephan