Setup code crashes my old 486 box

Setup code crashes my old 486 box

[Joerg Roedel]

Hello,

yesterday I tried to reactivate my old 486 box and wanted to install a
current Linux with latest kernel on it. But it turned out that the
latest kernel does not boot because the machine crashes early in the
setup code.
After some debugging it turned out that the problem is the query_ist()
function. If this interrupt with that function is called the machine
simply locks up. It looks like a BIOS bug. Looking for a workaround for
this problem I wrote the attached patch. It checks for the CPUID
instruction and if it is not implemented it does not call the speedstep
BIOS function. As far as I know speedstep should be available since some
Pentium earliest.
Is this an acceptable workaround or is there a better one for this?

Regards,

Joerg

Here is the patch which makes the kernel boot on my old box again:

From: Joerg Roedel <joro@8bytes.org>

This patch adds a check to the query_ist function to check for the cpuid
instruction before calling the speedstep init code. This works around a
buggy 486 BIOS which crashes the machine on the speedstep BIOS initcall.

Signed-off-by: Joerg Roedel <joro@8bytes.org>
---
 arch/x86/boot/main.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/arch/x86/boot/main.c b/arch/x86/boot/main.c
index 2296164..3bf5fa0 100644
--- a/arch/x86/boot/main.c
+++ b/arch/x86/boot/main.c
@@ -68,11 +68,58 @@ static void keyboard_set_repeat(void)
 		     : : "ecx", "edx", "esi", "edi");
 }
 
+static unsigned read_flags(void)
+{
+	unsigned eax;
+
+	asm volatile("pushf\n\t"
+		     "popl %%eax\n\t"
+		     : "=a" (eax));
+
+	return eax;
+}
+
+static inline void write_flags(unsigned f)
+{
+	asm volatile("pushl %0\n\t"
+		     "popf\n\t"
+		     : : "r" (f));
+}
+
+static int has_cpuid(void)
+{
+	unsigned f = read_flags();
+	unsigned id_mask = (1 << 21);
+
+	write_flags(f | id_mask);
+	f = read_flags() & id_mask;
+	if (!f)
+		return 0;
+	f = read_flags() & ~id_mask;
+	write_flags(f);
+	f = read_flags() & id_mask;
+	if (f)
+		return 0;
+
+	return 1;
+}
+
+static int no_ist_check(void)
+{
+	if (!has_cpuid())
+		return 1;
+
+	return 0;
+}
+
 /*
  * Get Intel SpeedStep (IST) information.
  */
 static void query_ist(void)
 {
+	if (no_ist_check())
+		return;
+
 	asm("int $0x15"
 	    : "=a" (boot_params.ist_info.signature),
 	      "=b" (boot_params.ist_info.command),
-- 
1.5.4.3

Re: Setup code crashes my old 486 box

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

Joerg Roedel wrote:
> Hello,
> 
> yesterday I tried to reactivate my old 486 box and wanted to install a
> current Linux with latest kernel on it. But it turned out that the
> latest kernel does not boot because the machine crashes early in the
> setup code.
> After some debugging it turned out that the problem is the query_ist()
> function. If this interrupt with that function is called the machine
> simply locks up. It looks like a BIOS bug. Looking for a workaround for
> this problem I wrote the attached patch. It checks for the CPUID
> instruction and if it is not implemented it does not call the speedstep
> BIOS function. As far as I know speedstep should be available since some
> Pentium earliest.
> Is this an acceptable workaround or is there a better one for this?
> 
> Regards,
> 
> Joerg

Right in concept, but I dislike the implementation (duplication of the 
CPU detect code we already have).  Could you try this patch and see if 
it works for you?

	-hpa

[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 1463 bytes --]

diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index a34b998..9ce8d29 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -242,6 +242,12 @@ int cmdline_find_option(const char *option, char *buffer, int bufsize);
 int cmdline_find_option_bool(const char *option);
 
 /* cpu.c, cpucheck.c */
+struct cpu_features {
+	int level;		/* Family, or 64 for x86-64 */
+	int model;
+	u32 flags[NCAPINTS];
+};
+extern struct cpu_features cpu;
 int check_cpu(int *cpu_level_ptr, int *req_level_ptr, u32 **err_flags_ptr);
 int validate_cpu(void);
 
diff --git a/arch/x86/boot/cpucheck.c b/arch/x86/boot/cpucheck.c
index 7804389..c1ce030 100644
--- a/arch/x86/boot/cpucheck.c
+++ b/arch/x86/boot/cpucheck.c
@@ -30,13 +30,7 @@
 #include <asm/required-features.h>
 #include <asm/msr-index.h>
 
-struct cpu_features {
-	int level;		/* Family, or 64 for x86-64 */
-	int model;
-	u32 flags[NCAPINTS];
-};
-
-static struct cpu_features cpu;
+struct cpu_features cpu;
 static u32 cpu_vendor[3];
 static u32 err_flags[NCAPINTS];
 
diff --git a/arch/x86/boot/main.c b/arch/x86/boot/main.c
index 2296164..9b04773 100644
--- a/arch/x86/boot/main.c
+++ b/arch/x86/boot/main.c
@@ -73,6 +73,10 @@ static void keyboard_set_repeat(void)
  */
 static void query_ist(void)
 {
+	/* Some 486 BIOSes apparently crash on this call */
+	if (cpu.level < 5)
+		return;
+
 	asm("int $0x15"
 	    : "=a" (boot_params.ist_info.signature),
 	      "=b" (boot_params.ist_info.command),

Re: Setup code crashes my old 486 box

[Alan Cox]

> Right in concept, but I dislike the implementation (duplication of the 
> CPU detect code we already have).  Could you try this patch and see if 
> it works for you?

I was under the impression that spudstop came in with the Pentium II so
presumably < 6 would be even safer ?

Alan

Re: Setup code crashes my old 486 box

[H. Peter Anvin]

Alan Cox wrote:
>> Right in concept, but I dislike the implementation (duplication of the 
>> CPU detect code we already have).  Could you try this patch and see if 
>> it works for you?
> 
> I was under the impression that spudstop came in with the Pentium II so
> presumably < 6 would be even safer ?

I think you're right about that.

	-hpa

Re: Setup code crashes my old 486 box

[Joerg Roedel]

[-- Attachment #1: Type: text/plain, Size: 624 bytes --]

On Tue, Aug 12, 2008 at 03:57:31PM -0700, H. Peter Anvin wrote:
> Right in concept, but I dislike the implementation (duplication of the 
> CPU detect code we already have).  Could you try this patch and see if 
> it works for you?

Cool, with a small modification to fix a build error with it the
resulting kernel boots my machine. Thanks.
Would be cool to have this patch one merged soon :-)
Attached is your patch with my little build fix. It redefines a values
already defines in cpufeatures.h but I can't include that file in the
boot code so I simply redeclared it. Maybe there is a cleaner solution
for this.

Joerg


[-- Attachment #2: ist_fix.diff --]
[-- Type: text/x-diff, Size: 1652 bytes --]

diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index a34b998..9d4b4b4 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -25,6 +25,8 @@
 #include <asm/boot.h>
 #include <asm/setup.h>
 
+#define NCAPINTS   8
+
 /* Useful macros */
 #define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
 
@@ -242,6 +244,12 @@ int cmdline_find_option(const char *option, char *buffer, int bufsize);
 int cmdline_find_option_bool(const char *option);
 
 /* cpu.c, cpucheck.c */
+struct cpu_features {
+	int level;		/* Family, or 64 for x86-64 */
+	int model;
+	u32 flags[NCAPINTS];
+};
+extern struct cpu_features cpu;
 int check_cpu(int *cpu_level_ptr, int *req_level_ptr, u32 **err_flags_ptr);
 int validate_cpu(void);
 
diff --git a/arch/x86/boot/cpucheck.c b/arch/x86/boot/cpucheck.c
index 7804389..c1ce030 100644
--- a/arch/x86/boot/cpucheck.c
+++ b/arch/x86/boot/cpucheck.c
@@ -30,13 +30,7 @@
 #include <asm/required-features.h>
 #include <asm/msr-index.h>
 
-struct cpu_features {
-	int level;		/* Family, or 64 for x86-64 */
-	int model;
-	u32 flags[NCAPINTS];
-};
-
-static struct cpu_features cpu;
+struct cpu_features cpu;
 static u32 cpu_vendor[3];
 static u32 err_flags[NCAPINTS];
 
diff --git a/arch/x86/boot/main.c b/arch/x86/boot/main.c
index 2296164..01aa64b 100644
--- a/arch/x86/boot/main.c
+++ b/arch/x86/boot/main.c
@@ -73,6 +73,10 @@ static void keyboard_set_repeat(void)
  */
 static void query_ist(void)
 {
+	/* Some 486 BIOSes apparently crash on this call */
+	if (cpu.level < 6)
+		return;
+
 	asm("int $0x15"
 	    : "=a" (boot_params.ist_info.signature),
 	      "=b" (boot_params.ist_info.command),

Re: Setup code crashes my old 486 box

[Ingo Molnar]

* Joerg Roedel <joro@8bytes.org> wrote:

> On Tue, Aug 12, 2008 at 03:57:31PM -0700, H. Peter Anvin wrote:
> > Right in concept, but I dislike the implementation (duplication of the 
> > CPU detect code we already have).  Could you try this patch and see if 
> > it works for you?
> 
> Cool, with a small modification to fix a build error with it the
> resulting kernel boots my machine. Thanks.
> Would be cool to have this patch one merged soon :-)
> Attached is your patch with my little build fix. It redefines a values
> already defines in cpufeatures.h but I can't include that file in the
> boot code so I simply redeclared it. Maybe there is a cleaner solution
> for this.

thanks, i've commited the fix below to tip/x86/urgent - it should show 
up in v2.6.27-rc4 and it might even be backportable to -stable. I've 
added your Signed-off-by - is that ok with you?

	Ingo

------------->
>From 7b27718bdb1b70166383dec91391df5534d449ee Mon Sep 17 00:00:00 2001
From: Joerg Roedel <joro@8bytes.org>
Date: Wed, 13 Aug 2008 10:07:05 +0200
Subject: [PATCH] x86: fix setup code crashes on my old 486 box

yesterday I tried to reactivate my old 486 box and wanted to install a
current Linux with latest kernel on it. But it turned out that the
latest kernel does not boot because the machine crashes early in the
setup code.

After some debugging it turned out that the problem is the query_ist()
function. If this interrupt with that function is called the machine
simply locks up. It looks like a BIOS bug. Looking for a workaround for
this problem I wrote the attached patch. It checks for the CPUID
instruction and if it is not implemented it does not call the speedstep
BIOS function. As far as I know speedstep should be available since some
Pentium earliest.

Alan Cox observed that it's available since the Pentium II, so cpuid
levels 4 and 5 can be excluded altogether.

H. Peter Anvin cleaned up the code some more:

> Right in concept, but I dislike the implementation (duplication of the
> CPU detect code we already have).  Could you try this patch and see if
> it works for you?

which, with a small modification to fix a build error with it the
resulting kernel boots on my machine.

Signed-off-by: Joerg Roedel <joro@8bytes.org>
Signed-off-by: "H. Peter Anvin" <hpa@zytor.com>
Cc: <stable@kernel.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/boot/boot.h     |    8 ++++++++
 arch/x86/boot/cpucheck.c |    8 +-------
 arch/x86/boot/main.c     |    4 ++++
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index a34b998..9d4b4b4 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -25,6 +25,8 @@
 #include <asm/boot.h>
 #include <asm/setup.h>
 
+#define NCAPINTS   8
+
 /* Useful macros */
 #define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
 
@@ -242,6 +244,12 @@ int cmdline_find_option(const char *option, char *buffer, int bufsize);
 int cmdline_find_option_bool(const char *option);
 
 /* cpu.c, cpucheck.c */
+struct cpu_features {
+	int level;		/* Family, or 64 for x86-64 */
+	int model;
+	u32 flags[NCAPINTS];
+};
+extern struct cpu_features cpu;
 int check_cpu(int *cpu_level_ptr, int *req_level_ptr, u32 **err_flags_ptr);
 int validate_cpu(void);
 
diff --git a/arch/x86/boot/cpucheck.c b/arch/x86/boot/cpucheck.c
index 7804389..c1ce030 100644
--- a/arch/x86/boot/cpucheck.c
+++ b/arch/x86/boot/cpucheck.c
@@ -30,13 +30,7 @@
 #include <asm/required-features.h>
 #include <asm/msr-index.h>
 
-struct cpu_features {
-	int level;		/* Family, or 64 for x86-64 */
-	int model;
-	u32 flags[NCAPINTS];
-};
-
-static struct cpu_features cpu;
+struct cpu_features cpu;
 static u32 cpu_vendor[3];
 static u32 err_flags[NCAPINTS];
 
diff --git a/arch/x86/boot/main.c b/arch/x86/boot/main.c
index 2296164..01aa64b 100644
--- a/arch/x86/boot/main.c
+++ b/arch/x86/boot/main.c
@@ -73,6 +73,10 @@ static void keyboard_set_repeat(void)
  */
 static void query_ist(void)
 {
+	/* Some 486 BIOSes apparently crash on this call */
+	if (cpu.level < 6)
+		return;
+
 	asm("int $0x15"
 	    : "=a" (boot_params.ist_info.signature),
 	      "=b" (boot_params.ist_info.command),

Re: Setup code crashes my old 486 box

[Joerg Roedel]

On Wed, Aug 13, 2008 at 12:00:13PM +0200, Ingo Molnar wrote:
> 
> * Joerg Roedel <joro@8bytes.org> wrote:
> 
> > On Tue, Aug 12, 2008 at 03:57:31PM -0700, H. Peter Anvin wrote:
> > > Right in concept, but I dislike the implementation (duplication of the 
> > > CPU detect code we already have).  Could you try this patch and see if 
> > > it works for you?
> > 
> > Cool, with a small modification to fix a build error with it the
> > resulting kernel boots my machine. Thanks.
> > Would be cool to have this patch one merged soon :-)
> > Attached is your patch with my little build fix. It redefines a values
> > already defines in cpufeatures.h but I can't include that file in the
> > boot code so I simply redeclared it. Maybe there is a cleaner solution
> > for this.
> 
> thanks, i've commited the fix below to tip/x86/urgent - it should show 
> up in v2.6.27-rc4 and it might even be backportable to -stable. I've 
> added your Signed-off-by - is that ok with you?

Ok, cool. Thats fine for me. Thanks.

Joerg

Re: Setup code crashes my old 486 box

[H. Peter Anvin]

Joerg Roedel wrote:
> On Tue, Aug 12, 2008 at 03:57:31PM -0700, H. Peter Anvin wrote:
>> Right in concept, but I dislike the implementation (duplication of the 
>> CPU detect code we already have).  Could you try this patch and see if 
>> it works for you?
> 
> Cool, with a small modification to fix a build error with it the
> resulting kernel boots my machine. Thanks.
> Would be cool to have this patch one merged soon :-)
> Attached is your patch with my little build fix. It redefines a values
> already defines in cpufeatures.h but I can't include that file in the
> boot code so I simply redeclared it. Maybe there is a cleaner solution
> for this.
> 

There is... let me cook it up and add Alan's correction.

	-hpa