[PATCH v11 0/5] Enabling Ring 3 MONITOR/MWAIT feature for Knights Landing

[PATCH v11 0/5] Enabling Ring 3 MONITOR/MWAIT feature for Knights Landing

[Grzegorz Andrejczuk]

Following patches enable the use of the feature that allows
the Intel Xeon Phi x200 devices to use MONITOR/MWAIT instructions
outside ring 0. It allows userspace applications to use
more efficient synchronization operations, which improves performance
and energy efficiency.

v11:
Removed warning from 32-bit build
Removed "This patch" from commit messages

v10:
Included Piotr's patch for Knights Mill
Included Dave's comments from internal review
Rewritten commit messages
Remove x86_64 requirement
Fixed kernel boot parameter description
Used set_bit to update HWCAP2 bit
Rebased to kernel 4.9

v9:
Removed PHI from defines

v8:
Fixed commit messages
Removed logging
Used msr_set/clear_bit functions instesd of wrmsrl
Fixed documentation
Renamed HWCAP2_PHIR3MWAIT to HWCAP2_RING3MWAIT

v7:
Change order of the patches, with this code looks cleaner.
Changed the name of MSR to MSR_MISC_FEATURE_ENABLES.
Used Word 3 25th bit to expose feature.

v6: 

v5:
When phir3mwait=disable is cmdline switch off r3 mwait feature
Fix typos

v4:
Wrapped the enabling code by CONFIG_X86_64
Add documentation for phir3mwait=disable cmdline switch
Move probe_ function call from early_intel_init to intel_init
Fixed commit messages

v3:
Included Daves and Thomas comments

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

Grzegorz Andrejczuk (4):
  x86/msr: add MSR_MISC_FEATURE_ENABLES and RING3MWAIT bit
  x86/elf: add HWCAP2 to expose ring 3 MONITOR/MWAIT
  x86/cpufeature: add RING3MWAIT to CPU features
  x86/cpufeature: enable RING3MWAIT for Knights Landing

Piotr Luc (1):
  x86/cpufeature: enable RING3MWAIT for Knights Mill

 Documentation/kernel-parameters.txt |  3 +++
 arch/x86/include/asm/cpufeatures.h  |  2 +-
 arch/x86/include/asm/elf.h          |  9 +++++++++
 arch/x86/include/asm/msr-index.h    |  6 ++++++
 arch/x86/include/uapi/asm/hwcap2.h  |  7 +++++++
 arch/x86/kernel/cpu/common.c        |  3 +++
 arch/x86/kernel/cpu/intel.c         | 36 ++++++++++++++++++++++++++++++++++++
 7 files changed, 65 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/include/uapi/asm/hwcap2.h

-- 
2.5.1

[PATCH v11 1/5] x86/msr: add MSR_MISC_FEATURE_ENABLES and RING3MWAIT bit

[Grzegorz Andrejczuk]

Define new MSR MISC_FEATURE_ENABLES (0x140).
On supported CPUs if bit 1 of this new register is set,
then calling MONITOR and MWAIT instructions outside of ring 0 will
not cause invalid-opcode exception.

The MSR MISC_FEATURE_ENABLES is not yet documented in the SDM. Here is
the relevant documentation:

Hex   Dec  Name                     Scope
140H  320  MISC_FEATURE_ENABLES     Thread
           0    Reserved
           1    If set to 1, the MONITOR and MWAIT instructions do not
                cause invalid-opcode exceptions when executed with CPL > 0
                or in virtual-8086 mode. If MWAIT is executed when CPL > 0
                or in virtual-8086 mode, and if EAX indicates a C-state
                other than C0 or C1, the instruction operates as if EAX
                indicated the C-state C1.
           63:2 Reserved

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

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 78f3760..55ffae0 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -539,6 +539,12 @@
 #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)
 
+/* MISC_FEATURE_ENABLES non-architectural features */
+#define MSR_MISC_FEATURE_ENABLES		0x00000140
+
+#define MSR_MISC_FEATURE_ENABLES_RING3MWAIT_BIT		1
+#define MSR_MISC_FEATURE_ENABLES_RING3MWAIT		(1ULL << MSR_MISC_FEATURE_ENABLES_RING3MWAIT_BIT)
+
 #define MSR_IA32_TSC_DEADLINE		0x000006E0
 
 /* P4/Xeon+ specific */
-- 
2.5.1

[PATCH v11 2/5] x86/elf: add HWCAP2 to expose ring 3 MONITOR/MWAIT

[Grzegorz Andrejczuk]

Introduce ELF_HWCAP2 variable for x86 and reserve its bit 0
to expose the ring 3 MONITOR/MWAIT.

HWCAP variables contain bitmask which can be used by userspace
applications to detect what instruction sets are supported by CPU.
On x86 architecture information about CPU capabilities can be checked
via CPUID instructions, unfortunately presence of ring 3 MONITOR/MWAIT
feature cannot be checked this way. ELF_HWCAP cannot be used as well,
because on x86 it is set to CPUID[1].EDX which means that all bits
are reserved there.

HWCAP2 approach was chosen because it reuses existing solution present
in other architectures, so only minor modifications are required to the
kernel and userspace applications. When ELF_HWCAP2 is defined
kernel maps it to AT_HWCAP2 during the start of the application.
This way the ring 3 MONITOR/MWAIT feature can be detected using getauxval()
API in a simple and fast manner.

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

diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
index e7f155c..59703aa 100644
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -258,6 +258,15 @@ extern int force_personality32;
 
 #define ELF_HWCAP		(boot_cpu_data.x86_capability[CPUID_1_EDX])
 
+extern unsigned int elf_hwcap2;
+
+/*
+ * HWCAP2 supplies mask with kernel enabled CPU features, so that
+ * the application can discover that it can safely use them.
+ * The bits are defined in uapi/asm/hwcap2.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/hwcap2.h b/arch/x86/include/uapi/asm/hwcap2.h
new file mode 100644
index 0000000..0bd2be5
--- /dev/null
+++ b/arch/x86/include/uapi/asm/hwcap2.h
@@ -0,0 +1,7 @@
+#ifndef _ASM_X86_HWCAP2_H
+#define _ASM_X86_HWCAP2_H
+
+/* MONITOR/MWAIT enabled in Ring 3 */
+#define HWCAP2_RING3MWAIT		(1 << 0)
+
+#endif
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index cc9e980..217697b 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/hwcap2.h>
 #include <linux/numa.h>
 #include <asm/asm.h>
 #include <asm/bugs.h>
@@ -51,6 +52,8 @@
 
 #include "cpu.h"
 
+unsigned int 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 v11 3/5] x86/cpufeature: add RING3MWAIT to CPU features

[Grzegorz Andrejczuk]

Add software-defined CPUID bit for the non-architectural ring 3
MONITOR/MWAIT feature.

Signed-off-by: Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com>
---
 arch/x86/include/asm/cpufeatures.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index a396292..dc0255e 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -100,7 +100,7 @@
 #define X86_FEATURE_XTOPOLOGY	( 3*32+22) /* cpu topology enum extensions */
 #define X86_FEATURE_TSC_RELIABLE ( 3*32+23) /* TSC is known to be reliable */
 #define X86_FEATURE_NONSTOP_TSC	( 3*32+24) /* TSC does not stop in C states */
-/* free, was #define X86_FEATURE_CLFLUSH_MONITOR ( 3*32+25) * "" clflush reqd with monitor */
+#define X86_FEATURE_RING3MWAIT	( 3*32+25) /* ring 3 MONITOR/MWAIT */
 #define X86_FEATURE_EXTD_APICID	( 3*32+26) /* has extended APICID (8 bits) */
 #define X86_FEATURE_AMD_DCM     ( 3*32+27) /* multi-node processor */
 #define X86_FEATURE_APERFMPERF	( 3*32+28) /* APERFMPERF */
-- 
2.5.1

[PATCH v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights Landing

[Grzegorz Andrejczuk]

Enable ring 3 MONITOR/MWAIT for Intel Xeon Phi x200
codenamed Knights Landing.

The patch:
- Sets CPU feature X86_FEATURE_RING3MWAIT.
- Sets HWCAP2_RING3MWAIT bit in ELF_HWCAP2.
- Adds the ring3mwait=disable command line parameter.
- Sets bit 1 of the MSR MISC_FEATURE_ENABLES or clears it when
  the ring3mwait=disable command line parameter is used.

Presence of this feature cannot be detected automatically
(by reading any other MSR) therefore it is required to
explicitly check for the family and model of the CPU before attempting
to enable it.

Signed-off-by: Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com>
---
 Documentation/kernel-parameters.txt |  3 +++
 arch/x86/kernel/cpu/intel.c         | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 37babf9..c8bca65 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3734,6 +3734,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 	rhash_entries=	[KNL,NET]
 			Set number of hash buckets for route cache
 
+	ring3mwait=	[KNL]
+		disable	Disable ring 3 MONITOR/MWAIT feature on supported CPUs.
+
 	ro		[KNL] Mount root device read-only on boot
 
 	rodata=		[KNL]
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index fcd484d..9d07bee 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -14,6 +14,8 @@
 #include <asm/bugs.h>
 #include <asm/cpu.h>
 #include <asm/intel-family.h>
+#include <asm/hwcap2.h>
+#include <asm/elf.h>
 
 #ifdef CONFIG_X86_64
 #include <linux/topology.h>
@@ -61,6 +63,36 @@ void check_mpx_erratum(struct cpuinfo_x86 *c)
 	}
 }
 
+static int ring3mwait_disabled __read_mostly;
+
+static int __init ring3mwait_disable(char *__unused)
+{
+	ring3mwait_disabled = 1;
+	return 1;
+}
+__setup("ring3mwait=disable", ring3mwait_disable);
+
+static void probe_xeon_phi_r3mwait(struct cpuinfo_x86 *c)
+{
+	/*
+	 * Ring 3 MONITOR/MWAIT feature cannot be detected without
+	 * cpu model and family comparison.
+	 */
+	if (c->x86 != 6 || c->x86_model != INTEL_FAM6_XEON_PHI_KNL)
+		return;
+
+	if (ring3mwait_disabled) {
+		msr_clear_bit(MSR_MISC_FEATURE_ENABLES,
+			      MSR_MISC_FEATURE_ENABLES_RING3MWAIT_BIT);
+		return;
+	}
+
+	msr_set_bit(MSR_MISC_FEATURE_ENABLES,
+		    MSR_MISC_FEATURE_ENABLES_RING3MWAIT_BIT);
+	set_cpu_cap(c, X86_FEATURE_RING3MWAIT);
+	set_bit(HWCAP2_RING3MWAIT, (unsigned long *)&ELF_HWCAP2);
+}
+
 static void early_init_intel(struct cpuinfo_x86 *c)
 {
 	u64 misc_enable;
@@ -565,6 +597,8 @@ static void init_intel(struct cpuinfo_x86 *c)
 		detect_vmx_virtcap(c);
 
 	init_intel_energy_perf(c);
+
+	probe_xeon_phi_r3mwait(c);
 }
 
 #ifdef CONFIG_X86_32
-- 
2.5.1

[PATCH v11 5/5] x86/cpufeature: enable RING3MWAIT for Knights Mill

[Grzegorz Andrejczuk]

From: Piotr Luc <piotr.luc@intel.com>

Enable ring 3 MONITOR/MWAIT for Intel Xeon Phi
codenamed Knights Mill. We can't guarantee that this (KNM)
will be the last CPU model that needs this hack.
But, we do recognize that this is far from optimal,
and there is an effort to ensure we don't keep doing
extending this hack forever

Signed-off-by: Piotr Luc <piotr.luc@intel.com>
---
 arch/x86/kernel/cpu/intel.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 9d07bee..a1c28ea 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -78,7 +78,9 @@ static void probe_xeon_phi_r3mwait(struct cpuinfo_x86 *c)
 	 * Ring 3 MONITOR/MWAIT feature cannot be detected without
 	 * cpu model and family comparison.
 	 */
-	if (c->x86 != 6 || c->x86_model != INTEL_FAM6_XEON_PHI_KNL)
+	if (c->x86 != 6 ||
+	   (c->x86_model != INTEL_FAM6_XEON_PHI_KNL &&
+	    c->x86_model != INTEL_FAM6_XEON_PHI_KNM))
 		return;
 
 	if (ring3mwait_disabled) {
-- 
2.5.1

[Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights Landing

[Grzegorz Andrejczuk]

Enable ring 3 MONITOR/MWAIT for Intel Xeon Phi x200
codenamed Knights Landing.

The patch:
- Sets CPU feature X86_FEATURE_RING3MWAIT.
- Sets HWCAP2_RING3MWAIT bit in ELF_HWCAP2.
- Adds the ring3mwait=disable command line parameter.
- Sets bit 1 of the MSR MISC_FEATURE_ENABLES or clears it when
  the ring3mwait=disable command line parameter is used.

Presence of this feature cannot be detected automatically
(by reading any other MSR) therefore it is required to
explicitly check for the family and model of the CPU before attempting
to enable it.

Signed-off-by: Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com>
---
 Documentation/kernel-parameters.txt |  4 ++++
 arch/x86/kernel/cpu/intel.c         | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 37babf9..b8b4ac8 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3734,6 +3734,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 	rhash_entries=	[KNL,NET]
 			Set number of hash buckets for route cache
 
+	ring3mwait=disable
+			[KNL] Disable ring 3 MONITOR/MWAIT feature on supported
+			CPUs.
+
 	ro		[KNL] Mount root device read-only on boot
 
 	rodata=		[KNL]
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index fcd484d..9d07bee 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -14,6 +14,8 @@
 #include <asm/bugs.h>
 #include <asm/cpu.h>
 #include <asm/intel-family.h>
+#include <asm/hwcap2.h>
+#include <asm/elf.h>
 
 #ifdef CONFIG_X86_64
 #include <linux/topology.h>
@@ -61,6 +63,36 @@ void check_mpx_erratum(struct cpuinfo_x86 *c)
 	}
 }
 
+static int ring3mwait_disabled __read_mostly;
+
+static int __init ring3mwait_disable(char *__unused)
+{
+	ring3mwait_disabled = 1;
+	return 1;
+}
+__setup("ring3mwait=disable", ring3mwait_disable);
+
+static void probe_xeon_phi_r3mwait(struct cpuinfo_x86 *c)
+{
+	/*
+	 * Ring 3 MONITOR/MWAIT feature cannot be detected without
+	 * cpu model and family comparison.
+	 */
+	if (c->x86 != 6 || c->x86_model != INTEL_FAM6_XEON_PHI_KNL)
+		return;
+
+	if (ring3mwait_disabled) {
+		msr_clear_bit(MSR_MISC_FEATURE_ENABLES,
+			      MSR_MISC_FEATURE_ENABLES_RING3MWAIT_BIT);
+		return;
+	}
+
+	msr_set_bit(MSR_MISC_FEATURE_ENABLES,
+		    MSR_MISC_FEATURE_ENABLES_RING3MWAIT_BIT);
+	set_cpu_cap(c, X86_FEATURE_RING3MWAIT);
+	set_bit(HWCAP2_RING3MWAIT, (unsigned long *)&ELF_HWCAP2);
+}
+
 static void early_init_intel(struct cpuinfo_x86 *c)
 {
 	u64 misc_enable;
@@ -565,6 +597,8 @@ static void init_intel(struct cpuinfo_x86 *c)
 		detect_vmx_virtcap(c);
 
 	init_intel_energy_perf(c);
+
+	probe_xeon_phi_r3mwait(c);
 }
 
 #ifdef CONFIG_X86_32
-- 
2.5.1

Re: [PATCH v11 1/5] x86/msr: add MSR_MISC_FEATURE_ENABLES and RING3MWAIT bit

[Thomas Gleixner]

On Tue, 20 Dec 2016, Grzegorz Andrejczuk wrote:
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 78f3760..55ffae0 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -539,6 +539,12 @@
>  #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)
>  
> +/* MISC_FEATURE_ENABLES non-architectural features */
> +#define MSR_MISC_FEATURE_ENABLES		0x00000140
> +
> +#define MSR_MISC_FEATURE_ENABLES_RING3MWAIT_BIT		1
> +#define MSR_MISC_FEATURE_ENABLES_RING3MWAIT		(1ULL << MSR_MISC_FEATURE_ENABLES_RING3MWAIT_BIT)
> +

This last define is not used anywhere. I told you before, but addressing my
review comments completely is an unduly burden, or what?

Thanks,

	tglx

Re: [Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights Landing

[Thomas Gleixner]

On Tue, 20 Dec 2016, Grzegorz Andrejczuk wrote:

So how am I supposed to know which version of these patches is the right
one? Both subject lines are identical. 

> Enable ring 3 MONITOR/MWAIT for Intel Xeon Phi x200
> codenamed Knights Landing.
> 
> The patch:

>From your cover letter:

     Removed "This patch" from commit messages

Why is 'The patch any better' ?

> - Sets CPU feature X86_FEATURE_RING3MWAIT.
> - Sets HWCAP2_RING3MWAIT bit in ELF_HWCAP2.
> - Adds the ring3mwait=disable command line parameter.
> - Sets bit 1 of the MSR MISC_FEATURE_ENABLES or clears it when
>   the ring3mwait=disable command line parameter is used.

Changelogs should not describe WHAT the patch is doing. We can see that
from the patch. Changelogs should describe the WHY and CONCEPTS not
implementation details.

> +static void probe_xeon_phi_r3mwait(struct cpuinfo_x86 *c)
> +{
> +	/*
> +	 * Ring 3 MONITOR/MWAIT feature cannot be detected without
> +	 * cpu model and family comparison.
> +	 */
> +	if (c->x86 != 6 || c->x86_model != INTEL_FAM6_XEON_PHI_KNL)
> +		return;
> +
> +	if (ring3mwait_disabled) {
> +		msr_clear_bit(MSR_MISC_FEATURE_ENABLES,
> +			      MSR_MISC_FEATURE_ENABLES_RING3MWAIT_BIT);
> +		return;
> +	}
> +
> +	msr_set_bit(MSR_MISC_FEATURE_ENABLES,
> +		    MSR_MISC_FEATURE_ENABLES_RING3MWAIT_BIT);
> +	set_cpu_cap(c, X86_FEATURE_RING3MWAIT);
> +	set_bit(HWCAP2_RING3MWAIT, (unsigned long *)&ELF_HWCAP2);

>From your cover letter:

     "Removed warning from 32-bit build"

First of all, the warning

   arch/x86/include/asm/bitops.h:72:1: note: expected 'volatile long unsigned int *'
but argument is of type 'unsigned int *'
    set_bit(long nr, volatile unsigned long *addr)

is not at all 32bit specific.

Handing an unsigned int pointer to a function which expects a unsigned long
is even more wrong on 64bit.

So now for your 'removal fix': It's just as sloppy as anything else what
I've seen from you before.

Handing a typecasted unsigned int pointer to a function which expects an
unsigned long pointer is just broken and a clear sign of careless
tinkering.

The only reason why this 'works' is because x86 is a little endian
architecture and the bit number is a constant and set_bit gets translated
it into:

    orb 0x02, 0x0(%rip) 

Now if you look really close to that disassembly then you might notice,
that this sets bit 1 and not as you tell in patch 2/5:

   "Introduce ELF_HWCAP2 variable for x86 and reserve its bit 0 to expose
    the ring 3 MONITOR/MWAIT."

So why does it not set bit 0?

Simply because you hand in HWCAP2_RING3MWAIT as bit number, which is
defined as:

+#define HWCAP2_RING3MWAIT              (1 << 0)

Crap, crap, crap.

What's so !$@&*(? wrong with doing the simple, obvious and correct:

       ELF_HWCAP2 |= HWCAP2_RING3MWAIT;

C is really hard, right?

Yours grumpy

      tglx



       

RE: [PATCH v11 1/5] x86/msr: add MSR_MISC_FEATURE_ENABLES and RING3MWAIT bit

[Andrejczuk, Grzegorz]

>
> This last define is not used anywhere. I told you before, but addressing my review comments completely is an unduly burden, or what?
>

I left to be consistent with the rest of the file. I will remove the line. 

RE: [Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights Landing

[Andrejczuk, Grzegorz]

>
> Changelogs should not describe WHAT the patch is doing. We can see that from the patch. Changelogs should describe the WHY and CONCEPTS not implementation details.

So enable for Ring 3 MWAIT for Knights Landing + explanation of model comparison and potential issues below. Should be Ok. 

>From your cover letter:
>
>     "Removed warning from 32-bit build"
>
>First of all, the warning
>
 >  arch/x86/include/asm/bitops.h:72:1: note: expected 'volatile long unsigned int *'
>but argument is of type 'unsigned int *'
 >   set_bit(long nr, volatile unsigned long *addr)
>
>is not at all 32bit specific.
>
>Handing an unsigned int pointer to a function which expects a unsigned long is even more wrong on 64bit.
>
>So now for your 'removal fix': It's just as sloppy as anything else what I've seen from you before.
>
>Handing a typecasted unsigned int pointer to a function which expects an unsigned long pointer is just broken and a clear sign of careless tinkering.

I thought this to be 32 issue because it popped up in 32 build. The reason for this is probably that sizeof(int) is equal to sizeof(long) on x64.
I used the cast following set_cpu_cap define which does exactly the same thing with u32* type. 
Is using unsigned long would be OK.  

>The only reason why this 'works' is because x86 is a little endian architecture and the bit number is a constant and set_bit gets translated it into:
>
>    orb 0x02, 0x0(%rip) 
>
>Now if you look really close to that disassembly then you might notice, that this sets bit 1 and not as you tell in patch 2/5:
>
>  "Introduce ELF_HWCAP2 variable for x86 and reserve its bit 0 to expose
>    the ring 3 MONITOR/MWAIT."
>
> So why does it not set bit 0?
>
> Simply because you hand in HWCAP2_RING3MWAIT as bit number, which is defined as:
>
>+#define HWCAP2_RING3MWAIT              (1 << 0)
>
>Crap, crap, crap.
>
> What's so !$@&*(? wrong with doing the simple, obvious and correct:
>
>       ELF_HWCAP2 |= HWCAP2_RING3MWAIT;
>
> C is really hard, right?

I used set_bit because I wanted to be sure that this operation to be done atomically. There might be data race when multiple values of ELF_HWCAP2 will be set by multiple threads.
Thanks for the bit 1 issue I missed that. I can define HWCAP_RING3MWAIT_BIT 0 and set it by set_bit?
I could use OR operator as there are no other HWCAP2 values.
I would choose option 1 but as you have seen I have some tendency to write sloppy code and not respond to emails.
But I am willing to change.

Best Regards,
Grzegorz

       

RE: [Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights Landing

[Thomas Gleixner]

On Thu, 22 Dec 2016, Andrejczuk, Grzegorz wrote:

> > Handing a typecasted unsigned int pointer to a function which expects an
> > unsigned long pointer is just broken and a clear sign of careless
> > tinkering.
> 
> I thought this to be 32 issue because it popped up in 32 build.

It also warns on the 64bit build.

> The reason for this is probably that sizeof(int) is equal to sizeof(long)
> on x64.

Huch? sizeof(int) is equal to sizeof(long) on 32bit, but definitely not on 64 bit.

> I used the cast following set_cpu_cap define which does exactly the same
> thing with u32* type.

set_cpu_cap() operates on 'c->x86_capability', which is an array of u32 and
the bit numbers are linear. That works because x86 is little endian. It's
not pretty, but it's not a template for general abuse.

> I used set_bit because I wanted to be sure that this operation to be
> done atomically. There might be data race when multiple values of
> ELF_HWCAP2 will be set by multiple threads.

Touching ELF_HWCAP2 from anything else than the boot cpu is pointless
anyway. This should be done once.

Aside of that CPU bringup and therefor the call to init_intel() is
serialized by the cpu hotplug code and if we lift that, then ELF_HWCAP2
will be the least of our worries.

Thanks,

	tglx

RE: [Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights Landing

[Andrejczuk, Grzegorz]

>It also warns on the 64bit build.

It is, I missed it. I changed the type of elf_hwcap2 to long unsigned int. 

>> I used set_bit because I wanted to be sure that this operation to be 
>> done atomically. There might be data race when multiple values of
>> ELF_HWCAP2 will be set by multiple threads.
>
> Touching ELF_HWCAP2 from anything else than the boot cpu is pointless anyway. This should be done once.

MSR (0x140) is thread specific it has to be set for all physical threads. Also the kernel parameters are handled after boot cpu is initialized and this make disabling harder.

> Aside of that CPU bringup and therefor the call to init_intel() is serialized by the cpu hotplug code and if we lift that, then ELF_HWCAP2 will be the least of our worries.

I do not understand.  

RE: [Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights Landing

[Thomas Gleixner]

On Thu, 22 Dec 2016, Andrejczuk, Grzegorz wrote:

> >It also warns on the 64bit build.
> 
> It is, I missed it. I changed the type of elf_hwcap2 to long unsigned int. 

Changing types to match is the proper solution to all problems? You cannot
just change types to make the compiler happy. You have to check what type
is expected for it in the places which consume it, including compat mode.

> >> I used set_bit because I wanted to be sure that this operation to be 
> >> done atomically. There might be data race when multiple values of
> >> ELF_HWCAP2 will be set by multiple threads.
> >
> > Touching ELF_HWCAP2 from anything else than the boot cpu is pointless
> > anyway. This should be done once.
> 
> MSR (0x140) is thread specific it has to be set for all physical
> threads. Also the kernel parameters are handled after boot cpu is
> initialized and this make disabling harder.

What has the MSR to do with ELF_HWCAP2? ELF_HWCAP2 is a system global
variable. The MSR is of course per hardware thread.

> > Aside of that CPU bringup and therefor the call to init_intel() is
> > serialized by the cpu hotplug code and if we lift that, then ELF_HWCAP2
> > will be the least of our worries.

CPU bringup is serialized, i.e. init_intel() cannot run concurrently on
different CPUs.

If we would remove that serialization then we would have more serious
problems than the concurrent access to ELF_HWCAP2.

Thanks,

	tglx

RE: [Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights Landing

[Andrejczuk, Grzegorz]

On Thursday, December 22, 2016 3:21 PM Thomas Gleixner wrote:
> Changing types to match is the proper solution to all problems? You cannot
> just change types to make the compiler happy. You have to check what type
> is expected for it in the places which consume it, including compat mode.

HWCAP is u32 on x86 architecture, HWCAP2 on other architectures is uint.
I think I will be consistent with architecture and use u32 and bit OR operator.
It should be handled same way as ELF_HWCAP in user space and in 32 bits.
Please let me know if you agree to this solution. 

> What has the MSR to do with ELF_HWCAP2? ELF_HWCAP2 is a system global
> variable. The MSR is of course per hardware thread.

You are right - nothing. I can set HWCAP2 once for boot cpu.
To achieve that I need to distinguish boot cpu in probe_xeon_phi_r3mwait.
>From looking around the kernel code I suspect it can be done like below:

if (c == &boot_cpu_data)
        ELF_HWCAP2 |= HWCAP2_RING3MWAIT;

If there's a better way, please let me know.  

> CPU bringup is serialized, i.e. init_intel() cannot run concurrently on
> different CPUs.

Thank you for your explanation, time and effort.
Merry Christmas,
Grzegorz

Re: [PATCH v11 0/5] Enabling Ring 3 MONITOR/MWAIT feature for Knights Landing

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 700 bytes --]

On Tue 2016-12-20 14:48:41, Grzegorz Andrejczuk wrote:
> Following patches enable the use of the feature that allows
> the Intel Xeon Phi x200 devices to use MONITOR/MWAIT instructions
> outside ring 0. It allows userspace applications to use
> more efficient synchronization operations, which improves performance
> and energy efficiency.

What kind of security holes does it bring us?

rdseed can be used for two processes to communicate when they should
not (serious problem for android). Can this be used for something similar?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v11 0/5] Enabling Ring 3 MONITOR/MWAIT feature for Knights Landing

[Thomas Gleixner]

On Tue, 3 Jan 2017, Pavel Machek wrote:

> On Tue 2016-12-20 14:48:41, Grzegorz Andrejczuk wrote:
> > Following patches enable the use of the feature that allows
> > the Intel Xeon Phi x200 devices to use MONITOR/MWAIT instructions
> > outside ring 0. It allows userspace applications to use
> > more efficient synchronization operations, which improves performance
> > and energy efficiency.
> 
> What kind of security holes does it bring us?
> 
> rdseed can be used for two processes to communicate when they should
> not (serious problem for android). Can this be used for something similar?

No. The monitor address must be in the virtual address space of the
caller, so it's like any other 'read' operation.

Thanks,

	tglx

Re: [PATCH v11 0/5] Enabling Ring 3 MONITOR/MWAIT feature for Knights Landing

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1383 bytes --]

On Mon 2017-01-09 09:59:42, Thomas Gleixner wrote:
> On Tue, 3 Jan 2017, Pavel Machek wrote:
> 
> > On Tue 2016-12-20 14:48:41, Grzegorz Andrejczuk wrote:
> > > Following patches enable the use of the feature that allows
> > > the Intel Xeon Phi x200 devices to use MONITOR/MWAIT instructions
> > > outside ring 0. It allows userspace applications to use
> > > more efficient synchronization operations, which improves performance
> > > and energy efficiency.
> > 
> > What kind of security holes does it bring us?
> > 
> > rdseed can be used for two processes to communicate when they should
> > not (serious problem for android). Can this be used for something similar?
> 
> No. The monitor address must be in the virtual address space of the
> caller, so it's like any other 'read' operation.

Hmm. Unlike any other 'read' operation, it allows userland to modulate
CPU frequency with great precision; probably allowing userland to
transmit at radio bands, and allowing sound production without
syscalls (singing capacitors).

But userland can probably do that already, with help of
high-resolution timers, so nothing new theree... that I can think off.

Lets wait for the presentation at 39c3 :-).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]