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

2016-12-23 Thread 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 4/5] x86/cpufeature: enable RING3MWAIT for Knights Landing

2016-12-22 Thread 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

2016-12-22 Thread 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

2016-12-22 Thread 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

2016-12-22 Thread 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

2016-12-21 Thread 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



   


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

2016-12-20 Thread 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 
---
 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 
 #include 
 #include 
+#include 
+#include 
 
 #ifdef CONFIG_X86_64
 #include 
@@ -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 4/5] x86/cpufeature: enable RING3MWAIT for Knights Landing

2016-12-20 Thread 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 
---
 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 
 #include 
 #include 
+#include 
+#include 
 
 #ifdef CONFIG_X86_64
 #include 
@@ -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