Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1499507 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/4] Enabling Ring 3 MONITOR/MWAIT feature for Knights Landing Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com> - 2016-10-12 14:20 +0200
    [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com> - 2016-10-12 14:20 +0200
      Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for  KNL Thomas Gleixner <tglx@linutronix.de> - 2016-10-12 15:40 +0200
        Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL Dave Hansen <dave.hansen@linux.intel.com> - 2016-10-12 16:50 +0200
      Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for  KNL Thomas Gleixner <tglx@linutronix.de> - 2016-10-12 15:40 +0200
        Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL Dave Hansen <dave.hansen@linux.intel.com> - 2016-10-12 18:40 +0200
          Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for  KNL Thomas Gleixner <tglx@linutronix.de> - 2016-10-13 18:10 +0200
      Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL Dave Hansen <dave.hansen@linux.intel.com> - 2016-10-12 19:30 +0200
    [PATCH v2 4/4] Add R3MWAIT to CPU features Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com> - 2016-10-12 14:30 +0200
      Re: [PATCH v2 4/4] Add R3MWAIT to CPU features Borislav Petkov <bp@suse.de> - 2016-10-12 15:00 +0200
      Re: [PATCH v2 4/4] Add R3MWAIT to CPU features Thomas Gleixner <tglx@linutronix.de> - 2016-10-12 15:30 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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'?

[toc] | [prev] | [next] | [standalone]

#1499513 — [PATCH v2 4/4] Add R3MWAIT to CPU features

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

[toc] | [prev] | [next] | [standalone]

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

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)
-- 

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web