Skip to contentSkip to conversation

3.2-rc2 freezes on boot for AMD K6 - bisected to commit bcb80e53877c2045d9e52f4a71372c3fe6501f6f

DISCUSSION Larry Finger 7 participants 10 messages Nov 30, 2011 – Dec 5, 2011 x86

lore
Share & tools
lore

Larry Finger

author
#0 lore raw

I have an ancient laptop with an AMD-K6 CPU that freezes on boot with 3.2-rc2. The problem was bisected to commit bcb80e53877c2.

The cpuinfo for this box is:

Quoted 1 line
Quoted 1 line

cpu.1: cpuinfo

----- /proc/cpuinfo -----

  processor     : 0
  vendor_id     : AuthenticAMD
  cpu family    : 5
  model         : 8
  model name    : AMD-K6(tm) 3D processor
  stepping      : 12
  cpu MHz               : 428.804
  cache size    : 64 KB
  fdiv_bug      : no
  hlt_bug               : no
  f00f_bug      : no
  coma_bug      : no
  fpu           : yes
  fpu_exception : yes
  cpuid level   : 1
  wp            : yes
  flags         : fpu vme de pse tsc msr cx8 pge mmx syscall 3dnow k6_mtrr up
  bogomips      : 857.60
  clflush size  : 32

cache_alignment : 32
address sizes : 32 bits physical, 32 bits virtual power management:

----- /proc/cpuinfo end -----

This patch is quite simple, thus it appears that rdmsr() is OK, but that rdmsr_safe() is not.

I am happy to provide any other info regarding this box.

Larry

#1 lore raw
Quoted 31 lines · Larry Finger

I have an ancient laptop with an AMD-K6 CPU that freezes on boot with 3.2-rc2. The problem was bisected to commit bcb80e53877c2.

The cpuinfo for this box is:

Quoted 1 line
Quoted 1 line

cpu.1: cpuinfo

----- /proc/cpuinfo -----

  processor     : 0
  vendor_id     : AuthenticAMD
  cpu family    : 5
  model         : 8
  model name    : AMD-K6(tm) 3D processor
  stepping      : 12
  cpu MHz               : 428.804
  cache size    : 64 KB
  fdiv_bug      : no
  hlt_bug               : no
  f00f_bug      : no
  coma_bug      : no
  fpu           : yes
  fpu_exception : yes
  cpuid level   : 1
  wp            : yes
  flags         : fpu vme de pse tsc msr cx8 pge mmx syscall 3dnow

k6_mtrr up
bogomips : 857.60
clflush size : 32
cache_alignment : 32
address sizes : 32 bits physical, 32 bits virtual power management:

Quoted 7 lines

----- /proc/cpuinfo end -----

This patch is quite simple, thus it appears that rdmsr() is OK, but that rdmsr_safe() is not.

I am happy to provide any other info regarding this box.

Hi Larry,

Can you please try out the patch posted in
https://lkml.org/lkml/2011/11/28/178 ?

This patch will be available in mainline kernel from -rc4.
https://lkml.org/lkml/2011/11/28/457

Signature · 3 lines
Regards,
Srivatsa S. Bhat
IBM Linux Technology Center
author
#2 lore raw
Quoted 42 lines · Srivatsa S. Bhat
Quoted 31 lines · Larry Finger

I have an ancient laptop with an AMD-K6 CPU that freezes on boot with 3.2-rc2. The problem was bisected to commit bcb80e53877c2.

The cpuinfo for this box is:

Quoted 1 line
Quoted 1 line

cpu.1: cpuinfo

----- /proc/cpuinfo -----

   processor     : 0
   vendor_id     : AuthenticAMD
   cpu family    : 5
   model         : 8
   model name    : AMD-K6(tm) 3D processor
   stepping      : 12
   cpu MHz               : 428.804
   cache size    : 64 KB
   fdiv_bug      : no
   hlt_bug               : no
   f00f_bug      : no
   coma_bug      : no
   fpu           : yes
   fpu_exception : yes
   cpuid level   : 1
   wp            : yes
   flags         : fpu vme de pse tsc msr cx8 pge mmx syscall 3dnow

k6_mtrr up
bogomips : 857.60
clflush size : 32
cache_alignment : 32
address sizes : 32 bits physical, 32 bits virtual power management:

Quoted 7 lines

----- /proc/cpuinfo end -----

This patch is quite simple, thus it appears that rdmsr() is OK, but that rdmsr_safe() is not.

I am happy to provide any other info regarding this box.

Quoted 7 lines

Hi Larry,

Can you please try out the patch posted in
https://lkml.org/lkml/2011/11/28/178 ?

This patch will be available in mainline kernel from -rc4.
https://lkml.org/lkml/2011/11/28/457

Srivatsa,

Thanks for the reference. That patch does fix my problem. Too bad that the bad commit was not in the subject of that other thread so that I could have found it with Google.

Larry

author
#3 lore raw
Quoted 48 lines · Larry Finger
Quoted 47 lines · Srivatsa S. Bhat
Quoted 31 lines · Larry Finger

I have an ancient laptop with an AMD-K6 CPU that freezes on boot with 3.2-rc2. The problem was bisected to commit bcb80e53877c2.

The cpuinfo for this box is:

Quoted 1 line
Quoted 1 line

cpu.1: cpuinfo

----- /proc/cpuinfo -----
processor : 0
vendor_id : AuthenticAMD
cpu family : 5
model : 8
model name : AMD-K6(tm) 3D processor
stepping : 12
cpu MHz : 428.804
cache size : 64 KB
fdiv_bug : no
hlt_bug : no
f00f_bug : no
coma_bug : no
fpu : yes
fpu_exception : yes
cpuid level : 1
wp : yes
flags : fpu vme de pse tsc msr cx8 pge mmx syscall 3dnow k6_mtrr up
bogomips : 857.60
clflush size : 32
cache_alignment : 32
address sizes : 32 bits physical, 32 bits virtual power management:

Quoted 7 lines

----- /proc/cpuinfo end -----

This patch is quite simple, thus it appears that rdmsr() is OK, but that rdmsr_safe() is not.

I am happy to provide any other info regarding this box.

Hi Larry,

Can you please try out the patch posted in
https://lkml.org/lkml/2011/11/28/178 ?

Quoted 8 lines
Quoted 2 lines

This patch will be available in mainline kernel from -rc4.
https://lkml.org/lkml/2011/11/28/457

Srivatsa,

Thanks for the reference. That patch does fix my problem. Too bad that the bad commit was not in the subject of that other thread so that I could have found it with Google.

Borislav,

I just updated mainline to 3.2-rc4, and that patch is not included. Please check with Ingo to see why it was not available. It is a real show stopper for old AMD CPUs.

Thanks,

Larry

#4 lore raw
Quoted 5 lines · Larry Finger
Quoted 4 lines · Larry Finger
Quoted 3 lines · Srivatsa S. Bhat

Can you please try out the patch posted in
https://lkml.org/lkml/2011/11/28/178 ?

Ugh. I hate that patch.

It's completely stupid. If "rdmsr_safe()" doesn't work at that point in the boot, then it's pointless to call it.

So this change is pure and utter crap:

  -	rdmsr_safe(MSR_AMD64_PATCH_LEVEL, &c->microcode, &dummy);
  +	if (c->x86 >= 0xf)
  +		rdmsr_safe(MSR_AMD64_PATCH_LEVEL, &c->microcode, &dummy);

because it is misleading as hell: that rdmsr isn't *safe* at all, so why are we calling "rdmsr_safe()"?

It's wrong.

The right patch would either just remove the "safe" part (and just say that the register has to be supported if c->x86 >= 0xf), but quite honestly, I don't see why we do that thing in early_init_amd() AT ALL. Afaik, the microcode version field isn't really *needed* by the kernelin the first place, much less is it needed by the *early* boot, so why isn't this in 'init_amd()' a bit later when the "safe" version actually *works*?

IOW, I think the patch should be something like the attached (TOTALLY UNTESTED) patch. Larry, does this work for you? It just moves the rdmsr_safe() to the later function.

Borislav?

Quoted 3 lines

I just updated mainline to 3.2-rc4, and that patch is not included. Please check with Ingo to see why it was not available. It is a real show stopper for old AMD CPUs.

Ingo seems to have fallen off the earth for the last two weeks. There's *one* email form him about 12 hours ago, before that the last one I see is from early November.

Ingo, everything ok?

Linus

 arch/x86/kernel/cpu/amd.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
arch/x86/kernel/cpu/amd.c +4 -4
index c7e46cb35327..0bab2b18bb20 100644
@@ -442,8 +442,6 @@ static void __cpuinit bsp_init_amd(struct cpuinfo_x86 *c)
 
 static void __cpuinit early_init_amd(struct cpuinfo_x86 *c)
 {
-	u32 dummy;
-
 	early_init_amd_mc(c);
 
 	/*

@@ -473,12 +471,12 @@ static void __cpuinit early_init_amd(struct cpuinfo_x86 *c)
 			set_cpu_cap(c, X86_FEATURE_EXTD_APICID);
 	}
 #endif
-
-	rdmsr_safe(MSR_AMD64_PATCH_LEVEL, &c->microcode, &dummy);
 }
 
 static void __cpuinit init_amd(struct cpuinfo_x86 *c)
 {
+	u32 dummy;
+
 #ifdef CONFIG_SMP
 	unsigned long long value;
 

@@ -657,6 +655,8 @@ static void __cpuinit init_amd(struct cpuinfo_x86 *c)
 			checking_wrmsrl(MSR_AMD64_MCx_MASK(4), mask);
 		}
 	}
+
+	rdmsr_safe(MSR_AMD64_PATCH_LEVEL, &c->microcode, &dummy);
 }
 
 #ifdef CONFIG_X86_32
author
#5 lore raw
Quoted 36 lines · Linus Torvalds

On Sat, Dec 3, 2011 at 12:43 PM, Larry Finger<[email protected]> wrote:

Ugh. I hate that patch.

It's completely stupid. If "rdmsr_safe()" doesn't work at that point in the boot, then it's pointless to call it.

So this change is pure and utter crap:

   -	rdmsr_safe(MSR_AMD64_PATCH_LEVEL,&c->microcode,&dummy);
   +	if (c->x86>= 0xf)
   +		rdmsr_safe(MSR_AMD64_PATCH_LEVEL,&c->microcode,&dummy);

because it is misleading as hell: that rdmsr isn't *safe* at all, so why are we calling "rdmsr_safe()"?

It's wrong.

The right patch would either just remove the "safe" part (and just say that the register has to be supported if c->x86>= 0xf), but quite honestly, I don't see why we do that thing in early_init_amd() AT ALL. Afaik, the microcode version field isn't really *needed* by the kernelin the first place, much less is it needed by the *early* boot, so why isn't this in 'init_amd()' a bit later when the "safe" version actually *works*?

IOW, I think the patch should be something like the attached (TOTALLY UNTESTED) patch. Larry, does this work for you? It just moves the rdmsr_safe() to the later function.

Borislav?

Quoted 3 lines

I just updated mainline to 3.2-rc4, and that patch is not included. Please check with Ingo to see why it was not available. It is a real show stopper for old AMD CPUs.

Quoted 7 lines

Ingo seems to have fallen off the earth for the last two weeks. There's *one* email form him about 12 hours ago, before that the last one I see is from early November.

Ingo, everything ok?

Linus

Linus,

With your patch, my K8 box boots fine running 3.2-rc4. If you wish, you may add a "Tested-by: Larry Finger <[email protected]>.

Thanks for the explanation, and the patch,

Larry

#6 lore raw
In reply to Linus Torvalds
Quoted 4 lines · Linus Torvalds

(...)
IOW, I think the patch should be something like the attached (TOTALLY UNTESTED) patch. Larry, does this work for you? It just moves the rdmsr_safe() to the later function.

Works for me as well: AMD K6-III/450.

--Bob

maintainer
#7 lore raw
In reply to Linus Torvalds
Quoted 11 lines · Linus Torvalds

It's completely stupid. If "rdmsr_safe()" doesn't work at that point in the boot, then it's pointless to call it.

So this change is pure and utter crap:

  -	rdmsr_safe(MSR_AMD64_PATCH_LEVEL, &c->microcode, &dummy);
  +	if (c->x86 >= 0xf)
  +		rdmsr_safe(MSR_AMD64_PATCH_LEVEL, &c->microcode, &dummy);

because it is misleading as hell: that rdmsr isn't *safe* at all, so why are we calling "rdmsr_safe()"?

Well, here's the whole story behing this f*ckup:

I didn't want to have yet another family check there, thus the rdmsr_safe version instead for machines which don't sport the 0x8B MSR. But, the stupid exception tables are not up at early_init_* time.

hpa suggested I fix that but for that we need to sort them at build time which is still outstanding as a patchset.

Therefore, I did this temporary fix with the intent to revisit this later once the tables sorting is done and upstream.

Quoted 3 lines

The right patch would either just remove the "safe" part (and just say that the register has to be supported if c->x86 >= 0xf), but quite honestly, I don't see why we do that thing in early_init_amd() AT ALL.

Well, no real reason, just 506ed6b53e00ba303ad778122f08e1fca7cf5efb, which added the Intel side of this, added it there with a family check too.

The earliest we will use the microcode version is when printing an MCE when you get an MCE very early, right after having initted MCE in identify_cpu->mcheck_cpu_int. But that's still fine because the vendor-specific ->c_init hooks are called before mcheck_cpu_int anyway, in the same function.

Quoted 4 lines

Afaik, the microcode version field isn't really *needed* by the kernelin the first place, much less is it needed by the *early* boot, so why isn't this in 'init_amd()' a bit later when the "safe" version actually *works*?

Agreed.

Quoted 5 lines

IOW, I think the patch should be something like the attached (TOTALLY UNTESTED) patch. Larry, does this work for you? It just moves the rdmsr_safe() to the later function.

Borislav?

So yes, your version works too here, so please go ahead an apply it so that people can boot their old AMD boxes again. Sorry again for the trouble.

Quoted 10 lines
Quoted 4 lines

I just updated mainline to 3.2-rc4, and that patch is not included. Please check with Ingo to see why it was not available. It is a real show stopper for old AMD CPUs.

Ingo seems to have fallen off the earth for the last two weeks. There's *one* email form him about 12 hours ago, before that the last one I see is from early November.

Ingo, everything ok?

Oh yeah, and the fix didn't hit mainline yet thus the frustration.

Thanks.

Signature · 2 lines
Regards/Gruss,
Boris.
#8 lore raw
In reply to Linus Torvalds
Quoted 12 lines · Linus Torvalds
Quoted 5 lines · Larry Finger
Quoted 4 lines · Larry Finger
Quoted 3 lines · Srivatsa S. Bhat

Can you please try out the patch posted in
https://lkml.org/lkml/2011/11/28/178 ?

Ugh. I hate that patch.

It's completely stupid. If "rdmsr_safe()" doesn't work at that point in the boot, then it's pointless to call it.

Quoted 8 lines

So this change is pure and utter crap:

  -	rdmsr_safe(MSR_AMD64_PATCH_LEVEL, &c->microcode, &dummy);
  +	if (c->x86 >= 0xf)
  +		rdmsr_safe(MSR_AMD64_PATCH_LEVEL, &c->microcode, &dummy);

because it is misleading as hell: that rdmsr isn't *safe* at all, so why are we calling "rdmsr_safe()"?

Yeah.

Quoted 11 lines

It's wrong.

The right patch would either just remove the "safe" part (and just say that the register has to be supported if c->x86 >= 0xf), but quite honestly, I don't see why we do that thing in early_init_amd() AT ALL. Afaik, the microcode version field isn't really *needed* by the kernelin the first place, much less is it needed by the *early* boot, so why isn't this in 'init_amd()' a bit later when the "safe" version actually *works*?

Quoted 3 lines

IOW, I think the patch should be something like the attached (TOTALLY UNTESTED) patch. Larry, does this work for you? It just moves the rdmsr_safe() to the later function.

Looks sane to me.

We can improve the early init properties some more - but there's always going to be a chicken-and-egg problem there. At minimum we should add a comment to rdmsr_safe() that explains its dependencies.

Thanks,

Ingo

#9 lore raw

[tip:x86/urgent] x86: Document rdmsr_safe restrictions

In reply to Larry Finger
Commit-ID:  ce37defc0f6673f5ca2c92ed5cfcaf290ae7dd16
Gitweb:     http://git.kernel.org/tip/ce37defc0f6673f5ca2c92ed5cfcaf290ae7dd16
Author:     Borislav Petkov <[email protected]>
AuthorDate: Mon, 5 Dec 2011 14:28:37 +0100
Committer:  Borislav Petkov <[email protected]>
CommitDate: Mon, 5 Dec 2011 14:28:37 +0100

x86: Document rdmsr_safe restrictions

Recently, I got bitten by using rdmsr_safe too early in the boot process. Document its shortcomings for future reference.

Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Borislav Petkov <[email protected]>
---
 arch/x86/include/asm/msr.h |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)
arch/x86/include/asm/msr.h +8 -1
index 084ef95..95203d4 100644
@@ -169,7 +169,14 @@ static inline int wrmsr_safe(unsigned msr, unsigned low, unsigned high)
 	return native_write_msr_safe(msr, low, high);
 }
 
-/* rdmsr with exception handling */
+/*
+ * rdmsr with exception handling.
+ *
+ * Please note that the exception handling works only after we've
+ * switched to the "smart" #GP handler in trap_init() which knows about
+ * exception tables - using this macro earlier than that causes machine
+ * hangs on boxes which do not implement the @msr in the first argument.
+ */
 #define rdmsr_safe(msr, p1, p2)					\
 ({								\
 	int __err;						\