x86/xsaves: Fix XSAVES issues
PATCH v6 9/13 x86/xsaves: Fix xstate_offsets, xstate_sizes for non-extended states
In reply to Yu-cheng YuThe arrays xstate_offsets[] and xstate_sizes[] record XSAVE standard- format offsets and sizes. Values for non-extended state components fpu and xmm's were not initialized or used. Ptrace format conversion needs them. Fix it.
--- arch/x86/kernel/fpu/xstate.c | 9 +++++++++ 1 file changed, 9 insertions(+)
arch/x86/kernel/fpu/xstate.c +9 -0
@@ -246,6 +246,15 @@ static void __init setup_xstate_features(void)/* start at the beginnning of the "extended state" */ unsigned int last_good_offset = offsetof(struct xregs_state , extended_state_area); +/* +* The FP xstates and SSE xstates are legacy states. They are always +* in the fixed offsets in the xsave area in either compacted form +* or standard form. +*/ + xstate_offsets[0 ] =0 ; + xstate_sizes[0 ] = offsetof(struct fxregs_state , xmm_space); + xstate_offsets[1 ] = xstate_sizes[0 ]; + xstate_sizes[1] = FIELD_SIZEOF(struct fxregs_state, xmm_space);for (i =FIRST_EXTENDED_XFEATURE ; i <XFEATURE_MAX ; i++) {if (!xfeature_enabled (i))
Signature · 1 line
1.9.1
PATCH v6 8/13 x86/xsaves: Fix XSTATE component offset print out
In reply to Yu-cheng YuComponent offset print out was incorrect for XSAVES. Correct it and move to a separate function.
--- arch/x86/kernel/fpu/xstate.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
arch/x86/kernel/fpu/xstate.c +16 -2
@@ -269,8 +269,6 @@ static void __init setup_xstate_features(void)WARN_ONCE (last_good_offset > xstate_offsets[i],"x86/fpu: misordered xstate at %d\n" , last_good_offset); last_good_offset = xstate_offsets[i]; - -printk (KERN_INFO "x86/fpu: xstate_offset[%d]: %4d, xstate_sizes[%d]: %4d\n" , i, ebx, i, eax); } }
@@ -374,6 +372,21 @@ static void __init setup_xstate_comp(void) }/* +* Print out xstate component offsets and sizes +*/ +static void __init print_xstate_offset_size (void ) +{ +int i; + +for (i =FIRST_EXTENDED_XFEATURE ; i <XFEATURE_MAX ; i++) { +if (!xfeature_enabled (i)) +continue ; +pr_info ("x86/fpu: xstate_offset[%d]: %4d, xstate_sizes[%d]: %4d\n" , + i, xstate_comp_offsets[i], i, xstate_sizes[i]); + } +} + +/* * setup the xstate image representing the init state */ static void __init setup_init_fpu_buf (void )
@@ -708,6 +721,7 @@ void __init fpu__init_system_xstate(void)fpu__init_prepare_fx_sw_frame ();setup_init_fpu_buf ();setup_xstate_comp (); +print_xstate_offset_size (); pr_info("x86/fpu: Enabled xstate features 0x%llx, context size is %d bytes, using '%s' format.\n", xfeatures_mask,
Signature · 1 line
1.9.1
PATCH v6 12/13 x86/xsaves: Fix fpstate_init() for XRSTORS
In reply to Yu-cheng YuIn XSAVES mode if fpstate_init() is used to initialize a task's extended state area, xsave.header.xcomp_bv[63] must be set. Otherwise, when the task is scheduled, a warning is triggered from copy_kernel_to_xregs().
One such test case is: setting an invalid extended state through PTRACE. When xstateregs_set() rejects the syscall and re-initializes the task's extended state area. This triggers the warning mentioned above.
--- arch/x86/include/asm/fpu/types.h | 6 ++++++ arch/x86/kernel/fpu/core.c | 4 ++++ 2 files changed, 10 insertions(+)
arch/x86/include/asm/fpu/types.h +6 -0
@@ -232,6 +232,12 @@ struct xstate_header { } __attribute__((packed));/* +* xstate_header.xcomp_bv[63] indicates that the extended_state_area +* is in compacted format. +*/ +#define XCOMP_BV_COMPACTED_FORMAT ((u64)1 << 63) + +/* * This is our most modern FPU state format, as saved by the XSAVE * and restored by the XRSTOR instructions. *
arch/x86/kernel/fpu/core.c +4 -0
@@ -8,6 +8,7 @@#include <asm/fpu/internal.h> #include <asm/fpu/regset.h> #include <asm/fpu/signal.h> +#include <asm/fpu/types.h> #include <asm/traps.h> #include <linux/hardirq.h>
@@ -224,6 +225,9 @@ void fpstate_init(union fpregs_state *state)memset (state,0 , fpu_kernel_xstate_size); +if (cpu_has_xsaves) + state->xsave.header.xcomp_bv =XCOMP_BV_COMPACTED_FORMAT ; +if (cpu_has_fxsr)fpstate_init_fxstate (&state->fxsave);else
Signature · 1 line
1.9.1
v6 cover x86/xsaves: Fix XSAVES issues
XSAVES is a kernel-mode instruction. It offers a compacted format and memory-write optimization. These patches fix issues in the first implementation.
Changes since Version 5:
Patch 1, 2 - Change names to fpu_user_xstate_size and fpu_kernel_xstate_size;
fix some comments, etc.
Patch 11 - In addition to a warning, make __raw_xsave_addr() return NULL
when a disabled xstate component address is requested.
Patch 12 - Add more details in the comments of why this patch is needed;
make a new macro for xcomp_bv[63].
Yu-cheng Yu (13):
x86/xsaves: Define and use fpu_user_xstate_size x86/xsaves: Rename xstate_size to fpu_kernel_xstate_size to
distinguish from fpu_user_xstate_size
x86/xsaves: Keep init_fpstate.xsave.header.xfeatures as zero for init
optimization
x86/xsaves: Introduce a new check that allows correct xstates copy
from kernel to user directly
x86/xsaves: Align xstate components according to CPUID x86/xsaves: Supervisor state component offset
x86/xsaves: Fix PTRACE frames for XSAVES
x86/xsaves: Fix XSTATE component offset print out x86/xsaves: Fix xstate_offsets, xstate_sizes for non-extended states x86/xsaves: Fix __fpu_restore_sig() for XSAVES
x86/xsaves: When a disabled xstate component offset is requested,
return NULL
x86/xsaves: Fix fpstate_init() for XRSTORS
x86/xsaves: Re-enable XSAVES
arch/x86/include/asm/fpu/types.h | 7 + arch/x86/include/asm/fpu/xstate.h | 10 +- arch/x86/include/asm/processor.h | 3 +- arch/x86/kernel/fpu/core.c | 11 +- arch/x86/kernel/fpu/init.c | 34 ++-- arch/x86/kernel/fpu/regset.c | 56 +++-- arch/x86/kernel/fpu/signal.c | 43 +++- arch/x86/kernel/fpu/xstate.c | 417 ++++++++++++++++++++++++++++++-------- 8 files changed, 440 insertions(+), 141 deletions(-)
Signature · 1 line
1.9.1
PATCH v6 7/13 x86/xsaves: Fix PTRACE frames for XSAVES
XSAVES uses compacted format and is a kernel instruction. The kernel should use standard-format, non-supervisor state data for PTRACE.
--- arch/x86/include/asm/fpu/xstate.h | 5 +- arch/x86/kernel/fpu/regset.c | 56 +++++++++---- arch/x86/kernel/fpu/xstate.c | 167 +++++++++++++++++++++++++++++++++++++- 3 files changed, 212 insertions(+), 16 deletions(-)
arch/x86/include/asm/fpu/xstate.h +4 -1
@@ -51,5 +51,8 @@ void fpu__xstate_clear_all_cpu_caps(void);void *get_xsave_addr (struct xregs_state *xsave,int xstate);const void *get_xsave_field_ptr (int xstate_field);int using_compacted_format (void ); - +int copyout_from_xsaves (unsigned int pos,unsigned int count,void *kbuf, +void __user *ubuf,struct xregs_state *xsave); +int copyin_to_xsaves (const void *kbuf,const void __user *ubuf, +struct xregs_state *xsave);#endif
arch/x86/kernel/fpu/regset.c +42 -14
80 lines not on this page · raw message
arch/x86/kernel/fpu/xstate.c +166 -1
188 lines not on this page · raw message
Signature · 1 line
1.9.1
PATCH v6 13/13 x86/xsaves: Re-enable XSAVES
In reply to Yu-cheng YuWe did not handle XSAVES instructions correctly. There were issues in converting between standard and compacted format when interfacing with user-space. These issues have been corrected.
Add a WARN_ONCE() to make it clear that XSAVES supervisor states are not yet implemented.
--- arch/x86/kernel/fpu/init.c | 15 --------------- arch/x86/kernel/fpu/xstate.c | 10 ++++++++++ 2 files changed, 10 insertions(+), 15 deletions(-)
arch/x86/kernel/fpu/init.c +0 -15
@@ -230,21 +230,6 @@ static void __init fpu__init_system_xstate_size_legacy(void) } fpu_user_xstate_size = fpu_kernel_xstate_size; - -/* -* Quirk: we don't yet handle the XSAVES* instructions -* correctly, as we don't correctly convert between -* standard and compacted format when interfacing -* with user-space - so disable it for now. -* -* The difference is small: with recent CPUs the -* compacted format is only marginally smaller than -* the standard FPU state format. -* -* ( This is easy to backport while we are fixing -* XSAVES* support. ) -*/ -setup_clear_cpu_cap (X86_FEATURE_XSAVES ); }/*
arch/x86/kernel/fpu/xstate.c +10 -0
@@ -222,6 +222,16 @@ void fpu__init_cpu_xstate(void)if (!cpu_has_xsave || !xfeatures_mask)return ; +/* +* Make it clear that XSAVES supervisor states are not yet +* implemented should anyone expect it to work by changing +* bits in XFEATURE_MASK_* macros and XCR0. +*/ +WARN_ONCE ((xfeatures_mask &XFEATURE_MASK_SUPERVISOR ), +"x86/fpu: XSAVES supervisor states are not yet implemented.\n" ); + + xfeatures_mask &= ~XFEATURE_MASK_SUPERVISOR ; +cr4_set_bits (X86_CR4_OSXSAVE );xsetbv (XCR_XFEATURE_ENABLED_MASK , xfeatures_mask); }
Signature · 1 line
1.9.1
PATCH v6 6/13 x86/xsaves: Supervisor state component offset
In reply to Yu-cheng YuCPUID function 0x0d, sub function (i, i > 1) returns in ebx the offset of xstate component i. Zero is returned for a supervisor state. A supervisor state can only be saved by XSAVES and XSAVES uses a compacted format. There is no fixed offset for a supervisor state. This patch checks and makes sure a supervisor state offset is not recorded or mis-used. This has no effect in practice as we currently use no supervisor states, but it would be good to fix.
--- arch/x86/include/asm/fpu/types.h | 1 + arch/x86/include/asm/fpu/xstate.h | 3 ++ arch/x86/kernel/fpu/xstate.c | 62 ++++++++++++++++++++++++--------------- 3 files changed, 43 insertions(+), 23 deletions(-)
arch/x86/include/asm/fpu/types.h +1 -0
@@ -122,6 +122,7 @@ enum xfeature {#define XFEATURE_MASK_OPMASK (1 << XFEATURE_OPMASK)#define XFEATURE_MASK_ZMM_Hi256 (1 << XFEATURE_ZMM_Hi256)#define XFEATURE_MASK_Hi16_ZMM (1 << XFEATURE_Hi16_ZMM) +#define XFEATURE_MASK_PT (1 << XFEATURE_PT_UNIMPLEMENTED_SO_FAR)#define XFEATURE_MASK_PKRU (1 << XFEATURE_PKRU)#define XFEATURE_MASK_FPSSE (XFEATURE_MASK_FP | XFEATURE_MASK_SSE)
arch/x86/include/asm/fpu/xstate.h +3 -0
@@ -18,6 +18,9 @@#define XSAVE_YMM_SIZE 256#define XSAVE_YMM_OFFSET (XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET) +/* Supervisor features */ +#define XFEATURE_MASK_SUPERVISOR (XFEATURE_MASK_PT) +/* Supported features which support lazy state saving */ #define XFEATURE_MASK_LAZY (XFEATURE_MASK_FP | \ XFEATURE_MASK_SSE | \
arch/x86/kernel/fpu/xstate.c +39 -23
84 lines not on this page · raw message
Signature · 1 line
1.9.1
PATCH v6 5/13 x86/xsaves: Align xstate components according to CPUID
In reply to Yu-cheng YuCPUID function 0x0d, sub function (i, i > 1) returns in ecx[1] the alignment requirement of component i when the compacted format is used.
If ecx[1] is 0, component i is located immediately following the preceding component. If ecx[1] is 1, component i is located on the next 64-byte boundary following the preceding component.
--- arch/x86/kernel/fpu/xstate.c | 60 +++++++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 28 deletions(-)
arch/x86/kernel/fpu/xstate.c +32 -28
@@ -270,6 +270,33 @@ static void __init print_xstate_features(void) }/* +* This check is important because it is easy to get XSTATE_* +* confused with XSTATE_BIT_*. +*/ +#define CHECK_XFEATURE (nr) do { \ + WARN_ON(nr < FIRST_EXTENDED_XFEATURE); \ + WARN_ON(nr >= XFEATURE_MAX); \ +} while (0) + +/* +* We could cache this like xstate_size[], but we only use +* it here, so it would be a waste of space. +*/ +static int xfeature_is_aligned (int xfeature_nr) +{ +u32 eax, ebx, ecx, edx; + +CHECK_XFEATURE (xfeature_nr); +cpuid_count (XSTATE_CPUID , xfeature_nr, &eax, &ebx, &ecx, &edx); +/* +* The value returned by ECX[1] indicates the alignment +* of state component i when the compacted format +* of the extended region of an XSAVE area is used +*/ +return !!(ecx &2 ); +} + +/* * This function sets up offsets and sizes of all extended states in * xsave area. This supports both standard format and compacted format * of the xsave aread.
@@ -306,10 +333,14 @@ static void __init setup_xstate_comp(void)else xstate_comp_sizes[i] =0 ; -if (i >FIRST_EXTENDED_XFEATURE ) +if (i >FIRST_EXTENDED_XFEATURE ) { xstate_comp_offsets[i] = xstate_comp_offsets[i-1 ] + xstate_comp_sizes[i-1 ]; +if (xfeature_is_aligned (i)) + xstate_comp_offsets[i] = +ALIGN (xstate_comp_offsets[i],64 ); + } } }
@@ -366,33 +397,6 @@ static int xfeature_is_user(int xfeature_nr)} */ -/* -* This check is important because it is easy to get XSTATE_* -* confused with XSTATE_BIT_*. -*/ -#define CHECK_XFEATURE (nr) do { \ - WARN_ON(nr < FIRST_EXTENDED_XFEATURE); \ - WARN_ON(nr >= XFEATURE_MAX); \ -} while (0) - -/* -* We could cache this like xstate_size[], but we only use -* it here, so it would be a waste of space. -*/ -static int xfeature_is_aligned (int xfeature_nr) -{ -u32 eax, ebx, ecx, edx; - -CHECK_XFEATURE (xfeature_nr); -cpuid_count (XSTATE_CPUID , xfeature_nr, &eax, &ebx, &ecx, &edx); -/* -* The value returned by ECX[1] indicates the alignment -* of state component i when the compacted format -* of the extended region of an XSAVE area is used -*/ -return !!(ecx &2 ); -} -static int xfeature_uncompacted_offset (int xfeature_nr) {u32 eax, ebx, ecx, edx;
Signature · 1 line
1.9.1
PATCH v6 4/13 x86/xsaves: Introduce a new check that allows correct xstates copy from kernel to user directly
In reply to Yu-cheng YuXSAVES is a kernel instruction and uses a compacted format. When working with user space, the kernel should provide standard-format, non-supervisor state data. We cannot do __copy_to_user() from a compacted- format kernel xstate area to a signal frame.
Dave Hansen proposes this method to simplify copy xstate directly to user.
Signed-off by: Yu-cheng Yu <[email protected]>
--- arch/x86/include/asm/fpu/xstate.h | 1 + arch/x86/kernel/fpu/signal.c | 3 ++- arch/x86/kernel/fpu/xstate.c | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-)
arch/x86/include/asm/fpu/xstate.h +1 -0
@@ -47,5 +47,6 @@ extern void update_regset_xstate_info(unsigned int size, u64 xstate_mask);void fpu__xstate_clear_all_cpu_caps (void );void *get_xsave_addr (struct xregs_state *xsave,int xstate);const void *get_xsave_field_ptr (int xstate_field); +int using_compacted_format (void );#endif
arch/x86/kernel/fpu/signal.c +2 -1
@@ -8,6 +8,7 @@#include <asm/fpu/internal.h> #include <asm/fpu/signal.h> #include <asm/fpu/regset.h> +#include <asm/fpu/xstate.h> #include <asm/sigframe.h>
@@ -168,7 +169,7 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)sizeof (struct user_i387_ia32_struct ),NULL , (struct _fpstate_32 __user *) buf) ? -1 : 1; -if (fpregs_active ()) { +if (fpregs_active () ||using_compacted_format ()) {/* Save the live register state to the user directly. */ if (copy_fpregs_to_sigframe (buf_fx))return -1 ;
arch/x86/kernel/fpu/xstate.c +1 -1
@@ -420,7 +420,7 @@ static int xfeature_size(int xfeature_nr)* that it is obvious which aspect of 'XSAVES' is being handled * by the calling code. */ -static int using_compacted_format (void ) +int using_compacted_format (void ) {return cpu_has_xsaves; }
Signature · 1 line
1.9.1
PATCH v6 3/13 x86/xsaves: Keep init_fpstate.xsave.header.xfeatures as zero for init optimization
In reply to Yu-cheng YuKeep init_fpstate.xsave.header.xfeatures as zero for init optimization. This is important for init optimization that is implemented in processor. If a bit corresponding to an xstate in xstate_bv is 0, it means the xstate is in init status and will not be read from memory to the processor during XRSTOR/XRSTORS instruction. This largely impacts context switch performance.
--- arch/x86/kernel/fpu/xstate.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
arch/x86/kernel/fpu/xstate.c +2 -4
@@ -329,13 +329,11 @@ static void __init setup_init_fpu_buf(void)setup_xstate_features ();print_xstate_features (); -if (cpu_has_xsaves) { +if (cpu_has_xsaves) init_fpstate.xsave.header.xcomp_bv = (u64 )1 <<63 | xfeatures_mask; - init_fpstate.xsave.header.xfeatures = xfeatures_mask; - }/* -* Init all the features state with header_bv being 0x0 +* Init all the features state with header.xfeatures being 0x0 */ copy_kernel_to_xregs_booting (&init_fpstate.xsave);
Signature · 1 line
1.9.1
PATCH v6 2/13 x86/xsaves: Rename xstate_size to fpu_kernel_xstate_size to distinguish from fpu_user_xstate_size
In reply to Yu-cheng YuUser space uses standard format xsave area. fpstate in signal frame should have standard format size.
To explicitly distinguish between xstate size in kernel space and the one in user space, we rename xstate_size to fpu_kernel_xstate_size. This patch is not fixing a bug. It just makes kernel code more clear.
--- arch/x86/include/asm/processor.h | 2 +- arch/x86/kernel/fpu/core.c | 7 ++++--- arch/x86/kernel/fpu/init.c | 20 +++++++++++--------- arch/x86/kernel/fpu/signal.c | 2 +- arch/x86/kernel/fpu/xstate.c | 8 ++++---- 5 files changed, 21 insertions(+), 18 deletions(-)
arch/x86/include/asm/processor.h +1 -1
@@ -367,7 +367,7 @@ DECLARE_PER_CPU(struct irq_stack *, hardirq_stack); DECLARE_PER_CPU(struct irq_stack *, softirq_stack);#endif /* X86_64 */ -extern unsigned int xstate_size; +extern unsigned int fpu_kernel_xstate_size;extern unsigned int fpu_user_xstate_size;struct perf_event ;
arch/x86/kernel/fpu/core.c +4 -3
@@ -222,7 +222,7 @@ void fpstate_init(union fpregs_state *state)return ; } -memset (state,0 , xstate_size); +memset (state,0 , fpu_kernel_xstate_size);if (cpu_has_fxsr)fpstate_init_fxstate (&state->fxsave);
@@ -247,7 +247,7 @@ int fpu__copy(struct fpu *dst_fpu, struct fpu *src_fpu)* leak into the child task: */ if (use_eager_fpu ()) -memset (&dst_fpu->state.xsave,0 , xstate_size); +memset (&dst_fpu->state.xsave,0 , fpu_kernel_xstate_size);/* * Save current FPU registers directly into the child
@@ -266,7 +266,8 @@ int fpu__copy(struct fpu *dst_fpu, struct fpu *src_fpu)*/ preempt_disable ();if (!copy_fpregs_to_fpstate (dst_fpu)) { -memcpy (&src_fpu->state, &dst_fpu->state, xstate_size); +memcpy (&src_fpu->state, &dst_fpu->state, + fpu_kernel_xstate_size);if (use_eager_fpu ())copy_kernel_to_fpregs (&src_fpu->state);
arch/x86/kernel/fpu/init.c +11 -9
@@ -145,8 +145,8 @@ static void __init fpu__init_system_generic(void)* This is inherent to the XSAVE architecture which puts all state * components into a single, continuous memory block: */ -unsigned int xstate_size; -EXPORT_SYMBOL_GPL (xstate_size); +unsigned int fpu_kernel_xstate_size; +EXPORT_SYMBOL_GPL (fpu_kernel_xstate_size);/* Get alignment of the TYPE. */ #define TYPE_ALIGN (TYPE ) offsetof(struct { char x; TYPE test; }, test)
@@ -178,7 +178,7 @@ static void __init fpu__init_task_struct_size(void)* Add back the dynamically-calculated register state * size. */ - task_size += xstate_size; + task_size += fpu_kernel_xstate_size;/* * We dynamically size 'struct fpu', so we require that
@@ -195,7 +195,7 @@ static void __init fpu__init_task_struct_size(void) }/* -* Set up the user and kernel xstate_size based on the legacy FPU context size. +* Set up the user and kernel xstate sizes based on the legacy FPU context size. * * We set this up first, and later it will be overwritten by * fpu__init_system_xstate() if the CPU knows about xstates.
@@ -208,7 +208,7 @@ static void __init fpu__init_system_xstate_size_legacy(void) on_boot_cpu =0 ;/* -* Note that xstate_size might be overwriten later during +* Note that xstate sizes might be overwritten later during * fpu__init_system_xstate(). */
@@ -219,15 +219,17 @@ static void __init fpu__init_system_xstate_size_legacy(void)*/ setup_clear_cpu_cap (X86_FEATURE_XSAVE );setup_clear_cpu_cap (X86_FEATURE_XSAVEOPT ); - xstate_size =sizeof (struct swregs_state ); + fpu_kernel_xstate_size =sizeof (struct swregs_state ); }else {if (cpu_has_fxsr) - xstate_size =sizeof (struct fxregs_state ); + fpu_kernel_xstate_size = +sizeof (struct fxregs_state );else - xstate_size =sizeof (struct fregs_state ); + fpu_kernel_xstate_size = +sizeof (struct fregs_state ); } - fpu_user_xstate_size = xstate_size; + fpu_user_xstate_size = fpu_kernel_xstate_size;/* * Quirk: we don't yet handle the XSAVES* instructions
arch/x86/kernel/fpu/signal.c +1 -1
@@ -262,7 +262,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)int ia32_fxstate = (buf != buf_fx);struct task_struct *tsk = current;struct fpu *fpu = &tsk->thread.fpu; -int state_size = xstate_size; +int state_size = fpu_kernel_xstate_size;u64 xfeatures =0 ;int fx_only =0 ;
arch/x86/kernel/fpu/xstate.c +4 -4
@@ -537,7 +537,7 @@ static void do_extra_xstate_size_checks(void)*/ paranoid_xstate_size +=xfeature_size (i); } -XSTATE_WARN_ON (paranoid_xstate_size != xstate_size); +XSTATE_WARN_ON (paranoid_xstate_size != fpu_kernel_xstate_size); }
@@ -616,7 +616,7 @@ static int init_xstate_size(void)* The size is OK, we are definitely going to use xsave, * make it known to the world that we need more space. */ - xstate_size = possible_xstate_size; + fpu_kernel_xstate_size = possible_xstate_size;do_extra_xstate_size_checks ();/*
@@ -679,14 +679,14 @@ void __init fpu__init_system_xstate(void)return ; } -update_regset_xstate_info (xstate_size, xfeatures_mask); +update_regset_xstate_info (fpu_kernel_xstate_size, xfeatures_mask);fpu__init_prepare_fx_sw_frame ();setup_init_fpu_buf ();setup_xstate_comp ();pr_info ("x86/fpu: Enabled xstate features 0x%llx, context size is %d bytes, using '%s' format.\n" , xfeatures_mask, - xstate_size, + fpu_kernel_xstate_size, cpu_has_xsaves ?"compacted" :"standard" ); }
Signature · 1 line
1.9.1
PATCH v6 1/13 x86/xsaves: Define and use fpu_user_xstate_size
In reply to Yu-cheng YuThe XSAVE area of kernel can be in standard or compacted format; it is always in standard format for user mode. When XSAVES is enabled, the kernel uses the compacted format and it is necessary to use a separate fpu_user_xstate_size for signal/ptrace frames.
Based on an earlier patch from Fenghua Yu <[email protected]>
--- arch/x86/include/asm/fpu/xstate.h | 1 - arch/x86/include/asm/processor.h | 1 + arch/x86/kernel/fpu/init.c | 5 ++- arch/x86/kernel/fpu/signal.c | 27 ++++++++++---- arch/x86/kernel/fpu/xstate.c | 76 ++++++++++++++++++++++++--------------- 5 files changed, 73 insertions(+), 37 deletions(-)
arch/x86/include/asm/fpu/xstate.h +0 -1
@@ -39,7 +39,6 @@#define REX_PREFIX #endif -extern unsigned int xstate_size;extern u64 xfeatures_mask;extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS ];
arch/x86/include/asm/processor.h +1 -0
@@ -368,6 +368,7 @@ DECLARE_PER_CPU(struct irq_stack *, softirq_stack);#endif /* X86_64 */ extern unsigned int xstate_size; +extern unsigned int fpu_user_xstate_size;struct perf_event ;
arch/x86/kernel/fpu/init.c +4 -1
@@ -195,7 +195,7 @@ static void __init fpu__init_task_struct_size(void) }/* -* Set up the xstate_size based on the legacy FPU context size. +* Set up the user and kernel xstate_size based on the legacy FPU context size. * * We set this up first, and later it will be overwritten by * fpu__init_system_xstate() if the CPU knows about xstates.
@@ -226,6 +226,9 @@ static void __init fpu__init_system_xstate_size_legacy(void)else xstate_size =sizeof (struct fregs_state ); } + + fpu_user_xstate_size = xstate_size; +/* * Quirk: we don't yet handle the XSAVES* instructions * correctly, as we don't correctly convert between
arch/x86/kernel/fpu/signal.c +20 -7
@@ -31,7 +31,7 @@ static inline int check_for_xstate(struct fxregs_state __user *buf,/* Check for the first magic field and other error scenarios. */ if (fx_sw->magic1 !=FP_XSTATE_MAGIC1 || fx_sw->xstate_size < min_xstate_size || - fx_sw->xstate_size > xstate_size || + fx_sw->xstate_size > fpu_user_xstate_size || fx_sw->xstate_size > fx_sw->extended_size)return -1 ;
@@ -88,7 +88,8 @@ static inline int save_xstate_epilog(void __user *buf, int ia32_frame)if (!use_xsave ())return err; - err |=__put_user (FP_XSTATE_MAGIC2 , (__u32 *)(buf + xstate_size)); + err |=__put_user (FP_XSTATE_MAGIC2 , + (__u32 *)(buf + fpu_user_xstate_size));/* * Read the xfeatures which we copied (directly from the cpu or
@@ -125,7 +126,7 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)else err =copy_fregs_to_user ((struct fregs_state __user *) buf); -if (unlikely (err) &&__clear_user (buf, xstate_size)) +if (unlikely (err) &&__clear_user (buf, fpu_user_xstate_size)) err = -EFAULT ;return err; }
@@ -175,8 +176,19 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)if (ia32_fxstate)copy_fxregs_to_kernel (&tsk->thread.fpu); }else { +/* +* It is a *bug* if kernel uses compacted-format for xsave +* area and we copy it out directly to a signal frame. It +* should have been handled above by saving the registers +* directly. +*/ +if (boot_cpu_has (X86_FEATURE_XSAVES )) { +WARN_ONCE (1 ,"x86/fpu: saving compacted-format xsave area to a signal frame!\n" ); +return -1 ; + } +fpstate_sanitize_xstate (&tsk->thread.fpu); -if (__copy_to_user (buf_fx, xsave, xstate_size)) +if (__copy_to_user (buf_fx, xsave, fpu_user_xstate_size))return -1 ; }
@@ -341,7 +353,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)static inline int xstate_sigframe_size (void ) { -return use_xsave () ? xstate_size +FP_XSTATE_MAGIC2_SIZE : xstate_size; +return use_xsave () ? fpu_user_xstate_size +FP_XSTATE_MAGIC2_SIZE : + fpu_user_xstate_size; }/*
@@ -385,12 +398,12 @@ fpu__alloc_mathframe(unsigned long sp, int ia32_frame,*/ void fpu__init_prepare_fx_sw_frame (void ) { -int size = xstate_size +FP_XSTATE_MAGIC2_SIZE ; +int size = fpu_user_xstate_size +FP_XSTATE_MAGIC2_SIZE ; fx_sw_reserved.magic1 =FP_XSTATE_MAGIC1 ; fx_sw_reserved.extended_size = size; fx_sw_reserved.xfeatures = xfeatures_mask; - fx_sw_reserved.xstate_size = xstate_size; + fx_sw_reserved.xstate_size = fpu_user_xstate_size;if (config_enabled (CONFIG_IA32_EMULATION ) ||config_enabled (CONFIG_X86_32 )) {
arch/x86/kernel/fpu/xstate.c +48 -28
116 lines not on this page · raw message
Signature · 1 line
1.9.1
PATCH v6 11/13 x86/xsaves: When a disabled xstate component offset is requested, return NULL
In reply to Yu-cheng YuIt is an error to request a disabled XSAVE/XSAVES component address. For that case, make __raw_xsave_addr() return a NULL and issue a warning.
--- arch/x86/kernel/fpu/xstate.c | 5 +++++ 1 file changed, 5 insertions(+)
arch/x86/kernel/fpu/xstate.c +5 -0
@@ -761,6 +761,11 @@ void *__raw_xsave_addr(struct xregs_state *xsave, int xstate_feature_mask) {int feature_nr =fls64 (xstate_feature_mask) -1 ; +if (!xfeature_enabled (feature_nr)) { +WARN_ON_FPU (1 ); +return NULL ; + } +return (void *)xsave + xstate_comp_offsets[feature_nr]; }
Signature · 1 line
1.9.1
PATCH v6 10/13 x86/xsaves: Fix __fpu_restore_sig() for XSAVES
In reply to Yu-cheng YuWhen the kernel is using XSAVES compacted format, we cannot do __copy_from_user() from a signal frame, which has standard-format data. Fix it by using copyin_to_xsaves(), which converts between formats and filters out all supervisor states that we do not allow userspace to write.
--- arch/x86/kernel/fpu/signal.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
arch/x86/kernel/fpu/signal.c +9 -2
@@ -321,8 +321,15 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)*/ fpu__drop (fpu); -if (__copy_from_user (&fpu->state.xsave, buf_fx, state_size) || -__copy_from_user (&env, buf,sizeof (env))) { +if (using_compacted_format ()) { + err =copyin_to_xsaves (NULL , buf_fx, + &fpu->state.xsave); + }else { + err =__copy_from_user (&fpu->state.xsave, + buf_fx, state_size); + } + +if (err ||__copy_from_user (&env, buf,sizeof (env))) {fpstate_init (&fpu->state); err =-1 ; }else {
Signature · 1 line
1.9.1
Quoted 5 lines · Yu-cheng Yu
XSAVES is a kernel-mode instruction. It offers a compacted format and memory-write optimization. These patches fix issues in the first implementation.
Changes since Version 5:
Please, for the future, do send your patchset roughly only once a week - you just sent v5 a day or two ago without giving proper time to people (me?) to review them all. Consult Documentation/SubmittingPatches if there are doubts.
Thanks.
Signature · 5 lines
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- Re: [PATCH v6 07/13] x86/xsaves: Fix PTRACE frames for XSAVES
In reply to Yu-cheng YuQuoted 4 lines · Yu-cheng Yu
XSAVES uses compacted format and is a kernel instruction. The kernel should use standard-format, non-supervisor state data for PTRACE.
Signed-off-by Yu-cheng Yu <[email protected]>
This looks fine. There are a few comments that could use some sprucing up, like:
+ /*
+ * Reject if the user sets any forbidden features.
+ */
But it's minor stuff.
Re: [PATCH v6 07/13] x86/xsaves: Fix PTRACE frames for XSAVES
Quoted 13 lines · Dave Hansen
Quoted 4 lines · Yu-cheng Yu
XSAVES uses compacted format and is a kernel instruction. The kernel should use standard-format, non-supervisor state data for PTRACE.
Signed-off-by Yu-cheng Yu <[email protected]>This looks fine. There are a few comments that could use some sprucing up, like:
arch/x86/kernel/fpu/xstate.c +3
-0+/* +* Reject if the user sets any forbidden features. +*/
Quoted 3 lines
But it's minor stuff.
Reviewed-by Dave Hansen <[email protected]>
Well it's too late for v4.7 due to intrusiveness, so please refine the series and also incorporate the cleanups requested by Boris. I can put it into -tip whenever you guys submit it, but it will have to wait for v4.8 most likely.
Thanks,
Ingo
Re: [PATCH v6 01/13] x86/xsaves: Define and use fpu_user_xstate_size
In reply to Yu-cheng YuQuoted 1 line · Yu-cheng Yu
The XSAVE area of kernel can be in standard or compacted format;
"The kernel xstate area... "
and can we call it the xstate area as there are a bunch of XSAVE* insns touching it. The file which deals with it is even called that:arch/x86/kernel/fpu/xstate.c
Quoted 10 lines
it is always in standard format for user mode. When XSAVES is enabled, the kernel uses the compacted format and it is necessary to use a separate
fpu_user_xstate_sizefor signal/ptrace frames.Based on an earlier patch from Fenghua Yu <[email protected]>
Signed-off-by Fenghua Yu <[email protected]>[[email protected]: rebase to current, rename to fpu_user_xstate_size]Signed-off-by Yu-cheng Yu <[email protected]>Reviewed-by Dave Hansen <[email protected]>
Maybe I wasn't as clear as I hoped to be. Let me be more specific:
So you either need to do:
---
From: Fenghua
...
Signed-off-by: Fenghua
Signed-off-by: You
...
---
or
---
Based on an earlier patch from Fenghua Yu <[email protected]>.
---
with the second variant making you the author implicitly because you're the sender.
Makes more sense this way?
Quoted 7 lines
--- arch/x86/include/asm/fpu/xstate.h | 1 - arch/x86/include/asm/processor.h | 1 + arch/x86/kernel/fpu/init.c | 5 ++- arch/x86/kernel/fpu/signal.c | 27 ++++++++++---- arch/x86/kernel/fpu/xstate.c | 76 ++++++++++++++++++++++++--------------- 5 files changed, 73 insertions(+), 37 deletions(-)
...
arch/x86/kernel/fpu/xstate.c static unsigned int xstate_sizes[XFEATURE_MAX] = { [ 0 ... XFEATURE_MAX - 1] = · line 47
Quoted 9 lines
arch/x86/kernel/fpu/xstate.c +1
-0@@ -44,6 +44,13 @@ static unsigned int xstate_sizes[XFEATURE_MAX] = { [ 0 ... XFEATURE_MAX - 1] =static unsigned int xstate_comp_offsets[sizeof (xfeatures_mask)*8 ];/* +* The XSAVE area of kernel can be in standard or compacted format;
"The kernel xstate area... "
arch/x86/kernel/fpu/xstate.c static unsigned int xstate_sizes[XFEATURE_MAX] = { [ 0 ... XFEATURE_MAX - 1] = · line 50
Quoted 3 lines
arch/x86/kernel/fpu/xstate.c +3
-0+* it is always in standard format for user mode. This is the user +* mode standard format size used for signal and ptrace frames. +*/
But yes, that's very nice commenting.
Quoted 6 lines
arch/x86/kernel/fpu/xstate.c +3
-0+unsigned int fpu_user_xstate_size; + +/* * Clear all of the X86_FEATURE_* bits that are unavailable * when the CPU has no XSAVE support. */
...
arch/x86/kernel/fpu/xstate.c static bool is_supported_xstate_size(unsigned int test_xstate_size) · line 606
Quoted 11 lines
arch/x86/kernel/fpu/xstate.c +6
-1@@ -591,7 +598,15 @@ static bool is_supported_xstate_size(unsigned int test_xstate_size)static int init_xstate_size (void ) {/* Recompute the context size for enabled features: */ -unsigned int possible_xstate_size =calculate_xstate_size (); +unsigned int possible_xstate_size; +unsigned int xsave_size; + + xsave_size =get_xsave_size (); + +if (cpu_has_xsaves)
So next time you submit patches against -tip, please merge them with tip/master and see if they still build, at least.
During last review I told you to use boot_cpu_has(X86_FEATURE_XSAVES) because those cpu_has_XXX things are going away and are gone in tip already.
Please be more careful when incorporating review comments - otherwise it is waste of both yours and reviewer's time.
Thanks.
Signature · 5 lines
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- RE: [PATCH v6 01/13] x86/xsaves: Define and use fpu_user_xstate_size
Quoted 53 lines
From: Borislav Petkov [mailto:[email protected]]
Sent: Wednesday, May 11, 2016 10:21 AM
To: Yu, Yu-cheng <[email protected]>
Cc: [email protected]; [email protected]; H. Peter Anvin <[email protected]>; Thomas Gleixner <[email protected]>; Ingo Molnar <[email protected]>; Dave Hansen <[email protected]>; Andy Lutomirski <[email protected]>; Prakhya, Sai Praneeth <[email protected]>; Shankar, Ravi V <[email protected]>; Yu, Fenghua <[email protected]>
Subject: Re: [PATCH v6 01/13] x86/xsaves: Define and usefpu_user_xstate_sizeQuoted 1 line · Yu-cheng Yu
The XSAVE area of kernel can be in standard or compacted format;
"The kernel xstate area... "
and can we call it the xstate area as there are a bunch of XSAVE* insns touching it. The file which deals with it is even called that:
arch/x86/kernel/fpu/xstate.cQuoted 11 lines
it is always in standard format for user mode. When XSAVES is enabled, the kernel uses the compacted format and it is necessary to use a separate
fpu_user_xstate_sizefor signal/ptrace frames.Based on an earlier patch from Fenghua Yu <[email protected]>
Signed-off-by Fenghua Yu <[email protected]>[[email protected]: rebase to current, rename to
fpu_user_xstate_size]Signed-off-by Yu-cheng Yu <[email protected]>Reviewed-by Dave Hansen <[email protected]>Maybe I wasn't as clear as I hoped to be. Let me be more specific:
So you either need to do:
---
From: Fenghua...
Signed-off-by: Fenghua
Signed-off-by: You
...
---or
---
Based on an earlier patch from Fenghua Yu <[email protected]>.
Quoted 8 lines
Signed-off-by You---
with the second variant making you the author implicitly because you're the sender.
Makes more sense this way?
Is this possible to have the third one?
From: Yu-cheng
Thanks.
-Fenghua
Re: [PATCH v6 01/13] x86/xsaves: Define and use fpu_user_xstate_size
Quoted 6 lines · Fenghua
Is this possible to have the third one?
From: Yu-cheng
Signed-off-by Yu-chengSigned-off-by Fenghua
This SOB chain says that Yu-cheng is the author and then you've received it from him, have done something to it (which we sometimes put in [] to denote it) and then you've sent it out.
The tip maintainer which applies it will add his SOB too, at the end.
Let me ask you this: what exactly do you want to express with the SOB chain?
Just a reminder: the SOB chain should give a clear and precise representation of the patch path from author to upstream.
Section
11) Sign your work
------------------
in Documentation/SubmittingPatches explains the whole story.
Signature · 5 lines
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- Quoted 4 lines · Borislav Petkov
Please, for the future, do send your patchset roughly only once a week - you just sent v5 a day or two ago without giving proper time to people (me?) to review them all. Consult
Documentation/SubmittingPatchesif there are doubts.
There are no actual functional changes. I wanted to respond to comments quickly. I will try to be normal next time :-)
Yu-cheng
Quoted 2 lines · Yu-cheng Yu
There are no actual functional changes. I wanted to respond to comments quickly.
This is not how you respond to comments quickly - you wait roughly a week until people have had time to review the whole thing and *then* you send again. Spamming people with a patchset everyday will make most of them not look at your stuff anymore.
And go read Documentation/SubmittingPatches already because I'm tired of typing the same things over and over again.
Signature · 5 lines
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- Re: [PATCH v6 01/13] x86/xsaves: Define and use fpu_user_xstate_size
In reply to Yu, FenghuaQuoted 6 lines · Yu, Fenghua
Is this possible to have the third one?
From: Yu-cheng
Signed-off-by Yu-chengSigned-off-by Fenghua
Yeah, that's a valid SOB chain if Yu-cheng wrote much of the patch and you just forwarded it along with some fixes/enhancements. As Boris mentioned, you can further improve on this via:
Quoted 3 lines
Signed-off-by Yu-cheng[ Fixed bug X, cleaned up Y. ]Signed-off-by Fenghua
to signal that you made changes over Yu-cheng's version.
Thanks,
Ingo
Quoted 9 lines · Yu-cheng Yu
XSAVES is a kernel-mode instruction. It offers a compacted format and memory-write optimization. These patches fix issues in the first implementation.
Changes since Version 5:
Patch 1, 2 - Change names to
fpu_user_xstate_sizeandfpu_kernel_xstate_size;
fix some comments, etc.
Quoted 8 lines
Patch 11 - In addition to a warning, make
__raw_xsave_addr()return NULL
when a disabled xstate component address is requested.Patch 12 - Add more details in the comments of why this patch is needed;
make a new macro for xcomp_bv[63].Yu-cheng Yu (13):
x86/xsaves: Define and usefpu_user_xstate_size
So if you look at the git log of arch/x86/kernel/fpu/ then you'll see that the standard tag for FPU code is 'x86/fpu: ', for purely xstate related patch it's 'x86/fpu/xstate: '.
Quoted 5 lines
x86/xsaves: Rename
xstate_sizetofpu_kernel_xstate_sizeto distinguish fromfpu_user_xstate_sizex86/xsaves: Keep init_fpstate.xsave.header.xfeatures as zero for init optimization x86/xsaves: Introduce a new check that allows correct xstates copy from kernel to user directly x86/xsaves: Align xstate components according to CPUID
x86/xsaves: Supervisor state component offset
Please start all commit titles with verbs.
Also, this series is clearly too big. I was under the impression that the series was ready, but there's a number of small details that were pointed out (none of them very serious) - so how about you send a series of the first 4 patches next time around, and only send another 4 once those 4 first patches get applied to x86/fpu?
This makes it much easier to review, which reduces reviewer frustration; and it also allows you to focus more on incorporating feedback, without having to juggle 13 patches all the time.
Ok?
Thanks,
Ingo