Non-booting current Linus' tree
Hello,
so I was wondering why I cannot boot current Linus' tree in my kvm
instance. The boot dies after writing "Booting the kernel" message. After some bisection I have identified the culprit is in commit 91a8c2a5b43fc4be4adb4bda50cd331697e289e0 (x86/fpu: Clean up and fix MXCSR handling). After applying that commit I start to get oopses like:
general protection fault: 0000 [#1] SMP Modules linked in: CPU: 0 PID: 0 Comm: swapper Not tainted 4.1.0-rc4-xen+ #18 Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 task: ffffffff8180d4c0 ti: ffffffff81800000 task.ti: ffffffff81800000 RIP: 0010:[<ffffffff81048a6c>] [<ffffffff81048a6c>] mxcsr_feature_mask_init+0x1c/0x40 RSP: 0000:ffffffff81803ba8 EFLAGS: 00010082 RAX: 0000000000000000 RBX: 00000000ffff8800 RCX: 0000000000000000 RDX: 0020400000000000 RSI: 0000000000000000 RDI: ffffffff81803da8 RBP: ffffffff81803da8 R08: 7f0089011d402087 R09: 00000000ffff8800 R10: 7f0089011d402087 R11: 00000000ffff8800 R12: ffff88007f004000 R13: 0000000000000000 R14: 0000000000000004 R15: 0000000000000008 FS: 0000000000000000(0000) GS:ffff88007f000000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: ffff88000269a000 CR3: 0000000001808000 CR4: 00000000000006b0 Stack: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 0000000000000000 0000000000000000 0000000000000000 0000000000000000 0000000000000000 0000000000000000 0000000000000000 0000000000000000 Call Trace: [<ffffffff81048beb>] fpu__init_system+0x2b/0x190 [<ffffffff81048d5e>] fpu__cpu_init+0xe/0x10 [<ffffffff8104eac3>] cpu_init+0x3a3/0x430 [<ffffffff810794b1>] ? fill_pte+0x31/0x140 [<ffffffff8107975d>] ? set_pte_vaddr_pud+0x4d/0x60 [<ffffffff814058c0>] ? do_softirq_own_stack+0x30/0x30 [<ffffffff81881519>] trap_init+0x4bb/0x602 [<ffffffff8189d583>] ? inode_init_early+0x5a/0x98 [<ffffffff8187efea>] start_kernel+0x265/0x4e1 [<ffffffff8187eb7d>] ? set_init_arg+0x6a/0x6a [<ffffffff8187e565>] x86_64_start_reservations+0x1b/0x32 [<ffffffff8187e6c3>] x86_64_start_kernel+0x147/0x156 [<ffffffff8187e120>] ? early_idt_handlers+0x120/0x120 Code: 05 00 c9 c3 66 2e 0f 1f 84 00 00 00 00 00 90 55 31 c0 b9 40 00 00 00 48 89 e5 48 81 ec 00 02 00 00 48 8d bd 00 fe ff ff f3 48 ab <0f> ae 85 00 fe ff ff 8b 85 1c fe ff ff ba bf ff 00 00 85 c0 0f RIP [<ffffffff81048a6c>] mxcsr_feature_mask_init+0x1c/0x40 RSP <ffffffff81803ba8>
And indeed the oops happens at:
2b:* 0f ae 85 00 fe ff ff fxsave -0x200(%rbp) <--
trapping instruction
Because the address isn't 32-byte aligned (which I assume is the requirement from looking into the code). So clearly my gcc messed up and miscompiled the thing by ignoring the alignment attribute. Now my build host is rather old installation (SLES 11 SP3) running gcc 4.3.4 but there are quite a few installations of it still running. So do we care or should I just upgrade the build host?
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 5 lines · Jan Kara
so I was wondering why I cannot boot current Linus' tree in my kvm
instance. The boot dies after writing "Booting the kernel" message. After some bisection I have identified the culprit is in commit91a8c2a5b43fc4be4adb4bda50cd331697e289e0(x86/fpu: Clean up and fix MXCSR handling). After applying that commit I start to get oopses like:
At least you get oopses. My hardware simply refused to spit out anything useful and my bisect just ended up at the same commit.
Quoted 10 lines
And indeed the oops happens at:
2b:* 0f ae 85 00 fe ff ff fxsave -0x200(%rbp) <--
trapping instructionBecause the address isn't 32-byte aligned (which I assume is the requirement from looking into the code). So clearly my gcc messed up and miscompiled the thing by ignoring the alignment attribute. Now my build host is rather old installation (SLES 11 SP3) running gcc 4.3.4 but there are quite a few installations of it still running. So do we care or should I just upgrade the build host?
My gcc is old as well. 4.3.0-8.
Thanks,
tglx
Quoted 4 lines · Jan Kara
Because the address isn't 32-byte aligned (which I assume is the requirement from looking into the code). So clearly my gcc messed up and miscompiled the thing by ignoring the alignment attribute.
Well, it's probably a mistake to begin with to expect gcc to get stack alignment right. Especially since we tell gcc to not align the stack as much as it usually wants to with -mpreferred-stack-boundary=2.
The code is broken in other ways too. The fxsave alignment isn't 32 bytes. It's 16 bytes. And that's already encoded in the "struct fxregs_state", so adding that extra alignment is just bogus crud anyway.
So I seriously think that the whole commit 91a8c2a5b43f is just fundamentally broken and should probably be reverted. The theoretical issue it fixes is a smaller problem than the broken code it introduces (and I'm not just talking about gcc bugs).
Plus the code just sets up and writes to a global variable *anyway*, so the alleged race with using a static allocation is bogus: if that's a real concern, then the code is fundamentally buggy in other ways anyway.
I don't think you can boot with different CPU's having different mxcsr features anyway, so..
Ingo?
Linus
PATCH 1/1 x86/fpu: Fix boot crash in the early FPU code
Quoted 6 lines · Linus Torvalds
Quoted 4 lines · Jan Kara
Because the address isn't 32-byte aligned (which I assume is the requirement from looking into the code). So clearly my gcc messed up and miscompiled the thing by ignoring the alignment attribute.
Quoted 7 lines
Well, it's probably a mistake to begin with to expect gcc to get stack alignment right. Especially since we tell gcc to not align the stack as much as it usually wants to with -mpreferred-stack-boundary=2.
The code is broken in other ways too. The fxsave alignment isn't 32 bytes. It's 16 bytes. And that's already encoded in the "struct
fxregs_state", so adding that extra alignment is just bogus crud anyway.
Yeah.
Quoted 8 lines
So I seriously think that the whole commit
91a8c2a5b43fis just fundamentally broken and should probably be reverted. The theoretical issue it fixes is a smaller problem than the broken code it introduces (and I'm not just talking about gcc bugs).Plus the code just sets up and writes to a global variable *anyway*, so the alleged race with using a static allocation is bogus: if that's a real concern, then the code is fundamentally buggy in other ways anyway.
So this function used to be called from multiple sites, from per CPU init for example, with unclear semantics so when I wrote the patch the claimed parallelism could occur, my worry was things like (future ...) parallel bootup sequences writing to the same variable at once.
Even in that hypothetical case it _should_ work even in the racy case but it seemed like a neat idea to move it to the stack and eliminate any possibility of interaction. Today it does not look like such a neat idea anymore!
But note that the FPU series cleaned the messy init dependencies up, and now this function (called fpu__init_system_mxcsr()) is only called once, during system init
- and the name now clearly reflects that. So there are no race worries whatsoever.
Quoted 2 lines
I don't think you can boot with different CPU's having different mxcsr features anyway, so..
Yes.
Also, because the function is now __init, this can be __initdata - further weakening the original 'this is better on the stack' argument.
Btw., is it a GCC bug or a known GCC property that a structure with a 16-byte alignment attribute does not get properly aligned on the stack?
In any case the patch below should fix the bogosities. Only very lighly tested.
Thanks,
Ingo
===========================>
Quoted 1 line
From
2d1bba21cb64d9181eea6a9ec3083983a77678bfMon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Sat, 4 Jul 2015 09:40:55 +0200
Subject: [PATCH] x86/fpu: Fix boot crash in the early FPU code
Jan Kara and Thomas Gleixner reported boot crashes in the FPU code:
general protection fault: 0000 [#1] SMP RIP: 0010:[<ffffffff81048a6c>] [<ffffffff81048a6c>] mxcsr_feature_mask_init+0x1c/0x40
2b:* 0f ae 85 00 fe ff ff fxsave -0x200(%rbp)
and bisected it down to the following FPU commit:
91a8c2a5b43f ("x86/fpu: Clean up and fix MXCSR handling")
The reason is that the on-stack FPU registers state variable, used by the FXSAVE instruction, did not have the required minimum alignment of 16 bytes, causing the general protection fault.
This is most likely a GCC bug in older GCC versions, but the offending commit also added a bogus extra 32-byte alignment (which GCC ignored too).
So fix this bug by making the variable static again, but also mark it __initdata this time, because fpu__init_system_mxcsr() is now an __init function.
--- arch/x86/kernel/fpu/init.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
arch/x86/kernel/fpu/init.c +4 -3
@@ -95,11 +95,12 @@ static void __init fpu__init_system_mxcsr(void)unsigned int mask =0 ;if (cpu_has_fxsr) { -struct fxregs_state fx_tmp__aligned(32) = { }; +/* Static because GCC does not get 16-byte stack alignment right: */ +static struct fxregs_state fxregs__initdata ; - asmvolatile ("fxsave %0" :"+m" (fx_tmp)); + asmvolatile ("fxsave %0" :"+m" (fxregs)); - mask = fx_tmp.mxcsr_mask; + mask = fxregs.mxcsr_mask;/* * If zero then use the default features mask,
tip-bot for Ingo Molnar
merge bot[tip:x86/urgent] x86/fpu: Fix boot crash in the early FPU code
x86/fpu: Fix boot crash in the early FPU code
Commit-ID: b96fecbfa8c88b057e2bbf10021521c232bb3650 Gitweb: http://git.kernel.org/tip/b96fecbfa8c88b057e2bbf10021521c232bb3650 Author: Ingo Molnar <[email protected]> AuthorDate: Sat, 4 Jul 2015 09:58:19 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Sat, 4 Jul 2015 10:05:56 +0200
x86/fpu: Fix boot crash in the early FPU code
Jan Kara and Thomas Gleixner reported boot crashes in the FPU code:
general protection fault: 0000 [#1] SMP RIP: 0010:[<ffffffff81048a6c>] [<ffffffff81048a6c>] mxcsr_feature_mask_init+0x1c/0x40
2b:* 0f ae 85 00 fe ff ff fxsave -0x200(%rbp)
and bisected it down to the following FPU commit:
91a8c2a5b43f ("x86/fpu: Clean up and fix MXCSR handling")
The reason is that the on-stack FPU registers state variable, used by the FXSAVE instruction, did not have the required minimum alignment of 16 bytes, causing the general protection fault.
This is most likely a GCC bug in older GCC versions, but the offending commit also added a bogus extra 32-byte alignment (which GCC ignored too).
So fix this bug by making the variable static again, but also mark it __initdata this time, because fpu__init_system_mxcsr() is now an __init function.
--- arch/x86/kernel/fpu/init.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
arch/x86/kernel/fpu/init.c +4 -3
@@ -95,11 +95,12 @@ static void __init fpu__init_system_mxcsr(void)unsigned int mask =0 ;if (cpu_has_fxsr) { -struct fxregs_state fx_tmp__aligned(32) = { }; +/* Static because GCC does not get 16-byte stack alignment right: */ +static struct fxregs_state fxregs__initdata ; - asmvolatile ("fxsave %0" :"+m" (fx_tmp)); + asmvolatile ("fxsave %0" :"+m" (fxregs)); - mask = fx_tmp.mxcsr_mask; + mask = fxregs.mxcsr_mask;/* * If zero then use the default features mask,
Re: [PATCH] x86/fpu: Fix boot crash in the early FPU code
In reply to Ingo MolnarQuoted 2 lines · Ingo Molnar
Btw., is it a GCC bug or a known GCC property that a structure with a 16-byte alignment attribute does not get properly aligned on the stack?
What's the stack alignment mandated by the x86 psABI? Is that anything
beyond what the PUSH instructions imply (i.e. 4 bytes for 32-bit and 8 bytes for 64-bit targets)? Otherwise GCC might not be smart enough to apply the right subtract and mask operations to the stack/frame pointer. This might even have been documented with the alignment attribute.
Quoted 3 lines
So fix this bug by making the variable static again, but also mark it
__initdatathis time, becausefpu__init_system_mxcsr()is now an__initfunction.
You could have used `alloca' and manually aligned the structure within
the area obtained too (to avoid static storage if desired).
FWIW,
Maciej
Re: [PATCH] x86/fpu: Fix boot crash in the early FPU code
In reply to Ingo MolnarQuoted 2 lines · Ingo Molnar
Reported-and-bisected-by Jan Kara <[email protected]>Reported-and-bisected-by Thomas Gleixner <[email protected]>
Tested-by-me
Re: [PATCH] x86/fpu: Fix boot crash in the early FPU code
Quoted 5 lines · Thomas Gleixner
Quoted 2 lines · Ingo Molnar
Reported-and-bisected-by Jan Kara <[email protected]>Reported-and-bisected-by Thomas Gleixner <[email protected]>Tested-by-me
Ok, thanks, I've pushed it out!
Ingo
Quoted 10 lines · Linus Torvalds
Quoted 4 lines · Jan Kara
Because the address isn't 32-byte aligned (which I assume is the requirement from looking into the code). So clearly my gcc messed up and miscompiled the thing by ignoring the alignment attribute.
Well, it's probably a mistake to begin with to expect gcc to get stack alignment right. Especially since we tell gcc to not align the stack as much as it usually wants to with -mpreferred-stack-boundary=2.
Are you sure?
The 64-bit part of arch/x86/Makefile contains:
# Use -mpreferred-stack-boundary=3 if supported. KBUILD_CFLAGS += $(call cc-option,-mpreferred-stack-boundary=3)
but make V=1 isn't showing -mpreferred-stack-boundary=3. This may be because:
error: -mpreferred-stack-boundary=3 is not between 4 and 12
extern int bar(const char *);
^
I found:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53383
and, indeed, -mno-sse -mpreferred-stack-boundary=3 is accepted. This make me think that the makefile is broken -- cc-option isn't working because it doesn't check -mpreferred-stack-boundary in conjunction with -mno-sse.
Given that -mpreferred-stack-boundary=3 doesn't appear to being set, I think this is really our bug: our asm code makes no effort to align the stack to a 16-byte boundary as required by the ABI, and we're not overriding the ABI correctly.
I'll send a patch for the Makefile issue.
--Andy