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, 0x0007, 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 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, 0x0007, 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 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 
> ---
>  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, 0x0007, 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

[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 
> ---
>  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, 0x0007, 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)
-- 

[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 
---
 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, 0x0007, 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

[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 
---
 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, 0x0007, 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