[PATCH] x86/fpu: Fix boot crash in the early FPU code

[Ingo Molnar]

* Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Fri, Jul 3, 2015 at 8:23 AM, Jan Kara <jack@xxxxxxx> wrote:
> >
> > 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.

Yeah.

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

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.

> 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

===========================>