Speedfreq-SMI call clobbers ECX

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 <[email protected]>

--

--- 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;

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

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

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.

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

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: <[email protected]>

@@ -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;

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

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 ;-)

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

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

Regards,
Stephan

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

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

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

-Andi

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

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

Many thanks!

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