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

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

[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:

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

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

[Srivatsa S. Bhat]

On 11/30/2011 10:24 AM, Larry Finger wrote:

> 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:
> 
>>> 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.
> 
 

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

-- 
Regards,
Srivatsa S. Bhat
IBM Linux Technology Center

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

[Larry Finger]

On 11/29/2011 11:59 PM, Srivatsa S. Bhat wrote:
> On 11/30/2011 10:24 AM, Larry Finger wrote:
>
>> 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:
>>
>>>> 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.
>>
>
>
> 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

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

[Larry Finger]

On 11/30/2011 01:09 AM, Larry Finger wrote:
> On 11/29/2011 11:59 PM, Srivatsa S. Bhat wrote:
>> On 11/30/2011 10:24 AM, Larry Finger wrote:
>>
>>> 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:
>>>
>>>>> 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.
>>>
>>
>>
>> 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.

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

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

[Linus Torvalds]

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

On Sat, Dec 3, 2011 at 12:43 PM, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> On 11/30/2011 01:09 AM, Larry Finger wrote:
>> On 11/29/2011 11:59 PM, Srivatsa S. Bhat wrote:
>>>
>>> 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?

> 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

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

 arch/x86/kernel/cpu/amd.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index c7e46cb35327..0bab2b18bb20 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -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

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

[Larry Finger]

On 12/03/2011 05:07 PM, Linus Torvalds wrote:
> On Sat, Dec 3, 2011 at 12:43 PM, Larry Finger<Larry.Finger@lwfinger.net>  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?
>
>> 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

Linus,

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

Thanks for the explanation, and the patch,

Larry

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

[Bob Tracy]

On Sat, Dec 03, 2011 at 03:07:56PM -0800, Linus Torvalds wrote:
> (...)
> 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

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

[Borislav Petkov]

On Sat, Dec 03, 2011 at 03:07:56PM -0800, Linus Torvalds wrote:
> 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.

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

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

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

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

-- 
Regards/Gruss,
Boris.

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

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, Dec 3, 2011 at 12:43 PM, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> > On 11/30/2011 01:09 AM, Larry Finger wrote:
> >> On 11/29/2011 11:59 PM, Srivatsa S. Bhat wrote:
> >>>
> >>> 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()"?

Yeah.

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

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

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

[tip-bot for Borislav Petkov]

Commit-ID:  ce37defc0f6673f5ca2c92ed5cfcaf290ae7dd16
Gitweb:     http://git.kernel.org/tip/ce37defc0f6673f5ca2c92ed5cfcaf290ae7dd16
Author:     Borislav Petkov <borislav.petkov@amd.com>
AuthorDate: Mon, 5 Dec 2011 14:28:37 +0100
Committer:  Borislav Petkov <borislav.petkov@amd.com>
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/4ED5B70F.606@lwfinger.net
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/include/asm/msr.h |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 084ef95..95203d4 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -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;						\