[PATCH 11/11] x86, fpu: check CPU-provided sizes against struct declarations

[PATCH 11/11] x86, fpu: check CPU-provided sizes against struct declarations

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

We now have C structures defined for each of the XSAVE state
components that we support.  This patch adds checks during our
verification pass to ensure that the CPU-provided data
enumerated in CPUID leaves matches our C structures.

If not, we warn and dump all the XSAVE CPUID leaves.

Note: this *actually* found an inconsistency with the MPX
'bndcsr' state.  The hardware pads it out differently from
our C structures.  This patch caught it and warned.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
---

 b/appendme                         |    6 +++
 b/arch/x86/include/asm/fpu/types.h |    1 
 b/arch/x86/kernel/fpu/xstate.c     |   62 +++++++++++++++++++++++++++++++++++--
 3 files changed, 67 insertions(+), 2 deletions(-)

diff -puN /dev/null appendme
--- /dev/null	2015-07-13 14:24:11.435656502 -0700
+++ b/appendme	2015-08-25 12:50:01.857661314 -0700
@@ -0,0 +1,6 @@
+Cc: Ingo Molnar <mingo@redhat.com>
+Cc: x86@kernel.org
+Cc: Borislav Petkov <bp@alien8.de>
+Cc: Fenghua Yu <fenghua.yu@intel.com>
+Cc: Tim Chen <tim.c.chen@linux.intel.com>
+Cc: linux-kernel@vger.kernel.org
diff -puN arch/x86/include/asm/fpu/types.h~x86-fpu-check-against-struct-declarations arch/x86/include/asm/fpu/types.h
--- a/arch/x86/include/asm/fpu/types.h~x86-fpu-check-against-struct-declarations	2015-08-25 12:50:01.853661133 -0700
+++ b/arch/x86/include/asm/fpu/types.h	2015-08-25 12:50:01.858661359 -0700
@@ -108,6 +108,7 @@ enum xfeature_nr {
 	XFEATURE_NR_OPMASK,
 	XFEATURE_NR_ZMM_Hi256,
 	XFEATURE_NR_Hi16_ZMM,
+	XFEATURE_NR_PT_UNIMPLEMENTED_SO_FAR,
 
 	XFEATURES_NR_MAX,
 };
diff -puN arch/x86/kernel/fpu/xstate.c~x86-fpu-check-against-struct-declarations arch/x86/kernel/fpu/xstate.c
--- a/arch/x86/kernel/fpu/xstate.c~x86-fpu-check-against-struct-declarations	2015-08-25 12:50:01.854661178 -0700
+++ b/arch/x86/kernel/fpu/xstate.c	2015-08-25 12:50:01.858661359 -0700
@@ -182,7 +182,8 @@ static int xfeature_nr_enabled(enum xfea
 static void __init setup_xstate_features(void)
 {
 	u32 eax, ebx, ecx, edx, i;
-	unsigned int last_good_offset = -1;
+	/* start at the beginnning of the "extended state" */
+	unsigned int last_good_offset = offsetof(struct xregs_state, __reserved);
 
 	for (i = FIRST_EXTENDED_XFEATURE_NR; i < XFEATURES_NR_MAX; i++) {
 		if (!xfeature_nr_enabled(i))
@@ -196,7 +197,8 @@ static void __init setup_xstate_features
 		 * highest-numbered xstate feature has the
 		 * highest offset in the buffer.  Ensure it does.
 		 */
-		WARN_ON(last_good_offset > xstate_offsets[i]);
+		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]: %04x, xstate_sizes[%d]: %04x\n", i, ebx, i, eax);
@@ -407,6 +409,52 @@ static void __xstate_dump_leaves(void)
 	}									\
 } while (0)
 
+
+#define XCHECK_SZ(sz, nr, nr_macro, __struct) do {			\
+	if ((nr == nr_macro) &&						\
+	    WARN_ONCE(sz != sizeof(__struct),				\
+		"%s: struct is %ld bytes, cpu state %d bytes\n",	\
+		__stringify(nr_macro), sizeof(__struct), sz)) {		\
+		__xstate_dump_leaves();					\
+	}								\
+} while (0)
+
+
+/*
+ * We have a C struct for each 'xstate'.  We need to ensure
+ * that our software representation matches what the CPU
+ * tells us about the state's size.
+ */
+static void check_xstate_against_struct(int nr)
+{
+	/*
+	 * Ask the CPU for the size of the state.
+	 */
+	int sz = xfeature_size(nr);
+	/*
+	 * Match each CPU state with the corresponding software
+	 * structure.
+	 */
+	XCHECK_SZ(sz, nr, XFEATURE_NR_YMM,       struct ymmh_struct);
+	XCHECK_SZ(sz, nr, XFEATURE_NR_BNDREGS,   struct mpx_bndreg_state);
+	XCHECK_SZ(sz, nr, XFEATURE_NR_BNDCSR,    struct mpx_bndcsr_state);
+	XCHECK_SZ(sz, nr, XFEATURE_NR_OPMASK,    struct avx_512_opmask_state);
+	XCHECK_SZ(sz, nr, XFEATURE_NR_ZMM_Hi256, struct avx_512_zmm_uppers_state);
+	XCHECK_SZ(sz, nr, XFEATURE_NR_Hi16_ZMM,  struct avx_512_hi16_state);
+
+	/*
+	 * Make *SURE* to add any feature numbers in below if
+	 * there are "holes" in the xsave state component
+	 * numbers.
+	 */
+	if ((nr < XFEATURE_NR_YMM) ||
+	    (nr >= XFEATURES_NR_MAX) ||
+	    (nr == XFEATURE_NR_PT_UNIMPLEMENTED_SO_FAR)) {
+		WARN_ONCE(1, "no structure for xstate: %d\n", nr);
+		XSTATE_WARN_ON(1);
+	}
+}
+
 /*
  * This essentially double-checks what the cpu told us about
  * how large the XSAVE buffer needs to be.  We are recalculating
@@ -420,6 +468,8 @@ static void do_extra_xstate_size_checks(
 	for (i = FIRST_EXTENDED_XFEATURE_NR; i < XFEATURES_NR_MAX; i++) {
 		if (!xfeature_nr_enabled(i))
 			continue;
+
+		check_xstate_against_struct(i);
 		/*
 		 * Supervisor state components can be managed only by
 		 * XSAVES, which is compacted-format only.
@@ -445,6 +495,14 @@ static void do_extra_xstate_size_checks(
 		paranoid_xstate_size += xfeature_size(i);
 	}
 	XSTATE_WARN_ON(paranoid_xstate_size != xstate_size);
+	/*
+	 * Basically, make sure that XSTATE_RESERVE has forced
+	 * xregs_state to be large enough.  This is not fatal
+	 * because we reserve a *lot* of extra room in the init
+	 * task struct, but we should at least know we got it
+	 * wrong.
+	 */
+	XSTATE_WARN_ON(xstate_size > sizeof(struct xregs_state));
 }
 
 /*
_

Re: [PATCH 11/11] x86, fpu: check CPU-provided sizes against struct declarations

[Tim Chen]

On Tue, 2015-08-25 at 13:12 -0700, Dave Hansen wrote:
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> We now have C structures defined for each of the XSAVE state
> components that we support.  This patch adds checks during our
> verification pass to ensure that the CPU-provided data
> enumerated in CPUID leaves matches our C structures.
> 
> If not, we warn and dump all the XSAVE CPUID leaves.
> 
> Note: this *actually* found an inconsistency with the MPX
> 'bndcsr' state.  The hardware pads it out differently from
> our C structures.  This patch caught it and warned.
> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: x86@kernel.org
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Tim Chen <tim.c.chen@linux.intel.com>
> Cc: linux-kernel@vger.kernel.org
> ---
> 
>  b/appendme                         |    6 +++
>  b/arch/x86/include/asm/fpu/types.h |    1 
>  b/arch/x86/kernel/fpu/xstate.c     |   62 +++++++++++++++++++++++++++++++++++--
>  3 files changed, 67 insertions(+), 2 deletions(-)
> 
> diff -puN /dev/null appendme
> --- /dev/null	2015-07-13 14:24:11.435656502 -0700
> +++ b/appendme	2015-08-25 12:50:01.857661314 -0700
> @@ -0,0 +1,6 @@
> +Cc: Ingo Molnar <mingo@redhat.com>
> +Cc: x86@kernel.org
> +Cc: Borislav Petkov <bp@alien8.de>
> +Cc: Fenghua Yu <fenghua.yu@intel.com>
> +Cc: Tim Chen <tim.c.chen@linux.intel.com>
> +Cc: linux-kernel@vger.kernel.org
> diff -puN arch/x86/include/asm/fpu/types.h~x86-fpu-check-against-struct-declarations arch/x86/include/asm/fpu/types.h
> --- a/arch/x86/include/asm/fpu/types.h~x86-fpu-check-against-struct-declarations	2015-08-25 12:50:01.853661133 -0700
> +++ b/arch/x86/include/asm/fpu/types.h	2015-08-25 12:50:01.858661359 -0700
> @@ -108,6 +108,7 @@ enum xfeature_nr {
>  	XFEATURE_NR_OPMASK,
>  	XFEATURE_NR_ZMM_Hi256,
>  	XFEATURE_NR_Hi16_ZMM,
> +	XFEATURE_NR_PT_UNIMPLEMENTED_SO_FAR,
>  

Maybe move this to a separate patch with a bit more
explanation of this XFEATURE?

Tim

Re: [PATCH 11/11] x86, fpu: check CPU-provided sizes against struct declarations

[Dave Hansen]

On 08/26/2015 09:18 AM, Tim Chen wrote:
>> diff -puN arch/x86/include/asm/fpu/types.h~x86-fpu-check-against-struct-declarations arch/x86/include/asm/fpu/types.h
>> > --- a/arch/x86/include/asm/fpu/types.h~x86-fpu-check-against-struct-declarations	2015-08-25 12:50:01.853661133 -0700
>> > +++ b/arch/x86/include/asm/fpu/types.h	2015-08-25 12:50:01.858661359 -0700
>> > @@ -108,6 +108,7 @@ enum xfeature_nr {
>> >  	XFEATURE_NR_OPMASK,
>> >  	XFEATURE_NR_ZMM_Hi256,
>> >  	XFEATURE_NR_Hi16_ZMM,
>> > +	XFEATURE_NR_PT_UNIMPLEMENTED_SO_FAR,
>> >  
> Maybe move this to a separate patch with a bit more
> explanation of this XFEATURE?

Yeah, plus the obvious garbage in my 'appendme' file.  Will do.

[PATCH 00/11] [v2] x86, fpu: XSAVE cleanups and sanity checks

[Dave Hansen]

Changes in v2:
 * remove references to Processor Trace XSAVE state
   (will defer to another patch set)
 * Remove some cruft from last patch
 * move last_good_offset fix in to the patch that
   introduced it


These patches make some updates to the x86 XSAVE code.

There are basically 5 things going on here:
 * removal of the LWP (lightweight profiling) code
 * naming and type cleanups
 * removal of xfeatures_nr variable
 * addition of AVX-512 C structures
 * new sanity checks of XSAVE buffer sizing

Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: linux-kernel@vger.kernel.org

[PATCH 01/11] x86, fpu: kill LWP support

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

LightWeight Profiling was evidently an AMD profiling feature that
we never got around to implementing.  Remove the references to it.

This patch is a *bit* worrisome becuase it will implicitly cause
'struct xregs_state' to shrink.  This effectively removes some
unused padding that we had in there.  It might expose other bugs.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
---

 b/arch/x86/include/asm/fpu/types.h |    6 ------
 1 file changed, 6 deletions(-)

diff -puN arch/x86/include/asm/fpu/types.h~axe-lwp arch/x86/include/asm/fpu/types.h
--- a/arch/x86/include/asm/fpu/types.h~axe-lwp	2015-08-27 10:08:01.195617135 -0700
+++ b/arch/x86/include/asm/fpu/types.h	2015-08-27 10:08:01.198617272 -0700
@@ -132,11 +132,6 @@ struct ymmh_struct {
 	u8				ymmh_space[256];
 };
 
-/* We don't support LWP yet: */
-struct lwp_struct {
-	u8				reserved[128];
-};
-
 /* Intel MPX support: */
 struct bndreg {
 	u64				lower_bound;
@@ -161,7 +156,6 @@ struct xstate_header {
 
 /* New processor state extensions should be added here: */
 #define XSTATE_RESERVE			(sizeof(struct ymmh_struct) + \
-					 sizeof(struct lwp_struct)  + \
 					 sizeof(struct mpx_struct)  )
 /*
  * This is our most modern FPU state format, as saved by the XSAVE
_

[PATCH 03/11] x86, fpu: rework XSTATE_* macros to remove magic '2'

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

The 'xstate.c' code has a bunch of references to '2'.  This
is because we have a lot more work to do for the "extended"
xstates than the "legacy" ones and state component 2 is the
first "extended" state.

This patch replaces all of the instances of '2' with
FIRST_EXTENDED_XFEATURE_NR, which clearly explains what is
going on.


Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
---

 b/arch/x86/include/asm/fpu/types.h |    2 ++
 b/arch/x86/kernel/fpu/xstate.c     |   13 +++++++------
 2 files changed, 9 insertions(+), 6 deletions(-)

diff -puN arch/x86/include/asm/fpu/types.h~remove-hard-coded-values arch/x86/include/asm/fpu/types.h
--- a/arch/x86/include/asm/fpu/types.h~remove-hard-coded-values	2015-08-27 10:08:01.942651168 -0700
+++ b/arch/x86/include/asm/fpu/types.h	2015-08-27 10:08:01.946651350 -0700
@@ -124,6 +124,8 @@ enum xfeature_nr {
 #define XSTATE_FPSSE		(XSTATE_FP | XSTATE_SSE)
 #define XSTATE_AVX512		(XSTATE_OPMASK | XSTATE_ZMM_Hi256 | XSTATE_Hi16_ZMM)
 
+#define FIRST_EXTENDED_XFEATURE_NR	XFEATURE_NR_YMM
+
 /*
  * There are 16x 256-bit AVX registers named YMM0-YMM15.
  * The low 128 bits are aliased to the 16 SSE registers (XMM0-XMM15)
diff -puN arch/x86/kernel/fpu/xstate.c~remove-hard-coded-values arch/x86/kernel/fpu/xstate.c
--- a/arch/x86/kernel/fpu/xstate.c~remove-hard-coded-values	2015-08-27 10:08:01.943651213 -0700
+++ b/arch/x86/kernel/fpu/xstate.c	2015-08-27 10:08:01.947651395 -0700
@@ -181,7 +181,7 @@ static void __init setup_xstate_features
 
 	xfeatures_nr = fls64(xfeatures_mask);
 
-	for (leaf = 2; leaf < xfeatures_nr; leaf++) {
+	for (leaf = FIRST_EXTENDED_XFEATURE_NR; leaf < xfeatures_nr; leaf++) {
 		cpuid_count(XSTATE_CPUID, leaf, &eax, &ebx, &ecx, &edx);
 
 		xstate_offsets[leaf] = ebx;
@@ -233,7 +233,7 @@ static void __init setup_xstate_comp(voi
 	xstate_comp_offsets[1] = offsetof(struct fxregs_state, xmm_space);
 
 	if (!cpu_has_xsaves) {
-		for (i = 2; i < xfeatures_nr; i++) {
+		for (i = FIRST_EXTENDED_XFEATURE_NR; 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];
@@ -242,15 +242,16 @@ static void __init setup_xstate_comp(voi
 		return;
 	}
 
-	xstate_comp_offsets[2] = FXSAVE_SIZE + XSAVE_HDR_SIZE;
+	xstate_comp_offsets[FIRST_EXTENDED_XFEATURE_NR] =
+	  	FXSAVE_SIZE + XSAVE_HDR_SIZE;
 
-	for (i = 2; i < xfeatures_nr; i++) {
+	for (i = FIRST_EXTENDED_XFEATURE_NR; i < xfeatures_nr; i++) {
 		if (test_bit(i, (unsigned long *)&xfeatures_mask))
 			xstate_comp_sizes[i] = xstate_sizes[i];
 		else
 			xstate_comp_sizes[i] = 0;
 
-		if (i > 2)
+		if (i > FIRST_EXTENDED_XFEATURE_NR)
 			xstate_comp_offsets[i] = xstate_comp_offsets[i-1]
 					+ xstate_comp_sizes[i-1];
 
@@ -305,7 +306,7 @@ static void __init init_xstate_size(void
 	}
 
 	xstate_size = FXSAVE_SIZE + XSAVE_HDR_SIZE;
-	for (i = 2; i < 64; i++) {
+	for (i = FIRST_EXTENDED_XFEATURE_NR; i < 64; i++) {
 		if (test_bit(i, (unsigned long *)&xfeatures_mask)) {
 			cpuid_count(XSTATE_CPUID, i, &eax, &ebx, &ecx, &edx);
 			xstate_size += eax;
_

[PATCH 02/11] x86, fpu: rename xfeature_bit

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

The 'xfeature_bit's are at best xfeature bit _numbers_.  Calling
them bits is at best inconsistent with ending the enum list with
'XFEATURES_NR_MAX'.

This patch renames the enum to be 'xfeature_nr'.  These also
happen to be what the Intel documentation calls a "state
component".

We also want to differentiate these from the "XSTATE_*" macros.
The "XSTATE_*" macros are a mask, and should probably be
renamed, but that's another patch.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
---

 b/arch/x86/include/asm/fpu/types.h |   38 ++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff -puN arch/x86/include/asm/fpu/types.h~x86-fpu-rename-xfeature_bit arch/x86/include/asm/fpu/types.h
--- a/arch/x86/include/asm/fpu/types.h~x86-fpu-rename-xfeature_bit	2015-08-27 10:08:01.569634174 -0700
+++ b/arch/x86/include/asm/fpu/types.h	2015-08-27 10:08:01.572634311 -0700
@@ -95,27 +95,31 @@ struct swregs_state {
 /*
  * List of XSAVE features Linux knows about:
  */
-enum xfeature_bit {
-	XSTATE_BIT_FP,
-	XSTATE_BIT_SSE,
-	XSTATE_BIT_YMM,
-	XSTATE_BIT_BNDREGS,
-	XSTATE_BIT_BNDCSR,
-	XSTATE_BIT_OPMASK,
-	XSTATE_BIT_ZMM_Hi256,
-	XSTATE_BIT_Hi16_ZMM,
+enum xfeature_nr {
+	XFEATURE_NR_FP,
+	XFEATURE_NR_SSE,
+	/*
+	 * Values above here are "legacy states".
+	 * Those below are "extended states".
+	 */
+	XFEATURE_NR_YMM,
+	XFEATURE_NR_BNDREGS,
+	XFEATURE_NR_BNDCSR,
+	XFEATURE_NR_OPMASK,
+	XFEATURE_NR_ZMM_Hi256,
+	XFEATURE_NR_Hi16_ZMM,
 
 	XFEATURES_NR_MAX,
 };
 
-#define XSTATE_FP		(1 << XSTATE_BIT_FP)
-#define XSTATE_SSE		(1 << XSTATE_BIT_SSE)
-#define XSTATE_YMM		(1 << XSTATE_BIT_YMM)
-#define XSTATE_BNDREGS		(1 << XSTATE_BIT_BNDREGS)
-#define XSTATE_BNDCSR		(1 << XSTATE_BIT_BNDCSR)
-#define XSTATE_OPMASK		(1 << XSTATE_BIT_OPMASK)
-#define XSTATE_ZMM_Hi256	(1 << XSTATE_BIT_ZMM_Hi256)
-#define XSTATE_Hi16_ZMM		(1 << XSTATE_BIT_Hi16_ZMM)
+#define XSTATE_FP		(1 << XFEATURE_NR_FP)
+#define XSTATE_SSE		(1 << XFEATURE_NR_SSE)
+#define XSTATE_YMM		(1 << XFEATURE_NR_YMM)
+#define XSTATE_BNDREGS		(1 << XFEATURE_NR_BNDREGS)
+#define XSTATE_BNDCSR		(1 << XFEATURE_NR_BNDCSR)
+#define XSTATE_OPMASK		(1 << XFEATURE_NR_OPMASK)
+#define XSTATE_ZMM_Hi256	(1 << XFEATURE_NR_ZMM_Hi256)
+#define XSTATE_Hi16_ZMM		(1 << XFEATURE_NR_Hi16_ZMM)
 
 #define XSTATE_FPSSE		(XSTATE_FP | XSTATE_SSE)
 #define XSTATE_AVX512		(XSTATE_OPMASK | XSTATE_ZMM_Hi256 | XSTATE_Hi16_ZMM)
_

[PATCH 04/11] x86, fpu: remove xfeature_nr

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

xfeature_nr ended up being initialized too late for me to
use it in the "xsave size sanity check" patch which is
later in the series.  I tried to move around its initialization
but realized that it was just as easy to get rid of it.

We only have 9 XFEATURES.  Instead of dynamically calculating
and storing the last feature, just use the compile-time max:
XFEATURES_NR_MAX.  Note that even with 'xfeatures_nr' we can
had "holes" in the xfeatures_mask that we had to deal with.

We also change a 'leaf' variable to be a plain 'i'.  Although
it is used to grab a cpuid leaf in this one loop, all of the
other loops just use an 'i' and I find it much more obvious
to keep the naming consistent across all the similar loops.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
---

 b/arch/x86/kernel/fpu/xstate.c |   24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff -puN arch/x86/kernel/fpu/xstate.c~kill-xfeatures_nr arch/x86/kernel/fpu/xstate.c
--- a/arch/x86/kernel/fpu/xstate.c~kill-xfeatures_nr	2015-08-27 10:08:02.339669254 -0700
+++ b/arch/x86/kernel/fpu/xstate.c	2015-08-27 10:08:02.343669437 -0700
@@ -35,9 +35,6 @@ static unsigned int xstate_offsets[XFEAT
 static unsigned int xstate_sizes[XFEATURES_NR_MAX]   = { [ 0 ... XFEATURES_NR_MAX - 1] = -1};
 static unsigned int xstate_comp_offsets[sizeof(xfeatures_mask)*8];
 
-/* The number of supported xfeatures in xfeatures_mask: */
-static unsigned int xfeatures_nr;
-
 /*
  * Return whether the system supports a given xfeature.
  *
@@ -171,23 +168,18 @@ void fpu__init_cpu_xstate(void)
 /*
  * Record the offsets and sizes of various xstates contained
  * in the XSAVE state memory layout.
- *
- * ( Note that certain features might be non-present, for them
- *   we'll have 0 offset and 0 size. )
  */
 static void __init setup_xstate_features(void)
 {
-	u32 eax, ebx, ecx, edx, leaf;
-
-	xfeatures_nr = fls64(xfeatures_mask);
+	u32 eax, ebx, ecx, edx, i;
 
-	for (leaf = FIRST_EXTENDED_XFEATURE_NR; leaf < xfeatures_nr; leaf++) {
-		cpuid_count(XSTATE_CPUID, leaf, &eax, &ebx, &ecx, &edx);
+	for (i = FIRST_EXTENDED_XFEATURE_NR; i < XFEATURES_NR_MAX; i++) {
+		cpuid_count(XSTATE_CPUID, i, &eax, &ebx, &ecx, &edx);
 
-		xstate_offsets[leaf] = ebx;
-		xstate_sizes[leaf] = eax;
+		xstate_offsets[i] = ebx;
+		xstate_sizes[i] = eax;
 
-		printk(KERN_INFO "x86/fpu: xstate_offset[%d]: %04x, xstate_sizes[%d]: %04x\n", leaf, ebx, leaf, eax);
+		printk(KERN_INFO "x86/fpu: xstate_offset[%d]: %04x, xstate_sizes[%d]: %04x\n", i, ebx, i, eax);
 	}
 }
 
@@ -233,7 +225,7 @@ static void __init setup_xstate_comp(voi
 	xstate_comp_offsets[1] = offsetof(struct fxregs_state, xmm_space);
 
 	if (!cpu_has_xsaves) {
-		for (i = FIRST_EXTENDED_XFEATURE_NR; i < xfeatures_nr; i++) {
+		for (i = FIRST_EXTENDED_XFEATURE_NR; i < XFEATURES_NR_MAX; i++) {
 			if (test_bit(i, (unsigned long *)&xfeatures_mask)) {
 				xstate_comp_offsets[i] = xstate_offsets[i];
 				xstate_comp_sizes[i] = xstate_sizes[i];
@@ -245,7 +237,7 @@ static void __init setup_xstate_comp(voi
 	xstate_comp_offsets[FIRST_EXTENDED_XFEATURE_NR] =
 	  	FXSAVE_SIZE + XSAVE_HDR_SIZE;
 
-	for (i = FIRST_EXTENDED_XFEATURE_NR; i < xfeatures_nr; i++) {
+	for (i = FIRST_EXTENDED_XFEATURE_NR; i < XFEATURES_NR_MAX; i++) {
 		if (test_bit(i, (unsigned long *)&xfeatures_mask))
 			xstate_comp_sizes[i] = xstate_sizes[i];
 		else
_

[PATCH 05/11] x86, fpu: add helper xfeature_nr_enabled() instead of test_bit()

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

We currently use test_bit() in a few places to see if an xfeature
is enabled.  It ends up being a bit ugly because 'xfeatures_mask'
is a u64 and test_bit wants an 'unsigned long' so it requires a
cast.  The *_bit() functions are also techincally atomic, which
we have no need for here.

So, remove the test_bit()s and replace with the new
xfeature_nr_enabled() helper.

This also provides a central place to add a comment about the
future need to support 'system xstates'.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
---

 b/arch/x86/kernel/fpu/xstate.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff -puN arch/x86/kernel/fpu/xstate.c~use-xfeature_enabled arch/x86/kernel/fpu/xstate.c
--- a/arch/x86/kernel/fpu/xstate.c~use-xfeature_enabled	2015-08-27 10:08:02.716686431 -0700
+++ b/arch/x86/kernel/fpu/xstate.c	2015-08-27 10:08:02.720686613 -0700
@@ -207,6 +207,16 @@ static void __init print_xstate_features
 }
 
 /*
+ * Note that in the future we will likely need a pair of
+ * functions here: one for user xstates and the other for
+ * system xstates.  For now, they are the same.
+ */
+static int xfeature_nr_enabled(enum xfeature_nr xfeature_nr)
+{
+	return !!(xfeatures_mask & (1UL << xfeature_nr));
+}
+
+/*
  * 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.
@@ -226,7 +236,7 @@ static void __init setup_xstate_comp(voi
 
 	if (!cpu_has_xsaves) {
 		for (i = FIRST_EXTENDED_XFEATURE_NR; i < XFEATURES_NR_MAX; i++) {
-			if (test_bit(i, (unsigned long *)&xfeatures_mask)) {
+			if (xfeature_nr_enabled(i)) {
 				xstate_comp_offsets[i] = xstate_offsets[i];
 				xstate_comp_sizes[i] = xstate_sizes[i];
 			}
@@ -238,7 +248,7 @@ static void __init setup_xstate_comp(voi
 	  	FXSAVE_SIZE + XSAVE_HDR_SIZE;
 
 	for (i = FIRST_EXTENDED_XFEATURE_NR; i < XFEATURES_NR_MAX; i++) {
-		if (test_bit(i, (unsigned long *)&xfeatures_mask))
+		if (xfeature_nr_enabled(i))
 			xstate_comp_sizes[i] = xstate_sizes[i];
 		else
 			xstate_comp_sizes[i] = 0;
@@ -299,7 +309,7 @@ static void __init init_xstate_size(void
 
 	xstate_size = FXSAVE_SIZE + XSAVE_HDR_SIZE;
 	for (i = FIRST_EXTENDED_XFEATURE_NR; i < 64; i++) {
-		if (test_bit(i, (unsigned long *)&xfeatures_mask)) {
+		if (xfeature_nr_enabled(i)) {
 			cpuid_count(XSTATE_CPUID, i, &eax, &ebx, &ecx, &edx);
 			xstate_size += eax;
 		}
_

[PATCH 06/11] x86, fpu: rework MPX 'xstate' types

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

MPX includes two separate "extended state components".  There is
no real need to have an 'mpx_struct' because we never really
manage the states together.

We also separate out the actual data in 'mpx_bndcsr_state' from
the padding.  We will shortly be checking the state sizes against
our structures and need them to match.  For consistency, we also
ensure to prefix these types with 'mpx_'.

Lastly, we add some comments to mirror some of the descriptions
in the Intel documents (SDM) of the various state components.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
---

 b/arch/x86/include/asm/fpu/types.h |   36 +++++++++++++++++++++++++++---------
 b/arch/x86/include/asm/trace/mpx.h |    7 ++++---
 b/arch/x86/kernel/traps.c          |    2 +-
 b/arch/x86/mm/mpx.c                |    9 +++++----
 4 files changed, 37 insertions(+), 17 deletions(-)

diff -puN arch/x86/include/asm/fpu/types.h~x86-fpu-rework-mpx-types arch/x86/include/asm/fpu/types.h
--- a/arch/x86/include/asm/fpu/types.h~x86-fpu-rework-mpx-types	2015-08-27 10:08:03.093703606 -0700
+++ b/arch/x86/include/asm/fpu/types.h	2015-08-27 10:08:03.101703971 -0700
@@ -139,20 +139,37 @@ struct ymmh_struct {
 };
 
 /* Intel MPX support: */
-struct bndreg {
+
+struct mpx_bndreg {
 	u64				lower_bound;
 	u64				upper_bound;
 } __packed;
+/*
+ * State component 3 is used for the 4 128-bit bounds registers
+ */
+struct mpx_bndreg_state {
+	struct mpx_bndreg		bndreg[4];
+} __packed;
 
-struct bndcsr {
-	u64				bndcfgu;
-	u64				bndstatus;
+/*
+ * State component 4 is used for the 64-bit user-mode MPX
+ * configuration register BNDCFGU and the 64-bit MPX status
+ * register BNDSTATUS.  We call the pair "BNDCSR".
+ */
+struct mpx_bndcsr {
+ 	u64				bndcfgu;
+ 	u64				bndstatus;
 } __packed;
 
-struct mpx_struct {
-	struct bndreg			bndreg[4];
-	struct bndcsr			bndcsr;
-};
+/*
+ * The BNDCSR state is padded out to be 64-bytes in size.
+ */
+struct mpx_bndcsr_state {
+	union {
+		struct mpx_bndcsr		bndcsr;
+		u8				pad_to_64_bytes[64];
+	};
+} __packed;
 
 struct xstate_header {
 	u64				xfeatures;
@@ -162,7 +179,8 @@ struct xstate_header {
 
 /* New processor state extensions should be added here: */
 #define XSTATE_RESERVE			(sizeof(struct ymmh_struct) + \
-					 sizeof(struct mpx_struct)  )
+					 sizeof(struct mpx_bndreg_state) + \
+					 sizeof(struct mpx_bndcsr_state)  )
 /*
  * This is our most modern FPU state format, as saved by the XSAVE
  * and restored by the XRSTOR instructions.
diff -puN arch/x86/include/asm/trace/mpx.h~x86-fpu-rework-mpx-types arch/x86/include/asm/trace/mpx.h
--- a/arch/x86/include/asm/trace/mpx.h~x86-fpu-rework-mpx-types	2015-08-27 10:08:03.094703652 -0700
+++ b/arch/x86/include/asm/trace/mpx.h	2015-08-27 10:08:03.101703971 -0700
@@ -11,7 +11,7 @@
 TRACE_EVENT(mpx_bounds_register_exception,
 
 	TP_PROTO(void *addr_referenced,
-		 const struct bndreg *bndreg),
+		 const struct mpx_bndreg *bndreg),
 	TP_ARGS(addr_referenced, bndreg),
 
 	TP_STRUCT__entry(
@@ -44,7 +44,7 @@ TRACE_EVENT(mpx_bounds_register_exceptio
 
 TRACE_EVENT(bounds_exception_mpx,
 
-	TP_PROTO(const struct bndcsr *bndcsr),
+	TP_PROTO(const struct mpx_bndcsr *bndcsr),
 	TP_ARGS(bndcsr),
 
 	TP_STRUCT__entry(
@@ -116,7 +116,8 @@ TRACE_EVENT(mpx_new_bounds_table,
 /*
  * This gets used outside of MPX-specific code, so we need a stub.
  */
-static inline void trace_bounds_exception_mpx(const struct bndcsr *bndcsr)
+static inline
+void trace_bounds_exception_mpx(const struct mpx_bndcsr *bndcsr)
 {
 }
 
diff -puN arch/x86/kernel/traps.c~x86-fpu-rework-mpx-types arch/x86/kernel/traps.c
--- a/arch/x86/kernel/traps.c~x86-fpu-rework-mpx-types	2015-08-27 10:08:03.096703743 -0700
+++ b/arch/x86/kernel/traps.c	2015-08-27 10:08:03.102704016 -0700
@@ -372,7 +372,7 @@ dotraplinkage void do_double_fault(struc
 dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
 {
 	enum ctx_state prev_state;
-	const struct bndcsr *bndcsr;
+	const struct mpx_bndcsr *bndcsr;
 	siginfo_t *info;
 
 	prev_state = exception_enter();
diff -puN arch/x86/mm/mpx.c~x86-fpu-rework-mpx-types arch/x86/mm/mpx.c
--- a/arch/x86/mm/mpx.c~x86-fpu-rework-mpx-types	2015-08-27 10:08:03.098703834 -0700
+++ b/arch/x86/mm/mpx.c	2015-08-27 10:08:03.102704016 -0700
@@ -274,7 +274,8 @@ bad_opcode:
  */
 siginfo_t *mpx_generate_siginfo(struct pt_regs *regs)
 {
-	const struct bndreg *bndregs, *bndreg;
+	const struct mpx_bndreg_state *bndregs;
+	const struct mpx_bndreg *bndreg;
 	siginfo_t *info = NULL;
 	struct insn insn;
 	uint8_t bndregno;
@@ -301,7 +302,7 @@ siginfo_t *mpx_generate_siginfo(struct p
 		goto err_out;
 	}
 	/* now go select the individual register in the set of 4 */
-	bndreg = &bndregs[bndregno];
+	bndreg = &bndregs->bndreg[bndregno];
 
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
 	if (!info) {
@@ -343,7 +344,7 @@ err_out:
 
 static __user void *mpx_get_bounds_dir(void)
 {
-	const struct bndcsr *bndcsr;
+	const struct mpx_bndcsr *bndcsr;
 
 	if (!cpu_feature_enabled(X86_FEATURE_MPX))
 		return MPX_INVALID_BOUNDS_DIR;
@@ -526,7 +527,7 @@ out_unmap:
 static int do_mpx_bt_fault(void)
 {
 	unsigned long bd_entry, bd_base;
-	const struct bndcsr *bndcsr;
+	const struct mpx_bndcsr *bndcsr;
 	struct mm_struct *mm = current->mm;
 
 	bndcsr = get_xsave_field_ptr(XSTATE_BNDCSR);
_

[PATCH 07/11] x86, fpu: rework YMM definition

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

We are about to rework all of the "extended state" definitions.
This makes the 'ymm' naming consistent with the AVX-512 types
we will introduce later.

We also add a convenience type: "reg_128_bit" so that we do
not have to spell out our arithmetic.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
---

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

diff -puN arch/x86/include/asm/fpu/types.h~x86-fpu-rework-ymm-types arch/x86/include/asm/fpu/types.h
--- a/arch/x86/include/asm/fpu/types.h~x86-fpu-rework-ymm-types	2015-08-27 10:08:03.542724062 -0700
+++ b/arch/x86/include/asm/fpu/types.h	2015-08-27 10:08:03.545724199 -0700
@@ -126,17 +126,23 @@ enum xfeature_nr {
 
 #define FIRST_EXTENDED_XFEATURE_NR	XFEATURE_NR_YMM
 
+struct reg_128_bit {
+	u8      regbytes[128/8];
+};
+
 /*
+ * State component 2:
+ *
  * 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 fxregs_state::xmm_space[]'.
+ * and are stored in 'struct fxregs_state::xmm_space[]' in the
+ * "legacy" area.
  *
- * The high 128 bits are stored here:
- *    16x 128 bits == 256 bytes.
+ * The high 128 bits are stored here.
  */
 struct ymmh_struct {
-	u8				ymmh_space[256];
-};
+	struct reg_128_bit              hi_ymm[16];
+} __packed;
 
 /* Intel MPX support: */
 
_

[PATCH 10/11] x86, fpu: check to ensure increasing-offset xstate offsets

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

The xstate CPUID leaves enumerate where each state component is
inside the XSAVE buffer, along with the size of the entire
buffer.  Our new XSAVE sanity-checking code extrapolates an
expected _total_ buffer size by looking at the last component
that it encounters.

That method requires that the highest-numbered component also
be the one with the highest offset.  This is a pretty safe
assumption, but let's add some code to ensure it stays true.

To make this check work correctly, we also need to ensure we
only consider the offsets from enabled features because the
offset register (ebx) will return 0 on unsupported features.

This also means that we will preserve the -1's that we
initialized xstate_offsets/sizes[] with.  That will help
find bugs.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
---

 b/arch/x86/kernel/fpu/xstate.c |   34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff -puN arch/x86/kernel/fpu/xstate.c~x86-fpu-increasing-offset-assumption arch/x86/kernel/fpu/xstate.c
--- a/arch/x86/kernel/fpu/xstate.c~x86-fpu-increasing-offset-assumption	2015-08-27 10:08:04.657774861 -0700
+++ b/arch/x86/kernel/fpu/xstate.c	2015-08-27 10:08:04.660774998 -0700
@@ -166,18 +166,40 @@ void fpu__init_cpu_xstate(void)
 }
 
 /*
+ * Note that in the future we will likely need a pair of
+ * functions here: one for user xstates and the other for
+ * system xstates.  For now, they are the same.
+ */
+static int xfeature_nr_enabled(enum xfeature_nr xfeature_nr)
+{
+	return !!(xfeatures_mask & (1UL << xfeature_nr));
+}
+
+/*
  * Record the offsets and sizes of various xstates contained
  * in the XSAVE state memory layout.
  */
 static void __init setup_xstate_features(void)
 {
 	u32 eax, ebx, ecx, edx, i;
+	/* start at the beginnning of the "extended state" */
+	unsigned int last_good_offset = offsetof(struct xregs_state, __reserved);
 
 	for (i = FIRST_EXTENDED_XFEATURE_NR; i < XFEATURES_NR_MAX; i++) {
-		cpuid_count(XSTATE_CPUID, i, &eax, &ebx, &ecx, &edx);
+		if (!xfeature_nr_enabled(i))
+			continue;
 
+		cpuid_count(XSTATE_CPUID, i, &eax, &ebx, &ecx, &edx);
 		xstate_offsets[i] = ebx;
 		xstate_sizes[i] = eax;
+		/*
+		 * In our xstate size checks, we assume that the
+		 * highest-numbered xstate feature has the
+		 * highest offset in the buffer.  Ensure it does.
+		 */
+		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]: %04x, xstate_sizes[%d]: %04x\n", i, ebx, i, eax);
 	}
@@ -207,16 +229,6 @@ static void __init print_xstate_features
 }
 
 /*
- * Note that in the future we will likely need a pair of
- * functions here: one for user xstates and the other for
- * system xstates.  For now, they are the same.
- */
-static int xfeature_nr_enabled(enum xfeature_nr xfeature_nr)
-{
-	return !!(xfeatures_mask & (1UL << xfeature_nr));
-}
-
-/*
  * 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.
_

[PATCH 09/11] x86, fpu: correct and check XSAVE xstate size calculations

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

Note: our xsaves support is currently broken and disabled.  This
patch does not fix it, but it is an incremental improvement.
This might be useful to someone backporting the entire set of
XSAVES patches at some point, but it should not be backported
alone.

Ingo said he wanted something like this (bullets 2 and 3):

	http://lkml.kernel.org/r/20150808091508.GB32641@gmail.com

There are currently two xsave buffer formats: standard and
compacted.  The standard format is waht 'XSAVE' and 'XSAVEOPT'
produce while 'XSAVES' and 'XSAVEC' produce a compacted-formet
buffer.  (The kernel never uses XSAVEC)

But, the XSAVES buffer *ALSO* contains "system state components"
which are never saved by a plain XSAVE.  So, XSAVES has two
things that might make its buffer differently-sized from an
XSAVE-produced one.

The current code assumes that an XSAVES buffer's size is simply
the sum of the sizes of the (user) states which are supported.
This seems to work in most cases, but it is not consistent with
what the SDM says, and it breaks if we 'align' a component in the
buffer.  The calculation is also unnecessary work since the CPU
*tells* us the size of the buffer directly.

This patch just reads the size of the buffer right out of the
CPUID leaf instead of trying to derive it.

But, blindly trusting the CPU like this is dangerous.  We add
a verification pass in do_extra_xstate_size_checks() to ensure
that the size we calculate matches with what we see from the
hardware.  When it comes down to it, we trust but verify the
CPU.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
---

 b/arch/x86/kernel/fpu/xstate.c |  181 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 171 insertions(+), 10 deletions(-)

diff -puN arch/x86/kernel/fpu/xstate.c~fix-xstate_size-calculation arch/x86/kernel/fpu/xstate.c
--- a/arch/x86/kernel/fpu/xstate.c~fix-xstate_size-calculation	2015-08-27 10:08:04.283757822 -0700
+++ b/arch/x86/kernel/fpu/xstate.c	2015-08-27 10:08:04.287758004 -0700
@@ -293,27 +293,188 @@ static void __init setup_init_fpu_buf(vo
 	copy_xregs_to_kernel_booting(&init_fpstate.xsave);
 }
 
+static int xfeature_is_supervisor(int xfeature_nr)
+{
+	/*
+	 * We currently do not suport supervisor states, but if
+	 * we did, we could find out like this.
+	 *
+	 * SDM says: If state component i is a user state component,
+	 * ECX[0] return 0; if state component i is a supervisor
+	 * state component, ECX[0] returns 1.
+	u32 eax, ebx, ecx, edx;
+	cpuid_count(XSTATE_CPUID, xfeature_nr, &eax, &ebx, &ecx, &edx);
+	return !!(ecx & 1);
+	*/
+	return 0;
+}
+/*
+static int xfeature_is_user(int xfeature_nr)
+{
+	return !xfeature_is_supervisor(xfeature_nr);
+}
+*/
+
+/*
+ * This check is important because it is easy to get XSTATE_*
+ * confused with XSTATE_BIT_*.
+ */
+#define CHECK_XFEATURE_NR(nr) do {		\
+	WARN_ON(nr < FIRST_EXTENDED_XFEATURE_NR);	\
+	WARN_ON(nr >= XFEATURES_NR_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_NR(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;
+	CHECK_XFEATURE_NR(xfeature_nr);
+	cpuid_count(XSTATE_CPUID, xfeature_nr, &eax, &ebx, &ecx, &edx);
+	return ebx;
+}
+
+static int xfeature_size(int xfeature_nr)
+{
+	u32 eax, ebx, ecx, edx;
+	CHECK_XFEATURE_NR(xfeature_nr);
+	cpuid_count(XSTATE_CPUID, xfeature_nr, &eax, &ebx, &ecx, &edx);
+	return eax;
+}
+
+/*
+ * 'XSAVES' implies two different things:
+ * 1. saving of supervisor/system state
+ * 2. using the compacted format
+ *
+ * Use this function when dealing with the compacted format so
+ * that it is obvious which aspect of 'XSAVES' is being handled
+ * by the calling code.
+ */
+static int using_compacted_format(void)
+{
+	return cpu_has_xsaves;
+}
+
+static void __xstate_dump_leaves(void)
+{
+	int i;
+	u32 eax, ebx, ecx, edx;
+	static int dumped_once = 0;
+
+	if (dumped_once)
+		return;
+	dumped_once = 1;
+	/*
+	 * Dump out a few leaves past the ones that we support
+	 * just in case there are some goodies up there
+	 */
+	for (i = 0; i < XFEATURES_NR_MAX + 10; i++) {
+		cpuid_count(XSTATE_CPUID, i, &eax, &ebx, &ecx, &edx);
+		pr_warn("CPUID[%02x, %02x]: eax=%08x ebx=%08x ecx=%08x edx=%08x\n",
+			XSTATE_CPUID, i, eax, ebx, ecx, edx);
+	}
+}
+
+#define XSTATE_WARN_ON(x) do {							\
+	if (WARN_ONCE(x, "XSAVE consistency problem, dumping leaves")) {	\
+		__xstate_dump_leaves();						\
+	}									\
+} while (0)
+
+/*
+ * This essentially double-checks what the cpu told us about
+ * how large the XSAVE buffer needs to be.  We are recalculating
+ * it to be safe.
+ */
+static void do_extra_xstate_size_checks(void)
+{
+	int paranoid_xstate_size = FXSAVE_SIZE + XSAVE_HDR_SIZE;
+	int i;
+
+	for (i = FIRST_EXTENDED_XFEATURE_NR; i < XFEATURES_NR_MAX; i++) {
+		if (!xfeature_nr_enabled(i))
+			continue;
+		/*
+		 * Supervisor state components can be managed only by
+		 * XSAVES, which is compacted-format only.
+		 */
+		if (!using_compacted_format())
+			XSTATE_WARN_ON(xfeature_is_supervisor(i));
+
+		/* Align from the end of the previous feature */
+		if (xfeature_is_aligned(i))
+			paranoid_xstate_size = ALIGN(paranoid_xstate_size, 64);
+		/*
+		 * The offset of a given state in the non-compacted
+		 * format is given to us in a CPUID leaf.  We check
+		 * them for being ordered (increasing offsets) in
+		 * setup_xstate_features().
+		 */
+		if (!using_compacted_format())
+			paranoid_xstate_size = xfeature_uncompacted_offset(i);
+		/*
+		 * The compacted-format offset always depends on where
+		 * the previous state ended.
+		 */
+		paranoid_xstate_size += xfeature_size(i);
+	}
+	XSTATE_WARN_ON(paranoid_xstate_size != xstate_size);
+}
+
 /*
  * Calculate total size of enabled xstates in XCR0/xfeatures_mask.
+ *
+ * Note the SDM's wording here.  "sub-function 0" only enumerates
+ * the size of the *user* states.  If we use it to size a buffer
+ * that we use 'XSAVES' on, we could potentially overflow the
+ * buffer because 'XSAVES' saves system states too.
+ *
+ * Note that we do not currently set any bits on IA32_XSS so
+ * 'XCR0 | IA32_XSS == XCR0' for now.
  */
 static void __init init_xstate_size(void)
 {
 	unsigned int eax, ebx, ecx, edx;
-	int i;
 
 	if (!cpu_has_xsaves) {
+		/*
+		 * - CPUID function 0DH, sub-function 0:
+		 *    EBX enumerates the size (in bytes) required by
+		 *    the XSAVE instruction for an XSAVE area
+		 *    containing all the *user* state components
+		 *    corresponding to bits currently set in XCR0.
+		 */
 		cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
 		xstate_size = ebx;
-		return;
-	}
-
-	xstate_size = FXSAVE_SIZE + XSAVE_HDR_SIZE;
-	for (i = FIRST_EXTENDED_XFEATURE_NR; i < 64; i++) {
-		if (xfeature_nr_enabled(i)) {
-			cpuid_count(XSTATE_CPUID, i, &eax, &ebx, &ecx, &edx);
-			xstate_size += eax;
-		}
+	} else {
+		/*
+		 * - CPUID function 0DH, sub-function 1:
+		 *    EBX enumerates the size (in bytes) required by
+		 *    the XSAVES instruction for an XSAVE area
+		 *    containing all the state components
+		 *    corresponding to bits currently set in
+		 *    XCR0 | IA32_XSS.
+		 */
+		cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);
+		xstate_size = ebx;
 	}
+	do_extra_xstate_size_checks();
 }
 
 /*
_

[PATCH 08/11] x86, fpu: add C structures for AVX-512 state components

[Dave Hansen]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2752 bytes --]


From: Dave Hansen <dave.hansen@linux.intel.com>

AVX-512 has 3 separate state components:
1. opmask registers
2. zmm upper half of registers 0-15
3. new zmm registers (16-31)

This patch adds C structures for the three components along with
a few comments mostly lifted from the SDM to explain what they
do.  This will allow us to check our structures against what the
hardware tells us about the sizes of the components.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
---

 b/arch/x86/include/asm/fpu/types.h |   43 ++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff -puN arch/x86/include/asm/fpu/types.h~avx-512-structs arch/x86/include/asm/fpu/types.h
--- a/arch/x86/include/asm/fpu/types.h~avx-512-structs	2015-08-27 10:08:03.909740783 -0700
+++ b/arch/x86/include/asm/fpu/types.h	2015-08-27 10:08:03.912740919 -0700
@@ -129,6 +129,12 @@ enum xfeature_nr {
 struct reg_128_bit {
 	u8      regbytes[128/8];
 };
+struct reg_256_bit {
+	u8	regbytes[256/8];
+};
+struct reg_512_bit {
+	u8	regbytes[512/8];
+};
 
 /*
  * State component 2:
@@ -177,6 +183,33 @@ struct mpx_bndcsr_state {
 	};
 } __packed;
 
+/* AVX-512 Components: */
+
+/*
+ * State component 5 is used for the 8 64-bit opmask registers
+ * k0–k7 (opmask state).
+ */
+struct avx_512_opmask_state {
+	u64 				opmask_reg[8];
+} __packed;
+
+/*
+ * State component 6 is used for the upper 256 bits of the
+ * registers ZMM0–ZMM15. These 16 256-bit values are denoted
+ * ZMM0_H–ZMM15_H (ZMM_Hi256 state).
+ */
+struct avx_512_zmm_uppers_state {
+	struct reg_256_bit		zmm_upper[16];
+} __packed;
+
+/*
+ * State component 7 is used for the 16 512-bit registers
+ * ZMM16–ZMM31 (Hi16_ZMM state).
+ */
+struct avx_512_hi16_state {
+	struct reg_512_bit		hi16_zmm[16];
+} __packed;
+
 struct xstate_header {
 	u64				xfeatures;
 	u64				xcomp_bv;
@@ -184,9 +217,13 @@ struct xstate_header {
 } __attribute__((packed));
 
 /* New processor state extensions should be added here: */
-#define XSTATE_RESERVE			(sizeof(struct ymmh_struct) + \
-					 sizeof(struct mpx_bndreg_state) + \
-					 sizeof(struct mpx_bndcsr_state)  )
+#define XSTATE_RESERVE		(sizeof(struct ymmh_struct) 		+ \
+				 sizeof(struct mpx_bndreg_state) 	+ \
+				 sizeof(struct mpx_bndcsr_state) 	+ \
+				 sizeof(struct avx_512_opmask_state) 	+ \
+				 sizeof(struct avx_512_zmm_uppers_state) + \
+				 sizeof(struct avx_512_hi16_state))
+
 /*
  * This is our most modern FPU state format, as saved by the XSAVE
  * and restored by the XRSTOR instructions.
_

[PATCH 11/11] x86, fpu: check CPU-provided sizes against struct declarations

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

We now have C structures defined for each of the XSAVE state
components that we support.  This patch adds checks during our
verification pass to ensure that the CPU-provided data
enumerated in CPUID leaves matches our C structures.

If not, we warn and dump all the XSAVE CPUID leaves.

Note: this *actually* found an inconsistency with the MPX
'bndcsr' state.  The hardware pads it out differently from
our C structures.  This patch caught it and warned.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
---

 b/arch/x86/kernel/fpu/xstate.c |   53 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff -puN arch/x86/kernel/fpu/xstate.c~x86-fpu-check-against-struct-declarations arch/x86/kernel/fpu/xstate.c
--- a/arch/x86/kernel/fpu/xstate.c~x86-fpu-check-against-struct-declarations	2015-08-27 10:08:05.023791536 -0700
+++ b/arch/x86/kernel/fpu/xstate.c	2015-08-27 10:08:05.027791718 -0700
@@ -409,6 +409,49 @@ static void __xstate_dump_leaves(void)
 	}									\
 } while (0)
 
+#define XCHECK_SZ(sz, nr, nr_macro, __struct) do {			\
+	if ((nr == nr_macro) &&						\
+	    WARN_ONCE(sz != sizeof(__struct),				\
+		"%s: struct is %ld bytes, cpu state %d bytes\n",	\
+		__stringify(nr_macro), sizeof(__struct), sz)) {		\
+		__xstate_dump_leaves();					\
+	}								\
+} while (0)
+
+/*
+ * We have a C struct for each 'xstate'.  We need to ensure
+ * that our software representation matches what the CPU
+ * tells us about the state's size.
+ */
+static void check_xstate_against_struct(int nr)
+{
+	/*
+	 * Ask the CPU for the size of the state.
+	 */
+	int sz = xfeature_size(nr);
+	/*
+	 * Match each CPU state with the corresponding software
+	 * structure.
+	 */
+	XCHECK_SZ(sz, nr, XFEATURE_NR_YMM,       struct ymmh_struct);
+	XCHECK_SZ(sz, nr, XFEATURE_NR_BNDREGS,   struct mpx_bndreg_state);
+	XCHECK_SZ(sz, nr, XFEATURE_NR_BNDCSR,    struct mpx_bndcsr_state);
+	XCHECK_SZ(sz, nr, XFEATURE_NR_OPMASK,    struct avx_512_opmask_state);
+	XCHECK_SZ(sz, nr, XFEATURE_NR_ZMM_Hi256, struct avx_512_zmm_uppers_state);
+	XCHECK_SZ(sz, nr, XFEATURE_NR_Hi16_ZMM,  struct avx_512_hi16_state);
+
+	/*
+	 * Make *SURE* to add any feature numbers in below if
+	 * there are "holes" in the xsave state component
+	 * numbers.
+	 */
+	if ((nr < XFEATURE_NR_YMM) ||
+	    (nr >= XFEATURES_NR_MAX)) {
+		WARN_ONCE(1, "no structure for xstate: %d\n", nr);
+		XSTATE_WARN_ON(1);
+	}
+}
+
 /*
  * This essentially double-checks what the cpu told us about
  * how large the XSAVE buffer needs to be.  We are recalculating
@@ -422,6 +465,8 @@ static void do_extra_xstate_size_checks(
 	for (i = FIRST_EXTENDED_XFEATURE_NR; i < XFEATURES_NR_MAX; i++) {
 		if (!xfeature_nr_enabled(i))
 			continue;
+
+		check_xstate_against_struct(i);
 		/*
 		 * Supervisor state components can be managed only by
 		 * XSAVES, which is compacted-format only.
@@ -447,6 +492,14 @@ static void do_extra_xstate_size_checks(
 		paranoid_xstate_size += xfeature_size(i);
 	}
 	XSTATE_WARN_ON(paranoid_xstate_size != xstate_size);
+	/*
+	 * Basically, make sure that XSTATE_RESERVE has forced
+	 * xregs_state to be large enough.  This is not fatal
+	 * because we reserve a *lot* of extra room in the init
+	 * task struct, but we should at least know we got it
+	 * wrong.
+	 */
+	XSTATE_WARN_ON(xstate_size > sizeof(struct xregs_state));
 }
 
 /*
_

Re: [PATCH 01/11] x86, fpu: kill LWP support

[Ingo Molnar]

* Dave Hansen <dave@sr71.net> wrote:

> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> LightWeight Profiling was evidently an AMD profiling feature that
> we never got around to implementing.  Remove the references to it.
> 
> This patch is a *bit* worrisome becuase it will implicitly cause
> 'struct xregs_state' to shrink.  This effectively removes some
> unused padding that we had in there.  It might expose other bugs.
> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: x86@kernel.org
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Tim Chen <tim.c.chen@linux.intel.com>
> Cc: linux-kernel@vger.kernel.org
> ---
> 
>  b/arch/x86/include/asm/fpu/types.h |    6 ------
>  1 file changed, 6 deletions(-)
> 
> diff -puN arch/x86/include/asm/fpu/types.h~axe-lwp arch/x86/include/asm/fpu/types.h
> --- a/arch/x86/include/asm/fpu/types.h~axe-lwp	2015-08-27 10:08:01.195617135 -0700
> +++ b/arch/x86/include/asm/fpu/types.h	2015-08-27 10:08:01.198617272 -0700
> @@ -132,11 +132,6 @@ struct ymmh_struct {
>  	u8				ymmh_space[256];
>  };
>  
> -/* We don't support LWP yet: */
> -struct lwp_struct {
> -	u8				reserved[128];
> -};
> -
>  /* Intel MPX support: */
>  struct bndreg {
>  	u64				lower_bound;
> @@ -161,7 +156,6 @@ struct xstate_header {
>  
>  /* New processor state extensions should be added here: */
>  #define XSTATE_RESERVE			(sizeof(struct ymmh_struct) + \
> -					 sizeof(struct lwp_struct)  + \
>  					 sizeof(struct mpx_struct)  )

Btw., now that we allocate all this dynamically and sanity-check that the CPU 
gives us, I'd suggest we remove the XSTATE_RESERVE define altogether, and any 
excess __reserved[] bytes from the xstate header:

struct xregs_state {
        struct fxregs_state             i387;
        struct xstate_header            header;
        u8                              __reserved[XSTATE_RESERVE];
} __attribute__ ((packed, aligned (64)));

any latent bug this might trigger we want to fix.

Thanks,

	Ingo

Re: [PATCH 09/11] x86, fpu: correct and check XSAVE xstate size calculations

[Ingo Molnar]

* Dave Hansen <dave@sr71.net> wrote:

> +static int xfeature_is_supervisor(int xfeature_nr)
> +{
> +	/*
> +	 * We currently do not suport supervisor states, but if
> +	 * we did, we could find out like this.
> +	 *
> +	 * SDM says: If state component i is a user state component,
> +	 * ECX[0] return 0; if state component i is a supervisor
> +	 * state component, ECX[0] returns 1.
> +	u32 eax, ebx, ecx, edx;
> +	cpuid_count(XSTATE_CPUID, xfeature_nr, &eax, &ebx, &ecx, &edx);
> +	return !!(ecx & 1);
> +	*/
> +	return 0;
> +}

So if this CPUID is documented to work, why not use it to sanity check things?

I.e. do something like:

	u32 eax, ebx, ecx, edx;

	cpuid_count(XSTATE_CPUID, xfeature_nr, &eax, &ebx, &ecx, &edx);

	/* Linux doesn't support supervisor states (yet): */
	WARN_ON_ONCE(ecx & 1);

	return 0;

That would give us a gentle way to double check our assumptions here.

Thanks,

	Ingo

Re: [PATCH 08/11] x86, fpu: add C structures for AVX-512 state components

[Ingo Molnar]

* Dave Hansen <dave@sr71.net> wrote:

> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> AVX-512 has 3 separate state components:
> 1. opmask registers
> 2. zmm upper half of registers 0-15
> 3. new zmm registers (16-31)
> 
> This patch adds C structures for the three components along with
> a few comments mostly lifted from the SDM to explain what they
> do.  This will allow us to check our structures against what the
> hardware tells us about the sizes of the components.
> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: x86@kernel.org
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Tim Chen <tim.c.chen@linux.intel.com>
> Cc: linux-kernel@vger.kernel.org
> ---
> 
>  b/arch/x86/include/asm/fpu/types.h |   43 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff -puN arch/x86/include/asm/fpu/types.h~avx-512-structs arch/x86/include/asm/fpu/types.h
> --- a/arch/x86/include/asm/fpu/types.h~avx-512-structs	2015-08-27 10:08:03.909740783 -0700
> +++ b/arch/x86/include/asm/fpu/types.h	2015-08-27 10:08:03.912740919 -0700
> @@ -129,6 +129,12 @@ enum xfeature_nr {
>  struct reg_128_bit {
>  	u8      regbytes[128/8];
>  };
> +struct reg_256_bit {
> +	u8	regbytes[256/8];
> +};
> +struct reg_512_bit {
> +	u8	regbytes[512/8];
> +};
>  
>  /*
>   * State component 2:
> @@ -177,6 +183,33 @@ struct mpx_bndcsr_state {
>  	};
>  } __packed;
>  
> +/* AVX-512 Components: */
> +
> +/*
> + * State component 5 is used for the 8 64-bit opmask registers
> + * k0–k7 (opmask state).
> + */
> +struct avx_512_opmask_state {
> +	u64 				opmask_reg[8];
> +} __packed;
> +
> +/*
> + * State component 6 is used for the upper 256 bits of the
> + * registers ZMM0–ZMM15. These 16 256-bit values are denoted
> + * ZMM0_H–ZMM15_H (ZMM_Hi256 state).
> + */
> +struct avx_512_zmm_uppers_state {
> +	struct reg_256_bit		zmm_upper[16];
> +} __packed;
> +
> +/*
> + * State component 7 is used for the 16 512-bit registers
> + * ZMM16–ZMM31 (Hi16_ZMM state).
> + */
> +struct avx_512_hi16_state {
> +	struct reg_512_bit		hi16_zmm[16];
> +} __packed;
> +
>  struct xstate_header {
>  	u64				xfeatures;
>  	u64				xcomp_bv;
> @@ -184,9 +217,13 @@ struct xstate_header {
>  } __attribute__((packed));
>  
>  /* New processor state extensions should be added here: */
> -#define XSTATE_RESERVE			(sizeof(struct ymmh_struct) + \
> -					 sizeof(struct mpx_bndreg_state) + \
> -					 sizeof(struct mpx_bndcsr_state)  )
> +#define XSTATE_RESERVE		(sizeof(struct ymmh_struct) 		+ \
> +				 sizeof(struct mpx_bndreg_state) 	+ \
> +				 sizeof(struct mpx_bndcsr_state) 	+ \
> +				 sizeof(struct avx_512_opmask_state) 	+ \
> +				 sizeof(struct avx_512_zmm_uppers_state) + \
> +				 sizeof(struct avx_512_hi16_state))

So in a previous mail I suggested getting rid of XSTATE_RESERVE, which removes the 
usage of the C structures..

What we could use these C structures for is to double check that the xstate size 
given by CPUID for that particular xstate feature is equal to the C structure size 
- or if it's larger, it's a multiple of the natural cache line size, or so?

Any 'failure' in such a check should be print-once and non-fatal, as in 90% of the 
cases I'd expect the kernel assumptions/checks to be buggy, not the hardware 
itself.

We should perhaps also print a gentle (non-warning) kernel message if we find an 
xfeature that the kernel doesn't know about, with its essential parameters: the 
bit number, the size and the offset position within the xstate buffer.

Thanks,

	Ingo

Re: [PATCH 05/11] x86, fpu: add helper xfeature_nr_enabled() instead of test_bit()

[Ingo Molnar]

* Dave Hansen <dave@sr71.net> wrote:

> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> We currently use test_bit() in a few places to see if an xfeature
> is enabled.  It ends up being a bit ugly because 'xfeatures_mask'
> is a u64 and test_bit wants an 'unsigned long' so it requires a
> cast.  The *_bit() functions are also techincally atomic, which
> we have no need for here.
> 
> So, remove the test_bit()s and replace with the new
> xfeature_nr_enabled() helper.

Small nit: please name it xfeature_enabled(), as it's shorter and already pretty 
unambiguous.

Thanks,

	Ingo

Re: [PATCH 04/11] x86, fpu: remove xfeature_nr

[Ingo Molnar]

* Dave Hansen <dave@sr71.net> wrote:

> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> xfeature_nr ended up being initialized too late for me to
> use it in the "xsave size sanity check" patch which is
> later in the series.  I tried to move around its initialization
> but realized that it was just as easy to get rid of it.
> 
> We only have 9 XFEATURES.  Instead of dynamically calculating
> and storing the last feature, just use the compile-time max:
> XFEATURES_NR_MAX.  Note that even with 'xfeatures_nr' we can
> had "holes" in the xfeatures_mask that we had to deal with.

s/we can had/we can have

Thanks,

	Ingo

Re: [PATCH 02/11] x86, fpu: rename xfeature_bit

[Ingo Molnar]

* Dave Hansen <dave@sr71.net> wrote:

> +++ b/arch/x86/include/asm/fpu/types.h	2015-08-27 10:08:01.572634311 -0700
> @@ -95,27 +95,31 @@ struct swregs_state {
>  /*
>   * List of XSAVE features Linux knows about:
>   */
> -enum xfeature_bit {
> -	XSTATE_BIT_FP,
> -	XSTATE_BIT_SSE,
> -	XSTATE_BIT_YMM,
> -	XSTATE_BIT_BNDREGS,
> -	XSTATE_BIT_BNDCSR,
> -	XSTATE_BIT_OPMASK,
> -	XSTATE_BIT_ZMM_Hi256,
> -	XSTATE_BIT_Hi16_ZMM,
> +enum xfeature_nr {
> +	XFEATURE_NR_FP,
> +	XFEATURE_NR_SSE,
> +	/*
> +	 * Values above here are "legacy states".
> +	 * Those below are "extended states".
> +	 */
> +	XFEATURE_NR_YMM,
> +	XFEATURE_NR_BNDREGS,
> +	XFEATURE_NR_BNDCSR,
> +	XFEATURE_NR_OPMASK,
> +	XFEATURE_NR_ZMM_Hi256,
> +	XFEATURE_NR_Hi16_ZMM,
>  
>  	XFEATURES_NR_MAX,
>  };
> +#define XSTATE_FP		(1 << XFEATURE_NR_FP)
> +#define XSTATE_SSE		(1 << XFEATURE_NR_SSE)
> +#define XSTATE_YMM		(1 << XFEATURE_NR_YMM)
> +#define XSTATE_BNDREGS		(1 << XFEATURE_NR_BNDREGS)
> +#define XSTATE_BNDCSR		(1 << XFEATURE_NR_BNDCSR)
> +#define XSTATE_OPMASK		(1 << XFEATURE_NR_OPMASK)
> +#define XSTATE_ZMM_Hi256	(1 << XFEATURE_NR_ZMM_Hi256)
> +#define XSTATE_Hi16_ZMM		(1 << XFEATURE_NR_Hi16_ZMM)

So I think this is still somewhat confusing.

'NR' is often used as a maximum kind of thing, not as a bit index.

So I think we should instead take up the existing conventions of the cpufeatures.h 
definitions which are pretty sane, and simply name the bit indices XFEATURE_XYZ:

enum xfeatures {
	XFEATURE_FP,
	XFEATURE_SSE,
	...
	XFEATURE_MAX
};

this way we ensure that bitmasks are visibly masks, i.e.:

	#define XFEATURE_MASK_FP		(1 << XFEATURE_FP)
	#define XFEATURE_MASK_SSE		(1 << XFEATURE_SSE)

it's slightly longer to write but unambiguous, and it also matches what we use for 
the x86 CPU ID feature definitions.

Similarly we could rename other mask fields from 'xstate' to 'xfeature_mask', to 
make it all consistent throughout.

What do you think?

Thanks,

	Ingo

Re: [PATCH 00/11] [v2] x86, fpu: XSAVE cleanups and sanity checks

[Ingo Molnar]

* Dave Hansen <dave@sr71.net> wrote:

> 
> Changes in v2:
>  * remove references to Processor Trace XSAVE state
>    (will defer to another patch set)
>  * Remove some cruft from last patch
>  * move last_good_offset fix in to the patch that
>    introduced it
> 
> 
> These patches make some updates to the x86 XSAVE code.
> 
> There are basically 5 things going on here:
>  * removal of the LWP (lightweight profiling) code
>  * naming and type cleanups
>  * removal of xfeatures_nr variable
>  * addition of AVX-512 C structures
>  * new sanity checks of XSAVE buffer sizing
> 
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: x86@kernel.org
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Tim Chen <tim.c.chen@linux.intel.com>
> Cc: linux-kernel@vger.kernel.org

So conceptually I like this series, modulo the handful of small suggestions I made 
in my review.

(The extra patch suggestions I made could be added later on in a separate series.)

Thanks,

	Ingo

Re: [PATCH 11/11] x86, fpu: check CPU-provided sizes against struct declarations

[Ingo Molnar]

* Dave Hansen <dave@sr71.net> wrote:

> @@ -447,6 +492,14 @@ static void do_extra_xstate_size_checks(
>  		paranoid_xstate_size += xfeature_size(i);
>  	}
>  	XSTATE_WARN_ON(paranoid_xstate_size != xstate_size);
> +	/*
> +	 * Basically, make sure that XSTATE_RESERVE has forced
> +	 * xregs_state to be large enough.  This is not fatal
> +	 * because we reserve a *lot* of extra room in the init
> +	 * task struct, but we should at least know we got it
> +	 * wrong.
> +	 */
> +	XSTATE_WARN_ON(xstate_size > sizeof(struct xregs_state));

So do we need to warn about this? arch_task_struct_size is already dynamic today.

The only problem would be the init task, which is allocated statically - can we 
fix that?

Thanks,

	Ingo

Re: [PATCH 09/11] x86, fpu: correct and check XSAVE xstate size calculations

[Dave Hansen]

On 08/27/2015 09:54 PM, Ingo Molnar wrote:
> 
> * Dave Hansen <dave@sr71.net> wrote:
> 
>> +static int xfeature_is_supervisor(int xfeature_nr)
>> +{
>> +	/*
>> +	 * We currently do not suport supervisor states, but if
>> +	 * we did, we could find out like this.
>> +	 *
>> +	 * SDM says: If state component i is a user state component,
>> +	 * ECX[0] return 0; if state component i is a supervisor
>> +	 * state component, ECX[0] returns 1.
>> +	u32 eax, ebx, ecx, edx;
>> +	cpuid_count(XSTATE_CPUID, xfeature_nr, &eax, &ebx, &ecx, &edx);
>> +	return !!(ecx & 1);
>> +	*/
>> +	return 0;
>> +}
> 
> So if this CPUID is documented to work, why not use it to sanity check things?
> 
> I.e. do something like:
> 
> 	u32 eax, ebx, ecx, edx;
> 
> 	cpuid_count(XSTATE_CPUID, xfeature_nr, &eax, &ebx, &ecx, &edx);
> 
> 	/* Linux doesn't support supervisor states (yet): */
> 	WARN_ON_ONCE(ecx & 1);
> 
> 	return 0;
> 
> That would give us a gentle way to double check our assumptions here.

Actually, the newest state that you will see in the wild is for
Processor Trace, and it _is_ a supervisor state.  However, we don't use
it in Linux for our Processor Trace support, and Andi says we probably
never will.

So we probably shouldn't warn on it.

Re: [PATCH 11/11] x86, fpu: check CPU-provided sizes against struct declarations

[Dave Hansen]

On 08/27/2015 10:25 PM, Ingo Molnar wrote:
> * Dave Hansen <dave@sr71.net> wrote:
>> @@ -447,6 +492,14 @@ static void do_extra_xstate_size_checks(
>>  		paranoid_xstate_size += xfeature_size(i);
>>  	}
>>  	XSTATE_WARN_ON(paranoid_xstate_size != xstate_size);
>> +	/*
>> +	 * Basically, make sure that XSTATE_RESERVE has forced
>> +	 * xregs_state to be large enough.  This is not fatal
>> +	 * because we reserve a *lot* of extra room in the init
>> +	 * task struct, but we should at least know we got it
>> +	 * wrong.
>> +	 */
>> +	XSTATE_WARN_ON(xstate_size > sizeof(struct xregs_state));
> 
> So do we need to warn about this? arch_task_struct_size is already dynamic today.

I'm unsure what _actually_ blew up, but I missed adding protection keys
and AVX-512 to XSTATE_RESERVE and the kernel crashed the first time I
did a non-init-state-PKRU XSAVE.

> The only problem would be the init task, which is allocated statically - can we 
> fix that?

We could theoretically make it dynamic, but I'm really not sure it's
worth the trouble.  I just removed the init_task=INIT_TASK()
initialization to see what would happen and something blew up early
(last I saw on the console was the "early console in setup code").

The current size of the non-XSAVE data in task_struct is ~2k.  The xsave
data is 800-something bytes, so say ~1k.  Our init_task ends up being
~6k, 3k of which is wasted.  On an AVX-512 CPU, that means 1k of waste.

>From how early things died, I'm going to go out on a limb and say that
we'll need to bootmem alloc our new dynamic init_task and probably can't
practically wait for the slab to show up.  Bootmem can only do full
pages, so our 6k can be trimmed to 4k.  On an AVX-512 CPU, the 6k goes
*up* to 8k.

It doesn't look like a fun exercise for 2k of memory savings.

Re: [PATCH 09/11] x86, fpu: correct and check XSAVE xstate size calculations

[Andi Kleen]

Dave Hansen <dave@sr71.net> writes:

>> 
>> That would give us a gentle way to double check our assumptions here.
>
> Actually, the newest state that you will see in the wild is for
> Processor Trace, and it _is_ a supervisor state.  However, we don't use
> it in Linux for our Processor Trace support, and Andi says we probably
> never will.
>
> So we probably shouldn't warn on it.

Yes we shouldn't. It would be unnecessary scary for people with old
kernels on newer hardware. Since supervisor state needs generally a
driver, it's harmless to ignore if there is none.

As to why Processor Trace is not using it:

The XSAVE support assumes that there is a single buffer for each
thread. But perf generally doesn't work this way, it usually has
only a single perf event per CPU per user, and when tracing
multiple threads on that CPU it inherits perf event buffers between
different threads. So XSAVE per thread cannot handle this inheritance
case directly.

Using multiple XSAVE areas (another one per perf event) would defeat
some of the state caching that the CPUs do.

In theory it would be possible to detect when inheritance is not used
and only use XSAVE in this case. But it's not clear that is common
enough and worth the trouble.

For now the manual MSR based switching seems to work well enough,
although it's more overhead.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only