big x86 FPU code rewrite

[Second part of the series - Gmail didn't like me sending so many mails.]

Over the past 10 years the x86 FPU has organically grown into somewhat of a spaghetti monster that few (if any) kernel developers understand and which code few people enjoy to hack.

Many people suggested over the years that it needs a major cleanup, and some time ago I went "what the heck" and started doing it step by step to see where it leads - it cannot be that hard!

Three weeks and 200+ patches later I think I have to admit that I seriously underestimated the magnitude of the project! ;-)

This work in progress series is large, but it I think makes the code maintainable and hackable again. It's pretty complete, as per the 9 high level goals laid out further below. Individual patches are all finegrained, so should be easy to review - Boris Petkov already reviewed most of the patches so they are not entirely raw.

Individual patches have been tested heavily for bisectability, they were both build and boot on a relatively wide range of x86 hardware that I have access to. But nevertheless the changes are pretty invasive, so I'd expect there to be test failures.

This is the only time I intend to post them to lkml in their entirety, to not spam lkml too much. (Future additions will be posted as delta series.)

I'd like to ask interested people to test this tree, and to comment on the patches. The changes can be found in the following Git tree:

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git tmp.fpu

(The tree might be rebased, depending on feedback.)

Here are the main themes that motivated most of the changes:

1)

I collected all FPU code into arch/x86/kernel/fpu/*.c and split it all up into the following, topically organized source code files:

  -rw-rw-r-- 1 mingo mingo  1423 May  5 16:36 arch/x86/kernel/fpu/bugs.c
  -rw-rw-r-- 1 mingo mingo 12206 May  5 16:36 arch/x86/kernel/fpu/core.c
  -rw-rw-r-- 1 mingo mingo  7342 May  5 16:36 arch/x86/kernel/fpu/init.c
  -rw-rw-r-- 1 mingo mingo 10909 May  5 16:36 arch/x86/kernel/fpu/measure.c
  -rw-rw-r-- 1 mingo mingo  9012 May  5 16:36 arch/x86/kernel/fpu/regset.c
  -rw-rw-r-- 1 mingo mingo 11188 May  5 16:36 arch/x86/kernel/fpu/signal.c
  -rw-rw-r-- 1 mingo mingo 10140 May  5 16:36 arch/x86/kernel/fpu/xstate.c

Similarly I've collected and split up all FPU related header files, and organized them topically:

  -rw-rw-r-- 1 mingo mingo  1690 May  5 16:35 arch/x86/include/asm/fpu/api.h
  -rw-rw-r-- 1 mingo mingo 12937 May  5 16:36 arch/x86/include/asm/fpu/internal.h
  -rw-rw-r-- 1 mingo mingo   278 May  5 16:36 arch/x86/include/asm/fpu/measure.h
  -rw-rw-r-- 1 mingo mingo   596 May  5 16:35 arch/x86/include/asm/fpu/regset.h
  -rw-rw-r-- 1 mingo mingo  1013 May  5 16:35 arch/x86/include/asm/fpu/signal.h
  -rw-rw-r-- 1 mingo mingo  8137 May  5 16:36 arch/x86/include/asm/fpu/types.h
  -rw-rw-r-- 1 mingo mingo  5691 May  5 16:36 arch/x86/include/asm/fpu/xstate.h

<fpu/api.h> is the only 'public' API left, used in various drivers.

I decoupled drivers and non-FPU x86 code from various FPU internals.

2)

I renamed various internal data types, APIs and helpers, and organized its support functions accordingly.

For example, all functions that deal with copying FPU registers in and out of the FPU, are now named consistently:

copy_fxregs_to_kernel() # was: fpu_fxsave()
copy_xregs_to_kernel() # was: xsave_state()

copy_kernel_to_fregs() # was: frstor_checking()
copy_kernel_to_fxregs() # was: fxrstor_checking()
copy_kernel_to_xregs() # was: fpu_xrstor_checking()
copy_kernel_to_xregs_booting() # was: xrstor_state_booting()

copy_fregs_to_user() # was: fsave_user()
copy_fxregs_to_user() # was: fxsave_user()
copy_xregs_to_user() # was: xsave_user()

copy_user_to_fregs() # was: frstor_user()
copy_user_to_fxregs() # was: fxrstor_user()
copy_user_to_xregs() # was: xrestore_user()
copy_user_to_fpregs_zeroing() # was: restore_user_xstate()

'xregs' stands for registers supported by XSAVE
'fxregs' stands for registers supported by FXSAVE
'fregs' stands for registers supported by FSAVE
'fpregs' stands for generic FPU registers.

Similarly, the high level FPU functions got reorganized as well:

    extern void fpu__activate_curr(struct fpu *fpu);
    extern void fpu__activate_stopped(struct fpu *fpu);
    extern void fpu__save(struct fpu *fpu);
    extern void fpu__restore(struct fpu *fpu);
    extern int  fpu__restore_sig(void __user *buf, int ia32_frame);
    extern void fpu__drop(struct fpu *fpu);
    extern int  fpu__copy(struct fpu *dst_fpu, struct fpu *src_fpu);
    extern void fpu__clear(struct fpu *fpu);
    extern int  fpu__exception_code(struct fpu *fpu, int trap_nr);

Those functions that used to take a task_struct argument now take the more limited 'struct fpu' argument, and their naming is consistent and logical as well.

Likewise, the FP state data types are now consistently named as well:

    struct fregs_state;
    struct fxregs_state;
    struct swregs_state;
    struct xregs_state;

    union fpregs_state;

3)

Various core data types got streamlined around four byte flags in 'struct fpu':

fpu->fpstate_active # was: tsk->flags & PF_USED_MATH
fpu->fpregs_active # was: fpu->has_fpu
fpu->last_cpu
fpu->counter

which now fit into a single word.

4)

task->thread.fpu->state got embedded again, as task->thread.fpu.state. This eliminated a lot of awkward late dynamic memory allocation of FPU state and the problematic handling of failures.

Note that while the allocation is static right now, this is a WIP interim
state: we can still do dynamic allocation of FPU state, by moving the FPU state last in task_struct and then allocating task_struct accordingly.

5)

The amazingly convoluted init dependencies got sorted out, into two cleanly separated families of initialization functions: the fpu__init_system_*() functions, and the fpu__init_cpu_*() functions.

This allowed the removal of various __init annotation hacks and obscure boot time checks.

6)

Decoupled the FPU core from the save code. xsave.c and xsave.h got shrunk quite a bit, and it now hosts only XSAVE/etc. related functionality, not generic FPU handling functions.

7)

Added a ton of comments explaining how things works and why, hopefully making this code accessible to everyone interested.

8)

Added FPU debugging code (CONFIG_X86_DEBUG_FPU=y) and added an FPU hw benchmarking subsystem (CONFIG_X86_DEBUG_FPU_MEASUREMENTS=y), which performs boot time measurements like:

x86/fpu:################################################################## x86/fpu: Running FPU performance measurement suite (cache hot):
x86/fpu: Cost of: null : 108 cycles

  x86/fpu:########  CPU instructions:           ############################
  x86/fpu: Cost of: NOP                         insn          :     0 cycles
  x86/fpu: Cost of: RDTSC                       insn          :    12 cycles
  x86/fpu: Cost of: RDMSR                       insn          :   100 cycles
  x86/fpu: Cost of: WRMSR                       insn          :   396 cycles
  x86/fpu: Cost of: CLI                         insn  same-IF :     0 cycles
  x86/fpu: Cost of: CLI                         insn  flip-IF :     0 cycles
  x86/fpu: Cost of: STI                         insn  same-IF :     0 cycles
  x86/fpu: Cost of: STI                         insn  flip-IF :     0 cycles
  x86/fpu: Cost of: PUSHF                       insn          :     0 cycles
  x86/fpu: Cost of: POPF                        insn  same-IF :    20 cycles
  x86/fpu: Cost of: POPF                        insn  flip-IF :    28 cycles
  x86/fpu:########  IRQ save/restore APIs:      ############################
  x86/fpu: Cost of: local_irq_save()            fn            :    20 cycles
  x86/fpu: Cost of: local_irq_restore()         fn    same-IF :    24 cycles
  x86/fpu: Cost of: local_irq_restore()         fn    flip-IF :    28 cycles
  x86/fpu: Cost of: irq_save()+restore()        fn    same-IF :    48 cycles
  x86/fpu: Cost of: irq_save()+restore()        fn    flip-IF :    48 cycles
  x86/fpu:########  locking APIs:               ############################
  x86/fpu: Cost of: smp_mb()                    fn            :    40 cycles
  x86/fpu: Cost of: cpu_relax()                 fn            :     8 cycles
  x86/fpu: Cost of: spin_lock()+unlock()        fn            :    64 cycles
  x86/fpu: Cost of: read_lock()+unlock()        fn            :    76 cycles
  x86/fpu: Cost of: write_lock()+unlock()       fn            :    52 cycles
  x86/fpu: Cost of: rcu_read_lock()+unlock()    fn            :    16 cycles
  x86/fpu: Cost of: preempt_disable()+enable()  fn            :    20 cycles
  x86/fpu: Cost of: mutex_lock()+unlock()       fn            :    56 cycles
  x86/fpu:########  MM instructions:            ############################
  x86/fpu: Cost of: __flush_tlb()               fn            :   132 cycles
  x86/fpu: Cost of: __flush_tlb_global()        fn            :   920 cycles
  x86/fpu: Cost of: __flush_tlb_one()           fn            :   288 cycles
  x86/fpu: Cost of: __flush_tlb_range()         fn            :   412 cycles
  x86/fpu:########  FPU instructions:           ############################
  x86/fpu: Cost of: CR0                         read          :     4 cycles
  x86/fpu: Cost of: CR0                         write         :   208 cycles
  x86/fpu: Cost of: CR0::TS                     fault         :  1156 cycles
  x86/fpu: Cost of: FNINIT                      insn          :    76 cycles
  x86/fpu: Cost of: FWAIT                       insn          :     0 cycles
  x86/fpu: Cost of: FSAVE                       insn          :   168 cycles
  x86/fpu: Cost of: FRSTOR                      insn          :   160 cycles
  x86/fpu: Cost of: FXSAVE                      insn          :    84 cycles
  x86/fpu: Cost of: FXRSTOR                     insn          :    44 cycles
  x86/fpu: Cost of: FXRSTOR                     fault         :   688 cycles
  x86/fpu: Cost of: XSAVE                       insn          :   104 cycles
  x86/fpu: Cost of: XRSTOR                      insn          :    80 cycles
  x86/fpu: Cost of: XRSTOR                      fault         :   884 cycles

x86/fpu:##################################################################

Based on such measurements we'll be able to do performance tuning, set default policies and do optimizations in a more informed fashion, as the speed of various x86 hardware varies a lot.

9)

Reworked many ancient inlining and uninlining decisions based on modern principles.

Any feedback is welcome!

Thanks,

Ingo

=====
Ingo Molnar (208):
x86/fpu: Rename unlazy_fpu() to fpu__save()
x86/fpu: Add comments to fpu__save() and restrict its export
x86/fpu: Add debugging check to fpu__save()
x86/fpu: Rename fpu_detect() to fpu__detect()
x86/fpu: Remove stale init_fpu() prototype
x86/fpu: Split an fpstate_alloc_init() function out of init_fpu()
x86/fpu: Make init_fpu() static
x86/fpu: Rename init_fpu() to fpu__unlazy_stopped() and add debugging check x86/fpu: Optimize fpu__unlazy_stopped()
x86/fpu: Simplify fpu__unlazy_stopped()
x86/fpu: Remove fpu_allocated()
x86/fpu: Move fpu_alloc() out of line
x86/fpu: Rename fpu_alloc() to fpstate_alloc()
x86/fpu: Rename fpu_free() to fpstate_free()
x86/fpu: Rename fpu_finit() to fpstate_init()
x86/fpu: Rename fpu_init() to fpu__cpu_init()
x86/fpu: Rename init_thread_xstate() to fpstate_xstate_init_size()
x86/fpu: Move thread_info::fpu_counter into thread_info::fpu.counter
x86/fpu: Improve the comment for the fpu::counter field
x86/fpu: Move FPU data structures to asm/fpu_types.h
x86/fpu: Clean up asm/fpu/types.h
x86/fpu: Move i387.c and xsave.c to arch/x86/kernel/fpu/
x86/fpu: Fix header file dependencies of fpu-internal.h
x86/fpu: Split out the boot time FPU init code into fpu/init.c
x86/fpu: Remove unnecessary includes from core.c
x86/fpu: Move the no_387 handling and FPU detection code into init.c
x86/fpu: Remove the free_thread_xstate() complication
x86/fpu: Factor out fpu__flush_thread() from flush_thread()
x86/fpu: Move math_state_restore() to fpu/core.c
x86/fpu: Rename math_state_restore() to fpu__restore()
x86/fpu: Factor out the FPU bug detection code into fpu__init_check_bugs()
x86/fpu: Simplify the xsave_state*() methods
x86/fpu: Remove fpu_xsave()
x86/fpu: Move task_xstate_cachep handling to core.c
x86/fpu: Factor out fpu__copy()
x86/fpu: Uninline fpstate_free() and move it next to the allocation function x86/fpu: Make task_xstate_cachep static
x86/fpu: Make kernel_fpu_disable/enable() static
x86/fpu: Add debug check to kernel_fpu_disable()
x86/fpu: Add kernel_fpu_disabled()
x86/fpu: Remove __save_init_fpu()
x86/fpu: Move fpu_copy() to fpu/core.c
x86/fpu: Add debugging check to fpu_copy()
x86/fpu: Print out whether we are doing lazy/eager FPU context switches
x86/fpu: Eliminate the __thread_has_fpu() wrapper
x86/fpu: Change __thread_clear_has_fpu() to 'struct fpu' parameter
x86/fpu: Move 'PER_CPU(fpu_owner_task)' to fpu/core.c
x86/fpu: Change fpu_owner_task to fpu_fpregs_owner_ctx
x86/fpu: Remove 'struct task_struct' usage from __thread_set_has_fpu()
x86/fpu: Remove 'struct task_struct' usage from __thread_fpu_end()
x86/fpu: Remove 'struct task_struct' usage from __thread_fpu_begin()
x86/fpu: Open code PF_USED_MATH usages
x86/fpu: Document fpu__unlazy_stopped()
x86/fpu: Get rid of PF_USED_MATH usage, convert it to fpu->fpstate_active
x86/fpu: Remove 'struct task_struct' usage from drop_fpu()
x86/fpu: Remove task_disable_lazy_fpu_restore()
x86/fpu: Use 'struct fpu' in fpu_lazy_restore()
x86/fpu: Use 'struct fpu' in restore_fpu_checking()
x86/fpu: Use 'struct fpu' in fpu_reset_state()
x86/fpu: Use 'struct fpu' in switch_fpu_prepare()
x86/fpu: Use 'struct fpu' in switch_fpu_finish()
x86/fpu: Move __save_fpu() into fpu/core.c
x86/fpu: Use 'struct fpu' in __fpu_save()
x86/fpu: Use 'struct fpu' in fpu__save()
x86/fpu: Use 'struct fpu' in fpu_copy()
x86/fpu: Use 'struct fpu' in fpu__copy()
x86/fpu: Use 'struct fpu' in fpstate_alloc_init()
x86/fpu: Use 'struct fpu' in fpu__unlazy_stopped()
x86/fpu: Rename fpu__flush_thread() to fpu__clear()
x86/fpu: Clean up fpu__clear() a bit
x86/fpu: Rename i387.h to fpu/api.h
x86/fpu: Move xsave.h to fpu/xsave.h
x86/fpu: Rename fpu-internal.h to fpu/internal.h
x86/fpu: Move MXCSR_DEFAULT to fpu/internal.h
x86/fpu: Remove xsave_init() __init obfuscation
x86/fpu: Remove assembly guard from asm/fpu/api.h
x86/fpu: Improve FPU detection kernel messages
x86/fpu: Print supported xstate features in human readable way
x86/fpu: Rename 'pcntxt_mask' to 'xfeatures_mask'
x86/fpu: Rename 'xstate_features' to 'xfeatures_nr'
x86/fpu: Move XCR0 manipulation to the FPU code proper
x86/fpu: Clean up regset functions
x86/fpu: Rename 'xsave_hdr' to 'header'
x86/fpu: Rename xsave.header::xstate_bv to 'xfeatures'
x86/fpu: Clean up and fix MXCSR handling
x86/fpu: Rename regset FPU register accessors
x86/fpu: Explain the AVX register layout in the xsave area
x86/fpu: Improve the __sanitize_i387_state() documentation
x86/fpu: Rename fpu->has_fpu to fpu->fpregs_active
x86/fpu: Rename __thread_set_has_fpu() to __fpregs_activate()
x86/fpu: Rename __thread_clear_has_fpu() to __fpregs_deactivate()
x86/fpu: Rename __thread_fpu_begin() to fpregs_activate()
x86/fpu: Rename __thread_fpu_end() to fpregs_deactivate()
x86/fpu: Remove fpstate_xstate_init_size() boot quirk
x86/fpu: Remove xsave_init() bootmem allocations
x86/fpu: Make setup_init_fpu_buf() run-once explicitly
x86/fpu: Remove 'init_xstate_buf' bootmem allocation
x86/fpu: Split fpu__cpu_init() into early-boot and cpu-boot parts
x86/fpu: Make the system/cpu init distinction clear in the xstate code as well x86/fpu: Move CPU capability check into fpu__init_cpu_xstate()
x86/fpu: Move legacy check to fpu__init_system_xstate()
x86/fpu: Propagate once per boot quirk into fpu__init_system_xstate()
x86/fpu: Remove xsave_init()
x86/fpu: Do fpu__init_system_xstate only from fpu__init_system()
x86/fpu: Set up the legacy FPU init image from fpu__init_system()
x86/fpu: Remove setup_init_fpu_buf() call from eager_fpu_init()
x86/fpu: Move all eager-fpu setup code to eager_fpu_init()
x86/fpu: Move eager_fpu_init() to fpu/init.c
x86/fpu: Clean up eager_fpu_init() and rename it to fpu__ctx_switch_init()
x86/fpu: Split fpu__ctx_switch_init() into _cpu() and _system() portions
x86/fpu: Do CLTS fpu__init_system()
x86/fpu: Move the fpstate_xstate_init_size() call into fpu__init_system()
x86/fpu: Call fpu__init_cpu_ctx_switch() from fpu__init_cpu()
x86/fpu: Do system-wide setup from fpu__detect()
x86/fpu: Remove fpu__init_cpu_ctx_switch() call from fpu__init_system()
x86/fpu: Simplify fpu__cpu_init()
x86/fpu: Factor out fpu__init_cpu_generic()
x86/fpu: Factor out fpu__init_system_generic()
x86/fpu: Factor out fpu__init_system_early_generic()
x86/fpu: Move !FPU check ingo fpu__init_system_early_generic()
x86/fpu: Factor out FPU bug checks into fpu/bugs.c
x86/fpu: Make check_fpu() init ordering independent
x86/fpu: Move fpu__init_system_early_generic() out of fpu__detect()
x86/fpu: Remove the extra fpu__detect() layer
x86/fpu: Rename fpstate_xstate_init_size() to fpu__init_system_xstate_size_legacy() x86/fpu: Reorder init methods
x86/fpu: Add more comments to the FPU init code
x86/fpu: Move fpu__save() to fpu/internals.h
x86/fpu: Uninline kernel_fpu_begin()/end()
x86/fpu: Move various internal function prototypes to fpu/internal.h
x86/fpu: Uninline the irq_ts_save()/restore() functions
x86/fpu: Rename fpu_save_init() to copy_fpregs_to_fpstate()
x86/fpu: Optimize copy_fpregs_to_fpstate() by removing the FNCLEX synchronization with FP exceptions x86/fpu: Simplify FPU handling by embedding the fpstate in task_struct (again) x86/fpu: Remove failure paths from fpstate-alloc low level functions
x86/fpu: Remove failure return from fpstate_alloc_init()
x86/fpu: Rename fpstate_alloc_init() to fpstate_init_curr()
x86/fpu: Simplify fpu__unlazy_stopped() error handling
x86/fpu, kvm: Simplify fx_init()
x86/fpu: Simplify fpstate_init_curr() usage
x86/fpu: Rename fpu__unlazy_stopped() to fpu__activate_stopped()
x86/fpu: Factor out FPU hw activation/deactivation
x86/fpu: Simplify __save_fpu()
x86/fpu: Eliminate __save_fpu()
x86/fpu: Simplify fpu__save()
x86/fpu: Optimize fpu__save()
x86/fpu: Optimize fpu_copy()
x86/fpu: Optimize fpu_copy() some more on lazy switching systems
x86/fpu: Rename fpu/xsave.h to fpu/xstate.h
x86/fpu: Rename fpu/xsave.c to fpu/xstate.c
x86/fpu: Introduce cpu_has_xfeatures(xfeatures_mask, feature_name)
x86/fpu: Simplify print_xstate_features()
x86/fpu: Enumerate xfeature bits
x86/fpu: Move xfeature type enumeration to fpu/types.h
x86/fpu, crypto x86/camellia_aesni_avx: Simplify the camellia_aesni_init() xfeature checks x86/fpu, crypto x86/sha256_ssse3: Simplify the sha256_ssse3_mod_init() xfeature checks x86/fpu, crypto x86/camellia_aesni_avx2: Simplify the camellia_aesni_init() xfeature checks x86/fpu, crypto x86/twofish_avx: Simplify the twofish_init() xfeature checks x86/fpu, crypto x86/serpent_avx: Simplify the serpent_init() xfeature checks x86/fpu, crypto x86/cast5_avx: Simplify the cast5_init() xfeature checks
x86/fpu, crypto x86/sha512_ssse3: Simplify the sha512_ssse3_mod_init() xfeature checks x86/fpu, crypto x86/cast6_avx: Simplify the cast6_init() xfeature checks
x86/fpu, crypto x86/sha1_ssse3: Simplify the sha1_ssse3_mod_init() xfeature checks x86/fpu, crypto x86/serpent_avx2: Simplify the init() xfeature checks
x86/fpu, crypto x86/sha1_mb: Remove FPU internal headers from sha1_mb.c
x86/fpu: Move asm/xcr.h to asm/fpu/internal.h
x86/fpu: Rename sanitize_i387_state() to fpstate_sanitize_xstate()
x86/fpu: Simplify fpstate_sanitize_xstate() calls
x86/fpu: Pass 'struct fpu' to fpstate_sanitize_xstate()
x86/fpu: Rename save_xstate_sig() to copy_fpstate_to_sigframe()
x86/fpu: Rename save_user_xstate() to copy_fpregs_to_sigframe()
x86/fpu: Clarify ancient comments in fpu__restore()
x86/fpu: Rename user_has_fpu() to fpregs_active()
x86/fpu: Initialize fpregs in fpu__init_cpu_generic()
x86/fpu: Clean up fpu__clear() state handling
x86/alternatives, x86/fpu: Add 'alternatives_patched' debug flag and use it in xsave_state() x86/fpu: Synchronize the naming of drop_fpu() and fpu_reset_state()
x86/fpu: Rename restore_fpu_checking() to copy_fpstate_to_fpregs()
x86/fpu: Move all the fpu__*() high level methods closer to each other
x86/fpu: Move fpu__clear() to 'struct fpu *' parameter passing
x86/fpu: Rename restore_xstate_sig() to fpu__restore_sig()
x86/fpu: Move the signal frame handling code closer to each other
x86/fpu: Merge fpu__reset() and fpu__clear()
x86/fpu: Move is_ia32*frame() helpers out of fpu/internal.h
x86/fpu: Split out fpu/signal.h from fpu/internal.h for signal frame handling functions x86/fpu: Factor out fpu/regset.h from fpu/internal.h
x86/fpu: Remove run-once init quirks
x86/fpu: Factor out the exception error code handling code
x86/fpu: Harmonize the names of the fpstate_init() helper functions
x86/fpu: Create 'union thread_xstate' helper for fpstate_init()
x86/fpu: Generalize 'init_xstate_ctx'
x86/fpu: Move restore_init_xstate() out of fpu/internal.h
x86/fpu: Rename all the fpregs, xregs, fxregs and fregs handling functions
x86/fpu: Factor out fpu/signal.c
x86/fpu: Factor out the FPU regset code into fpu/regset.c
x86/fpu: Harmonize FPU register state types
x86/fpu: Change fpu->fpregs_active from 'int' to 'char', add lazy switching comments x86/fpu: Document the various fpregs state formats
x86/fpu: Move debugging check from kernel_fpu_begin() to __kernel_fpu_begin() x86/fpu/xstate: Don't assume the first zero xfeatures zero bit means the end x86/fpu: Clean up xstate feature reservation
x86/fpu/xstate: Clean up setup_xstate_comp() call
x86/fpu/init: Propagate __init annotations
x86/fpu: Pass 'struct fpu' to fpu__restore()
x86/fpu: Fix the 'nofxsr' boot parameter to also clear X86_FEATURE_FXSR_OPT x86/fpu: Add CONFIG_X86_DEBUG_FPU=y FPU debugging code
x86/fpu: Add FPU performance measurement subsystem
x86/fpu: Reorganize fpu/internal.h

 Documentation/preempt-locking.txt              |   2 +-
 arch/x86/Kconfig.debug                         |  27 ++
 arch/x86/crypto/aesni-intel_glue.c             |   2 +-
 arch/x86/crypto/camellia_aesni_avx2_glue.c     |  15 +-
 arch/x86/crypto/camellia_aesni_avx_glue.c      |  15 +-
 arch/x86/crypto/cast5_avx_glue.c               |  15 +-
 arch/x86/crypto/cast6_avx_glue.c               |  15 +-
 arch/x86/crypto/crc32-pclmul_glue.c            |   2 +-
 arch/x86/crypto/crc32c-intel_glue.c            |   3 +-
 arch/x86/crypto/crct10dif-pclmul_glue.c        |   2 +-
 arch/x86/crypto/fpu.c                          |   2 +-
 arch/x86/crypto/ghash-clmulni-intel_glue.c     |   2 +-
 arch/x86/crypto/serpent_avx2_glue.c            |  15 +-
 arch/x86/crypto/serpent_avx_glue.c             |  15 +-
 arch/x86/crypto/sha-mb/sha1_mb.c               |   5 +-
 arch/x86/crypto/sha1_ssse3_glue.c              |  16 +-
 arch/x86/crypto/sha256_ssse3_glue.c            |  16 +-
 arch/x86/crypto/sha512_ssse3_glue.c            |  16 +-
 arch/x86/crypto/twofish_avx_glue.c             |  16 +-
 arch/x86/ia32/ia32_signal.c                    |  13 +-
 arch/x86/include/asm/alternative.h             |   6 +
 arch/x86/include/asm/crypto/glue_helper.h      |   2 +-
 arch/x86/include/asm/efi.h                     |   2 +-
 arch/x86/include/asm/fpu-internal.h            | 626 ---------------------------------------
 arch/x86/include/asm/fpu/api.h                 |  48 +++
 arch/x86/include/asm/fpu/internal.h            | 488 ++++++++++++++++++++++++++++++
 arch/x86/include/asm/fpu/measure.h             |  13 +
 arch/x86/include/asm/fpu/regset.h              |  21 ++
 arch/x86/include/asm/fpu/signal.h              |  33 +++
 arch/x86/include/asm/fpu/types.h               | 293 ++++++++++++++++++
 arch/x86/include/asm/{xsave.h => fpu/xstate.h} |  60 ++--
 arch/x86/include/asm/i387.h                    | 108 -------
 arch/x86/include/asm/kvm_host.h                |   2 -
 arch/x86/include/asm/mpx.h                     |   8 +-
 arch/x86/include/asm/processor.h               | 141 +--------
 arch/x86/include/asm/simd.h                    |   2 +-
 arch/x86/include/asm/stackprotector.h          |   2 +
 arch/x86/include/asm/suspend_32.h              |   2 +-
 arch/x86/include/asm/suspend_64.h              |   2 +-
 arch/x86/include/asm/user.h                    |  12 +-
 arch/x86/include/asm/xcr.h                     |  49 ---
 arch/x86/include/asm/xor.h                     |   2 +-
 arch/x86/include/asm/xor_32.h                  |   2 +-
 arch/x86/include/asm/xor_avx.h                 |   2 +-
 arch/x86/include/uapi/asm/sigcontext.h         |   8 +-
 arch/x86/kernel/Makefile                       |   2 +-
 arch/x86/kernel/alternative.c                  |   5 +
 arch/x86/kernel/cpu/bugs.c                     |  57 +---
 arch/x86/kernel/cpu/bugs_64.c                  |   2 +
 arch/x86/kernel/cpu/common.c                   |  29 +-
 arch/x86/kernel/fpu/Makefile                   |  11 +
 arch/x86/kernel/fpu/bugs.c                     |  71 +++++
 arch/x86/kernel/fpu/core.c                     | 509 +++++++++++++++++++++++++++++++
 arch/x86/kernel/fpu/init.c                     | 288 ++++++++++++++++++
 arch/x86/kernel/fpu/measure.c                  | 509 +++++++++++++++++++++++++++++++
 arch/x86/kernel/fpu/regset.c                   | 356 ++++++++++++++++++++++
 arch/x86/kernel/fpu/signal.c                   | 404 +++++++++++++++++++++++++
 arch/x86/kernel/fpu/xstate.c                   | 406 +++++++++++++++++++++++++
 arch/x86/kernel/i387.c                         | 656 ----------------------------------------
 arch/x86/kernel/process.c                      |  52 +---
 arch/x86/kernel/process_32.c                   |  15 +-
 arch/x86/kernel/process_64.c                   |  13 +-
 arch/x86/kernel/ptrace.c                       |  12 +-
 arch/x86/kernel/signal.c                       |  38 ++-
 arch/x86/kernel/smpboot.c                      |   3 +-
 arch/x86/kernel/traps.c                        | 120 ++------
 arch/x86/kernel/xsave.c                        | 724 ---------------------------------------------
 arch/x86/kvm/cpuid.c                           |   2 +-
 arch/x86/kvm/vmx.c                             |   5 +-
 arch/x86/kvm/x86.c                             |  68 ++---
 arch/x86/lguest/boot.c                         |   2 +-
 arch/x86/lib/mmx_32.c                          |   2 +-
 arch/x86/math-emu/fpu_aux.c                    |   4 +-
 arch/x86/math-emu/fpu_entry.c                  |  20 +-
 arch/x86/math-emu/fpu_system.h                 |   2 +-
 arch/x86/mm/mpx.c                              |  15 +-
 arch/x86/power/cpu.c                           |  11 +-
 arch/x86/xen/enlighten.c                       |   2 +-
 drivers/char/hw_random/via-rng.c               |   2 +-
 drivers/crypto/padlock-aes.c                   |   2 +-
 drivers/crypto/padlock-sha.c                   |   2 +-
 drivers/lguest/x86/core.c                      |  12 +-
 lib/raid6/x86.h                                |   2 +-
 83 files changed, 3742 insertions(+), 2841 deletions(-)
 delete mode 100644 arch/x86/include/asm/fpu-internal.h
 create mode 100644 arch/x86/include/asm/fpu/api.h
 create mode 100644 arch/x86/include/asm/fpu/internal.h
 create mode 100644 arch/x86/include/asm/fpu/measure.h
 create mode 100644 arch/x86/include/asm/fpu/regset.h
 create mode 100644 arch/x86/include/asm/fpu/signal.h
 create mode 100644 arch/x86/include/asm/fpu/types.h
 rename arch/x86/include/asm/{xsave.h => fpu/xstate.h} (77%)
 delete mode 100644 arch/x86/include/asm/i387.h
 delete mode 100644 arch/x86/include/asm/xcr.h
 create mode 100644 arch/x86/kernel/fpu/Makefile
 create mode 100644 arch/x86/kernel/fpu/bugs.c
 create mode 100644 arch/x86/kernel/fpu/core.c
 create mode 100644 arch/x86/kernel/fpu/init.c
 create mode 100644 arch/x86/kernel/fpu/measure.c
 create mode 100644 arch/x86/kernel/fpu/regset.c
 create mode 100644 arch/x86/kernel/fpu/signal.c
 create mode 100644 arch/x86/kernel/fpu/xstate.c
 delete mode 100644 arch/x86/kernel/i387.c
 delete mode 100644 arch/x86/kernel/xsave.c

2.1.0

The name 'xstate_features' does not tell us whether it's a bitmap or any other value. That it's a count of features is only obvious if you read the code that calculates it.

Rename it to the more descriptive 'xfeatures_nr' name.

---
 arch/x86/kernel/fpu/xsave.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

index c0e95538d689..1d0e27128f18 100644
@@ -25,7 +25,9 @@ struct xsave_struct *init_xstate_buf;
 static struct _fpx_sw_bytes fx_sw_reserved, fx_sw_reserved_ia32;
 static unsigned int *xstate_offsets, *xstate_sizes;
 static unsigned int xstate_comp_offsets[sizeof(xfeatures_mask)*8];
-static unsigned int xstate_features;
+
+/* The number of supported xfeatures in xfeatures_mask: */
+static unsigned int xfeatures_nr;
 
 /*
  * If a processor implementation discern that a processor state component is

@@ -465,9 +467,9 @@ static void __init setup_xstate_features(void)
 {
 	int eax, ebx, ecx, edx, leaf = 0x2;
 
-	xstate_features = fls64(xfeatures_mask);
-	xstate_offsets = alloc_bootmem(xstate_features * sizeof(int));
-	xstate_sizes = alloc_bootmem(xstate_features * sizeof(int));
+	xfeatures_nr = fls64(xfeatures_mask);
+	xstate_offsets = alloc_bootmem(xfeatures_nr * sizeof(int));
+	xstate_sizes = alloc_bootmem(xfeatures_nr * sizeof(int));
 
 	do {
 		cpuid_count(XSTATE_CPUID, leaf, &eax, &ebx, &ecx, &edx);

@@ -528,7 +530,7 @@ void setup_xstate_comp(void)
 	xstate_comp_offsets[1] = offsetof(struct i387_fxsave_struct, xmm_space);
 
 	if (!cpu_has_xsaves) {
-		for (i = 2; i < xstate_features; i++) {
+		for (i = 2; i < xfeatures_nr; i++) {
 			if (test_bit(i, (unsigned long *)&xfeatures_mask)) {
 				xstate_comp_offsets[i] = xstate_offsets[i];
 				xstate_comp_sizes[i] = xstate_sizes[i];

@@ -539,7 +541,7 @@ void setup_xstate_comp(void)
 
 	xstate_comp_offsets[2] = FXSAVE_SIZE + XSAVE_HDR_SIZE;
 
-	for (i = 2; i < xstate_features; i++) {
+	for (i = 2; i < xfeatures_nr; i++) {
 		if (test_bit(i, (unsigned long *)&xfeatures_mask))
 			xstate_comp_sizes[i] = xstate_sizes[i];
 		else

2.1.0

The suspend code accesses FPU state internals, add a helper for it and isolate it.

---
 arch/x86/include/asm/fpu/api.h |  1 +
 arch/x86/kernel/fpu/xsave.c    | 12 ++++++++++++
 arch/x86/power/cpu.c           | 10 ++--------
 3 files changed, 15 insertions(+), 8 deletions(-)

index f1eddcccba16..5bdde8ca87bc 100644
@@ -23,6 +23,7 @@ extern void fpu__clear(struct task_struct *tsk);
 extern int dump_fpu(struct pt_regs *, struct user_i387_struct *);
 extern void fpu__restore(void);
 extern void fpu__init_check_bugs(void);
+extern void fpu__resume_cpu(void);
 
 extern bool irq_fpu_usable(void);
 

index 1d0e27128f18..a485180ebc32 100644
@@ -735,6 +735,18 @@ void __init_refok eager_fpu_init(void)
 }
 
 /*
+ * Restore minimal FPU state after suspend:
+ */
+void fpu__resume_cpu(void)
+{
+	/*
+	 * Restore XCR0 on xsave capable CPUs:
+	 */
+	if (cpu_has_xsave)
+		xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
+}
+
+/*
  * Given the xsave area and a state inside, this function returns the
  * address of the state.
  *

index 62054acbd0d8..ad0ce6b70fac 100644
@@ -18,10 +18,8 @@
 #include <asm/mtrr.h>
 #include <asm/page.h>
 #include <asm/mce.h>
-#include <asm/xcr.h>
 #include <asm/suspend.h>
 #include <asm/debugreg.h>
-#include <asm/fpu/internal.h> /* xfeatures_mask */
 #include <asm/cpu.h>
 
 #ifdef CONFIG_X86_32

@@ -155,6 +153,8 @@ static void fix_processor_context(void)
 #endif
 	load_TR_desc();				/* This does ltr */
 	load_LDT(&current->active_mm->context);	/* This does lldt */
+
+	fpu__resume_cpu();
 }
 
 /**

@@ -221,12 +221,6 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
 	wrmsrl(MSR_KERNEL_GS_BASE, ctxt->gs_kernel_base);
 #endif
 
-	/*
-	 * restore XCR0 for xsave capable cpu's.
-	 */
-	if (cpu_has_xsave)
-		xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
-
 	fix_processor_context();
 
 	do_fpu_end();

2.1.0

Clean up various regset handlers: use the 'fpu' pointer which is available in most cases.

---
 arch/x86/kernel/fpu/core.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

index 7b98da7e1b55..54070d817960 100644
@@ -438,7 +438,7 @@ int xfpregs_get(struct task_struct *target, const struct user_regset *regset,
 	sanitize_i387_state(target);
 
 	return user_regset_copyout(&pos, &count, &kbuf, &ubuf,
-				   &target->thread.fpu.state->fxsave, 0, -1);
+				   &fpu->state->fxsave, 0, -1);
 }
 
 int xfpregs_set(struct task_struct *target, const struct user_regset *regset,

@@ -458,19 +458,19 @@ int xfpregs_set(struct task_struct *target, const struct user_regset *regset,
 	sanitize_i387_state(target);
 
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
-				 &target->thread.fpu.state->fxsave, 0, -1);
+				 &fpu->state->fxsave, 0, -1);
 
 	/*
 	 * mxcsr reserved bits must be masked to zero for security reasons.
 	 */
-	target->thread.fpu.state->fxsave.mxcsr &= mxcsr_feature_mask;
+	fpu->state->fxsave.mxcsr &= mxcsr_feature_mask;
 
 	/*
 	 * update the header bits in the xsave header, indicating the
 	 * presence of FP and SSE state.
 	 */
 	if (cpu_has_xsave)
-		target->thread.fpu.state->xsave.xsave_hdr.xstate_bv |= XSTATE_FPSSE;
+		fpu->state->xsave.xsave_hdr.xstate_bv |= XSTATE_FPSSE;
 
 	return ret;
 }

@@ -490,7 +490,7 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset,
 	if (ret)
 		return ret;
 
-	xsave = &target->thread.fpu.state->xsave;
+	xsave = &fpu->state->xsave;
 
 	/*
 	 * Copy the 48bytes defined by the software first into the xstate

@@ -521,7 +521,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 	if (ret)
 		return ret;
 
-	xsave = &target->thread.fpu.state->xsave;
+	xsave = &fpu->state->xsave;
 
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, xsave, 0, -1);
 	/*

@@ -533,6 +533,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 	 * These bits must be zero.
 	 */
 	memset(&xsave->xsave_hdr.reserved, 0, 48);
+
 	return ret;
 }
 

@@ -690,7 +691,7 @@ int fpregs_get(struct task_struct *target, const struct user_regset *regset,
 
 	if (!cpu_has_fxsr)
 		return user_regset_copyout(&pos, &count, &kbuf, &ubuf,
-					   &target->thread.fpu.state->fsave, 0,
+					   &fpu->state->fsave, 0,
 					   -1);
 
 	sanitize_i387_state(target);

@@ -724,7 +725,7 @@ int fpregs_set(struct task_struct *target, const struct user_regset *regset,
 
 	if (!cpu_has_fxsr)
 		return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
-					  &target->thread.fpu.state->fsave, 0,
+					  &fpu->state->fsave, 0,
 					  -1);
 
 	if (pos > 0 || count < sizeof(env))

@@ -739,7 +740,7 @@ int fpregs_set(struct task_struct *target, const struct user_regset *regset,
 	 * presence of FP.
 	 */
 	if (cpu_has_xsave)
-		target->thread.fpu.state->xsave.xsave_hdr.xstate_bv |= XSTATE_FP;
+		fpu->state->xsave.xsave_hdr.xstate_bv |= XSTATE_FP;
 	return ret;
 }
 

2.1.0

Code like:

           fpu->state->xsave.xsave_hdr.xstate_bv |= XSTATE_FP;

is an eyesore, because not only is the words 'xsave' and 'state' are repeated twice times (!), but also because of the 'hdr' and 'bv' abbreviations that are pretty meaningless at a first glance.

Start cleaning this up by renaming 'xsave_hdr' to 'header'.

---
 arch/x86/include/asm/fpu/internal.h    |  2 +-
 arch/x86/include/asm/fpu/types.h       |  4 ++--
 arch/x86/include/asm/fpu/xsave.h       |  2 +-
 arch/x86/include/asm/user.h            | 10 +++++-----
 arch/x86/include/uapi/asm/sigcontext.h |  4 ++--
 arch/x86/kernel/fpu/core.c             |  8 ++++----
 arch/x86/kernel/fpu/xsave.c            | 22 +++++++++++-----------
 arch/x86/kvm/x86.c                     |  8 ++++----
 8 files changed, 30 insertions(+), 30 deletions(-)

index 0e9a7a37801a..3007df99833e 100644
@@ -261,7 +261,7 @@ static inline int fpu_save_init(struct fpu *fpu)
 		/*
 		 * xsave header may indicate the init state of the FP.
 		 */
-		if (!(fpu->state->xsave.xsave_hdr.xstate_bv & XSTATE_FP))
+		if (!(fpu->state->xsave.header.xstate_bv & XSTATE_FP))
 			return 1;
 	} else if (use_fxsr()) {
 		fpu_fxsave(fpu);

index 917d2e56426a..33c0c7b782db 100644
@@ -99,7 +99,7 @@ struct bndcsr {
 	u64				bndstatus;
 } __packed;
 
-struct xsave_hdr_struct {
+struct xstate_header {
 	u64				xstate_bv;
 	u64				xcomp_bv;
 	u64				reserved[6];

@@ -107,7 +107,7 @@ struct xsave_hdr_struct {
 
 struct xsave_struct {
 	struct i387_fxsave_struct	i387;
-	struct xsave_hdr_struct		xsave_hdr;
+	struct xstate_header		header;
 	struct ymmh_struct		ymmh;
 	struct lwp_struct		lwp;
 	struct bndreg			bndreg[4];

index 400d5b2e42eb..b27b4466f88d 100644
@@ -212,7 +212,7 @@ static inline int xsave_user(struct xsave_struct __user *buf)
 	 * Clear the xsave header first, so that reserved fields are
 	 * initialized to zero.
 	 */
-	err = __clear_user(&buf->xsave_hdr, sizeof(buf->xsave_hdr));
+	err = __clear_user(&buf->header, sizeof(buf->header));
 	if (unlikely(err))
 		return -EFAULT;
 

index ccab4af1646d..fa042410c42c 100644
@@ -14,7 +14,7 @@ struct user_ymmh_regs {
 	__u32 ymmh_space[64];
 };
 
-struct user_xsave_hdr {
+struct user_xstate_header {
 	__u64 xstate_bv;
 	__u64 reserved1[2];
 	__u64 reserved2[5];

@@ -41,11 +41,11 @@ struct user_xsave_hdr {
  * particular process/thread.
  *
  * Also when the user modifies certain state FP/SSE/etc through the
- * ptrace interface, they must ensure that the xsave_hdr.xstate_bv
+ * ptrace interface, they must ensure that the header.xstate_bv
  * bytes[512..519] of the memory layout are updated correspondingly.
  * i.e., for example when FP state is modified to a non-init state,
- * xsave_hdr.xstate_bv's bit 0 must be set to '1', when SSE is modified to
- * non-init state, xsave_hdr.xstate_bv's bit 1 must to be set to '1', etc.
+ * header.xstate_bv's bit 0 must be set to '1', when SSE is modified to
+ * non-init state, header.xstate_bv's bit 1 must to be set to '1', etc.
  */
 #define USER_XSTATE_FX_SW_WORDS 6
 #define USER_XSTATE_XCR0_WORD	0

@@ -55,7 +55,7 @@ struct user_xstateregs {
 		__u64 fpx_space[58];
 		__u64 xstate_fx_sw[USER_XSTATE_FX_SW_WORDS];
 	} i387;
-	struct user_xsave_hdr xsave_hdr;
+	struct user_xstate_header header;
 	struct user_ymmh_regs ymmh;
 	/* further processor state extensions go here */
 };

index 16dc4e8a2cd3..7f850f7b5c45 100644
@@ -209,7 +209,7 @@ struct sigcontext {
 
 #endif /* !__i386__ */
 
-struct _xsave_hdr {
+struct _header {
 	__u64 xstate_bv;
 	__u64 reserved1[2];
 	__u64 reserved2[5];

@@ -228,7 +228,7 @@ struct _ymmh_state {
  */
 struct _xstate {
 	struct _fpstate fpstate;
-	struct _xsave_hdr xstate_hdr;
+	struct _header xstate_hdr;
 	struct _ymmh_state ymmh;
 	/* new processor state extensions go here */
 };

index 54070d817960..eabf4380366a 100644
@@ -470,7 +470,7 @@ int xfpregs_set(struct task_struct *target, const struct user_regset *regset,
 	 * presence of FP and SSE state.
 	 */
 	if (cpu_has_xsave)
-		fpu->state->xsave.xsave_hdr.xstate_bv |= XSTATE_FPSSE;
+		fpu->state->xsave.header.xstate_bv |= XSTATE_FPSSE;
 
 	return ret;
 }

@@ -528,11 +528,11 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 	 * mxcsr reserved bits must be masked to zero for security reasons.
 	 */
 	xsave->i387.mxcsr &= mxcsr_feature_mask;
-	xsave->xsave_hdr.xstate_bv &= xfeatures_mask;
+	xsave->header.xstate_bv &= xfeatures_mask;
 	/*
 	 * These bits must be zero.
 	 */
-	memset(&xsave->xsave_hdr.reserved, 0, 48);
+	memset(&xsave->header.reserved, 0, 48);
 
 	return ret;
 }

@@ -740,7 +740,7 @@ int fpregs_set(struct task_struct *target, const struct user_regset *regset,
 	 * presence of FP.
 	 */
 	if (cpu_has_xsave)
-		fpu->state->xsave.xsave_hdr.xstate_bv |= XSTATE_FP;
+		fpu->state->xsave.header.xstate_bv |= XSTATE_FP;
 	return ret;
 }
 

index a485180ebc32..03639fa079b0 100644
@@ -32,7 +32,7 @@ static unsigned int xfeatures_nr;
 /*
  * If a processor implementation discern that a processor state component is
  * in its initialized state it may modify the corresponding bit in the
- * xsave_hdr.xstate_bv as '0', with out modifying the corresponding memory
+ * header.xstate_bv as '0', with out modifying the corresponding memory
  * layout in the case of xsaveopt. While presenting the xstate information to
  * the user, we always ensure that the memory layout of a feature will be in
  * the init state if the corresponding header bit is zero. This is to ensure

@@ -48,7 +48,7 @@ void __sanitize_i387_state(struct task_struct *tsk)
 	if (!fx)
 		return;
 
-	xstate_bv = tsk->thread.fpu.state->xsave.xsave_hdr.xstate_bv;
+	xstate_bv = tsk->thread.fpu.state->xsave.header.xstate_bv;
 
 	/*
 	 * None of the feature bits are in init state. So nothing else

@@ -106,7 +106,7 @@ static inline int check_for_xstate(struct i387_fxsave_struct __user *buf,
 				   struct _fpx_sw_bytes *fx_sw)
 {
 	int min_xstate_size = sizeof(struct i387_fxsave_struct) +
-			      sizeof(struct xsave_hdr_struct);
+			      sizeof(struct xstate_header);
 	unsigned int magic2;
 
 	if (__copy_from_user(fx_sw, &buf->sw_reserved[0], sizeof(*fx_sw)))

@@ -178,7 +178,7 @@ static inline int save_xstate_epilog(void __user *buf, int ia32_frame)
 	 * Read the xstate_bv which we copied (directly from the cpu or
 	 * from the state in task struct) to the user buffers.
 	 */
-	err |= __get_user(xstate_bv, (__u32 *)&x->xsave_hdr.xstate_bv);
+	err |= __get_user(xstate_bv, (__u32 *)&x->header.xstate_bv);
 
 	/*
 	 * For legacy compatible, we always set FP/SSE bits in the bit

@@ -193,7 +193,7 @@ static inline int save_xstate_epilog(void __user *buf, int ia32_frame)
 	 */
 	xstate_bv |= XSTATE_FPSSE;
 
-	err |= __put_user(xstate_bv, (__u32 *)&x->xsave_hdr.xstate_bv);
+	err |= __put_user(xstate_bv, (__u32 *)&x->header.xstate_bv);
 
 	return err;
 }

@@ -280,20 +280,20 @@ sanitize_restored_xstate(struct task_struct *tsk,
 			 u64 xstate_bv, int fx_only)
 {
 	struct xsave_struct *xsave = &tsk->thread.fpu.state->xsave;
-	struct xsave_hdr_struct *xsave_hdr = &xsave->xsave_hdr;
+	struct xstate_header *header = &xsave->header;
 
 	if (use_xsave()) {
 		/* These bits must be zero. */
-		memset(xsave_hdr->reserved, 0, 48);
+		memset(header->reserved, 0, 48);
 
 		/*
 		 * Init the state that is not present in the memory
 		 * layout and not enabled by the OS.
 		 */
 		if (fx_only)
-			xsave_hdr->xstate_bv = XSTATE_FPSSE;
+			header->xstate_bv = XSTATE_FPSSE;
 		else
-			xsave_hdr->xstate_bv &= (xfeatures_mask & xstate_bv);
+			header->xstate_bv &= (xfeatures_mask & xstate_bv);
 	}
 
 	if (use_fxsr()) {

@@ -574,9 +574,9 @@ static void __init setup_init_fpu_buf(void)
 	print_xstate_features();
 
 	if (cpu_has_xsaves) {
-		init_xstate_buf->xsave_hdr.xcomp_bv =
+		init_xstate_buf->header.xcomp_bv =
 						(u64)1 << 63 | xfeatures_mask;
-		init_xstate_buf->xsave_hdr.xstate_bv = xfeatures_mask;
+		init_xstate_buf->header.xstate_bv = xfeatures_mask;
 	}
 
 	/*

index 91d7f3b1e50c..ac24889c8bc3 100644
@@ -3197,7 +3197,7 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
 static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu)
 {
 	struct xsave_struct *xsave = &vcpu->arch.guest_fpu.state->xsave;
-	u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
+	u64 xstate_bv = xsave->header.xstate_bv;
 	u64 valid;
 
 	/*

@@ -3243,9 +3243,9 @@ static void load_xsave(struct kvm_vcpu *vcpu, u8 *src)
 	memcpy(xsave, src, XSAVE_HDR_OFFSET);
 
 	/* Set XSTATE_BV and possibly XCOMP_BV.  */
-	xsave->xsave_hdr.xstate_bv = xstate_bv;
+	xsave->header.xstate_bv = xstate_bv;
 	if (cpu_has_xsaves)
-		xsave->xsave_hdr.xcomp_bv = host_xcr0 | XSTATE_COMPACTION_ENABLED;
+		xsave->header.xcomp_bv = host_xcr0 | XSTATE_COMPACTION_ENABLED;
 
 	/*
 	 * Copy each region from the non-compacted offset to the

@@ -7014,7 +7014,7 @@ int fx_init(struct kvm_vcpu *vcpu)
 
 	fpstate_init(&vcpu->arch.guest_fpu);
 	if (cpu_has_xsaves)
-		vcpu->arch.guest_fpu.state->xsave.xsave_hdr.xcomp_bv =
+		vcpu->arch.guest_fpu.state->xsave.header.xcomp_bv =
 			host_xcr0 | XSTATE_COMPACTION_ENABLED;
 
 	/*

2.1.0

'xsave.header::xstate_bv' is a misnomer - what does 'bv' stand for?

It probably comes from the 'XGETBV' instruction name, but I could not find in the Intel documentation where that abbreviation comes from. It could mean 'bit vector' - or something else?

But how about - instead of guessing about a weird name - we named the field in an obvious and descriptive way that tells us exactly what it does?

So rename it to 'xfeatures', which is a bitmask of the xfeatures that are fpstate_active in that context structure.

Eyesore like:

           fpu->state->xsave.xsave_hdr.xstate_bv |= XSTATE_FP;

is now much more readable:

           fpu->state->xsave.header.xfeatures |= XSTATE_FP;

Which form is not just infinitely more readable, but is also shorter as well.

---
 arch/x86/include/asm/fpu/internal.h    |  2 +-
 arch/x86/include/asm/fpu/types.h       |  2 +-
 arch/x86/include/asm/user.h            |  8 ++++----
 arch/x86/include/uapi/asm/sigcontext.h |  4 ++--
 arch/x86/kernel/fpu/core.c             |  6 +++---
 arch/x86/kernel/fpu/xsave.c            | 52 ++++++++++++++++++++++++++--------------------------
 arch/x86/kvm/x86.c                     |  4 ++--
 7 files changed, 39 insertions(+), 39 deletions(-)

index 3007df99833e..07c6adc02f68 100644
@@ -261,7 +261,7 @@ static inline int fpu_save_init(struct fpu *fpu)
 		/*
 		 * xsave header may indicate the init state of the FP.
 		 */
-		if (!(fpu->state->xsave.header.xstate_bv & XSTATE_FP))
+		if (!(fpu->state->xsave.header.xfeatures & XSTATE_FP))
 			return 1;
 	} else if (use_fxsr()) {
 		fpu_fxsave(fpu);

index 33c0c7b782db..9bd2cd1a19fd 100644
@@ -100,7 +100,7 @@ struct bndcsr {
 } __packed;
 
 struct xstate_header {
-	u64				xstate_bv;
+	u64				xfeatures;
 	u64				xcomp_bv;
 	u64				reserved[6];
 } __attribute__((packed));

index fa042410c42c..59a54e869f15 100644
@@ -15,7 +15,7 @@ struct user_ymmh_regs {
 };
 
 struct user_xstate_header {
-	__u64 xstate_bv;
+	__u64 xfeatures;
 	__u64 reserved1[2];
 	__u64 reserved2[5];
 };

@@ -41,11 +41,11 @@ struct user_xstate_header {
  * particular process/thread.
  *
  * Also when the user modifies certain state FP/SSE/etc through the
- * ptrace interface, they must ensure that the header.xstate_bv
+ * ptrace interface, they must ensure that the header.xfeatures
  * bytes[512..519] of the memory layout are updated correspondingly.
  * i.e., for example when FP state is modified to a non-init state,
- * header.xstate_bv's bit 0 must be set to '1', when SSE is modified to
- * non-init state, header.xstate_bv's bit 1 must to be set to '1', etc.
+ * header.xfeatures's bit 0 must be set to '1', when SSE is modified to
+ * non-init state, header.xfeatures's bit 1 must to be set to '1', etc.
  */
 #define USER_XSTATE_FX_SW_WORDS 6
 #define USER_XSTATE_XCR0_WORD	0

index 7f850f7b5c45..0e8a973de9ee 100644
@@ -25,7 +25,7 @@ struct _fpx_sw_bytes {
 	__u32 extended_size;	/* total size of the layout referred by
 				 * fpstate pointer in the sigcontext.
 				 */
-	__u64 xstate_bv;
+	__u64 xfeatures;
 				/* feature bit mask (including fp/sse/extended
 				 * state) that is present in the memory
 				 * layout.

@@ -210,7 +210,7 @@ struct sigcontext {
 #endif /* !__i386__ */
 
 struct _header {
-	__u64 xstate_bv;
+	__u64 xfeatures;
 	__u64 reserved1[2];
 	__u64 reserved2[5];
 };

index eabf4380366a..c12dd3c0aabb 100644
@@ -470,7 +470,7 @@ int xfpregs_set(struct task_struct *target, const struct user_regset *regset,
 	 * presence of FP and SSE state.
 	 */
 	if (cpu_has_xsave)
-		fpu->state->xsave.header.xstate_bv |= XSTATE_FPSSE;
+		fpu->state->xsave.header.xfeatures |= XSTATE_FPSSE;
 
 	return ret;
 }

@@ -528,7 +528,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 	 * mxcsr reserved bits must be masked to zero for security reasons.
 	 */
 	xsave->i387.mxcsr &= mxcsr_feature_mask;
-	xsave->header.xstate_bv &= xfeatures_mask;
+	xsave->header.xfeatures &= xfeatures_mask;
 	/*
 	 * These bits must be zero.
 	 */

@@ -740,7 +740,7 @@ int fpregs_set(struct task_struct *target, const struct user_regset *regset,
 	 * presence of FP.
 	 */
 	if (cpu_has_xsave)
-		fpu->state->xsave.header.xstate_bv |= XSTATE_FP;
+		fpu->state->xsave.header.xfeatures |= XSTATE_FP;
 	return ret;
 }
 

index ac24889c8bc3..0b58b9397098 100644
@@ -3197,7 +3197,7 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
 static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu)
 {
 	struct xsave_struct *xsave = &vcpu->arch.guest_fpu.state->xsave;
-	u64 xstate_bv = xsave->header.xstate_bv;
+	u64 xstate_bv = xsave->header.xfeatures;
 	u64 valid;
 
 	/*

@@ -3243,7 +3243,7 @@ static void load_xsave(struct kvm_vcpu *vcpu, u8 *src)
 	memcpy(xsave, src, XSAVE_HDR_OFFSET);
 
 	/* Set XSTATE_BV and possibly XCOMP_BV.  */
-	xsave->header.xstate_bv = xstate_bv;
+	xsave->header.xfeatures = xstate_bv;
 	if (cpu_has_xsaves)
 		xsave->header.xcomp_bv = host_xcr0 | XSTATE_COMPACTION_ENABLED;
 

2.1.0

xstate_bv is what it is called in the SDM. I'd really like to see the nomenclature match the SDM where it's sensible because it says lots of things like:

XSAVES does not write to any parts of the XSAVE header other than the XSTATE_BV and XCOMP_BV fields.

It's nice to have code that does:

	...->xstate_bv

to match up with that documentation IMNHO.

So I'm not attached to the ::xfeatures name (we could name it xstate_mask, etc.) - but xstate_bv? It's really nonsensical IMHO - and I wanted it to be more obvious.

We could put the SDM name into a comment, next to the field definition? Something like, if 'xfeatures' is too long:

struct xstate_header {

        u64     xfeat;        /* xstate components,           SDM: XSTATE_BV */
        u64     xfeat_comp;   /* compacted xstate components, SDM: XCOMP_BV */
        u64     reserved[6];

} __attribute__((packed));

or so? Then if you grep for 'XSTATE_BV', you'll immediately see that it's called xfeat_comp.

Where the SDM uses sensible names I'm all for that - but IMHO this is not such a case.

Thanks,

Ingo

When you're in the depths of the SDM and the kernel code, the fewer context switches you have to make, the better. I say this from the perspective of someone who's had a copy of the SDM open to xsave* for about a month straight.

In any case, having "xfeat" and "xfeat_comp" is a bad idea. They're not really related concepts other than their bits refer to the same states.
They should not have such similar names.

XSTATE_BV is the set of states written to the xsave area.

XCOMP_BV is essentially always XCR0 (aka pcntxt_mask, aka xfeatures_mask) or'd with bit 63.

But that's not the only consideration. While in general I'm all for following reference hardware documentation with names, there's a limit for how far we'll go in following stupid vendor names, and I think 'XSTATE_BV' and 'XCOMP_BV' are well beyond any sane limit (see further below my suggestion for better naming).

If only one of us worked at the company that invented those nonsensical names and complex SDMs, and could complain to them? ;-)

Agreed.

So how about this naming:

/*
* Mask of xstate components currently not in init state,
* typically written to by XSAVE*.
*/
u64 xfeat_mask_used; /* SDM: XSTATE_BV */

/*
* Mask of all state components saved/restored, plus the
* compaction flag. (Note that the XRSTORS instruction caches
* this value, and the next SAVES done for this same
* area expects it to match, before it can perform the 'were
* these registers modified' hardware optimization.) */
u64 xfeat_mask_all; /* SDM: XCOMP_BV */

(Note that I kept the SDM name easily greppable.)

The 'compaction' aspect of 'xfeat_mask_all' is just an additional quirk that does not deserve to be represented in the primary naming:
bit 63 of 'xfeat_mask_all' is set to 1 if the format is compacted:
basically 'compaction' can be thought of as an additional, special 'xfeature' that modifies the offsets in the save area to eliminate holes. [*]

Basically this naming tells us the biggest, most relevant differentiation between these two fields:

- the 'xfeat_mask_used' field reflects the current, momentary, optimized state of the area. This mask is content dependent, and it is a subset of:

- the 'xfeat_mask_all' field which reflects all states supported by that fpstate context. This mask is content independent.

The compaction aspect of 'xfeat_mask_all' is obviously important to the hardware (and depending on its value the position of various registers in the save area are different), but secondary to the big picture.

Note that once you have a good name, a lot of code becomes a lot more obvious - and I wish Intel did more than just googling for the first available historic QuickBASIC variable name when picking new CPU symbols.

Thanks,

Ingo

[*]

Btw., does Intel have any special plans with xstate compaction?

AFAICS in Linux we just want to enable xfeat_mask_all to the max, including compaction, and never really modify it (in the task's lifetime).

I'm also wondering whether there will be any real 'holes' in the xfeatures capability masks of future CPUs: right now xfeatures tend to be already 'compacted' (because new CPUs tend to support all xfeatures), so compaction mostly appears to be an academic feature. Or is there already hardware out there where it matter?

Maybe once we get AVX512 in addition to MPX we can use compaction
materially: as there will be lots of tasks without MPX state but with AVX512 state - in fact I suspect that will be the common case.

OTOH MPX state is relatively small compared to AVX and AVX512 state, so skipping the hole won't buy us much, and the question is, how expensive is compaction, will save/restore be slower with compaction enabled? Has to be measured I suspect.

Hm, so the problem with this naming is that for non-compacted XRSTOR, XCOMP_BV has to be zero. (This seems nonsensical btw., as there's a separate 'compaction' flag at bit 63 already.)

So a better name would be:

/*
* Mask of xstate components currently not in init state,
* typically written to by XSAVE*.
*/

	u64 xfeat_used_mask;	/* SDM: XSTATE_BV */

/*
* This mask is non-zero if the CPU supports state compaction:
* it is the mask of all state components to be saved/restored,
* plus the compaction flag at bit 63.
* (Note that the XRSTORS instruction caches this value, and
* the next SAVES done for this same area expects it to match,
* before it can perform the 'were these registers modified'
* hardware optimization.)
*/

	u64 xfeat_comp_mask;	/* SDM: XCOMP_BV */

?

Thanks,

Ingo

The comment and name make sense if we always call xsave* with an "instruction mask" where it has at least as many bits as we have set in 'pcntxt_mask' (aka xfeatures_mask).

If we ever get to a point where we are saving only a subset of the supported features (say if we only wanted to consult the MPX registers and none of the other state), then this stops making sense.

I think 'xfeat_saved_mask' or 'xstate_saved_mask' makes more sense. Maybe a comment like:

/*
* Mask of xstate components currently present in the buffer.
* A non-present bit can mean that the feature is in the init
* state or that we did not ask the instruction to save it.
* typically written to by XSAVE*.
*/

That's not correct. It's non-zero if it supports compaction and it was saved using an instruction that supports compaction. A CPU supporting xsaves, but using xsave will receive an uncompacted version with xcomp_bv=0.

That seems like a bit of a silly thing to mention in a comment since it never changes.

How about something like this?

/*
* Must be 0 when compacted state not supported. If compacted state is
* supported and XRSTOR variant understands both formats, Bit 63 tells
* instruction which format is to be used.
*
* This tells you the format of the buffer when using compacted layout.
* The format is determined by the features enabled in XCR* along with
* the features requested at XSAVE* time (SDM: RFBM).
*
* Note that even if a feature is present in this mask, it may still be
* absent from 'xfeat_used_mask', which means that space was allocated
* in the layout, but that it was not actually written.
*/

Good, that name works for me too: it still expresses the dynamic, content-dependent nature of this mask.

That is what I meant: under Linux this bit is non-zero if the CPU supports state compaction.

But that's the reason why in Linux we don't really change it - we don't go about trying to slice&dice the xstate into multiple components. We just save it all and let the 'init' and 'modified' optimizations in the hardware take care of the optimizations.

Yes.

Btw., as a side note, this is a silly hardware interface: not setting bit 63 is not valid as per the SDM (will cause a #GP), so it might as well have left the whole bit 63 complication out of it: if this mask is nonzero then a compacted format is requested. If it's zero, then a non-compacted format.

Right?

I has not 2 but 3 inputs: what is being saved/restored is determined by this very bitmask here. If a bit is missing from this mask, it won't be saved (it's 'compacted'), even if it's otherwise set in XCR* or is requested in the instruction mask.

That's the whole point of compaction.

So here I'd mention _why_ it can be left out from xfeat_used_mask:
when an xstate component is in 'init state', then only 0 is written to the xfeat_used_mask but the component area itself is not touched.

This means that previous contents of the saved area are still there and are stale, so the kernel has to be careful about not exposing these to user-space indiscriminately. That's what the 'sanitization' functions are about.

Thanks,

Ingo

Special plans?

If we do an XRSTORS on it before we do an XSAVES, then we need to worry.
But, if we do an XSAVES, the CPU will set it up for us.

There is a hole in the SDM today. See section 2.6 in the currently released 054 version. I also know of actual hardware platforms with holes. *PLUS*, someone can always shot down CPUID bits in their hypervisor or with kernel command-line options.

Right.

But we'd need to get to a point where we are calling 'xsaves' with a Requested Feature BitMask (aka RFBM[]) that had holes in it. As it stands today, we always call it with RFBM=-1 and so we always have XCOMP_BV = XCR0.

We'd need to determine which fields are in the init state before we do an xsaves.

Yep.

Btw, do we have any perf data as to the improvement the compacted variant brings?

I mean, it means a bunch of jumping through hoops in SW but is it worth it?

Thanks.

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

I.e. are there any plans beyond using it strictly for full state save/restore.

I see, so MPX (bits 3 and 4) aren't there yet.

Btw., there's a new xfeature it appears:

XCR0.PKRU (bit 9): If 1, the XSAVE feature set can be used to manage the PKRU register (see Section 2.7).

and bit 8 is a hole again.

Btw., regarding XCR0.PKRU: that enables 'Protection Keys' in the PTE format. What's the main purpose of these keys? They seem to duplicate the read/write bits in the PTE, with the exception that they don't impact instruction fetches. So is this used to allow user-space to execute but otherwise not read instructions?

Or some other purpose I missed?

In any case, these holes are really minor at the moment, and the question is, what is the performance difference between a 'compactede' XSAVE*/XRSTOR* pair, versus a standard format one?

XCOMP_BV must also have bit 63 set.

13.8.1
Standard Form of XRSTOR

The standard from of XRSTOR performs additional fault checking. Either of the following conditions causes ageneral-protection exception (#GP):

The XSTATE_BV field of the XSAVE header sets a bit that is not set in XCR0. Bytes 23:8 of the XSAVE header are not all 0 (this implies

                                                         ^^^^^^^^^^^^

that all bits in XCOMP_BV are 0).
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Note the part I underlined: all of XCOMP_BV has to be 0 for any standard form of XRSTOR, and if we use a compacted form, bit 63 must be set:

this is why bit 63 is a nonsensical interface: it being nonzero already tells the hardware that we requested compaction ...

Why? I don't think that's necessary.

The way I read the SDM both the 'init' and the 'modified' optimizations are mostly automatic: the CPU determines it automatically when a state component is (or returned to!) init state, and signals that via the relevant bit in XSTATE_BV being zeroed out.

This is what the SDM says about XSAVES (section 13.11 in the 054 SDM):

— If state component i is in its initial configuration, XSTATE_BV[i]
may be written with either 0 or 1.

so XSAVES itself performs the first step of the 'init optimization',
automatically: it will opportunistically write 0 to the relevant bit in XSTATE_BV and won't save the state.

Once there's 0 in XSTATE_BV, put there by XSAVES, the XRSTOR instruction is able to perform the other half of the optimization: by not restoring it but initializing it (if needed).

XSAVES will also set up XSTATE_BV and XCOMP_BV so that XRSTOR does not have to worry about it, it will do a compacted restore.

Thanks,

Ingo

None that I know of, but having two (relatively) tiny features (protection keys and MPX) might change things.

You can change the permissions of a given key with writes to the register without changing the PTE. No TLB shootdown, plus the permission changes are local to the CPU thread.

I have patches today if you're interested.

Yeah, that would be interesting to know.

...

"If RFBM[i] = 0, XSTATE_BV[i] is written as 0."

We need to pull XSTATE_BV in to the instruction mask when doing an XSAVE* if our RFBM has bits unset that *are* set in XSTATE_BV. Otherwise, we'll destroy the bits at XSAVE* time.

It's not a problem today because the instruction mask is always -1, so it always has every bit set that *MIGHT* be set in XSTATE_BV.

As for the whole bit 63 thing... It's possible and valid to have a XCOMP_BV[62:0]=0 because the instruction mask only contained bits that were unset in XCR0|IA32_XSS. You need bit 63 to tell you which format you are using.

interesting ... where are we doing that? kmap_atomic() is the only place I can think of in the kernel, but there we are already skipping the TLB shootdown by doing an INVLPG.

I'm always interested in new CPU features ;-)

Yes, that's natural: RFBM is the combination of XCR0 (constant) and the 'instruction mask' (constant as well) - i.e. it's a wide bitmask including all our xfeatures - essentially 'xfeatures_mask' (in tmp.fpu).

But why would our RFBM be narrower than any possible XSTATE_BV we handle? Our XCR0 is at the max.

Also, the XSTATE_BV value of the save area is immaterial when we do an
XSAVES: all the state is in CPU registers, we want to save it to the save area. XSAVES will write it for us.

Yes. And why would we ever want to narrow it?

So basically if you request an XSAVES to ... write nothing (XCR0 AND instruction mask is 0), then it will represent this as 0|1<<63 in XCOMP_BV?

In that case it does not matter whether the area is compacted or
standard: nothing was saved and nothing will have to be restored, only the xsave header area will be accessed. Am I missing something?

Thanks,

Ingo

Userspace. :)

It's for userspace-only.

Because it actually allows us to take advantage of the compaction. Think of the layout of a task using protection keys and MPX.

MPX = 8*4 + 8*2 = 48 bytes.
PKEYs = 4 bytes

They'll be spread out in the standard form *OR* the compacted form with a RFBM=-1. But, with the compacted form with RFBM=PK|MPX_BITS, they'll fit in a cacheline.

Take a look at the SDM. There are differences in the behavior when restoring the compacted vs. standard format. I don't know the deep reasons for *WHY*, just that there are some deltas clearly spelled out there.

but but ... if this is a normal userspace task then it will use AVX on modern Linux distros and then we are up to a 1K FPU context size already, 48 bytes won't make a visible dent - especially if compacted form has some saving cost. With AVX512 we are at around 2K?

I certainly don't _object_ to using the compacted format, as it makes sense and it's clearly getting quite a bit of attention from the hardware folks, but we should run some timings and such.

In any case I'm mostly just curious, not worried.

Thanks,

Ingo

The code has the following problems:

- it uses a single global 'fx_scratch' area that multiple CPUs could write into simultaneously, in theory.

- it wastes 512 bytes of .data for something that is only rarely used.

Fix this by moving the state buffer to the stack. Note that while this is 512 bytes, we don't ever call this function in very deep callchains, so its stack usage should not be a problem.

Also add comments to explain the magic 0x0000ffbf default value.

---
 arch/x86/kernel/fpu/init.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

index 33df056b1624..0b16f61cb2a4 100644
@@ -68,18 +68,26 @@ void fpu__init_check_bugs(void)
  * Boot time FPU feature detection code:
  */
 unsigned int mxcsr_feature_mask __read_mostly = 0xffffffffu;
+
 unsigned int xstate_size;
 EXPORT_SYMBOL_GPL(xstate_size);
-static struct i387_fxsave_struct fx_scratch;
 
 static void mxcsr_feature_mask_init(void)
 {
-	unsigned long mask = 0;
+	unsigned int mask = 0;
 
 	if (cpu_has_fxsr) {
-		memset(&fx_scratch, 0, sizeof(struct i387_fxsave_struct));
-		asm volatile("fxsave %0" : "+m" (fx_scratch));
-		mask = fx_scratch.mxcsr_mask;
+		struct i387_fxsave_struct fx_tmp __aligned(32) = { };
+
+		asm volatile("fxsave %0" : "+m" (fx_tmp));
+
+		mask = fx_tmp.mxcsr_mask;
+
+		/*
+		 * If zero then use the default features mask,
+		 * which has all features set, except the
+		 * denormals-are-zero feature bit:
+		 */
 		if (mask == 0)
 			mask = 0x0000ffbf;
 	}

2.1.0

Rename regset accessors to prefix them with 'regset_', because we want to start using the 'fpregs_active' name elsewhere.

---
 arch/x86/include/asm/fpu/internal.h | 8 ++++----
 arch/x86/kernel/fpu/core.c          | 6 +++---
 arch/x86/kernel/ptrace.c            | 6 +++---
 3 files changed, 10 insertions(+), 10 deletions(-)

index 07c6adc02f68..6eea81c068fb 100644
@@ -46,17 +46,17 @@ extern void convert_from_fxsr(struct user_i387_ia32_struct *env,
 extern void convert_to_fxsr(struct task_struct *tsk,
 			    const struct user_i387_ia32_struct *env);
 
-extern user_regset_active_fn fpregs_active, xfpregs_active;
+extern user_regset_active_fn regset_fpregs_active, regset_xregset_fpregs_active;
 extern user_regset_get_fn fpregs_get, xfpregs_get, fpregs_soft_get,
 				xstateregs_get;
 extern user_regset_set_fn fpregs_set, xfpregs_set, fpregs_soft_set,
 				 xstateregs_set;
 
 /*
- * xstateregs_active == fpregs_active. Please refer to the comment
- * at the definition of fpregs_active.
+ * xstateregs_active == regset_fpregs_active. Please refer to the comment
+ * at the definition of regset_fpregs_active.
  */
-#define xstateregs_active	fpregs_active
+#define xstateregs_active	regset_fpregs_active
 
 #ifdef CONFIG_MATH_EMULATION
 extern void finit_soft_fpu(struct i387_soft_struct *soft);

index c12dd3c0aabb..35ef1f9b56b3 100644
@@ -403,18 +403,18 @@ void fpu__clear(struct task_struct *tsk)
 }
 
 /*
- * The xstateregs_active() routine is the same as the fpregs_active() routine,
+ * The xstateregs_active() routine is the same as the regset_fpregs_active() routine,
  * as the "regset->n" for the xstate regset will be updated based on the feature
  * capabilites supported by the xsave.
  */
-int fpregs_active(struct task_struct *target, const struct user_regset *regset)
+int regset_fpregs_active(struct task_struct *target, const struct user_regset *regset)
 {
 	struct fpu *target_fpu = &target->thread.fpu;
 
 	return target_fpu->fpstate_active ? regset->n : 0;
 }
 
-int xfpregs_active(struct task_struct *target, const struct user_regset *regset)
+int regset_xregset_fpregs_active(struct task_struct *target, const struct user_regset *regset)
 {
 	struct fpu *target_fpu = &target->thread.fpu;
 

index c14a00f54b61..4c615661ec72 100644
@@ -1296,7 +1296,7 @@ static struct user_regset x86_64_regsets[] __read_mostly = {
 		.core_note_type = NT_PRFPREG,
 		.n = sizeof(struct user_i387_struct) / sizeof(long),
 		.size = sizeof(long), .align = sizeof(long),
-		.active = xfpregs_active, .get = xfpregs_get, .set = xfpregs_set
+		.active = regset_xregset_fpregs_active, .get = xfpregs_get, .set = xfpregs_set
 	},
 	[REGSET_XSTATE] = {
 		.core_note_type = NT_X86_XSTATE,

@@ -1337,13 +1337,13 @@ static struct user_regset x86_32_regsets[] __read_mostly = {
 		.core_note_type = NT_PRFPREG,
 		.n = sizeof(struct user_i387_ia32_struct) / sizeof(u32),
 		.size = sizeof(u32), .align = sizeof(u32),
-		.active = fpregs_active, .get = fpregs_get, .set = fpregs_set
+		.active = regset_fpregs_active, .get = fpregs_get, .set = fpregs_set
 	},
 	[REGSET_XFP] = {
 		.core_note_type = NT_PRXFPREG,
 		.n = sizeof(struct user32_fxsr_struct) / sizeof(u32),
 		.size = sizeof(u32), .align = sizeof(u32),
-		.active = xfpregs_active, .get = xfpregs_get, .set = xfpregs_set
+		.active = regset_xregset_fpregs_active, .get = xfpregs_get, .set = xfpregs_set
 	},
 	[REGSET_XSTATE] = {
 		.core_note_type = NT_X86_XSTATE,

2.1.0

The previous explanation was rather cryptic.

Also transform "u32 [64]" to the more readable "u8[256]" form.

No change in implementation.

---
 arch/x86/include/asm/fpu/types.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

index 9bd2cd1a19fd..8a5120a3b48b 100644
@@ -78,9 +78,16 @@ struct i387_soft_struct {
 	u32			entry_eip;
 };
 
+/*
+ * There are 16x 256-bit AVX registers named YMM0-YMM15.
+ * The low 128 bits are aliased to the 16 SSE registers (XMM0-XMM15)
+ * and are stored in 'struct i387_fxsave_struct::xmm_space[]'.
+ *
+ * The high 128 bits are stored here:
+ *    16x 128 bits == 256 bytes.
+ */
 struct ymmh_struct {
-	/* 16 * 16 bytes for each YMMH-reg = 256 bytes */
-	u32				ymmh_space[64];
+	u8				ymmh_space[256];
 };
 
 /* We don't support LWP yet: */

2.1.0

Improve the comments and add new ones, as this code isn't very obvious.

---
 arch/x86/kernel/fpu/xsave.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

index 467e4635bd29..f3d30f0c50f9 100644
@@ -30,19 +30,23 @@ static unsigned int xstate_comp_offsets[sizeof(xfeatures_mask)*8];
 static unsigned int xfeatures_nr;
 
 /*
- * If a processor implementation discern that a processor state component is
- * in its initialized state it may modify the corresponding bit in the
- * header.xfeatures as '0', with out modifying the corresponding memory
- * layout in the case of xsaveopt. While presenting the xstate information to
- * the user, we always ensure that the memory layout of a feature will be in
- * the init state if the corresponding header bit is zero. This is to ensure
- * that the user doesn't see some stale state in the memory layout during
- * signal handling, debugging etc.
+ * When executing XSAVEOPT (optimized XSAVE), if a processor implementation
+ * detects that an FPU state component is still (or is again) in its
+ * initialized state, it may clear the corresponding bit in the header.xfeatures
+ * field, and can skip the writeout of registers to the corresponding memory layout.
+ *
+ * This means that when the bit is zero, the state component might still contain
+ * some previous - non-initialized register state.
+ *
+ * Before writing xstate information to user-space we sanitize those components,
+ * to always ensure that the memory layout of a feature will be in the init state
+ * if the corresponding header bit is zero. This is to ensure that user-space doesn't
+ * see some stale state in the memory layout during signal handling, debugging etc.
  */
 void __sanitize_i387_state(struct task_struct *tsk)
 {
 	struct i387_fxsave_struct *fx = &tsk->thread.fpu.state->fxsave;
-	int feature_bit = 0x2;
+	int feature_bit;
 	u64 xfeatures;
 
 	if (!fx)

@@ -76,19 +80,25 @@ void __sanitize_i387_state(struct task_struct *tsk)
 	if (!(xfeatures & XSTATE_SSE))
 		memset(&fx->xmm_space[0], 0, 256);
 
+	/*
+	 * First two features are FPU and SSE, which above we handled
+	 * in a special way already:
+	 */
+	feature_bit = 0x2;
 	xfeatures = (xfeatures_mask & ~xfeatures) >> 2;
 
 	/*
-	 * Update all the other memory layouts for which the corresponding
-	 * header bit is in the init state.
+	 * Update all the remaining memory layouts according to their
+	 * standard xstate layout, if their header bit is in the init
+	 * state:
 	 */
 	while (xfeatures) {
 		if (xfeatures & 0x1) {
 			int offset = xstate_offsets[feature_bit];
 			int size = xstate_sizes[feature_bit];
 
-			memcpy(((void *) fx) + offset,
-			       ((void *) init_xstate_buf) + offset,
+			memcpy((void *)fx + offset,
+			       (void *)init_xstate_buf + offset,
 			       size);
 		}
 

2.1.0

So the current code uses fpu->has_cpu to determine whether a given user FPU context is actively loaded into the FPU's registers [*] and that those registers represent the task's current FPU state.

But this term is not unambiguous: especially the distinction between fpu->has_fpu, PF_USED_MATH and fpu_fpregs_owner_ctx is not clear.

Increase clarity by unambigously signalling that it's about hardware registers being active right now, by renaming it to fpu->fpregs_active.

( In later patches we'll use more of the 'fpregs' naming, which will
make it easier to grep for as well. )

[*] There's the kernel_fpu_begin()/end() primitive that also
activates FPU hw registers as well and uses them, without touching the fpu->fpregs_active flag.

---
 arch/x86/include/asm/fpu/api.h      |  2 +-
 arch/x86/include/asm/fpu/internal.h | 12 ++++++------
 arch/x86/include/asm/fpu/types.h    |  2 +-
 arch/x86/kernel/fpu/core.c          | 10 +++++-----
 4 files changed, 13 insertions(+), 13 deletions(-)

index 5bdde8ca87bc..4ca745c0d92e 100644
@@ -96,7 +96,7 @@ static inline void irq_ts_restore(int TS_state)
  */
 static inline int user_has_fpu(void)
 {
-	return current->thread.fpu.has_fpu;
+	return current->thread.fpu.fpregs_active;
 }
 
 extern void fpu__save(struct fpu *fpu);

index 6eea81c068fb..b546ec816fd6 100644
@@ -308,7 +308,7 @@ static inline int restore_fpu_checking(struct fpu *fpu)
 			"fnclex\n\t"
 			"emms\n\t"
 			"fildl %P[addr]"	/* set F?P to defined value */
-			: : [addr] "m" (fpu->has_fpu));
+			: : [addr] "m" (fpu->fpregs_active));
 	}
 
 	return fpu_restore_checking(fpu);

@@ -317,14 +317,14 @@ static inline int restore_fpu_checking(struct fpu *fpu)
 /* Must be paired with an 'stts' after! */
 static inline void __thread_clear_has_fpu(struct fpu *fpu)
 {
-	fpu->has_fpu = 0;
+	fpu->fpregs_active = 0;
 	this_cpu_write(fpu_fpregs_owner_ctx, NULL);
 }
 
 /* Must be paired with a 'clts' before! */
 static inline void __thread_set_has_fpu(struct fpu *fpu)
 {
-	fpu->has_fpu = 1;
+	fpu->fpregs_active = 1;
 	this_cpu_write(fpu_fpregs_owner_ctx, fpu);
 }
 

@@ -357,7 +357,7 @@ static inline void drop_fpu(struct fpu *fpu)
 	preempt_disable();
 	fpu->counter = 0;
 
-	if (fpu->has_fpu) {
+	if (fpu->fpregs_active) {
 		/* Ignore delayed exceptions from user space */
 		asm volatile("1: fwait\n"
 			     "2:\n"

@@ -416,14 +416,14 @@ switch_fpu_prepare(struct fpu *old_fpu, struct fpu *new_fpu, int cpu)
 	fpu.preload = new_fpu->fpstate_active &&
 		      (use_eager_fpu() || new_fpu->counter > 5);
 
-	if (old_fpu->has_fpu) {
+	if (old_fpu->fpregs_active) {
 		if (!fpu_save_init(old_fpu))
 			old_fpu->last_cpu = -1;
 		else
 			old_fpu->last_cpu = cpu;
 
 		/* But leave fpu_fpregs_owner_ctx! */
-		old_fpu->has_fpu = 0;
+		old_fpu->fpregs_active = 0;
 
 		/* Don't change CR0.TS if we just switch! */
 		if (fpu.preload) {

index 8a5120a3b48b..231a8f53b2f8 100644
@@ -142,7 +142,7 @@ struct fpu {
 	 */
 	unsigned int			last_cpu;
 
-	unsigned int			has_fpu;
+	unsigned int			fpregs_active;
 	union thread_xstate		*state;
 	/*
 	 * This counter contains the number of consecutive context switches

index 35ef1f9b56b3..a7fb56266a2d 100644
@@ -62,7 +62,7 @@ static bool interrupted_kernel_fpu_idle(void)
 	if (use_eager_fpu())
 		return true;
 
-	return !current->thread.fpu.has_fpu && (read_cr0() & X86_CR0_TS);
+	return !current->thread.fpu.fpregs_active && (read_cr0() & X86_CR0_TS);
 }
 
 /*

@@ -100,7 +100,7 @@ void __kernel_fpu_begin(void)
 
 	kernel_fpu_disable();
 
-	if (fpu->has_fpu) {
+	if (fpu->fpregs_active) {
 		fpu_save_init(fpu);
 	} else {
 		this_cpu_write(fpu_fpregs_owner_ctx, NULL);

@@ -114,7 +114,7 @@ void __kernel_fpu_end(void)
 {
 	struct fpu *fpu = &current->thread.fpu;
 
-	if (fpu->has_fpu) {
+	if (fpu->fpregs_active) {
 		if (WARN_ON(restore_fpu_checking(fpu)))
 			fpu_reset_state(fpu);
 	} else if (!use_eager_fpu()) {

@@ -147,7 +147,7 @@ void fpu__save(struct fpu *fpu)
 	WARN_ON(fpu != &current->thread.fpu);
 
 	preempt_disable();
-	if (fpu->has_fpu) {
+	if (fpu->fpregs_active) {
 		if (use_eager_fpu()) {
 			__save_fpu(fpu);
 		} else {

@@ -243,7 +243,7 @@ static void fpu_copy(struct fpu *dst_fpu, struct fpu *src_fpu)
 int fpu__copy(struct fpu *dst_fpu, struct fpu *src_fpu)
 {
 	dst_fpu->counter = 0;
-	dst_fpu->has_fpu = 0;
+	dst_fpu->fpregs_active = 0;
 	dst_fpu->state = NULL;
 	dst_fpu->last_cpu = -1;
 

2.1.0

Propagate the 'fpu->fpregs_active' naming to the functions that sets it.

---
 arch/x86/include/asm/fpu/internal.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index b546ec816fd6..3554a8cdaece 100644
@@ -322,7 +322,7 @@ static inline void __thread_clear_has_fpu(struct fpu *fpu)
 }
 
 /* Must be paired with a 'clts' before! */
-static inline void __thread_set_has_fpu(struct fpu *fpu)
+static inline void __fpregs_activate(struct fpu *fpu)
 {
 	fpu->fpregs_active = 1;
 	this_cpu_write(fpu_fpregs_owner_ctx, fpu);

@@ -346,7 +346,7 @@ static inline void __thread_fpu_begin(struct fpu *fpu)
 {
 	if (!use_eager_fpu())
 		clts();
-	__thread_set_has_fpu(fpu);
+	__fpregs_activate(fpu);
 }
 
 static inline void drop_fpu(struct fpu *fpu)

@@ -428,7 +428,7 @@ switch_fpu_prepare(struct fpu *old_fpu, struct fpu *new_fpu, int cpu)
 		/* Don't change CR0.TS if we just switch! */
 		if (fpu.preload) {
 			new_fpu->counter++;
-			__thread_set_has_fpu(new_fpu);
+			__fpregs_activate(new_fpu);
 			prefetch(new_fpu->state);
 		} else if (!use_eager_fpu())
 			stts();

2.1.0

Propagate the 'fpu->fpregs_active' naming to the functions that clears it.

---
 arch/x86/include/asm/fpu/internal.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 3554a8cdaece..7a235171be6c 100644
@@ -315,7 +315,7 @@ static inline int restore_fpu_checking(struct fpu *fpu)
 }
 
 /* Must be paired with an 'stts' after! */
-static inline void __thread_clear_has_fpu(struct fpu *fpu)
+static inline void __fpregs_deactivate(struct fpu *fpu)
 {
 	fpu->fpregs_active = 0;
 	this_cpu_write(fpu_fpregs_owner_ctx, NULL);

@@ -337,7 +337,7 @@ static inline void __fpregs_activate(struct fpu *fpu)
  */
 static inline void __thread_fpu_end(struct fpu *fpu)
 {
-	__thread_clear_has_fpu(fpu);
+	__fpregs_deactivate(fpu);
 	if (!use_eager_fpu())
 		stts();
 }

2.1.0

Propagate the 'fpu->fpregs_active' naming to the high level function that sets it.

---
 arch/x86/include/asm/fpu/internal.h | 6 +++---
 arch/x86/kernel/fpu/core.c          | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

index 7a235171be6c..18a62239c73d 100644
@@ -342,7 +342,7 @@ static inline void __thread_fpu_end(struct fpu *fpu)
 		stts();
 }
 
-static inline void __thread_fpu_begin(struct fpu *fpu)
+static inline void fpregs_activate(struct fpu *fpu)
 {
 	if (!use_eager_fpu())
 		clts();

@@ -441,7 +441,7 @@ switch_fpu_prepare(struct fpu *old_fpu, struct fpu *new_fpu, int cpu)
 				fpu.preload = 0;
 			else
 				prefetch(new_fpu->state);
-			__thread_fpu_begin(new_fpu);
+			fpregs_activate(new_fpu);
 		}
 	}
 	return fpu;

@@ -499,7 +499,7 @@ static inline void user_fpu_begin(void)
 
 	preempt_disable();
 	if (!user_has_fpu())
-		__thread_fpu_begin(fpu);
+		fpregs_activate(fpu);
 	preempt_enable();
 }
 

index a7fb56266a2d..75b94985c82e 100644
@@ -368,9 +368,9 @@ void fpu__restore(void)
 		local_irq_disable();
 	}
 
-	/* Avoid __kernel_fpu_begin() right after __thread_fpu_begin() */
+	/* Avoid __kernel_fpu_begin() right after fpregs_activate() */
 	kernel_fpu_disable();
-	__thread_fpu_begin(fpu);
+	fpregs_activate(fpu);
 	if (unlikely(restore_fpu_checking(fpu))) {
 		fpu_reset_state(fpu);
 		force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk);

2.1.0

Propagate the 'fpu->fpregs_active' naming to the high level function that clears it.

---
 arch/x86/include/asm/fpu/internal.h | 14 +++++++-------
 arch/x86/kernel/fpu/core.c          |  2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

index 18a62239c73d..0292fcc4d441 100644
@@ -335,18 +335,18 @@ static inline void __fpregs_activate(struct fpu *fpu)
  * These generally need preemption protection to work,
  * do try to avoid using these on their own.
  */
-static inline void __thread_fpu_end(struct fpu *fpu)
+static inline void fpregs_activate(struct fpu *fpu)
 {
-	__fpregs_deactivate(fpu);
 	if (!use_eager_fpu())
-		stts();
+		clts();
+	__fpregs_activate(fpu);
 }
 
-static inline void fpregs_activate(struct fpu *fpu)
+static inline void fpregs_deactivate(struct fpu *fpu)
 {
+	__fpregs_deactivate(fpu);
 	if (!use_eager_fpu())
-		clts();
-	__fpregs_activate(fpu);
+		stts();
 }
 
 static inline void drop_fpu(struct fpu *fpu)

@@ -362,7 +362,7 @@ static inline void drop_fpu(struct fpu *fpu)
 		asm volatile("1: fwait\n"
 			     "2:\n"
 			     _ASM_EXTABLE(1b, 2b));
-		__thread_fpu_end(fpu);
+		fpregs_deactivate(fpu);
 	}
 
 	fpu->fpstate_active = 0;

index 75b94985c82e..2c47bcf63e1e 100644
@@ -152,7 +152,7 @@ void fpu__save(struct fpu *fpu)
 			__save_fpu(fpu);
 		} else {
 			fpu_save_init(fpu);
-			__thread_fpu_end(fpu);
+			fpregs_deactivate(fpu);
 		}
 	}
 	preempt_enable();

2.1.0

fpstate_xstate_init_size() is called in fpu__cpu_init(), which is run on every CPU, every time they are brought online.

But we want to call fpstate_xstate_init_size() only once. Move it to fpu__detect(), which only runs once, on the boot CPU.

Also clean up the flow of fpstate_xstate_init_size() a bit, by removing a 'return' from the middle of the function.

---
 arch/x86/kernel/fpu/init.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

index 0b16f61cb2a4..3e0fee5bc2e7 100644
@@ -109,13 +109,12 @@ static void fpstate_xstate_init_size(void)
 		setup_clear_cpu_cap(X86_FEATURE_XSAVE);
 		setup_clear_cpu_cap(X86_FEATURE_XSAVEOPT);
 		xstate_size = sizeof(struct i387_soft_struct);
-		return;
+	} else {
+		if (cpu_has_fxsr)
+			xstate_size = sizeof(struct i387_fxsave_struct);
+		else
+			xstate_size = sizeof(struct i387_fsave_struct);
 	}
-
-	if (cpu_has_fxsr)
-		xstate_size = sizeof(struct i387_fxsave_struct);
-	else
-		xstate_size = sizeof(struct i387_fsave_struct);
 }
 
 /*

@@ -151,12 +150,6 @@ void fpu__cpu_init(void)
 		cr0 |= X86_CR0_EM;
 	write_cr0(cr0);
 
-	/*
-	 * fpstate_xstate_init_size() is only called once, to avoid overriding
-	 * 'xstate_size' during (secondary CPU) bootup or during CPU hotplug.
-	 */
-	if (xstate_size == 0)
-		fpstate_xstate_init_size();
 
 	mxcsr_feature_mask_init();
 	xsave_init();

@@ -194,5 +187,7 @@ void fpu__detect(struct cpuinfo_x86 *c)
 	else
 		clear_cpu_cap(c, X86_FEATURE_FPU);
 
-	/* The final cr0 value is set in fpu_init() */
+	/* The final cr0 value is set later, in fpu_init() */
+
+	fpstate_xstate_init_size();
 }

2.1.0

There's only 8 xstate bits at the moment, and it's not like we can support unknown bits - so put xstate_offsets[] and xstate_sizes[] into static allocation.

This is in preparation to be able to call the FPU init code earlier, when there's no bootmem available yet.

---
 arch/x86/include/asm/fpu/xsave.h | 3 +++
 arch/x86/kernel/fpu/xsave.c      | 4 +---
 2 files changed, 4 insertions(+), 3 deletions(-)

index b27b4466f88d..fd564344783e 100644
@@ -15,6 +15,9 @@
 #define XSTATE_ZMM_Hi256	0x40
 #define XSTATE_Hi16_ZMM		0x80
 
+/* The highest xstate bit above (of XSTATE_Hi16_ZMM): */
+#define XFEATURES_NR_MAX	8
+
 #define XSTATE_FPSSE	(XSTATE_FP | XSTATE_SSE)
 #define XSTATE_AVX512	(XSTATE_OPMASK | XSTATE_ZMM_Hi256 | XSTATE_Hi16_ZMM)
 /* Bit 63 of XCR0 is reserved for future expansion */

index f3d30f0c50f9..adeab16655ae 100644
@@ -23,7 +23,7 @@ u64 xfeatures_mask;
 struct xsave_struct *init_xstate_buf;
 
 static struct _fpx_sw_bytes fx_sw_reserved, fx_sw_reserved_ia32;
-static unsigned int *xstate_offsets, *xstate_sizes;
+static unsigned int xstate_offsets[XFEATURES_NR_MAX], xstate_sizes[XFEATURES_NR_MAX];
 static unsigned int xstate_comp_offsets[sizeof(xfeatures_mask)*8];
 
 /* The number of supported xfeatures in xfeatures_mask: */

@@ -478,8 +478,6 @@ static void __init setup_xstate_features(void)
 	int eax, ebx, ecx, edx, leaf = 0x2;
 
 	xfeatures_nr = fls64(xfeatures_mask);
-	xstate_offsets = alloc_bootmem(xfeatures_nr * sizeof(int));
-	xstate_sizes = alloc_bootmem(xfeatures_nr * sizeof(int));
 
 	do {
 		cpuid_count(XSTATE_CPUID, leaf, &eax, &ebx, &ecx, &edx);

2.1.0

Remove the dependency on the init_xstate_buf == NULL check to implement once-per-bootup logic in eager_fpu_init(), by making setup_init_fpu_buf() run once per bootup explicitly.

This is in preparation to make init_xstate_buf statically allocated.

The various boot-once quirks in the FPU init code will be removed in a later cleanup stage.

---
 arch/x86/kernel/fpu/xsave.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

index adeab16655ae..d11b33514130 100644
@@ -565,8 +565,14 @@ void setup_xstate_comp(void)
 /*
  * setup the xstate image representing the init state
  */
-static void __init setup_init_fpu_buf(void)
+static void setup_init_fpu_buf(void)
 {
+	static int on_boot_cpu = 1;
+
+	if (!on_boot_cpu)
+		return;
+	on_boot_cpu = 0;
+
 	/*
 	 * Setup init_xstate_buf to represent the init state of
 	 * all the features managed by the xsave

@@ -738,8 +744,7 @@ void __init_refok eager_fpu_init(void)
 		return;
 	}
 
-	if (!init_xstate_buf)
-		setup_init_fpu_buf();
+	setup_init_fpu_buf();
 }
 
 /*

2.1.0

Make init_xstate_buf allocated statically at build time.

This structure's maximum size is around 1KB - and it's allocated even on most modern embedded x86 CPUs which strive for FPU instruction set parity with desktop and server CPUs, so it's not like we can save much on smaller systems.

This removes the last bootmem allocation from the FPU init path, allowing it to be called earlier in the boot sequence.

---
 arch/x86/include/asm/fpu/internal.h |  4 ++--
 arch/x86/include/asm/fpu/xsave.h    |  2 +-
 arch/x86/kernel/fpu/xsave.c         | 26 +++++++++++---------------
 3 files changed, 14 insertions(+), 18 deletions(-)

index 0292fcc4d441..19b7cdf73efd 100644
@@ -373,9 +373,9 @@ static inline void drop_fpu(struct fpu *fpu)
 static inline void restore_init_xstate(void)
 {
 	if (use_xsave())
-		xrstor_state(init_xstate_buf, -1);
+		xrstor_state(&init_xstate_ctx, -1);
 	else
-		fxrstor_checking(&init_xstate_buf->i387);
+		fxrstor_checking(&init_xstate_ctx.i387);
 }
 
 /*

index fd564344783e..5c3ab4e17aea 100644
@@ -50,7 +50,7 @@
 extern unsigned int xstate_size;
 extern u64 xfeatures_mask;
 extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
-extern struct xsave_struct *init_xstate_buf;
+extern struct xsave_struct init_xstate_ctx;
 
 extern void xsave_init(void);
 extern void update_regset_xstate_info(unsigned int size, u64 xstate_mask);

index d11b33514130..45130ba6f328 100644
@@ -3,7 +3,6 @@
  *
  * Author: Suresh Siddha <[email protected]>
  */
-#include <linux/bootmem.h>
 #include <linux/compat.h>
 #include <linux/cpu.h>
 #include <asm/fpu/api.h>

@@ -20,7 +19,7 @@ u64 xfeatures_mask;
 /*
  * Represents init state for the supported extended state.
  */
-struct xsave_struct *init_xstate_buf;
+struct xsave_struct init_xstate_ctx;
 
 static struct _fpx_sw_bytes fx_sw_reserved, fx_sw_reserved_ia32;
 static unsigned int xstate_offsets[XFEATURES_NR_MAX], xstate_sizes[XFEATURES_NR_MAX];

@@ -98,7 +97,7 @@ void __sanitize_i387_state(struct task_struct *tsk)
 			int size = xstate_sizes[feature_bit];
 
 			memcpy((void *)fx + offset,
-			       (void *)init_xstate_buf + offset,
+			       (void *)&init_xstate_ctx + offset,
 			       size);
 		}
 

@@ -325,12 +324,12 @@ static inline int restore_user_xstate(void __user *buf, u64 xbv, int fx_only)
 	if (use_xsave()) {
 		if ((unsigned long)buf % 64 || fx_only) {
 			u64 init_bv = xfeatures_mask & ~XSTATE_FPSSE;
-			xrstor_state(init_xstate_buf, init_bv);
+			xrstor_state(&init_xstate_ctx, init_bv);
 			return fxrstor_user(buf);
 		} else {
 			u64 init_bv = xfeatures_mask & ~xbv;
 			if (unlikely(init_bv))
-				xrstor_state(init_xstate_buf, init_bv);
+				xrstor_state(&init_xstate_ctx, init_bv);
 			return xrestore_user(buf, xbv);
 		}
 	} else if (use_fxsr()) {

@@ -574,12 +573,10 @@ static void setup_init_fpu_buf(void)
 	on_boot_cpu = 0;
 
 	/*
-	 * Setup init_xstate_buf to represent the init state of
+	 * Setup init_xstate_ctx to represent the init state of
 	 * all the features managed by the xsave
 	 */
-	init_xstate_buf = alloc_bootmem_align(xstate_size,
-					      __alignof__(struct xsave_struct));
-	fx_finit(&init_xstate_buf->i387);
+	fx_finit(&init_xstate_ctx.i387);
 
 	if (!cpu_has_xsave)
 		return;

@@ -588,21 +585,20 @@ static void setup_init_fpu_buf(void)
 	print_xstate_features();
 
 	if (cpu_has_xsaves) {
-		init_xstate_buf->header.xcomp_bv =
-						(u64)1 << 63 | xfeatures_mask;
-		init_xstate_buf->header.xfeatures = xfeatures_mask;
+		init_xstate_ctx.header.xcomp_bv = (u64)1 << 63 | xfeatures_mask;
+		init_xstate_ctx.header.xfeatures = xfeatures_mask;
 	}
 
 	/*
 	 * Init all the features state with header_bv being 0x0
 	 */
-	xrstor_state_booting(init_xstate_buf, -1);
+	xrstor_state_booting(&init_xstate_ctx, -1);
 
 	/*
 	 * Dump the init state again. This is to identify the init state
 	 * of any feature which is not represented by all zero's.
 	 */
-	xsave_state_booting(init_xstate_buf);
+	xsave_state_booting(&init_xstate_ctx);
 }
 
 static enum { AUTO, ENABLE, DISABLE } eagerfpu = AUTO;

@@ -727,7 +723,7 @@ void xsave_init(void)
 
 /*
  * setup_init_fpu_buf() is __init and it is OK to call it here because
- * init_xstate_buf will be unset only once during boot.
+ * init_xstate_ctx will be unset only once during boot.
  */
 void __init_refok eager_fpu_init(void)
 {

2.1.0

Actually we could statically associate a biggerbuffer based on the XCR0 features we support. That would preclude dynamic enabling and really just adds complexity for no good reason.

Sent from my mobile phone.  Please pardon brevity and lack of formatting.

Yeah, so I got this prototype hardware boot crash reported in private mail and decoded it and after some debugging I suggested the +PAGE_SIZE hack - possibly you got that hack from the same person?

My suggestion was to solve this properly: if we list xstate features as supported then we should size their max size correctly. The AVX bits are currently not properly enumerated and sized - and I refuse to add feature support to the kernel where per task CPU state fields that the kernel saves/restores are opaque...

So please add proper AVX512 support structures to fpu/types.h and size XSTATE_RESERVE correctly - or alternatively we can remove the current incomplete AVX512 bits.

Thanks,

Ingo

...

Nope, I came up with that gem of a patch all on my own.

I also wouldn't characterize this as prototype hardware. There are obviously plenty of folks depending on mainline to boot and function on hardware that has AVX-512 support. That's why two different Intel folks came to you independently.

We might know the size and composition of the individual components, but we do not know the size of the buffer. Different implementations of a given feature are quite free to have different data stored in the buffer, or even to rearrange or pad it. That's why the sizes are not explicitly called out by the architecture and why we enumerated them before your patch that caused this regression.

The component itself may not be opaque, but the size of the *buffer* is not a simple sum of the component sizes. Here's a real-world example:

[    0.000000] x86/fpu: xstate_offset[2]: 0240, xstate_sizes[2]: 0100
[    0.000000] x86/fpu: xstate_offset[3]: 03c0, xstate_sizes[3]: 0040

Notice that component 3 is not at 0x240+0x100. This means our existing init_xstate_size(), and why any attempt to staticlly-size the buffer is broken.

I understand why you were misled by it, but the old "xsave_hdr_struct" was wrong. Fenghua even posted patches to remove it before the FPU rework (you were cc'd):

	https://lkml.org/lkml/2015/4/18/164

The old code sized the buffer in a fully architectural way and it worked. The CPU *tells* you how much memory the 'xsave' instruction is going to scribble on. The new code just merrily calls it and let it scribble away. This is as clear-cut a regression as I've ever seen.

The least we can do is detect that the kernel undersized the buffer and disable support for the features that do not fit. A very lightly tested patch to do that is attached. I'm not super eager to put that in to an -rc2 kernel though.

This came out a lot more complicated than I would have liked.

Instead of simply enabling all of the XSAVE features that we both know about and the CPU supports, we have to be careful to not overflow our buffer in 'init_fpstate.xsave'.

To do this, we enable each XSAVE feature and then ask the CPU how large that makes the buffer. If we would overflow the buffer we allocated, we turn off the feature.

This means that no matter what the CPU does, we will not corrupt random memory like we do before this patch. It also means that we can fall back in a way which cripples the system the least.

---

 b/arch/x86/include/asm/fpu/types.h |   20 ++++-
 b/arch/x86/kernel/fpu/xstate.c     |  140 +++++++++++++++++++++++++++++++------
 2 files changed, 134 insertions(+), 26 deletions(-)

diff -puN arch/x86/kernel/fpu/xstate.c~xsaves-are-my-friend arch/x86/kernel/fpu/xstate.c
@@ -161,11 +161,10 @@ void fpstate_sanitize_xstate(struct fpu
  */
 void fpu__init_cpu_xstate(void)
 {
-	if (!cpu_has_xsave || !xfeatures_mask)
+	if (!cpu_has_xsave)
 		return;
 
 	cr4_set_bits(X86_CR4_OSXSAVE);
-	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
 }
 
 /*

@@ -291,25 +290,129 @@ static void __init setup_init_fpu_buf(vo
 }
 
 /*
- * Calculate total size of enabled xstates in XCR0/xfeatures_mask.
+ * This returns the size that the 'xsave*' instructions will
+ * write to the xsave buffer.  This is dynamic and will change
+ * as we enable more feature bits in XCR0.
  */
-static void __init init_xstate_size(void)
+unsigned int kernel_xstate_buffer_size(void)
 {
 	unsigned int eax, ebx, ecx, edx;
-	int i;
 
 	if (!cpu_has_xsaves) {
 		cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
-		xstate_size = ebx;
-		return;
+		return ebx;
+	} else {
+		cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);
+		return ebx;
 	}
+}
+
+u64 cpu_supported_xfeatures_mask(void)
+{
+	unsigned int eax, ebx, ecx, edx;
+	u64 ret;
+
+	cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
+	ret = eax + ((u64)edx << 32);
+
+	return ret;
+}
+
+/*
+ * We enable the features in sets.  Some implementations are
+ * buggy and do not handle it if only one of two related
+ * xfeatures are enabled.
+ */
+u64 xfeature_sets[] = {
+	XSTATE_YMM,
+	XSTATE_BNDREGS | XSTATE_BNDCSR,
+	XSTATE_AVX512,
+};
+
+/*
+ * Enable each supported feature in XCR0, determine the required
+ * size of the save buffer with the feature enabled, and determine
+ * if it fits in the buffer we allocated.
+ *
+ * This function will initialize these global variables:
+ * 1. xfeatures_mask
+ * 2. xstate_size
+ */
+static void __init enable_supported_xfeatures(void)
+{
+	int i;
+	int xbufsz = sizeof(init_fpstate.xsave);
+	u64 cpu_support_mask = cpu_supported_xfeatures_mask();
 
-	xstate_size = FXSAVE_SIZE + XSAVE_HDR_SIZE;
-	for (i = 2; i < 64; i++) {
-		if (test_bit(i, (unsigned long *)&xfeatures_mask)) {
-			cpuid_count(XSTATE_CPUID, i, &eax, &ebx, &ecx, &edx);
-			xstate_size += eax;
+	/*
+	 * We are past sanity_check_xfeatures_supported() so we
+	 * know these states are supported.
+	 */
+	xfeatures_mask |= XSTATE_FPSSE;
+
+	for (i = 0; i < ARRAY_SIZE(xfeature_sets); i++) {
+		u64 feature_mask = xfeature_sets[i];
+		int tmp_xstate_size;
+
+		/* Support only the state known to the OS: */
+		if ((XCNTXT_MASK & feature_mask) != feature_mask)
+			continue;
+		/* Only try the states supported by the CPU */
+		if ((cpu_support_mask & feature_mask) != feature_mask)
+			continue;
+
+		/* Enable the potential feature in XCR0 */
+		xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask | feature_mask);
+		/* Ask the CPU how large of an XSAVE buffer we need now */
+		tmp_xstate_size = kernel_xstate_buffer_size();
+
+		if (tmp_xstate_size > xbufsz) {
+			/* reset XCR0 to its previous value */
+			xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
+			pr_warn("XSAVE feature 0x%02llx overflows init buf, "
+				"disabling (%d > %d)\n", feature_mask,
+				tmp_xstate_size, xbufsz);
+			continue;
 		}
+		/* "Commit" the feature to the permanent mask */
+		xfeatures_mask |= feature_mask;
+		xstate_size = tmp_xstate_size;
+	}
+	pr_info("x86/fpu: enabled %ld xfeatures (0x%llx), in %d byte buffer\n",
+			hweight64(xfeatures_mask), xfeatures_mask, xstate_size);
+	/* XCR0 should be fully initialized now */
+}
+
+/*
+ * The kernel generally only checks CPUID bits to tell if it
+ * should be using a given feature.  Some features are unusable
+ * unless we are able to enable XSAVE state management for them.
+ * So, we clear the CPUID bits for a features that we did not
+ * enable.
+ */
+void clear_unsupported_xfeature_cpuid_flags(void)
+{
+	/*
+	 * SDM: An operating system must enable its YMM state
+	 * management to support AVX... otherwise, an attempt
+	 * to execute an instruction cause a #UD exception.
+	 */
+	if (!(xfeatures_mask & XSTATE_YMM)) {
+		setup_clear_cpu_cap(X86_FEATURE_AVX);
+		setup_clear_cpu_cap(X86_FEATURE_AVX2);
+	}
+	/*
+	 * SDM: An OS must enable its ZMM and opmask state
+	 * management to support Intel AVX-512
+	 */
+	if (!(xfeatures_mask & XSTATE_OPMASK) ||
+	    !(xfeatures_mask & XSTATE_ZMM_Hi256) ||
+	    !(xfeatures_mask & XSTATE_Hi16_ZMM)) {
+		setup_clear_cpu_cap(X86_FEATURE_AVX512F);
+	}
+	if (!(xfeatures_mask & XSTATE_BNDREGS) ||
+	    !(xfeatures_mask & XSTATE_BNDCSR)) {
+		setup_clear_cpu_cap(X86_FEATURE_MPX);
 	}
 }
 

@@ -319,7 +422,6 @@ static void __init init_xstate_size(void
  */
 void __init fpu__init_system_xstate(void)
 {
-	unsigned int eax, ebx, ecx, edx;
 	static int on_boot_cpu = 1;
 
 	WARN_ON_FPU(!on_boot_cpu);

@@ -335,22 +437,16 @@ void __init fpu__init_system_xstate(void
 		return;
 	}
 
-	cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
-	xfeatures_mask = eax + ((u64)edx << 32);
-
-	if ((xfeatures_mask & XSTATE_FPSSE) != XSTATE_FPSSE) {
+	if ((cpu_supported_xfeatures_mask() & XSTATE_FPSSE) != XSTATE_FPSSE) {
 		pr_err("x86/fpu: FP/SSE not present amongst the CPU's xstate features: 0x%llx.\n", xfeatures_mask);
 		BUG();
 	}
 
-	/* Support only the state known to the OS: */
-	xfeatures_mask = xfeatures_mask & XCNTXT_MASK;
-
 	/* Enable xstate instructions to be able to continue with initialization: */
 	fpu__init_cpu_xstate();
 
-	/* Recompute the context size for enabled features: */
-	init_xstate_size();
+	enable_supported_xfeatures();
+	clear_unsupported_xfeature_cpuid_flags();
 
 	update_regset_xstate_info(xstate_size, xfeatures_mask);
 	fpu__init_prepare_fx_sw_frame();

diff -puN arch/x86/include/asm/fpu/types.h~xsaves-are-my-friend arch/x86/include/asm/fpu/types.h
@@ -159,10 +159,22 @@ struct xstate_header {
 	u64				reserved[6];
 } __attribute__((packed));
 
-/* New processor state extensions should be added here: */
-#define XSTATE_RESERVE			(sizeof(struct ymmh_struct) + \
-					 sizeof(struct lwp_struct)  + \
-					 sizeof(struct mpx_struct)  )
+/*
+ * The largest xsave buffer known today is 2752 bytes on a system
+ * implementing AVX-512.  This includes the 512-byte i387 state
+ * and 64-byte header.  We add a small amount of padding in case
+ * an implementation adds some padding or wastes some space.
+ *
+ * Note, if we overflow this, we will disable some XSTATE
+ * features.
+ *
+ * Also, note that the real size we need is enumerated by
+ * cpuid leaves and can not be known at compile time.
+ */
+#define XSTATE_RESERVE			(2752 + 256 -				\
+					 sizeof(struct xstate_header) - 	\
+					 sizeof(struct fxregs_state))
+
 /*
  * This is our most modern FPU state format, as saved by the XSAVE
  * and restored by the XRSTOR instructions.

_

Yes, I think we'll need to revert it, or do something else drastic like make that initial fp state allocation *much* bigger and then have a "disable xsaves if if it's still not big enough".

setup_xstate_features() should be able to easily just say "this was the maximum offset+size we saw", and we can take that to either do a proper allocation, or verify that the static allocation is indeed big enough.

Apparently a straight revert doesn't work, if only because things in that area have been renamed very aggressively (both files and functions and variables). Ingo?

Linus

Oh, and that patch looks like a good idea.

I wish there was some way to make sure sizeof() fail on it so that we'd enforce that nobody allocates that thing as-is. I had this dim memory that an unsized array at the end would do that, but I was clearly wrong. It's just the array itself you can't do sizeof on, not the structure that contains it. Is there some magic trick that I'm forgetting?

Linus

:)

Yeah, so I treat it as a regression even if it's unreleased hw, what matters to regressions is number of people affected, plus that the kernel should work for a reasonable set of future hardware as well, without much trouble.

Just curious: does any released hardware have AVX-512? I went by Wikipedia, which seems to list pre-release hw:

https://en.wikipedia.org/wiki/AVX-512#CPUs_with_AVX-512

    Intel
        Xeon Phi Knights Landing: AVX-512 F, CDI, PFI and ERI[1] in 2015[6]
        Xeon Skylake: AVX-512 F, CDI, VL, BW, and DQ[7] in 2015[8]
        Cannonlake (speculation)

But we _have_ to know their structure and layout of the XSAVE context for any reasonable ptrace and signal frame support. Can you set/get AVX-512 registers via ptrace? MPX state?

That's one of the reasons why I absolutely hate how this 'opaque per task CPU context blob' concept snuck into the x86 code via the XSAVE patches without proper enumeration of the data structures, sorry...

It makes it way too easy to 'support' CPU features without actually doing a good job of it - and in fact it makes certain reasonable things impossible or very, very hard, which makes me nervous.

But we'll fix the boot regression, no argument about that!

Yeah, so I thought the worst bugs were fixed and that these would re-emerge on top of the new code.

Whether we have a static limit or not is orthogonal to the issue of sizing it properly - and the plan was to have a dynamic context area in any case.

This is a regression which we'll fix, but the 'old' dynamic code clearly did not work for a long time, I'm sure you still remember my attempt at addressing the worst fallout in:

e88221c50cad ("x86/fpu: Disable XSAVES* support for now")

Those kinds of totally non-working aspects were what made me nervous about the opaque data structure aspect.

Because we can have dynamic sizing of the context area and non-opaque data structures.

Ok, this approach looks good to me as an interim fix. I'll give it a whirl on older hardware. I agree with you that it needs to be sized dynamically.

Yeah, and this can be fixed separately and on top of your fix: my plan during the FPU rework was to move the context area to the end of task_struct and size it dynamically.

This needs some (very minor) changes to kernel/fork.c to allow an architecture to determine the full task_struct size dynamically - but looks very doable and clean. Wanna try this, or should I?

Yes, agreed.

Thanks,

Ingo

Hm, so this patch crashed the boot of 2 out of 3 systems that I tried :-/

But it does not really matter, as I think the dynamic allocation is the right fix in any case (your last patch), so this patch should be moot.

Thanks,

Ingo

There are two different things here. One is the structure and layout inside of the state components. That obviously needs full kernel knowledge and can not be opaque, especially when the kernel needs to go looking at it (like with MPX's BNDCSR for instance).

But, the relative layout of the components is up for grabs. The CPU is completely free (architecturally) to pad components or rearrange things.
It's not opaque (it's fully enumerated in CPUID), but it's far from
something which is static or which we can realistically represent in a static structure.

The xsave buffer is just copied out to userspace with REGSET_XSTATE. Userspace needs to do the same song and dance with CPUID to parse it that the kernel does.

I think you already did this later in the thread.

Ok, agreed.

Indeed - I missed REGSET_XSTATE and its interaction with update_regset_xstate_info().

Good - I have no other complaints.

Yeah, wanted to get a fix for the regression to Linus ASAP. If we go changing core code in kernel/fork.c we better have it in -rc3.

So right now I have these two applied:

0f6df268588f x86/fpu, sched: Dynamically allocate 'struct fpu'
218d096a24b4 x86/fpu, sched: Introduce CONFIG_ARCH_WANTS_DYNAMIC_TASK_STRUCT and use it on x86

... do we need any of the other patches you sent to get working AVX512 support? I think we should be fine, but I don't have the hardware.

Thanks,

Ingo

I don't think so, but I'll go verify by actually running with those x86/fpu commits.

There are two kinds of FPU initialization sequences necessary to bring FPU functionality up: once per system bootup activities, such as detection, feature initialization, etc. of attributes that are shared by all CPUs in the system - and per cpu initialization sequences run when a CPU is brought online (either during bootup or during CPU hotplug onlining), such as CR0/CR4 register setting, etc.

The FPU code is mixing these roles together, with no clear distinction.

Start sorting this out by splitting the main FPU detection routine (fpu__cpu_init()) into two parts: fpu__init_system() for one per system init activities, and fpu__init_cpu() for the per CPU onlining init activities.

Note that xstate_init() is called from both variants for the time being, because it has a dual nature as well. We'll fix that in upcoming patches.

Just do the split and call it as we used to before, don't introduce any change in initialization behavior yet, beyond duplicate (and harmless) fpu__init_cpu() and xstate_init() calls - which we'll fix in later patches.

---
 arch/x86/kernel/fpu/init.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

index 3e0fee5bc2e7..d6234adc8ba0 100644
@@ -118,13 +118,9 @@ static void fpstate_xstate_init_size(void)
 }
 
 /*
- * Called on the boot CPU at bootup to set up the initial FPU state that
- * is later cloned into all processes.
- *
- * Also called on secondary CPUs to set up the FPU state of their
- * idle threads.
+ * Enable all supported FPU features. Called when a CPU is brought online.
  */
-void fpu__cpu_init(void)
+void fpu__init_cpu(void)
 {
 	unsigned long cr0;
 	unsigned long cr4_mask = 0;

@@ -150,12 +146,29 @@ void fpu__cpu_init(void)
 		cr0 |= X86_CR0_EM;
 	write_cr0(cr0);
 
+	xsave_init();
+}
+
+/*
+ * Called on the boot CPU once per system bootup, to set up the initial FPU state that
+ * is later cloned into all processes.
+ */
+void fpu__init_system(void)
+{
+	/* The FPU has to be operational for some of the later FPU init activities: */
+	fpu__init_cpu();
 
 	mxcsr_feature_mask_init();
 	xsave_init();
 	eager_fpu_init();
 }
 
+void fpu__cpu_init(void)
+{
+	fpu__init_cpu();
+	fpu__init_system();
+}
+
 static int __init no_387(char *s)
 {
 	setup_clear_cpu_cap(X86_FEATURE_FPU);

2.1.0

Rename existing xstate init functions along the system/cpu init principles:

fpu__init_system_xstate(): called once per system bootup
fpu__init_cpu_xstate(): called per CPU onlining

Also make the fpu__init_cpu_xstate() early code invariant:
if xfeatures_mask is not set yet then don't crash but return.

---
 arch/x86/include/asm/fpu/internal.h |  3 +++
 arch/x86/kernel/fpu/xsave.c         | 22 ++++++++++++++--------
 2 files changed, 17 insertions(+), 8 deletions(-)

index 19b7cdf73efd..71d44be5acb1 100644
@@ -39,6 +39,9 @@ extern unsigned int mxcsr_feature_mask;
 extern void fpu__cpu_init(void);
 extern void eager_fpu_init(void);
 
+extern void fpu__init_system_xstate(void);
+extern void fpu__init_cpu_xstate(void);
+
 DECLARE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx);
 
 extern void convert_from_fxsr(struct user_i387_ia32_struct *env,

index 45130ba6f328..961c25850c7f 100644
@@ -460,10 +460,14 @@ static void prepare_fx_sw_frame(void)
 }
 
 /*
- * Enable the extended processor state save/restore feature
+ * Enable the extended processor state save/restore feature.
+ * Called once per CPU onlining.
  */
-static inline void xstate_enable(void)
+void fpu__init_cpu_xstate(void)
 {
+	if (!xfeatures_mask)
+		return;
+
 	cr4_set_bits(X86_CR4_OSXSAVE);
 	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
 }

@@ -640,11 +644,12 @@ static void __init init_xstate_size(void)
 
 /*
  * Enable and initialize the xsave feature.
+ * Called once per system bootup.
  *
  * ( Not marked __init because of false positive section warnings
  *   generated by xsave_init(). )
  */
-static void /* __init */ xstate_enable_boot_cpu(void)
+void fpu__init_system_xstate(void)
 {
 	unsigned int eax, ebx, ecx, edx;
 

@@ -666,7 +671,8 @@ static void /* __init */ xstate_enable_boot_cpu(void)
 	 */
 	xfeatures_mask = xfeatures_mask & XCNTXT_MASK;
 
-	xstate_enable();
+	/* Enable xstate instructions to be able to continue with initialization: */
+	fpu__init_cpu_xstate();
 
 	/*
 	 * Recompute the context size for enabled features

@@ -698,8 +704,8 @@ static void /* __init */ xstate_enable_boot_cpu(void)
 }
 
 /*
- * For the very first instance, this calls xstate_enable_boot_cpu();
- * for all subsequent instances, this calls xstate_enable().
+ * For the very first instance, this calls fpu__init_system_xstate();
+ * for all subsequent instances, this calls fpu__init_cpu_xstate().
  */
 void xsave_init(void)
 {

@@ -715,9 +721,9 @@ void xsave_init(void)
 
 	if (on_boot_cpu) {
 		on_boot_cpu = 0;
-		xstate_enable_boot_cpu();
+		fpu__init_system_xstate();
 	} else {
-		xstate_enable();
+		fpu__init_cpu_xstate();
 	}
 }
 

2.1.0

fpu__init_system_xstate() does an FPU capability check that is better done in fpu__init_cpu_xstate(). This will allow us to call fpu__init_cpu_xstate() directly on legacy CPUs as well.

---
 arch/x86/kernel/fpu/xsave.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 961c25850c7f..0610f431f77f 100644
@@ -465,7 +465,7 @@ static void prepare_fx_sw_frame(void)
  */
 void fpu__init_cpu_xstate(void)
 {
-	if (!xfeatures_mask)
+	if (!cpu_has_xsave || !xfeatures_mask)
 		return;
 
 	cr4_set_bits(X86_CR4_OSXSAVE);

2.1.0

Now that legacy code can execute fpu__init_cpu_xstate() in xsave_init(), we can move the once per boot legacy check into fpu__init_system_xstate(), where it belongs.

---
 arch/x86/kernel/fpu/xsave.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

index 0610f431f77f..fd656cbdd315 100644
@@ -653,6 +653,11 @@ void fpu__init_system_xstate(void)
 {
 	unsigned int eax, ebx, ecx, edx;
 
+	if (!cpu_has_xsave) {
+		pr_info("x86/fpu: Legacy x87 FPU detected.\n");
+		return;
+	}
+
 	if (boot_cpu_data.cpuid_level < XSTATE_CPUID) {
 		WARN(1, "x86/fpu: XSTATE_CPUID missing!\n");
 		return;

@@ -711,14 +716,6 @@ void xsave_init(void)
 {
 	static char on_boot_cpu = 1;
 
-	if (!cpu_has_xsave) {
-		if (on_boot_cpu) {
-			on_boot_cpu = 0;
-			pr_info("x86/fpu: Legacy x87 FPU detected.\n");
-		}
-		return;
-	}
-
 	if (on_boot_cpu) {
 		on_boot_cpu = 0;
 		fpu__init_system_xstate();

2.1.0

Linearize the call sequence in xsave_init():

	fpu__init_system_xstate();
	fpu__init_cpu_xstate();

We do this by propagating the boot-once quirk into fpu__init_system_xstate(). fpu__init_cpu_xstate() is safe to be called multiple time.

---
 arch/x86/kernel/fpu/xsave.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

index fd656cbdd315..9d5ff90916b1 100644
@@ -652,6 +652,11 @@ static void __init init_xstate_size(void)
 void fpu__init_system_xstate(void)
 {
 	unsigned int eax, ebx, ecx, edx;
+	static bool on_boot_cpu = 1;
+
+	if (!on_boot_cpu)
+		return;
+	on_boot_cpu = 0;
 
 	if (!cpu_has_xsave) {
 		pr_info("x86/fpu: Legacy x87 FPU detected.\n");

@@ -714,14 +719,8 @@ void fpu__init_system_xstate(void)
  */
 void xsave_init(void)
 {
-	static char on_boot_cpu = 1;
-
-	if (on_boot_cpu) {
-		on_boot_cpu = 0;
-		fpu__init_system_xstate();
-	} else {
-		fpu__init_cpu_xstate();
-	}
+	fpu__init_system_xstate();
+	fpu__init_cpu_xstate();
 }
 
 /*

2.1.0

Expand fpu__init_system_xstate() and fpu__init_cpu_xstate() calls into xsave_init() calls.

(This will allow us to call the proper versions in higher level FPU init code later on.)

No change in functionality.

---
 arch/x86/include/asm/fpu/xsave.h |  1 -
 arch/x86/kernel/fpu/init.c       |  8 +++++---
 arch/x86/kernel/fpu/xsave.c      | 13 +------------
 3 files changed, 6 insertions(+), 16 deletions(-)

index 5c3ab4e17aea..a10e66582c1b 100644
@@ -52,7 +52,6 @@ extern u64 xfeatures_mask;
 extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
 extern struct xsave_struct init_xstate_ctx;
 
-extern void xsave_init(void);
 extern void update_regset_xstate_info(unsigned int size, u64 xstate_mask);
 
 /* These macros all use (%edi)/(%rdi) as the single memory argument. */

index d6234adc8ba0..77599fe8af56 100644
@@ -98,7 +98,7 @@ static void fpstate_xstate_init_size(void)
 {
 	/*
 	 * Note that xstate_size might be overwriten later during
-	 * xsave_init().
+	 * fpu__init_system_xstate().
 	 */
 
 	if (!cpu_has_fpu) {

@@ -146,7 +146,8 @@ void fpu__init_cpu(void)
 		cr0 |= X86_CR0_EM;
 	write_cr0(cr0);
 
-	xsave_init();
+	fpu__init_system_xstate();
+	fpu__init_cpu_xstate();
 }
 
 /*

@@ -159,7 +160,8 @@ void fpu__init_system(void)
 	fpu__init_cpu();
 
 	mxcsr_feature_mask_init();
-	xsave_init();
+	fpu__init_system_xstate();
+	fpu__init_cpu_xstate();
 	eager_fpu_init();
 }
 

index 9d5ff90916b1..fa9b954eb23a 100644
@@ -646,8 +646,7 @@ static void __init init_xstate_size(void)
  * Enable and initialize the xsave feature.
  * Called once per system bootup.
  *
- * ( Not marked __init because of false positive section warnings
- *   generated by xsave_init(). )
+ * ( Not marked __init because of false positive section warnings. )
  */
 void fpu__init_system_xstate(void)
 {

@@ -714,16 +713,6 @@ void fpu__init_system_xstate(void)
 }
 
 /*
- * For the very first instance, this calls fpu__init_system_xstate();
- * for all subsequent instances, this calls fpu__init_cpu_xstate().
- */
-void xsave_init(void)
-{
-	fpu__init_system_xstate();
-	fpu__init_cpu_xstate();
-}
-
-/*
  * setup_init_fpu_buf() is __init and it is OK to call it here because
  * init_xstate_ctx will be unset only once during boot.
  */

2.1.0

Only call xstate system setup routines from fpu__init_system().

Likewise, don't call fpu__init_cpu_xstate() from fpu__init_system().

---
 arch/x86/kernel/fpu/init.c | 2 --
 1 file changed, 2 deletions(-)

index 77599fe8af56..c1b2d1cfe745 100644
@@ -146,7 +146,6 @@ void fpu__init_cpu(void)
 		cr0 |= X86_CR0_EM;
 	write_cr0(cr0);
 
-	fpu__init_system_xstate();
 	fpu__init_cpu_xstate();
 }
 

@@ -161,7 +160,6 @@ void fpu__init_system(void)
 
 	mxcsr_feature_mask_init();
 	fpu__init_system_xstate();
-	fpu__init_cpu_xstate();
 	eager_fpu_init();
 }
 

2.1.0

The legacy FPU init image is used on older CPUs who don't run xstate init. But the init code is called within setup_init_fpu_buf(), an xstate method.

Move this legacy init out of the xstate code and put it into fpu/init.c.

---
 arch/x86/kernel/fpu/init.c  | 6 ++++++
 arch/x86/kernel/fpu/xsave.c | 6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)

index c1b2d1cfe745..30d2d5d03cb0 100644
@@ -158,6 +158,12 @@ void fpu__init_system(void)
 	/* The FPU has to be operational for some of the later FPU init activities: */
 	fpu__init_cpu();
 
+	/*
+	 * Set up the legacy init FPU context. (xstate init might overwrite this
+	 * with a more modern format, if the CPU supports it.)
+	 */
+	fx_finit(&init_xstate_ctx.i387);
+
 	mxcsr_feature_mask_init();
 	fpu__init_system_xstate();
 	eager_fpu_init();

index fa9b954eb23a..6be0a98238f6 100644
@@ -576,12 +576,6 @@ static void setup_init_fpu_buf(void)
 		return;
 	on_boot_cpu = 0;
 
-	/*
-	 * Setup init_xstate_ctx to represent the init state of
-	 * all the features managed by the xsave
-	 */
-	fx_finit(&init_xstate_ctx.i387);
-
 	if (!cpu_has_xsave)
 		return;
 

2.1.0

It's a pure xstate method now, no need for this duplicate call.