[PATCH] perf,x86,p6: Add userspace RDPMC quirk for P6

[PATCH] perf,x86,p6: Add userspace RDPMC quirk for P6

[Peter Zijlstra]

Subject: perf,x86,p6: Add userspace RDPMC quirk for P6
From: Peter Zijlstra <peterz@infradead.org>
Date: Wed Feb 5 11:17:46 CET 2014

P6 class machines can die hard when PCE gets enabled due to CPU
errata. The safe way it so disable it by default and keep it disabled.

See errata 26:

http://download.intel.com/design/archives/processors/pro/docs/24268935.pdf

Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Reported-by: Mark Davies <junk@eslaf.co.uk>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 arch/x86/kernel/cpu/perf_event.c    |    6 +++++-
 arch/x86/kernel/cpu/perf_event.h    |    1 +
 arch/x86/kernel/cpu/perf_event_p6.c |    8 ++++++++
 3 files changed, 14 insertions(+), 1 deletion(-)

--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1521,6 +1521,8 @@ static int __init init_hw_perf_events(vo
 
 	pr_cont("%s PMU driver.\n", x86_pmu.name);
 
+	x86_pmu.attr_rdpmc = 1; /* enable userspace RDPMC usage by default */
+
 	for (quirk = x86_pmu.quirks; quirk; quirk = quirk->next)
 		quirk->func();
 
@@ -1534,7 +1536,6 @@ static int __init init_hw_perf_events(vo
 		__EVENT_CONSTRAINT(0, (1ULL << x86_pmu.num_counters) - 1,
 				   0, x86_pmu.num_counters, 0, 0);
 
-	x86_pmu.attr_rdpmc = 1; /* enable userspace RDPMC usage by default */
 	x86_pmu_format_group.attrs = x86_pmu.format_attrs;
 
 	if (x86_pmu.event_attrs)
@@ -1820,6 +1821,9 @@ static ssize_t set_attr_rdpmc(struct dev
 	if (ret)
 		return ret;
 
+	if (x86_pmu.attr_rdpmc_broken)
+		return -ENOTSUPP;
+
 	if (!!val != !!x86_pmu.attr_rdpmc) {
 		x86_pmu.attr_rdpmc = !!val;
 		smp_call_function(change_rdpmc, (void *)val, 1);
--- a/arch/x86/kernel/cpu/perf_event.h
+++ b/arch/x86/kernel/cpu/perf_event.h
@@ -409,6 +409,7 @@ struct x86_pmu {
 	/*
 	 * sysfs attrs
 	 */
+	int		attr_rdpmc_broken;
 	int		attr_rdpmc;
 	struct attribute **format_attrs;
 	struct attribute **event_attrs;
--- a/arch/x86/kernel/cpu/perf_event_p6.c
+++ b/arch/x86/kernel/cpu/perf_event_p6.c
@@ -231,6 +231,13 @@ static __initconst const struct x86_pmu
 
 };
 
+static __init void p6_pmu_rdpmc_quirk(void)
+{
+	pr_warn("Userspace RDPMC support disabled due to CPU errata\n");
+	x86_pmu.attr_rdpmc_broken = 1;
+	x86_pmu.attr_rdpmc = 0;
+}
+
 __init int p6_pmu_init(void)
 {
 	switch (boot_cpu_data.x86_model) {
@@ -256,6 +263,7 @@ __init int p6_pmu_init(void)
 	memcpy(hw_cache_event_ids, p6_hw_cache_event_ids,
 		sizeof(hw_cache_event_ids));
 
+	x86_add_quirk(p6_pmu_rdpmc_quirk);
 
 	return 0;
 }

Re: [PATCH] perf,x86,p6: Add userspace RDPMC quirk for P6

[Vince Weaver]

On Wed, 5 Feb 2014, Peter Zijlstra wrote:

> P6 class machines can die hard when PCE gets enabled due to CPU
> errata. The safe way it so disable it by default and keep it disabled.
> 
> See errata 26:

so you take an errata applicable to a subset of Pentium Pro steppings and 
apply it to the whole p6 line (which includes the whole way up to Pentium 
III)?

Or am I missing something in the documentation you link to?

Vince

Re: [PATCH] perf,x86,p6: Add userspace RDPMC quirk for P6

[Peter Zijlstra]

On Wed, Feb 05, 2014 at 02:16:43PM -0500, Vince Weaver wrote:
> On Wed, 5 Feb 2014, Peter Zijlstra wrote:
> 
> > P6 class machines can die hard when PCE gets enabled due to CPU
> > errata. The safe way it so disable it by default and keep it disabled.
> > 
> > See errata 26:
> 
> so you take an errata applicable to a subset of Pentium Pro steppings and 
> apply it to the whole p6 line (which includes the whole way up to Pentium 
> III)?
> 
> Or am I missing something in the documentation you link to?

I forgot it went all the way to P-III, let me try and find the other
erratas.

Re: [PATCH] perf,x86,p6: Add userspace RDPMC quirk for P6

[Vince Weaver]

On Wed, 5 Feb 2014, Peter Zijlstra wrote:

> On Wed, Feb 05, 2014 at 02:16:43PM -0500, Vince Weaver wrote:
> > On Wed, 5 Feb 2014, Peter Zijlstra wrote:
> > 
> > > P6 class machines can die hard when PCE gets enabled due to CPU
> > > errata. The safe way it so disable it by default and keep it disabled.
> > > 
> > > See errata 26:
> > 
> > so you take an errata applicable to a subset of Pentium Pro steppings and 
> > apply it to the whole p6 line (which includes the whole way up to Pentium 
> > III)?
> > 
> > Or am I missing something in the documentation you link to?
> 
> I forgot it went all the way to P-III, let me try and find the other
> erratas.

from the document you linked it looks like it was fixed by the Pentium 
Pro stepping sB1 (6.1.9).

Has someone actually hit this bug?  People used PAPI/perfctr on these 
machines for years, and it uses rdpmc by default so I would think it would 
show up if it were an issue.  I should check the perfctr code to see if it 
had some sort of check.

Vince

Re: [PATCH] perf,x86,p6: Add userspace RDPMC quirk for P6

[Vince Weaver]

On Wed, 5 Feb 2014, Vince Weaver wrote:

> Has someone actually hit this bug?  People used PAPI/perfctr on these 
> machines for years, and it uses rdpmc by default so I would think it would 
> show up if it were an issue.  I should check the perfctr code to see if it 
> had some sort of check.

I can verify the perfctr code does have a case for PPRO errata 26, but it 
only disables things on pentium pros with stepping less than 9.

It also disables rdpmc on original Pentiums due to errata 74 but I don't 
think we have to worry about that one.

Vince

Re: [PATCH] perf,x86,p6: Add userspace RDPMC quirk for P6

[Peter Zijlstra]

On Wed, Feb 05, 2014 at 02:37:29PM -0500, Vince Weaver wrote:
> On Wed, 5 Feb 2014, Vince Weaver wrote:
> 
> > Has someone actually hit this bug?  People used PAPI/perfctr on these 
> > machines for years, and it uses rdpmc by default so I would think it would 
> > show up if it were an issue.  I should check the perfctr code to see if it 
> > had some sort of check.
> 
> I can verify the perfctr code does have a case for PPRO errata 26, but it 
> only disables things on pentium pros with stepping less than 9.

The doc said 0xB1, right? Should we keep it at that? Its not like
there's a lot of PPro machines left out there, so better safe than
sorry.

> It also disables rdpmc on original Pentiums due to errata 74 but I don't 
> think we have to worry about that one.

We don't have a Family 5 driver, so no.

Re: [PATCH] perf,x86,p6: Add userspace RDPMC quirk for P6

[Vince Weaver]

On Wed, 5 Feb 2014, Peter Zijlstra wrote:

> On Wed, Feb 05, 2014 at 02:37:29PM -0500, Vince Weaver wrote:
> > On Wed, 5 Feb 2014, Vince Weaver wrote:
> > 
> > > Has someone actually hit this bug?  People used PAPI/perfctr on these 
> > > machines for years, and it uses rdpmc by default so I would think it would 
> > > show up if it were an issue.  I should check the perfctr code to see if it 
> > > had some sort of check.
> > 
> > I can verify the perfctr code does have a case for PPRO errata 26, but it 
> > only disables things on pentium pros with stepping less than 9.
> 
> The doc said 0xB1, right? Should we keep it at that? Its not like
> there's a lot of PPro machines left out there, so better safe than
> sorry.

mfg steping "sB1" which means processor stepping 9.  See page 15 of the 
document linked in the original patch.

I used to have a Pentium Pro that was apparently affected by this erratum
but not any more.  The oldest machine I test perf stuff on is a 
Pentium II.

Vince

Re: [PATCH] perf,x86,p6: Add userspace RDPMC quirk for P6

[Peter Zijlstra]

On Wed, Feb 05, 2014 at 03:04:03PM -0500, Vince Weaver wrote:
> mfg steping "sB1" which means processor stepping 9.  See page 15 of the 
> document linked in the original patch.

Might confusing way to write 9. ok, I'll make it that.

Re: [PATCH] perf,x86,p6: Add userspace RDPMC quirk for P6

[Peter Zijlstra]

On Wed, Feb 05, 2014 at 02:29:36PM -0500, Vince Weaver wrote:
> from the document you linked it looks like it was fixed by the Pentium 
> Pro stepping sB1 (6.1.9).

OK, I'll add a boot_cpu_data.x86_mask >= 0xB1 test.

> Has someone actually hit this bug?  People used PAPI/perfctr on these 
> machines for years, and it uses rdpmc by default so I would think it would 
> show up if it were an issue.  I should check the perfctr code to see if it 
> had some sort of check.

Yeah, the reporter had a boot fail due to this.

Re: [PATCH] perf,x86,p6: Add userspace RDPMC quirk for P6

[Peter Zijlstra]

On Wed, Feb 05, 2014 at 08:20:12PM +0100, Peter Zijlstra wrote:
> On Wed, Feb 05, 2014 at 02:16:43PM -0500, Vince Weaver wrote:
> > On Wed, 5 Feb 2014, Peter Zijlstra wrote:
> > 
> > > P6 class machines can die hard when PCE gets enabled due to CPU
> > > errata. The safe way it so disable it by default and keep it disabled.
> > > 
> > > See errata 26:
> > 
> > so you take an errata applicable to a subset of Pentium Pro steppings and 
> > apply it to the whole p6 line (which includes the whole way up to Pentium 
> > III)?
> > 
> > Or am I missing something in the documentation you link to?
> 
> I forgot it went all the way to P-III, let me try and find the other
> erratas.

OK, Just checked the P-II errata and its not there anymore, so I suppose
only Family 6 model 1 should have this quirk.

Updated patch below.

---
Subject: perf,x86,p6: Add userspace RDPMC quirk for PPro
From: Peter Zijlstra <peterz@infradead.org>
Date: Wed Feb 5 11:17:46 CET 2014

PPro machines can die hard when PCE gets enabled due to CPU errata. The
safe way it so disable it by default and keep it disabled.

See errata 26:

http://download.intel.com/design/archives/processors/pro/docs/24268935.pdf

Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Reported-by: Mark Davies <junk@eslaf.co.uk>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/n/tip-jnbn0qpt3t7ogk4c2q79yq90@git.kernel.org
---
 arch/x86/kernel/cpu/perf_event.c    |    6 +++++-
 arch/x86/kernel/cpu/perf_event.h    |    1 +
 arch/x86/kernel/cpu/perf_event_p6.c |   35 +++++++++++++++++++++++++----------
 3 files changed, 31 insertions(+), 11 deletions(-)

--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1521,6 +1521,8 @@ static int __init init_hw_perf_events(vo
 
 	pr_cont("%s PMU driver.\n", x86_pmu.name);
 
+	x86_pmu.attr_rdpmc = 1; /* enable userspace RDPMC usage by default */
+
 	for (quirk = x86_pmu.quirks; quirk; quirk = quirk->next)
 		quirk->func();
 
@@ -1534,7 +1536,6 @@ static int __init init_hw_perf_events(vo
 		__EVENT_CONSTRAINT(0, (1ULL << x86_pmu.num_counters) - 1,
 				   0, x86_pmu.num_counters, 0, 0);
 
-	x86_pmu.attr_rdpmc = 1; /* enable userspace RDPMC usage by default */
 	x86_pmu_format_group.attrs = x86_pmu.format_attrs;
 
 	if (x86_pmu.event_attrs)
@@ -1820,6 +1821,9 @@ static ssize_t set_attr_rdpmc(struct dev
 	if (ret)
 		return ret;
 
+	if (x86_pmu.attr_rdpmc_broken)
+		return -ENOTSUPP;
+
 	if (!!val != !!x86_pmu.attr_rdpmc) {
 		x86_pmu.attr_rdpmc = !!val;
 		smp_call_function(change_rdpmc, (void *)val, 1);
--- a/arch/x86/kernel/cpu/perf_event.h
+++ b/arch/x86/kernel/cpu/perf_event.h
@@ -409,6 +409,7 @@ struct x86_pmu {
 	/*
 	 * sysfs attrs
 	 */
+	int		attr_rdpmc_broken;
 	int		attr_rdpmc;
 	struct attribute **format_attrs;
 	struct attribute **event_attrs;
--- a/arch/x86/kernel/cpu/perf_event_p6.c
+++ b/arch/x86/kernel/cpu/perf_event_p6.c
@@ -231,20 +231,35 @@ static __initconst const struct x86_pmu
 
 };
 
+static __init void p6_pmu_rdpmc_quirk(void)
+{
+	pr_warn("Userspace RDPMC support disabled due to CPU errata\n");
+	x86_pmu.attr_rdpmc_broken = 1;
+	x86_pmu.attr_rdpmc = 0;
+}
+
 __init int p6_pmu_init(void)
 {
 	switch (boot_cpu_data.x86_model) {
-	case 1:
-	case 3:  /* Pentium Pro */
-	case 5:
-	case 6:  /* Pentium II */
-	case 7:
-	case 8:
-	case 11: /* Pentium III */
-	case 9:
-	case 13:
-		/* Pentium M */
+	case  1: /* Pentium Pro */
+		x86_add_quirk(p6_pmu_rdpmc_quirk);
 		break;
+
+	case  3: /* Pentium II - Klamath */
+	case  5: /* Pentium II - Deschutes */
+	case  6: /* Pentium II - Mendocino */
+		break;
+
+	case  7: /* Pentium III - Katmai */
+	case  8: /* Pentium III - Coppermine */
+	case 10: /* Pentium III Xeon */
+	case 11: /* Pentium III - Tualatin */
+		break;
+
+	case  9: /* Pentium M - Banias */
+	case 13: /* Pentium M - Dothan */
+		break;
+
 	default:
 		pr_cont("unsupported p6 CPU model %d ",
 			boot_cpu_data.x86_model);

Re: [PATCH] perf,x86,p6: Add userspace RDPMC quirk for P6

[Mark Davies]

Peter

As I reported the problem I thought I better contribute by testing the
fix.

I took the patch below and managed to change the 3.4.78 code to match
and tried to boot my Pentium Pro system but it hung as before. After
some investigation I discovered the quirk is not being run, in fact no
quirks were being run in init_hw_perf_events as x86_pmu.quirks was NULL.

The problem as far as I can see is that the quirk is added to x86_pmu in
p6_pmu_init but then x86_pmu is reassigned at the end of that function
(x86_pmu = p6_pmu) resetting x86_pmu.quirks to NULL. x86_pmu looks like
it is reassigned again in intel_pmu_init to either core_pmu or intel_pmu
after the call to p6_pmu_init so I am not sure which x86_pmu.quirks
init_hw_perf_events tries to use.

Hopefully that makes sense.

Mark Davies

On Wed, Feb 05 2014, Peter Zijlstra wrote:

> On Wed, Feb 05, 2014 at 08:20:12PM +0100, Peter Zijlstra wrote:
>> On Wed, Feb 05, 2014 at 02:16:43PM -0500, Vince Weaver wrote:
>> > On Wed, 5 Feb 2014, Peter Zijlstra wrote:
>> > 
>> > > P6 class machines can die hard when PCE gets enabled due to CPU
>> > > errata. The safe way it so disable it by default and keep it disabled.
>> > > 
>> > > See errata 26:
>> > 
>> > so you take an errata applicable to a subset of Pentium Pro steppings and 
>> > apply it to the whole p6 line (which includes the whole way up to Pentium 
>> > III)?
>> > 
>> > Or am I missing something in the documentation you link to?
>> 
>> I forgot it went all the way to P-III, let me try and find the other
>> erratas.
>
> OK, Just checked the P-II errata and its not there anymore, so I suppose
> only Family 6 model 1 should have this quirk.
>
> Updated patch below.
>
> ---
> Subject: perf,x86,p6: Add userspace RDPMC quirk for PPro
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Wed Feb 5 11:17:46 CET 2014
>
> PPro machines can die hard when PCE gets enabled due to CPU errata. The
> safe way it so disable it by default and keep it disabled.
>
> See errata 26:
>
> http://download.intel.com/design/archives/processors/pro/docs/24268935.pdf
>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Reported-by: Mark Davies <junk@eslaf.co.uk>
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> Link: http://lkml.kernel.org/n/tip-jnbn0qpt3t7ogk4c2q79yq90@git.kernel.org
> ---
>  arch/x86/kernel/cpu/perf_event.c    |    6 +++++-
>  arch/x86/kernel/cpu/perf_event.h    |    1 +
>  arch/x86/kernel/cpu/perf_event_p6.c |   35 +++++++++++++++++++++++++----------
>  3 files changed, 31 insertions(+), 11 deletions(-)
>
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -1521,6 +1521,8 @@ static int __init init_hw_perf_events(vo
>  
>  	pr_cont("%s PMU driver.\n", x86_pmu.name);
>  
> +	x86_pmu.attr_rdpmc = 1; /* enable userspace RDPMC usage by default */
> +
>  	for (quirk = x86_pmu.quirks; quirk; quirk = quirk->next)
>  		quirk->func();
>  
> @@ -1534,7 +1536,6 @@ static int __init init_hw_perf_events(vo
>  		__EVENT_CONSTRAINT(0, (1ULL << x86_pmu.num_counters) - 1,
>  				   0, x86_pmu.num_counters, 0, 0);
>  
> -	x86_pmu.attr_rdpmc = 1; /* enable userspace RDPMC usage by default */
>  	x86_pmu_format_group.attrs = x86_pmu.format_attrs;
>  
>  	if (x86_pmu.event_attrs)
> @@ -1820,6 +1821,9 @@ static ssize_t set_attr_rdpmc(struct dev
>  	if (ret)
>  		return ret;
>  
> +	if (x86_pmu.attr_rdpmc_broken)
> +		return -ENOTSUPP;
> +
>  	if (!!val != !!x86_pmu.attr_rdpmc) {
>  		x86_pmu.attr_rdpmc = !!val;
>  		smp_call_function(change_rdpmc, (void *)val, 1);
> --- a/arch/x86/kernel/cpu/perf_event.h
> +++ b/arch/x86/kernel/cpu/perf_event.h
> @@ -409,6 +409,7 @@ struct x86_pmu {
>  	/*
>  	 * sysfs attrs
>  	 */
> +	int		attr_rdpmc_broken;
>  	int		attr_rdpmc;
>  	struct attribute **format_attrs;
>  	struct attribute **event_attrs;
> --- a/arch/x86/kernel/cpu/perf_event_p6.c
> +++ b/arch/x86/kernel/cpu/perf_event_p6.c
> @@ -231,20 +231,35 @@ static __initconst const struct x86_pmu
>  
>  };
>  
> +static __init void p6_pmu_rdpmc_quirk(void)
> +{
> +	pr_warn("Userspace RDPMC support disabled due to CPU errata\n");
> +	x86_pmu.attr_rdpmc_broken = 1;
> +	x86_pmu.attr_rdpmc = 0;
> +}
> +
>  __init int p6_pmu_init(void)
>  {
>  	switch (boot_cpu_data.x86_model) {
> -	case 1:
> -	case 3:  /* Pentium Pro */
> -	case 5:
> -	case 6:  /* Pentium II */
> -	case 7:
> -	case 8:
> -	case 11: /* Pentium III */
> -	case 9:
> -	case 13:
> -		/* Pentium M */
> +	case  1: /* Pentium Pro */
> +		x86_add_quirk(p6_pmu_rdpmc_quirk);
>  		break;
> +
> +	case  3: /* Pentium II - Klamath */
> +	case  5: /* Pentium II - Deschutes */
> +	case  6: /* Pentium II - Mendocino */
> +		break;
> +
> +	case  7: /* Pentium III - Katmai */
> +	case  8: /* Pentium III - Coppermine */
> +	case 10: /* Pentium III Xeon */
> +	case 11: /* Pentium III - Tualatin */
> +		break;
> +
> +	case  9: /* Pentium M - Banias */
> +	case 13: /* Pentium M - Dothan */
> +		break;
> +
>  	default:
>  		pr_cont("unsupported p6 CPU model %d ",
>  			boot_cpu_data.x86_model);

Re: [PATCH] perf,x86,p6: Add userspace RDPMC quirk for P6

[Peter Zijlstra]

On Thu, Feb 06, 2014 at 07:41:37PM +0000, Mark Davies wrote:
> Peter
> 
> As I reported the problem I thought I better contribute by testing the
> fix.
> 
> I took the patch below and managed to change the 3.4.78 code to match
> and tried to boot my Pentium Pro system but it hung as before. After
> some investigation I discovered the quirk is not being run, in fact no
> quirks were being run in init_hw_perf_events as x86_pmu.quirks was NULL.
> 
> The problem as far as I can see is that the quirk is added to x86_pmu in
> p6_pmu_init but then x86_pmu is reassigned at the end of that function
> (x86_pmu = p6_pmu) resetting x86_pmu.quirks to NULL.

Right so far.

> x86_pmu looks like
> it is reassigned again in intel_pmu_init to either core_pmu or intel_pmu
> after the call to p6_pmu_init so I am not sure which x86_pmu.quirks
> init_hw_perf_events tries to use.

Note how intel_pmu_init() does: return p6_pmu_init(), so it will never
get to the x86_pmu assignment for the ARCH_PERFMON drivers.

Yep, thanks for testing and you're (mostly) quite right.

We can do the copy early though, if we return -ENODEV nothing will end
up using the x86_pmu anyway, so it doesn't matter what lives in there.

---
Subject: perf,x86,p6: Add userspace RDPMC quirk for PPro
From: Peter Zijlstra <peterz@infradead.org>
Date: Wed, 5 Feb 2014 20:48:51 +0100

PPro machines can die hard when PCE gets enabled due to a CPU erratum.
The safe way it so disable it by default and keep it disabled.

See erratum 26 in:

http://download.intel.com/design/archives/processors/pro/docs/24268935.pdf

Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Vince Weaver <vince@deater.net>
Reported-by: Mark Davies <junk@eslaf.co.uk>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 arch/x86/kernel/cpu/perf_event.c    |    6 +++-
 arch/x86/kernel/cpu/perf_event.h    |    1 
 arch/x86/kernel/cpu/perf_event_p6.c |   48 ++++++++++++++++++++++++------------
 3 files changed, 39 insertions(+), 16 deletions(-)

--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1521,6 +1521,8 @@ static int __init init_hw_perf_events(vo
 
 	pr_cont("%s PMU driver.\n", x86_pmu.name);
 
+	x86_pmu.attr_rdpmc = 1; /* enable userspace RDPMC usage by default */
+
 	for (quirk = x86_pmu.quirks; quirk; quirk = quirk->next)
 		quirk->func();
 
@@ -1534,7 +1536,6 @@ static int __init init_hw_perf_events(vo
 		__EVENT_CONSTRAINT(0, (1ULL << x86_pmu.num_counters) - 1,
 				   0, x86_pmu.num_counters, 0, 0);
 
-	x86_pmu.attr_rdpmc = 1; /* enable userspace RDPMC usage by default */
 	x86_pmu_format_group.attrs = x86_pmu.format_attrs;
 
 	if (x86_pmu.event_attrs)
@@ -1820,6 +1821,9 @@ static ssize_t set_attr_rdpmc(struct dev
 	if (ret)
 		return ret;
 
+	if (x86_pmu.attr_rdpmc_broken)
+		return -ENOTSUPP;
+
 	if (!!val != !!x86_pmu.attr_rdpmc) {
 		x86_pmu.attr_rdpmc = !!val;
 		smp_call_function(change_rdpmc, (void *)val, 1);
--- a/arch/x86/kernel/cpu/perf_event.h
+++ b/arch/x86/kernel/cpu/perf_event.h
@@ -409,6 +409,7 @@ struct x86_pmu {
 	/*
 	 * sysfs attrs
 	 */
+	int		attr_rdpmc_broken;
 	int		attr_rdpmc;
 	struct attribute **format_attrs;
 	struct attribute **event_attrs;
--- a/arch/x86/kernel/cpu/perf_event_p6.c
+++ b/arch/x86/kernel/cpu/perf_event_p6.c
@@ -231,31 +231,49 @@ static __initconst const struct x86_pmu
 
 };
 
+static __init void p6_pmu_rdpmc_quirk(void)
+{
+	if (boot_cpu_data.x86_mask < 9) {
+		/*
+		 * PPro erratum 26; fixed in stepping 9 and above.
+		 */
+		pr_warn("Userspace RDPMC support disabled due to a CPU erratum\n");
+		x86_pmu.attr_rdpmc_broken = 1;
+		x86_pmu.attr_rdpmc = 0;
+	}
+}
+
 __init int p6_pmu_init(void)
 {
+	x86_pmu = p6_pmu;
+
 	switch (boot_cpu_data.x86_model) {
-	case 1:
-	case 3:  /* Pentium Pro */
-	case 5:
-	case 6:  /* Pentium II */
-	case 7:
-	case 8:
-	case 11: /* Pentium III */
-	case 9:
-	case 13:
-		/* Pentium M */
+	case  1: /* Pentium Pro */
+		x86_add_quirk(p6_pmu_rdpmc_quirk);
+		break;
+
+	case  3: /* Pentium II - Klamath */
+	case  5: /* Pentium II - Deschutes */
+	case  6: /* Pentium II - Mendocino */
 		break;
+
+	case  7: /* Pentium III - Katmai */
+	case  8: /* Pentium III - Coppermine */
+	case 10: /* Pentium III Xeon */
+	case 11: /* Pentium III - Tualatin */
+		break;
+
+	case  9: /* Pentium M - Banias */
+	case 13: /* Pentium M - Dothan */
+		break;
+
 	default:
-		pr_cont("unsupported p6 CPU model %d ",
-			boot_cpu_data.x86_model);
+		pr_cont("unsupported p6 CPU model %d ", boot_cpu_data.x86_model);
 		return -ENODEV;
 	}
 
-	x86_pmu = p6_pmu;
-
 	memcpy(hw_cache_event_ids, p6_hw_cache_event_ids,
 		sizeof(hw_cache_event_ids));
 
-
 	return 0;
 }

Re: [PATCH] perf,x86,p6: Add userspace RDPMC quirk for P6

[Mark Davies]

That fixed it. Booted patched 3.4.78 with CONFIG_APM=y with no problem.

dmesg contains the "Userspace RDPMC support disabled due to a CPU
erratum" message

"cat /sys/devices/cpu/rdpmc" gives "0"

"echo 1 >/sys/devices/cpu/rdpmc" gives "bash: echo: write error: Unknown
error 524" 

Thanks for fixing it. How I missed the return statements in
intel_pmu_init I don't know, probably got a bit over excited when I
spotted what the problem was.

Mark Davies

On Thu, Feb 06 2014, Peter Zijlstra wrote:

> On Thu, Feb 06, 2014 at 07:41:37PM +0000, Mark Davies wrote:
>> Peter
>> 
>> As I reported the problem I thought I better contribute by testing the
>> fix.
>> 
>> I took the patch below and managed to change the 3.4.78 code to match
>> and tried to boot my Pentium Pro system but it hung as before. After
>> some investigation I discovered the quirk is not being run, in fact no
>> quirks were being run in init_hw_perf_events as x86_pmu.quirks was NULL.
>> 
>> The problem as far as I can see is that the quirk is added to x86_pmu in
>> p6_pmu_init but then x86_pmu is reassigned at the end of that function
>> (x86_pmu = p6_pmu) resetting x86_pmu.quirks to NULL.
>
> Right so far.
>
>> x86_pmu looks like
>> it is reassigned again in intel_pmu_init to either core_pmu or intel_pmu
>> after the call to p6_pmu_init so I am not sure which x86_pmu.quirks
>> init_hw_perf_events tries to use.
>
> Note how intel_pmu_init() does: return p6_pmu_init(), so it will never
> get to the x86_pmu assignment for the ARCH_PERFMON drivers.
>
> Yep, thanks for testing and you're (mostly) quite right.
>
> We can do the copy early though, if we return -ENODEV nothing will end
> up using the x86_pmu anyway, so it doesn't matter what lives in there.
>
> ---
> Subject: perf,x86,p6: Add userspace RDPMC quirk for PPro
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Wed, 5 Feb 2014 20:48:51 +0100
>
> PPro machines can die hard when PCE gets enabled due to a CPU erratum.
> The safe way it so disable it by default and keep it disabled.
>
> See erratum 26 in:
>
> http://download.intel.com/design/archives/processors/pro/docs/24268935.pdf
>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Stephane Eranian <eranian@google.com>
> Cc: Vince Weaver <vince@deater.net>
> Reported-by: Mark Davies <junk@eslaf.co.uk>
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> ---
>  arch/x86/kernel/cpu/perf_event.c    |    6 +++-
>  arch/x86/kernel/cpu/perf_event.h    |    1 
>  arch/x86/kernel/cpu/perf_event_p6.c |   48 ++++++++++++++++++++++++------------
>  3 files changed, 39 insertions(+), 16 deletions(-)
>
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -1521,6 +1521,8 @@ static int __init init_hw_perf_events(vo
>  
>  	pr_cont("%s PMU driver.\n", x86_pmu.name);
>  
> +	x86_pmu.attr_rdpmc = 1; /* enable userspace RDPMC usage by default */
> +
>  	for (quirk = x86_pmu.quirks; quirk; quirk = quirk->next)
>  		quirk->func();
>  
> @@ -1534,7 +1536,6 @@ static int __init init_hw_perf_events(vo
>  		__EVENT_CONSTRAINT(0, (1ULL << x86_pmu.num_counters) - 1,
>  				   0, x86_pmu.num_counters, 0, 0);
>  
> -	x86_pmu.attr_rdpmc = 1; /* enable userspace RDPMC usage by default */
>  	x86_pmu_format_group.attrs = x86_pmu.format_attrs;
>  
>  	if (x86_pmu.event_attrs)
> @@ -1820,6 +1821,9 @@ static ssize_t set_attr_rdpmc(struct dev
>  	if (ret)
>  		return ret;
>  
> +	if (x86_pmu.attr_rdpmc_broken)
> +		return -ENOTSUPP;
> +
>  	if (!!val != !!x86_pmu.attr_rdpmc) {
>  		x86_pmu.attr_rdpmc = !!val;
>  		smp_call_function(change_rdpmc, (void *)val, 1);
> --- a/arch/x86/kernel/cpu/perf_event.h
> +++ b/arch/x86/kernel/cpu/perf_event.h
> @@ -409,6 +409,7 @@ struct x86_pmu {
>  	/*
>  	 * sysfs attrs
>  	 */
> +	int		attr_rdpmc_broken;
>  	int		attr_rdpmc;
>  	struct attribute **format_attrs;
>  	struct attribute **event_attrs;
> --- a/arch/x86/kernel/cpu/perf_event_p6.c
> +++ b/arch/x86/kernel/cpu/perf_event_p6.c
> @@ -231,31 +231,49 @@ static __initconst const struct x86_pmu
>  
>  };
>  
> +static __init void p6_pmu_rdpmc_quirk(void)
> +{
> +	if (boot_cpu_data.x86_mask < 9) {
> +		/*
> +		 * PPro erratum 26; fixed in stepping 9 and above.
> +		 */
> +		pr_warn("Userspace RDPMC support disabled due to a CPU erratum\n");
> +		x86_pmu.attr_rdpmc_broken = 1;
> +		x86_pmu.attr_rdpmc = 0;
> +	}
> +}
> +
>  __init int p6_pmu_init(void)
>  {
> +	x86_pmu = p6_pmu;
> +
>  	switch (boot_cpu_data.x86_model) {
> -	case 1:
> -	case 3:  /* Pentium Pro */
> -	case 5:
> -	case 6:  /* Pentium II */
> -	case 7:
> -	case 8:
> -	case 11: /* Pentium III */
> -	case 9:
> -	case 13:
> -		/* Pentium M */
> +	case  1: /* Pentium Pro */
> +		x86_add_quirk(p6_pmu_rdpmc_quirk);
> +		break;
> +
> +	case  3: /* Pentium II - Klamath */
> +	case  5: /* Pentium II - Deschutes */
> +	case  6: /* Pentium II - Mendocino */
>  		break;
> +
> +	case  7: /* Pentium III - Katmai */
> +	case  8: /* Pentium III - Coppermine */
> +	case 10: /* Pentium III Xeon */
> +	case 11: /* Pentium III - Tualatin */
> +		break;
> +
> +	case  9: /* Pentium M - Banias */
> +	case 13: /* Pentium M - Dothan */
> +		break;
> +
>  	default:
> -		pr_cont("unsupported p6 CPU model %d ",
> -			boot_cpu_data.x86_model);
> +		pr_cont("unsupported p6 CPU model %d ", boot_cpu_data.x86_model);
>  		return -ENODEV;
>  	}
>  
> -	x86_pmu = p6_pmu;
> -
>  	memcpy(hw_cache_event_ids, p6_hw_cache_event_ids,
>  		sizeof(hw_cache_event_ids));
>  
> -
>  	return 0;
>  }

Re: [PATCH] perf,x86,p6: Add userspace RDPMC quirk for P6

[Peter Zijlstra]

On Thu, Feb 06, 2014 at 10:15:23PM +0000, Mark Davies wrote:
> That fixed it. Booted patched 3.4.78 with CONFIG_APM=y with no problem.

Awesome, thanks!