Skip to content

Commit

Permalink
x86/fpu: Fix FPU state save area alignment bug
Browse files Browse the repository at this point in the history
On most configs task-struct is cache line aligned, which makes
the XSAVE area's 64-byte required alignment work out fine.

But on some .config's task_struct is aligned only to 16 bytes
(enforced by ARCH_MIN_TASKALIGN), which makes things like
fpu__copy() (that XSAVEOPT uses) not work so well.

I broke this in:

  7366ed7 ("x86/fpu: Simplify FPU handling by embedding the fpstate in task_struct (again)")

which embedded the fpstate in the task_struct.

The alignment requirements of the FPU code were originally present
in ARCH_MIN_TASKALIGN, which still has a value of 16, which was the
alignment requirement of the FPU state area prior XSAVE. But this
link was not documented (and not required) and the link got lost
when the FPU state area was made dynamic years ago.

With XSAVEOPT the minimum alignment requirment went up to 64 bytes,
and the embedding of the FPU state area in task_struct exposed it
again - and '16' was not increased to '64'.

So fix this bug, but also try to address the underlying lost link
of information that made it easier to happen:

  - document ARCH_MIN_TASKALIGN a bit better

  - use alignof() to recover the current alignment requirements.
    This would work in the future as well, should the alignment
    requirements go up to 128 bytes with things like AVX512.

( We should probably also use the vSMP alignment rules for all
  of x86, but that's for another patch. )

Reported-by: Peter Zijlstra <peterz@infradead.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
  • Loading branch information
Ingo Molnar committed May 25, 2015
1 parent b54b4bb commit b8c1b8e
Showing 1 changed file with 6 additions and 1 deletion.
7 changes: 6 additions & 1 deletion arch/x86/include/asm/processor.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,16 @@ static inline void *current_text_addr(void)
return pc;
}

/*
* These alignment constraints are for performance in the vSMP case,
* but in the task_struct case we must also meet hardware imposed
* alignment requirements of the FPU state:
*/
#ifdef CONFIG_X86_VSMP
# define ARCH_MIN_TASKALIGN (1 << INTERNODE_CACHE_SHIFT)
# define ARCH_MIN_MMSTRUCT_ALIGN (1 << INTERNODE_CACHE_SHIFT)
#else
# define ARCH_MIN_TASKALIGN 16
# define ARCH_MIN_TASKALIGN __alignof__(union fpregs_state)
# define ARCH_MIN_MMSTRUCT_ALIGN 0
#endif

Expand Down

0 comments on commit b8c1b8e

Please sign in to comment.