[PATCH v2 0/4] Enabling Ring 3 MONITOR/MWAIT feature for Knights Landing

[PATCH v2 0/4] Enabling Ring 3 MONITOR/MWAIT feature for Knights Landing

[Grzegorz Andrejczuk]

These patches enable Intel Xeon Phi x200 feature to use MONITOR/MWAIT
instruction in ring 3 (userspace) Patches set MSR 0x140 for all logical CPUs.
Then expose it as CPU feature and introduces elf HWCAP capability for x86.
Reference:
https://software.intel.com/en-us/blogs/2016/10/06/intel-xeon-phi-product-family-x200-knl-user-mode-ring-3-monitor-and-mwait

v2:
Check MSR before wrmsrl
Shortened names
Used Word 3 for feature init_scattered_cpuid_features()
Fixed commit messages

Grzegorz Andrejczuk (4):
  Add R3MWAIT register and bit to msr-info.h
  Add enabling of the R3 MWAIT during boot for KNL
  Add hwcap2 for x86
  Add R3MWAIT to CPU features

 arch/x86/include/asm/cpufeatures.h |  2 ++
 arch/x86/include/asm/elf.h         |  7 +++++++
 arch/x86/include/asm/msr-index.h   |  5 +++++
 arch/x86/include/uapi/asm/hwcap.h  |  7 +++++++
 arch/x86/kernel/cpu/common.c       |  6 ++++++
 arch/x86/kernel/cpu/intel.c        | 27 +++++++++++++++++++++++++++
 arch/x86/kernel/cpu/scattered.c    |  5 +++++
 7 files changed, 59 insertions(+)
 create mode 100644 arch/x86/include/uapi/asm/hwcap.h

-- 
2.5.1

[PATCH v2 1/4] Add R3MWAIT register and bit to msr-info.h

[Grzegorz Andrejczuk]

Intel Xeon Phi x200 (codenamed Knights Landing) has MSR
MISC_THD_FEATURE_ENABLE 0x140.

Setting its 2nd bit make MONITOR and MWAIT instructions do not cause
invalid-opcode exception.

This commit adds this register prefixed by PHI and bit to msr-info.h
Reference:
https://software.intel.com/en-us/blogs/2016/10/06/intel-xeon-phi-product-family-x200-knl-user-mode-ring-3-monitor-and-mwait

Change-Id: If3b14c78f4e66d734e5a00921023a8c7cafc0cf3
Signed-off-by: Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com>
---
 arch/x86/include/asm/msr-index.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 56f4c66..df9d8d3 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -540,6 +540,11 @@
 #define MSR_IA32_MISC_ENABLE_IP_PREF_DISABLE_BIT	39
 #define MSR_IA32_MISC_ENABLE_IP_PREF_DISABLE		(1ULL << MSR_IA32_MISC_ENABLE_IP_PREF_DISABLE_BIT)
 
+/* Intel Xeon Phi x200 ring 3 MONITOR/MWAIT */
+#define MSR_PHI_MISC_THD_FEATURE	0x00000140
+#define MSR_PHI_MISC_THD_FEATURE_R3MWAIT_BIT	1
+#define MSR_PHI_MISC_THD_FEATURE_R3MWAIT	(1ULL << MSR_PHI_MISC_THD_FEATURE_R3MWAIT_BIT)
+
 #define MSR_IA32_TSC_DEADLINE		0x000006E0
 
 /* P4/Xeon+ specific */
-- 
2.5.1

[PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL

[Grzegorz Andrejczuk]

If processor is Intel Xeon Phi we enable user-level mwait feature.
Enabling this feature suppreses invalid-opcode error, when MONITOR/MWAIT
is called from ring 3.

Change-Id: I1c7defb99296b022790a068a6c725b3e860cd68c
Signed-off-by: Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com>
---
 arch/x86/kernel/cpu/intel.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index fcd484d..ac6df08 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -61,6 +61,14 @@ void check_mpx_erratum(struct cpuinfo_x86 *c)
 	}
 }
 
+static int phir3mwait = 1;
+static int __init phir3mwait_disable(char *value)
+{
+	phir3mwait = 0;
+	return 1;
+}
+__setup("intel-phir3mwait=disable", phir3mwait_disable);
+
 static void early_init_intel(struct cpuinfo_x86 *c)
 {
 	u64 misc_enable;
@@ -211,6 +219,25 @@ static void early_init_intel(struct cpuinfo_x86 *c)
 	}
 
 	check_mpx_erratum(c);
+
+	/*
+	* Setting ring 3 MONITOR/MWAIT for all threads
+	* when CPU is Xeon Phi Family x200
+	* This can be disabled with phir3mwait=disable cmdline switch.
+	* We preserve the reserved values and set only 2nd bit.
+	* Ref:
+	* https://software.intel.com/en-us/blogs/2016/10/06/intel-xeon-phi-product-family-x200-knl-user-mode-ring-3-monitor-and-mwait
+	*/
+	if (c->x86 == 6 &&
+	    c->x86_model == INTEL_FAM6_XEON_PHI_KNL &&
+	    phir3mwait) {
+		u64 prev;
+
+		rdmsrl(MSR_PHI_MISC_THD_FEATURE, prev);
+		if ((prev & MSR_PHI_MISC_THD_FEATURE_R3MWAIT) == 0)
+			wrmsrl(MSR_PHI_MISC_THD_FEATURE,
+			       prev | MSR_PHI_MISC_THD_FEATURE_R3MWAIT);
+	}
 }
 
 #ifdef CONFIG_X86_32
-- 
2.5.1

[PATCH v2 3/4] Add hwcap2 for x86

[Grzegorz Andrejczuk]

Add hwcap2 attribute for x86.
Reserve 1st bit of HWCAP2 for exposing Xeon Phi ring 3 monitor/mwait.
With this userspace apps can detect Ring 3 MONITOR/MWAIT instructions.

Change-Id: I37d0354d1e2b9594d7feebc2bacda30b68163efe
Signed-off-by: Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com>
---
 arch/x86/include/asm/elf.h        | 7 +++++++
 arch/x86/include/uapi/asm/hwcap.h | 7 +++++++
 arch/x86/kernel/cpu/common.c      | 3 +++
 3 files changed, 17 insertions(+)
 create mode 100644 arch/x86/include/uapi/asm/hwcap.h

diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
index e7f155c..a3f7856 100644
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -258,6 +258,13 @@ extern int force_personality32;
 
 #define ELF_HWCAP		(boot_cpu_data.x86_capability[CPUID_1_EDX])
 
+extern unsigned int elf_hwcap2;
+
+/* HWCAP2 supplies kernel enabled CPU feature, so that the application
+   can know that it can safely use them. The bits are defined in
+   uapi/asm/hwcap.h. */
+#define ELF_HWCAP2		elf_hwcap2
+
 /* This yields a string that ld.so will use to load implementation
    specific libraries for optimization.  This is more specific in
    intent than poking at uname or /proc/cpuinfo.
diff --git a/arch/x86/include/uapi/asm/hwcap.h b/arch/x86/include/uapi/asm/hwcap.h
new file mode 100644
index 0000000..d1f4f98
--- /dev/null
+++ b/arch/x86/include/uapi/asm/hwcap.h
@@ -0,0 +1,7 @@
+#ifndef _ASM_HWCAP_H
+#define _ASM_HWCAP_H 1
+
+/* Kernel enabled Ring 3 MWAIT for Xeon Phi*/
+#define HWCAP2_PHIR3MWAIT		(1 << 0)
+/* upto bit 31 free */
+#endif
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index bcc9ccc..93ffaa5 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -35,6 +35,7 @@
 #include <asm/desc.h>
 #include <asm/fpu/internal.h>
 #include <asm/mtrr.h>
+#include <asm/hwcap.h>
 #include <linux/numa.h>
 #include <asm/asm.h>
 #include <asm/bugs.h>
@@ -51,6 +52,8 @@
 
 #include "cpu.h"
 
+unsigned elf_hwcap2 __read_mostly;
+
 /* all of these masks are initialized in setup_cpu_local_masks() */
 cpumask_var_t cpu_initialized_mask;
 cpumask_var_t cpu_callout_mask;
-- 
2.5.1

[PATCH v2 4/4] Add R3MWAIT to CPU features

[Grzegorz Andrejczuk]

Add cpu feature for ring 3 monitor/mwait.

Change-Id: Iba4d20639efd8d3637d37db9294cbc43a98f009a
Signed-off-by: Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com>
---
 arch/x86/include/asm/cpufeatures.h | 2 ++
 arch/x86/kernel/cpu/common.c       | 3 +++
 arch/x86/kernel/cpu/scattered.c    | 5 +++++
 3 files changed, 10 insertions(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 92a8308..9caf9c4 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -71,6 +71,8 @@
 #define X86_FEATURE_RECOVERY	( 2*32+ 0) /* CPU in recovery mode */
 #define X86_FEATURE_LONGRUN	( 2*32+ 1) /* Longrun power control */
 #define X86_FEATURE_LRTI	( 2*32+ 3) /* LongRun table interface */
+/* non architectural Intel-defined CPU features not present in CPUID */
+#define X86_FEATURE_PHIR3MWAIT	(2*32+ 4)
 
 /* Other features, Linux-defined mapping, word 3 */
 /* This range is used for feature bits which conflict or are synthesized */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 93ffaa5..15fe27f 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1108,6 +1108,9 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 #endif
 	/* The boot/hotplug time assigment got cleared, restore it */
 	c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);
+
+	if (cpu_has(c, X86_FEATURE_PHIR3MWAIT))
+		elf_hwcap2 |= HWCAP2_PHIR3MWAIT;
 }
 
 /*
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index 8cb57df..e4ff3d0 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -29,6 +29,7 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)
 	u32 max_level;
 	u32 regs[4];
 	const struct cpuid_bit *cb;
+	u64 misc_thd_enable;
 
 	static const struct cpuid_bit cpuid_bits[] = {
 		{ X86_FEATURE_INTEL_PT,		CR_EBX,25, 0x00000007, 0 },
@@ -54,4 +55,8 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)
 		if (regs[cb->reg] & (1 << cb->bit))
 			set_cpu_cap(c, cb->feature);
 	}
+
+	rdmsrl(MSR_PHI_MISC_THD_FEATURE, misc_thd_enable);
+	if ((misc_thd_enable & MSR_PHI_MISC_THD_FEATURE_R3MWAIT) != 0)
+		set_cpu_cap(c, X86_FEATURE_PHIR3MWAIT);
 }
-- 
2.5.1

Re: [PATCH v2 4/4] Add R3MWAIT to CPU features

[Borislav Petkov]

On Wed, Oct 12, 2016 at 02:16:25PM +0200, Grzegorz Andrejczuk wrote:
> Add cpu feature for ring 3 monitor/mwait.
> 
> Change-Id: Iba4d20639efd8d3637d37db9294cbc43a98f009a

Please take your time when incorporating review comments - these
internal commit IDs have no meaning when submitting upstream so please
remove them. I already pointed that out previously...

Also, please do not set v3 immediately but give other people a couple of
days to take a look at those patches and give you review comments.

> Signed-off-by: Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com>
> ---
>  arch/x86/include/asm/cpufeatures.h | 2 ++
>  arch/x86/kernel/cpu/common.c       | 3 +++
>  arch/x86/kernel/cpu/scattered.c    | 5 +++++
>  3 files changed, 10 insertions(+)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 92a8308..9caf9c4 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -71,6 +71,8 @@
>  #define X86_FEATURE_RECOVERY	( 2*32+ 0) /* CPU in recovery mode */
>  #define X86_FEATURE_LONGRUN	( 2*32+ 1) /* Longrun power control */
>  #define X86_FEATURE_LRTI	( 2*32+ 3) /* LongRun table interface */
> +/* non architectural Intel-defined CPU features not present in CPUID */

No need for that comment - we have other synthetic CPUID bits already.

> +#define X86_FEATURE_PHIR3MWAIT	(2*32+ 4)
>  
>  /* Other features, Linux-defined mapping, word 3 */
>  /* This range is used for feature bits which conflict or are synthesized */
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 93ffaa5..15fe27f 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1108,6 +1108,9 @@ static void identify_cpu(struct cpuinfo_x86 *c)
>  #endif
>  	/* The boot/hotplug time assigment got cleared, restore it */
>  	c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);
> +
> +	if (cpu_has(c, X86_FEATURE_PHIR3MWAIT))
> +		elf_hwcap2 |= HWCAP2_PHIR3MWAIT;
>  }
>  
>  /*
> diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
> index 8cb57df..e4ff3d0 100644
> --- a/arch/x86/kernel/cpu/scattered.c
> +++ b/arch/x86/kernel/cpu/scattered.c
> @@ -29,6 +29,7 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)
>  	u32 max_level;
>  	u32 regs[4];
>  	const struct cpuid_bit *cb;
> +	u64 misc_thd_enable;
>  
>  	static const struct cpuid_bit cpuid_bits[] = {
>  		{ X86_FEATURE_INTEL_PT,		CR_EBX,25, 0x00000007, 0 },
> @@ -54,4 +55,8 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)
>  		if (regs[cb->reg] & (1 << cb->bit))
>  			set_cpu_cap(c, cb->feature);
>  	}
> +
> +	rdmsrl(MSR_PHI_MISC_THD_FEATURE, misc_thd_enable);
> +	if ((misc_thd_enable & MSR_PHI_MISC_THD_FEATURE_R3MWAIT) != 0)
> +		set_cpu_cap(c, X86_FEATURE_PHIR3MWAIT);

I didn't realize this bit is not a CPUID bit, sorry.

In that case, you can move that code into init_intel() in
arch/x86/kernel/cpu/intel.c, i.e., vendor-specific code.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH v2 4/4] Add R3MWAIT to CPU features

[Thomas Gleixner]

On Wed, 12 Oct 2016, Grzegorz Andrejczuk wrote:
> diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
> index 8cb57df..e4ff3d0 100644
> --- a/arch/x86/kernel/cpu/scattered.c
> +++ b/arch/x86/kernel/cpu/scattered.c
> @@ -29,6 +29,7 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)
>  	u32 max_level;
>  	u32 regs[4];
>  	const struct cpuid_bit *cb;
> +	u64 misc_thd_enable;
>  
>  	static const struct cpuid_bit cpuid_bits[] = {
>  		{ X86_FEATURE_INTEL_PT,		CR_EBX,25, 0x00000007, 0 },
> @@ -54,4 +55,8 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)
>  		if (regs[cb->reg] & (1 << cb->bit))
>  			set_cpu_cap(c, cb->feature);
>  	}
> +
> +	rdmsrl(MSR_PHI_MISC_THD_FEATURE, misc_thd_enable);

And what makes you sure that you can just use rdmsrl() unconditionally and
assume that the MSR is actually there? This breaks the world and some
more. Either make sure that this is only ran on PHI or simply use
rdmsrl_safe() which is safe everywhere,


> +	if ((misc_thd_enable & MSR_PHI_MISC_THD_FEATURE_R3MWAIT) != 0)

	if (misc_thd_enable & MSR_PHI_MISC_THD_FEATURE_R3MWAIT)
	
is entirely sufficient.

> +		set_cpu_cap(c, X86_FEATURE_PHIR3MWAIT);

Thanks,

	tglx

Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL

[Thomas Gleixner]

On Wed, 12 Oct 2016, Grzegorz Andrejczuk wrote:
>  
> +static int phir3mwait = 1;
> +static int __init phir3mwait_disable(char *value)

Can someone @Intel please tell everyone to stop this annoying habit of
glueing variable declarations without a newline to the function? And the
variable should be not in the middle of the code either. We have such stuff
on top of the file normaly if there is not a damned good reason to stick it
elsewhere.

That's just horrible and hard to read.

> +{
> +	phir3mwait = 0;
> +	return 1;
> +}
> +__setup("intel-phir3mwait=disable", phir3mwait_disable);
> +
>  static void early_init_intel(struct cpuinfo_x86 *c)
>  {
>  	u64 misc_enable;
> @@ -211,6 +219,25 @@ static void early_init_intel(struct cpuinfo_x86 *c)
>  	}
>  
>  	check_mpx_erratum(c);
> +
> +	/*
> +	* Setting ring 3 MONITOR/MWAIT for all threads
> +	* when CPU is Xeon Phi Family x200
> +	* This can be disabled with phir3mwait=disable cmdline switch.
> +	* We preserve the reserved values and set only 2nd bit.
> +	* Ref:
> +	* https://software.intel.com/en-us/blogs/2016/10/06/intel-xeon-phi-product-family-x200-knl-user-mode-ring-3-monitor-and-mwait
> +	*/
> +	if (c->x86 == 6 &&
> +	    c->x86_model == INTEL_FAM6_XEON_PHI_KNL &&
> +	    phir3mwait) {
> +		u64 prev;
> +
> +		rdmsrl(MSR_PHI_MISC_THD_FEATURE, prev);
> +		if ((prev & MSR_PHI_MISC_THD_FEATURE_R3MWAIT) == 0)
> +			wrmsrl(MSR_PHI_MISC_THD_FEATURE,
> +			       prev | MSR_PHI_MISC_THD_FEATURE_R3MWAIT);

The codingstyle here is just convoluted crap. What's wrong with writing it
proper?

	if (c->x86_model == INTEL_FAM6_XEON_PHI_KNL && phir3mwait) {
		u64 msr;

		rdmsrl(MSR_PHI_MISC_THD_FEATURE, msr);
		msr |= MSR_PHI_MISC_THD_FEATURE_R3MWAIT;
		wrmsrl(MSR_PHI_MISC_THD_FEATURE, msr);

	}

No horrible to read line breaks, no redundant check for x->x86 == 6 because
model cannot be INTEL_FAM6_XEON_PHI_KNL if x->x86 != 6. Also the
conditional is pointless as the feature is default disabled. And even if it
is enabled the extra msr write is not a problem at all. This is early init
code and not some hot path.

Thanks,

	tglx

Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL

[Thomas Gleixner]

On Wed, 12 Oct 2016, Grzegorz Andrejczuk wrote:
> +	/*
> +	* Setting ring 3 MONITOR/MWAIT for all threads
> +	* when CPU is Xeon Phi Family x200
> +	* This can be disabled with phir3mwait=disable cmdline switch.
> +	* We preserve the reserved values and set only 2nd bit.
> +	* Ref:
> +	* https://software.intel.com/en-us/blogs/2016/10/06/intel-xeon-phi-product-family-x200-knl-user-mode-ring-3-monitor-and-mwait

Please don't put links like this into comments. They are stale before this
hits Linus tree.

Thanks,

	tglx

Re: [PATCH v2 3/4] Add hwcap2 for x86

[Thomas Gleixner]

On Wed, 12 Oct 2016, Grzegorz Andrejczuk wrote:
> +/* HWCAP2 supplies kernel enabled CPU feature, so that the application
> +   can know that it can safely use them. The bits are defined in
> +   uapi/asm/hwcap.h. */

Please read: http://lkml.iu.edu/hypermail/linux/kernel/1607.1/00627.html

Re: [PATCH v2 3/4] Add hwcap2 for x86

[Thomas Gleixner]

On Wed, 12 Oct 2016, Grzegorz Andrejczuk wrote:
> +extern unsigned int elf_hwcap2;
> +unsigned elf_hwcap2 __read_mostly;

Oh well. Getting two lines straight is hard, right?

Thanks,

	tglx

Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL

[Dave Hansen]

On 10/12/2016 06:35 AM, Thomas Gleixner wrote:
> On Wed, 12 Oct 2016, Grzegorz Andrejczuk wrote:
>> > +	/*
>> > +	* Setting ring 3 MONITOR/MWAIT for all threads
>> > +	* when CPU is Xeon Phi Family x200
>> > +	* This can be disabled with phir3mwait=disable cmdline switch.
>> > +	* We preserve the reserved values and set only 2nd bit.
>> > +	* Ref:
>> > +	* https://software.intel.com/en-us/blogs/2016/10/06/intel-xeon-phi-product-family-x200-knl-user-mode-ring-3-monitor-and-mwait
> Please don't put links like this into comments. They are stale before this
> hits Linus tree.

Yeah, I'm worried about this too.  It's probably best to include some
version of the text in that blog entry, or that text itself.  That way,
when Intel reorganizes and culls old blog entries, we won't completely
lose our source.

Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL

[Dave Hansen]

On 10/12/2016 06:34 AM, Thomas Gleixner wrote:
>> > +	if (c->x86 == 6 &&
>> > +	    c->x86_model == INTEL_FAM6_XEON_PHI_KNL &&
>> > +	    phir3mwait) {
>> > +		u64 prev;
>> > +
>> > +		rdmsrl(MSR_PHI_MISC_THD_FEATURE, prev);
>> > +		if ((prev & MSR_PHI_MISC_THD_FEATURE_R3MWAIT) == 0)
>> > +			wrmsrl(MSR_PHI_MISC_THD_FEATURE,
>> > +			       prev | MSR_PHI_MISC_THD_FEATURE_R3MWAIT);
> The codingstyle here is just convoluted crap. What's wrong with writing it
> proper?
> 
> 	if (c->x86_model == INTEL_FAM6_XEON_PHI_KNL && phir3mwait) {
> 		u64 msr;
> 
> 		rdmsrl(MSR_PHI_MISC_THD_FEATURE, msr);
> 		msr |= MSR_PHI_MISC_THD_FEATURE_R3MWAIT;
> 		wrmsrl(MSR_PHI_MISC_THD_FEATURE, msr);
> 
> 	}
> 
> No horrible to read line breaks, no redundant check for x->x86 == 6 because
> model cannot be INTEL_FAM6_XEON_PHI_KNL if x->x86 != 6. Also the
> conditional is pointless as the feature is default disabled. And even if it
> is enabled the extra msr write is not a problem at all. This is early init
> code and not some hot path.

Hi Thomas,

We really do need to check for family=6 (c->x86==6).
INTEL_FAM6_XEON_PHI_KNL is just for the model and doesn't check family.
 It implies that you've already checked for family 6.

Looking at the name, though, it's pretty clear that the naming can
easily trip folks up.

I do think we've probably screwed up the way we use our 'struct
x86_cpu_id' mechanism.  Maybe we should be providing the
vendor/family/model sets from a common place to the drivers, instead of
making them all repeat it individually.

Like have a big header full of:

	DECLARE_CPU(INTEL_XEON_PHI_KNL, INTEL..., 6, MODEL_XYZ...);

Once we have that, everybody can just do:

	if(cpu_is(c, INTEL_XEON_PHI_KNL))
		...

and get all the checking they need.

Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL

[Dave Hansen]

On 10/12/2016 05:16 AM, Grzegorz Andrejczuk wrote:
> @@ -211,6 +219,25 @@ static void early_init_intel(struct cpuinfo_x86 *c)
>  	}
>  
>  	check_mpx_erratum(c);
> +
> +	/*
> +	* Setting ring 3 MONITOR/MWAIT for all threads
> +	* when CPU is Xeon Phi Family x200
> +	* This can be disabled with phir3mwait=disable cmdline switch.
> +	* We preserve the reserved values and set only 2nd bit.
> +	* Ref:
> +	* https://software.intel.com/en-us/blogs/2016/10/06/intel-xeon-phi-product-family-x200-knl-user-mode-ring-3-monitor-and-mwait
> +	*/
> +	if (c->x86 == 6 &&
> +	    c->x86_model == INTEL_FAM6_XEON_PHI_KNL &&
> +	    phir3mwait) {
> +		u64 prev;
> +
> +		rdmsrl(MSR_PHI_MISC_THD_FEATURE, prev);
> +		if ((prev & MSR_PHI_MISC_THD_FEATURE_R3MWAIT) == 0)
> +			wrmsrl(MSR_PHI_MISC_THD_FEATURE,
> +			       prev | MSR_PHI_MISC_THD_FEATURE_R3MWAIT);
> +	}
>  }

I'd really prefer that we put hunks like this into helpers just like the
nice check_mpx_erratum().  I know early_init_intel() looks a lot like
what you have here, but I think a little:

	probe_xeon_phi_mwait()

would be a lot nicer.

BTW, this hunk totally indicates how badly named 'phir3mwait' is.  Could
you please give it a sane name like 'phi_r3_mwait_disabled'?

Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL

[Thomas Gleixner]

On Wed, 12 Oct 2016, Dave Hansen wrote:

> On 10/12/2016 06:34 AM, Thomas Gleixner wrote:
> >> > +	if (c->x86 == 6 &&
> >> > +	    c->x86_model == INTEL_FAM6_XEON_PHI_KNL &&
> >> > +	    phir3mwait) {
> >> > +		u64 prev;
> >> > +
> >> > +		rdmsrl(MSR_PHI_MISC_THD_FEATURE, prev);
> >> > +		if ((prev & MSR_PHI_MISC_THD_FEATURE_R3MWAIT) == 0)
> >> > +			wrmsrl(MSR_PHI_MISC_THD_FEATURE,
> >> > +			       prev | MSR_PHI_MISC_THD_FEATURE_R3MWAIT);
> > The codingstyle here is just convoluted crap. What's wrong with writing it
> > proper?
> > 
> > 	if (c->x86_model == INTEL_FAM6_XEON_PHI_KNL && phir3mwait) {
> > 		u64 msr;
> > 
> > 		rdmsrl(MSR_PHI_MISC_THD_FEATURE, msr);
> > 		msr |= MSR_PHI_MISC_THD_FEATURE_R3MWAIT;
> > 		wrmsrl(MSR_PHI_MISC_THD_FEATURE, msr);
> > 
> > 	}
> > 
> > No horrible to read line breaks, no redundant check for x->x86 == 6 because
> > model cannot be INTEL_FAM6_XEON_PHI_KNL if x->x86 != 6. Also the
> > conditional is pointless as the feature is default disabled. And even if it
> > is enabled the extra msr write is not a problem at all. This is early init
> > code and not some hot path.
> 
> Hi Thomas,
> 
> We really do need to check for family=6 (c->x86==6).
> INTEL_FAM6_XEON_PHI_KNL is just for the model and doesn't check family.
>  It implies that you've already checked for family 6.

Indeed. It came to me after sending the mail and closing the notebook to
head out for more conference fun. I expected someone to notice it :)
 
> Looking at the name, though, it's pretty clear that the naming can
> easily trip folks up.
> 
> I do think we've probably screwed up the way we use our 'struct
> x86_cpu_id' mechanism.  Maybe we should be providing the
> vendor/family/model sets from a common place to the drivers, instead of
> making them all repeat it individually.
> 
> Like have a big header full of:
> 
> 	DECLARE_CPU(INTEL_XEON_PHI_KNL, INTEL..., 6, MODEL_XYZ...);
> 
> Once we have that, everybody can just do:
> 
> 	if(cpu_is(c, INTEL_XEON_PHI_KNL))
> 		...
> 
> and get all the checking they need.

Right, and we should do the following:

		__u8		x86;
		__u8		x86_vendor;
		__u8		x86_model;
		__u8		x86_mask;
		u32		x86_fvm;

set x86_fvm to family | vendor << 8 | model << 16; and then do the
comparison on that instead of checking 3 bytes in a row.

Thanks,

	tglx