x86/xsaves: Fix XSAVES issues
v7 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 6:
There are no functional or code changes, only some cleanup work:
Patch 1 ~ 4: Fix Signed-off-by chain.
Patch 11, 12: Small changes in the comments.
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 address 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 | 15 +- 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, 444 insertions(+), 141 deletions(-)
Signature · 1 line
1.9.1
PATCH v7 1/13 x86/xsaves: Define and use fpu_user_xstate_size
From: Fenghua Yu <[email protected]>
The kernel xstate area 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.
--- 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 v7 2/13 x86/xsaves: Rename xstate_size to fpu_kernel_xstate_size to distinguish from fpu_user_xstate_size
In reply to Yu-cheng YuFrom: Fenghua Yu <[email protected]>
User 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 v7 3/13 x86/xsaves: Keep init_fpstate.xsave.header.xfeatures as zero for init optimization
In reply to Yu-cheng YuFrom: Fenghua Yu <[email protected]>
Keep 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 v7 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. Based on an earlier patch from Fenghua 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 v7 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 v7 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 v7 7/13 x86/xsaves: Fix PTRACE frames for XSAVES
In reply to Yu-cheng YuXSAVES 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 v7 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 v7 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 v7 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
PATCH v7 11/13 x86/xsaves: When a disabled xstate component address 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 v7 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 | 8 ++++++++ 2 files changed, 14 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 +8 -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,13 @@ void fpstate_init(union fpregs_state *state)memset (state,0 , fpu_kernel_xstate_size); +/* +* XRSTORS requires that this bit is set in xcomp_bv, or +* it will #GP. Make sure it is replaced after the memset(). +*/ +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
PATCH v7 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
Quoted 3 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.
Dude,
do you even read my emails?!
https://lkml.kernel.org/r/[email protected]
That's the third patchset in three days! What's with the spamming?!
Signature · 5 lines
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- Dave Hansen
maintainerQuoted 10 lines · Borislav Petkov
Quoted 3 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.
do you even read my emails?!
https://lkml.kernel.org/r/[email protected]
That's the third patchset in three days! What's with the spamming?!
Borislav, if you don't want to be cc'd on these patches, then please speak up. I'm sure Yu-cheng would be happy not to cc you.
But, seriously, he's churning through, responding to feedback and improving the patches quickly. I only wish most patch authors did this.
And, despite breaking it up into 13 pieces, there is *NOT* a lot of
code here. There's no sane reason to wait a week between iterations.
I encouraged Yu-cheng to keep up with the current pace and not wait a week between reposts, despite what you requested. Sorry. Just speak up if you want off the cc.
Quoted 4 lines · Dave Hansen
But, seriously, he's churning through, responding to feedback and improving the patches quickly. I only wish most patch authors did this.
And, despite breaking it up into 13 pieces, there is *NOT* a lot of
code here.
I don't think we review only code, do we?! And as I pointed out a couple of times, those patches weren't clean. Hell, they even didn't build on tip/master. So even the "responding to feedback" failed. So what is your point again?
Quoted 1 line
There's no sane reason to wait a week between iterations.
I don't think so - review is review and takes time. You should know this even better than me.
Quoted 1 line
Just speak up if you want off the cc.
Sure. I thought you'd never ask.
Signature · 5 lines
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--