[REGRESSION][4.0-rc1] My i386 fails to do CPU hotplug

[REGRESSION][4.0-rc1] My i386 fails to do CPU hotplug

[Steven Rostedt]

Running my test suite on 4.0-rc1, my i386 run of my tests crashed with
the following:

Initializing CPU#1
invalid opcode: 0000 [#1] SMP
Modules linked in: ip6t_REJECT nf_reject_ipv6 nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_filter ip6_tables ipv6 ppdev parport_pc microcode r8169 parport
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.0.0-rc1-test #2
Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6 02/22/2014
task: f2ee8000 ti: f2ee6000 task.ti: f2ee6000
EIP: 0060:[<c040ab1f>] EFLAGS: 00210046 CPU: 1
EIP is at xstate_enable+0x2b/0x30
EAX: 00000007 EBX: 00000001 ECX: 00000000 EDX: 00000000
ESI: f2ee8000 EDI: c144c930 EBP: f2ee7f58 ESP: f2ee7f58
 DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
CR0: 80050033 CR2: 00000000 CR3: 01139000 CR4: 000000b0
Stack:
 f2ee7f60 c040b47b f2ee7f6c c040a4fd f352ef00 f2ee7f94 c040f40d c0fd8c4c
 00000001 320f4000 f2ee7fec 00000000 02100800 00000000 00000000 f2ee7fb4
 c0425f1a 00000000 00000000 00000000 00000000 00000001 00200002 00000000
Call Trace:
 [<c040b47b>] xsave_init+0x25/0x27
 [<c040a4fd>] fpu_init+0xb3/0xbb
 [<c040f40d>] cpu_init+0x20e/0x225
 [<c0425f1a>] start_secondary+0xe/0x162
Code: 64 8b 15 88 d0 43 c1 89 d0 0d 00 00 04 00 55 39 d0 89 e5 74 09 64 a3 88 d0 43 c1 0f 22 e0 8b 15 3c c9 44 c1 31 c9 a1 38 c9 44 c1 <0f> 01 d1 5d c3 55 89 e5 57 56 53 3e 8d 74 26 00 83 cb ff 89 d7
EIP: [<c040ab1f>] xstate_enable+0x2b/0x30 SS:ESP 0068:f2ee7f58
---[ end trace 9afa73ad583edc40 ]---
Kernel panic - not syncing: Attempted to kill the idle task!


Interesting that the fault happened at "<0f> 01 d1" and that matches:

static inline void xsetbv(u32 index, u64 value)
{
	u32 eax = value;
	u32 edx = value >> 32;

	asm volatile(".byte 0x0f,0x01,0xd1" /* xsetbv */
		     : : "a" (eax), "d" (edx), "c" (index));
}


Doing a bisect, it ended on this commit:

commit 1e02ce4cccdcb9688386e5b8d2c9fa4660b45389
Author: Andy Lutomirski <luto@amacapital.net>
Date:   Fri Oct 24 15:58:08 2014 -0700

    x86: Store a per-cpu shadow copy of CR4


Reverting it fixed the regression, and my box can happily hotplug again!

-- Steve

Re: [REGRESSION][4.0-rc1] My i386 fails to do CPU hotplug

[Linus Torvalds]

On Fri, Feb 27, 2015 at 9:52 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Interesting that the fault happened at "<0f> 01 d1" and that matches:
>
> static inline void xsetbv(u32 index, u64 value)

Xsetbv is only available to CPU's that support xsave, and if
cr.osxsave is set (BIT 18). But you have CR4: 000000b0

Considering that xstate_enable() looks like this:

        cr4_set_bits(X86_CR4_OSXSAVE);
        xsetbv(XCR_XFEATURE_ENABLED_MASK, pcntxt_mask);

I think it's safe to say that cr4_set_bits() is buggered.

Your "Code:" line actually gives the whole disassembly:

   0: 64 8b 15 88 d0 43 c1 mov    %fs:0xc143d088,%edx
   7: 89 d0                 mov    %edx,%eax
   9: 0d 00 00 04 00       or     $0x40000,%eax
   e: 55                   push   %ebp
   f: 39 d0                 cmp    %edx,%eax
  11: 89 e5                 mov    %esp,%ebp
  13: 74 09                 je     0x1e
  15: 64 a3 88 d0 43 c1     mov    %eax,%fs:0xc143d088
  1b: 0f 22 e0             mov    %eax,%cr4
  1e: 8b 15 3c c9 44 c1     mov    0xc144c93c,%edx
  24: 31 c9                 xor    %ecx,%ecx
  26: a1 38 c9 44 c1       mov    0xc144c938,%eax
  2b:* 0f 01 d1             xsetbv <-- trapping instruction
  2e: 5d                   pop    %ebp
  2f: c3                   ret

and that actually looks right (setting bit 18 in the cached copy,
writing it back to cr4 if it's different), which makes me suspect that
the problem is that the cached copy was garbage.

Presumably there is some initialization missing - I suspect that the
cached value of cr4 is from the *previous* time the CPU was up, and we
don't correctly initialize the cached copy at early CPU bringup.

Andy, over to you,

                         Linus

[PATCH] x86: Init per-cpu shadow copy of CR4 for i386 too

[Steven Rostedt]

On Fri, 27 Feb 2015 10:59:52 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:
 
> Presumably there is some initialization missing - I suspect that the
> cached value of cr4 is from the *previous* time the CPU was up, and we
> don't correctly initialize the cached copy at early CPU bringup.

Found it. The problem is that there's two cpu_init()s in common.c. Andy
only added the cr4 init to one of them.


x86: Init per-cpu shadow copy of CR4 for i386 too

The commit 1e02ce4cccdc "x86: Store a per-cpu shadow copy of CR4" added
a shadow CR4 such that reads and writes that do not modify the CR4
execute much faster than always reading the register itself.
The change modified cpu_init() in common.c, so that the shadow CR4 gets
initialized before anything uses it. Unfortunately, there's two
cpu_init()s in common.c. There's one for x86_64 and one for i386. The
commit only added the shadow init to x86_64. The i386 needs the init
too.

Link: http://lkml.kernel.org/r/20150227125208.71c36402@gandalf.local.home
Fixes: 1e02ce4cccdc "x86: Store a per-cpu shadow copy of CR4"
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index b5c8ff5e9dfc..2346c95c6ab1 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1396,6 +1396,12 @@ void cpu_init(void)
 
 	wait_for_master_cpu(cpu);
 
+	/*
+	 * Initialize the CR4 shadow before doing anything that could
+	 * try to read it.
+	 */
+	cr4_init_shadow();
+
 	show_ucode_info_early();
 
 	printk(KERN_INFO "Initializing CPU#%d\n", cpu);

[PATCH] x86_32: Initialize the cr4 shadow on cpu init

[Andy Lutomirski]

32-bit secondary cpus had uninitialized cr4 shadows, causing random
failures.

Reported-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---

This is embarrassing.  I must have gotten lucky testing it.

arch/x86/kernel/cpu/common.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index b5c8ff5e9dfc..2346c95c6ab1 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1396,6 +1396,12 @@ void cpu_init(void)
 
 	wait_for_master_cpu(cpu);
 
+	/*
+	 * Initialize the CR4 shadow before doing anything that could
+	 * try to read it.
+	 */
+	cr4_init_shadow();
+
 	show_ucode_info_early();
 
 	printk(KERN_INFO "Initializing CPU#%d\n", cpu);
-- 
2.1.0

Re: [PATCH] x86: Init per-cpu shadow copy of CR4 for i386 too

[Andy Lutomirski]

On Fri, Feb 27, 2015 at 11:50 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 27 Feb 2015 10:59:52 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> Presumably there is some initialization missing - I suspect that the
>> cached value of cr4 is from the *previous* time the CPU was up, and we
>> don't correctly initialize the cached copy at early CPU bringup.
>
> Found it. The problem is that there's two cpu_init()s in common.c. Andy
> only added the cr4 init to one of them.
>
>
> x86: Init per-cpu shadow copy of CR4 for i386 too
>
> The commit 1e02ce4cccdc "x86: Store a per-cpu shadow copy of CR4" added
> a shadow CR4 such that reads and writes that do not modify the CR4
> execute much faster than always reading the register itself.
> The change modified cpu_init() in common.c, so that the shadow CR4 gets
> initialized before anything uses it. Unfortunately, there's two
> cpu_init()s in common.c. There's one for x86_64 and one for i386. The
> commit only added the shadow init to x86_64. The i386 needs the init
> too.
>
> Link: http://lkml.kernel.org/r/20150227125208.71c36402@gandalf.local.home
> Fixes: 1e02ce4cccdc "x86: Store a per-cpu shadow copy of CR4"
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Looks suspiciously identical to my patch :)

Acked-by: Andy Lutomirski <luto@amacapital.net>

> ---
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index b5c8ff5e9dfc..2346c95c6ab1 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1396,6 +1396,12 @@ void cpu_init(void)
>
>         wait_for_master_cpu(cpu);
>
> +       /*
> +        * Initialize the CR4 shadow before doing anything that could
> +        * try to read it.
> +        */
> +       cr4_init_shadow();
> +
>         show_ucode_info_early();
>
>         printk(KERN_INFO "Initializing CPU#%d\n", cpu);



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH] x86: Init per-cpu shadow copy of CR4 for i386 too

[Steven Rostedt]

On Fri, 27 Feb 2015 11:54:33 -0800
Andy Lutomirski <luto@amacapital.net> wrote:

> Looks suspiciously identical to my patch :)

Then it must be correct!

> 
> Acked-by: Andy Lutomirski <luto@amacapital.net>

It was a race to see who could do the obvious first ;-)

-- Steve

Re: [PATCH] x86_32: Initialize the cr4 shadow on cpu init

[Steven Rostedt]

On Fri, 27 Feb 2015 11:53:39 -0800
Andy Lutomirski <luto@amacapital.net> wrote:

> This is embarrassing.  I must have gotten lucky testing it.
> 

The thing is, it booted fine on all my tests. It wasn't until I did
hotplug where it crashed.

I'm not sure what's in CR4 when the CPU comes up, but perhaps zero is
fine to start out and the writes to CR4 update makes the shadow and reg
the same. It's when we hotplug that the two become out of sync.

Just my theory.

-- Steve

Re: [PATCH] x86_32: Initialize the cr4 shadow on cpu init

[Andy Lutomirski]

On Fri, Feb 27, 2015 at 12:19 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 27 Feb 2015 11:53:39 -0800
> Andy Lutomirski <luto@amacapital.net> wrote:
>
>> This is embarrassing.  I must have gotten lucky testing it.
>>
>
> The thing is, it booted fine on all my tests. It wasn't until I did
> hotplug where it crashed.
>
> I'm not sure what's in CR4 when the CPU comes up, but perhaps zero is
> fine to start out and the writes to CR4 update makes the shadow and reg
> the same. It's when we hotplug that the two become out of sync.
>

I bet that having the shadow set to zero is fine because the first
attempt to set a bit in cr4 will clear all the other bits without
harmful side effects.  But if the shadow starts out at 0xffffffff or
something like that, then the early attempts to set bits will have no
effect and we'll fail.

--Andy

> Just my theory.
>
> -- Steve



-- 
Andy Lutomirski
AMA Capital Management, LLC

[tip:x86/urgent] x86: Init per-cpu shadow copy of CR4 on 32-bit CPUs too

[tip-bot for Steven Rostedt]

Commit-ID:  5b2bdbc84556774afbe11bcfd24c2f6411cfa92b
Gitweb:     http://git.kernel.org/tip/5b2bdbc84556774afbe11bcfd24c2f6411cfa92b
Author:     Steven Rostedt <rostedt@goodmis.org>
AuthorDate: Fri, 27 Feb 2015 14:50:19 -0500
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 28 Feb 2015 08:04:20 +0100

x86: Init per-cpu shadow copy of CR4 on 32-bit CPUs too

Commit:

   1e02ce4cccdc ("x86: Store a per-cpu shadow copy of CR4")

added a shadow CR4 such that reads and writes that do not
modify the CR4 execute much faster than always reading the
register itself.

The change modified cpu_init() in common.c, so that the
shadow CR4 gets initialized before anything uses it.

Unfortunately, there's two cpu_init()s in common.c. There's
one for 64-bit and one for 32-bit. The commit only added
the shadow init to the 64-bit path, but the 32-bit path
needs the init too.

Link: http://lkml.kernel.org/r/20150227125208.71c36402@gandalf.local.home Fixes: 1e02ce4cccdc "x86: Store a per-cpu shadow copy of CR4"
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Andy Lutomirski <luto@amacapital.net>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/20150227145019.2bdd4354@gandalf.local.home
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/cpu/common.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index b5c8ff5..2346c95 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1396,6 +1396,12 @@ void cpu_init(void)
 
 	wait_for_master_cpu(cpu);
 
+	/*
+	 * Initialize the CR4 shadow before doing anything that could
+	 * try to read it.
+	 */
+	cr4_init_shadow();
+
 	show_ucode_info_early();
 
 	printk(KERN_INFO "Initializing CPU#%d\n", cpu);