Drop WARN on AMD lack of perfctrs

Drop WARN on AMD lack of perfctrs

[Josh Boyer]

Hi All,

If you boot a KVM guest on an AMD family 15h and specify -cpu host,
you'll get the following splat:

[    0.031000] ------------[ cut here ]------------
[    0.031000] WARNING: at arch/x86/kernel/cpu/perf_event_amd.c:772
amd_pmu_init+0x18c/0x249()
[    0.031000] Hardware name: Bochs
[    0.031000] Odd, counter constraints enabled but no core perfctrs
detected!
[    0.031000] Modules linked in:

[    0.031000] Pid: 1, comm: swapper/0 Not tainted
3.9.0-0.rc1.git0.4.fc19.x86_64 #1
[    0.031000] Call Trace:
[    0.031000]  [<ffffffff81d10c67>] ? amd_pmu_init+0x18c/0x249
[    0.031000]  [<ffffffff8105c9a0>] warn_slowpath_common+0x70/0xa0
[    0.031000]  [<ffffffff81d106b3>] ? check_bugs+0x2d/0x2d
[    0.031000]  [<ffffffff8105ca1c>] warn_slowpath_fmt+0x4c/0x50
[    0.031000]  [<ffffffff81d10c67>] amd_pmu_init+0x18c/0x249
[    0.031000]  [<ffffffff81d106e7>] init_hw_perf_events+0x34/0x428
[    0.031000]  [<ffffffff81d106b3>] ? check_bugs+0x2d/0x2d
[    0.031000]  [<ffffffff8100210a>] do_one_initcall+0x10a/0x160
[    0.031000]  [<ffffffff81d06fa5>] kernel_init_freeable+0xcf/0x1fa
[    0.031000]  [<ffffffff81629800>] ? rest_init+0x80/0x80
[    0.031000]  [<ffffffff8162980e>] kernel_init+0xe/0x190
[    0.031000]  [<ffffffff8164e22c>] ret_from_fork+0x7c/0xb0
[    0.031000]  [<ffffffff81629800>] ? rest_init+0x80/0x80
[    0.031000] ---[ end trace a1e57d3cb8668105 ]---

That seems a bit excessive, and it gets picked up by auto-reporting
tools like ABRT as a bug.  Can we remove the WARN and just use pr_err or
something else instead?

josh

Re: Drop WARN on AMD lack of perfctrs

[Peter Zijlstra]

On Thu, May 16, 2013 at 11:10:26AM -0400, Josh Boyer wrote:
> Hi All,
> 
> If you boot a KVM guest on an AMD family 15h and specify -cpu host,
> you'll get the following splat:
> 
> [    0.031000] ------------[ cut here ]------------
> [    0.031000] WARNING: at arch/x86/kernel/cpu/perf_event_amd.c:772
> amd_pmu_init+0x18c/0x249()
> [    0.031000] Hardware name: Bochs
> [    0.031000] Odd, counter constraints enabled but no core perfctrs
> detected!
> [    0.031000] Modules linked in:
> 
> [    0.031000] Pid: 1, comm: swapper/0 Not tainted
> 3.9.0-0.rc1.git0.4.fc19.x86_64 #1
> [    0.031000] Call Trace:
> [    0.031000]  [<ffffffff81d10c67>] ? amd_pmu_init+0x18c/0x249
> [    0.031000]  [<ffffffff8105c9a0>] warn_slowpath_common+0x70/0xa0
> [    0.031000]  [<ffffffff81d106b3>] ? check_bugs+0x2d/0x2d
> [    0.031000]  [<ffffffff8105ca1c>] warn_slowpath_fmt+0x4c/0x50
> [    0.031000]  [<ffffffff81d10c67>] amd_pmu_init+0x18c/0x249
> [    0.031000]  [<ffffffff81d106e7>] init_hw_perf_events+0x34/0x428
> [    0.031000]  [<ffffffff81d106b3>] ? check_bugs+0x2d/0x2d
> [    0.031000]  [<ffffffff8100210a>] do_one_initcall+0x10a/0x160
> [    0.031000]  [<ffffffff81d06fa5>] kernel_init_freeable+0xcf/0x1fa
> [    0.031000]  [<ffffffff81629800>] ? rest_init+0x80/0x80
> [    0.031000]  [<ffffffff8162980e>] kernel_init+0xe/0x190
> [    0.031000]  [<ffffffff8164e22c>] ret_from_fork+0x7c/0xb0
> [    0.031000]  [<ffffffff81629800>] ? rest_init+0x80/0x80
> [    0.031000] ---[ end trace a1e57d3cb8668105 ]---
> 
> That seems a bit excessive, and it gets picked up by auto-reporting
> tools like ABRT as a bug.  Can we remove the WARN and just use pr_err or
> something else instead?

Robert put that in, I suppose its because the CPUID crap indicates its got perf
counters but then it doesn't actually have them.

Clearly this is something that should be fixed in your virt thingy instead.

Re: Drop WARN on AMD lack of perfctrs

[Josh Boyer]

On Thu, May 16, 2013 at 07:51:17PM +0200, Peter Zijlstra wrote:
> On Thu, May 16, 2013 at 11:10:26AM -0400, Josh Boyer wrote:
> > Hi All,
> > 
> > If you boot a KVM guest on an AMD family 15h and specify -cpu host,
> > you'll get the following splat:
> > 
> > [    0.031000] ------------[ cut here ]------------
> > [    0.031000] WARNING: at arch/x86/kernel/cpu/perf_event_amd.c:772
> > amd_pmu_init+0x18c/0x249()
> > [    0.031000] Hardware name: Bochs
> > [    0.031000] Odd, counter constraints enabled but no core perfctrs
> > detected!
> > [    0.031000] Modules linked in:
> > 
> > [    0.031000] Pid: 1, comm: swapper/0 Not tainted
> > 3.9.0-0.rc1.git0.4.fc19.x86_64 #1
> > [    0.031000] Call Trace:
> > [    0.031000]  [<ffffffff81d10c67>] ? amd_pmu_init+0x18c/0x249
> > [    0.031000]  [<ffffffff8105c9a0>] warn_slowpath_common+0x70/0xa0
> > [    0.031000]  [<ffffffff81d106b3>] ? check_bugs+0x2d/0x2d
> > [    0.031000]  [<ffffffff8105ca1c>] warn_slowpath_fmt+0x4c/0x50
> > [    0.031000]  [<ffffffff81d10c67>] amd_pmu_init+0x18c/0x249
> > [    0.031000]  [<ffffffff81d106e7>] init_hw_perf_events+0x34/0x428
> > [    0.031000]  [<ffffffff81d106b3>] ? check_bugs+0x2d/0x2d
> > [    0.031000]  [<ffffffff8100210a>] do_one_initcall+0x10a/0x160
> > [    0.031000]  [<ffffffff81d06fa5>] kernel_init_freeable+0xcf/0x1fa
> > [    0.031000]  [<ffffffff81629800>] ? rest_init+0x80/0x80
> > [    0.031000]  [<ffffffff8162980e>] kernel_init+0xe/0x190
> > [    0.031000]  [<ffffffff8164e22c>] ret_from_fork+0x7c/0xb0
> > [    0.031000]  [<ffffffff81629800>] ? rest_init+0x80/0x80
> > [    0.031000] ---[ end trace a1e57d3cb8668105 ]---
> > 
> > That seems a bit excessive, and it gets picked up by auto-reporting
> > tools like ABRT as a bug.  Can we remove the WARN and just use pr_err or
> > something else instead?
> 
> Robert put that in, I suppose its because the CPUID crap indicates its got perf
> counters but then it doesn't actually have them.
> 
> Clearly this is something that should be fixed in your virt thingy instead.

Maybe.  But do you really need to dump a stack trace here?  What is a
user supposed to do with that information?  Can they fix the kernel?
Can the fix the CPU?  As far as I can tell, they can't do either.

Is using pr_err with the same message really somehow worse than using
WARN?

josh

Re: Drop WARN on AMD lack of perfctrs

[Peter Zijlstra]

On Thu, May 16, 2013 at 01:55:57PM -0400, Josh Boyer wrote:
> Maybe.  But do you really need to dump a stack trace here?  What is a
> user supposed to do with that information?  Can they fix the kernel?
> Can the fix the CPU?  As far as I can tell, they can't do either.

Send their CPU back to AMD is I suppose the best they can do ;-)
 
> Is using pr_err with the same message really somehow worse than using
> WARN?

I would make it a FW_BUG as well. But yeah, I suppose that is a better option
than the WARN_ON. Unless Robert had a different reason...

Re: Drop WARN on AMD lack of perfctrs

[Robert Richter]

On 16.05.13 20:10:18, Peter Zijlstra wrote:
> On Thu, May 16, 2013 at 01:55:57PM -0400, Josh Boyer wrote:
> > Maybe.  But do you really need to dump a stack trace here?  What is a
> > user supposed to do with that information?  Can they fix the kernel?
> > Can the fix the CPU?  As far as I can tell, they can't do either.
> 
> Send their CPU back to AMD is I suppose the best they can do ;-)
>  
> > Is using pr_err with the same message really somehow worse than using
> > WARN?
> 
> I would make it a FW_BUG as well. But yeah, I suppose that is a better option
> than the WARN_ON. Unless Robert had a different reason...

iirc the reason was the different msr range that is switched on fam15h
with a different counter to counter msr offset of 2 instead of 1. The
code relies on the assumption that the msrs exist on that cpu. Thus
the warning if not. Also note that code may have changed in 3.10 in
that area.

-Robert

Re: Drop WARN on AMD lack of perfctrs

[Borislav Petkov]

On Thu, May 16, 2013 at 10:55:58PM +0200, Robert Richter wrote:
> iirc the reason was the different msr range that is switched on fam15h
> with a different counter to counter msr offset of 2 instead of 1. The
> code relies on the assumption that the msrs exist on that cpu. Thus
> the warning if not. Also note that code may have changed in 3.10 in
> that area.

Stupid question: why is check_hw_exists() *after* the vendor-specific
counter detection code in init_hw_perf_events() even though it is
supposed to check whether hardware is emulated?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: Drop WARN on AMD lack of perfctrs

[Peter Zijlstra]

On Thu, May 16, 2013 at 11:34:20PM +0200, Borislav Petkov wrote:
> On Thu, May 16, 2013 at 10:55:58PM +0200, Robert Richter wrote:
> > iirc the reason was the different msr range that is switched on fam15h
> > with a different counter to counter msr offset of 2 instead of 1. The
> > code relies on the assumption that the msrs exist on that cpu. Thus
> > the warning if not. Also note that code may have changed in 3.10 in
> > that area.
> 
> Stupid question: why is check_hw_exists() *after* the vendor-specific
> counter detection code in init_hw_perf_events() even though it is
> supposed to check whether hardware is emulated?

Mostly so that check_hw_exists() doesn't need to know about the vendor
specifics like where the MSRs live, how many there are etc..

Re: Drop WARN on AMD lack of perfctrs

[Borislav Petkov]

On Fri, May 17, 2013 at 11:04:51AM +0200, Peter Zijlstra wrote:
> Mostly so that check_hw_exists() doesn't need to know about the vendor
> specifics like where the MSRs live, how many there are etc..

Yep, but there will still be issues with perf when booted on a guest and
kvm not supporting it. And AFAIU, they're signalling this by turning off
CPUID bits so that initializing perf doesn't happen.

So, I think init_hw_perf_events should as a first step look at CPUID
bits and then do anything else. And this is done on Intel with
X86_FEATURE_ARCH_PERFMON. But Robert is fixing this on AMD too so...

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: Drop WARN on AMD lack of perfctrs

[Peter Zijlstra]

On Fri, May 17, 2013 at 11:16:51AM +0200, Borislav Petkov wrote:
> On Fri, May 17, 2013 at 11:04:51AM +0200, Peter Zijlstra wrote:
> > Mostly so that check_hw_exists() doesn't need to know about the vendor
> > specifics like where the MSRs live, how many there are etc..
> 
> Yep, but there will still be issues with perf when booted on a guest and
> kvm not supporting it. And AFAIU, they're signalling this by turning off
> CPUID bits so that initializing perf doesn't happen.
> 
> So, I think init_hw_perf_events should as a first step look at CPUID
> bits and then do anything else. And this is done on Intel with
> X86_FEATURE_ARCH_PERFMON. But Robert is fixing this on AMD too so...

But not all x86 hardware even has the stuff enumerated in CPUID, and afaict
Intel and AMD use a different CPUID bit as well, so what's
init_hw_perf_events() to do?

Re: Drop WARN on AMD lack of perfctrs

[Borislav Petkov]

On Fri, May 17, 2013 at 11:27:41AM +0200, Peter Zijlstra wrote:
> But not all x86 hardware even has the stuff enumerated in CPUID, and
> afaict Intel and AMD use a different CPUID bit as well, so what's
> init_hw_perf_events() to do?

Yeah, I think the best solution would be if we force-enable the CPUID
bit on F10h very early and teach amd_pmu_init() to look at it. I even
had a patch which does something like that. I could dust it off and give
it a try... I just hope we can actually enable a reserved bit in CPUID.

:-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: Drop WARN on AMD lack of perfctrs

[Robert Richter]

On 17.05.13 11:45:51, Borislav Petkov wrote:
> On Fri, May 17, 2013 at 11:27:41AM +0200, Peter Zijlstra wrote:
> > But not all x86 hardware even has the stuff enumerated in CPUID, and
> > afaict Intel and AMD use a different CPUID bit as well, so what's
> > init_hw_perf_events() to do?
> 
> Yeah, I think the best solution would be if we force-enable the CPUID
> bit on F10h very early and teach amd_pmu_init() to look at it. I even
> had a patch which does something like that. I could dust it off and give
> it a try... I just hope we can actually enable a reserved bit in CPUID.

The cpuid bit indicates perfctrs that do not exist, this will setup a
wrong msr range on f10h. I guess the warning is harmless and the code
works properly, but I can't tell for sure now and need to look at it.

Also, the problem occurs on f15h there *no* core perfctrs exist but
are expected, not on a f10h system.

-Robert

Re: Drop WARN on AMD lack of perfctrs

[Peter Zijlstra]

So what about something like the below?

---
 arch/x86/kernel/cpu/perf_event_amd.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_amd.c b/arch/x86/kernel/cpu/perf_event_amd.c
index 7e28d94..87e8a7e 100644
--- a/arch/x86/kernel/cpu/perf_event_amd.c
+++ b/arch/x86/kernel/cpu/perf_event_amd.c
@@ -648,24 +648,19 @@ static __initconst const struct x86_pmu amd_pmu = {
 	.cpu_dead		= amd_pmu_cpu_dead,
 };
 
-static int setup_event_constraints(void)
+__init int amd_core_pmu_init(void)
 {
-	if (boot_cpu_data.x86 == 0x15)
+	switch (boot_cpu_data.x86) {
+	case 0x15:
+		pr_cont("Fam15h ");
 		x86_pmu.get_event_constraints = amd_get_event_constraints_f15h;
-	return 0;
-}
+		break;
 
-static int setup_perfctr_core(void)
-{
-	if (!cpu_has_perfctr_core) {
-		WARN(x86_pmu.get_event_constraints == amd_get_event_constraints_f15h,
-		     KERN_ERR "Odd, counter constraints enabled but no core perfctrs detected!");
+	default:
+		pr_err("core perfctr but no constraints; unknown hardware!\n");
 		return -ENODEV;
 	}
 
-	WARN(x86_pmu.get_event_constraints == amd_get_event_constraints,
-	     KERN_ERR "hw perf events core counters need constraints handler!");
-
 	/*
 	 * If core performance counter extensions exists, we must use
 	 * MSR_F15H_PERF_CTL/MSR_F15H_PERF_CTR msrs. See also
@@ -675,8 +670,7 @@ static int setup_perfctr_core(void)
 	x86_pmu.perfctr		= MSR_F15H_PERF_CTR;
 	x86_pmu.num_counters	= AMD64_NUM_COUNTERS_CORE;
 
-	printk(KERN_INFO "perf: AMD core performance counters detected\n");
-
+	pr_cont("core perfctr, ");
 	return 0;
 }
 
@@ -688,8 +682,8 @@ __init int amd_pmu_init(void)
 
 	x86_pmu = amd_pmu;
 
-	setup_event_constraints();
-	setup_perfctr_core();
+	if (cpu_has_perfctr_core && amd_core_pmu_init())
+		return -ENODEV;
 
 	/* Events are common for all AMDs */
 	memcpy(hw_cache_event_ids, amd_hw_cache_event_ids,

Re: Drop WARN on AMD lack of perfctrs

[Robert Richter]

On 17.05.13 12:57:30, Peter Zijlstra wrote:
> 
> So what about something like the below?

See my comments below, otherwise it looks fine to me.

There is the question about core performance counters and its
constraints on fam16h. Not sure if there are any. Cc'ing Jacob.

-Robert

> 
> ---
>  arch/x86/kernel/cpu/perf_event_amd.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/perf_event_amd.c b/arch/x86/kernel/cpu/perf_event_amd.c
> index 7e28d94..87e8a7e 100644
> --- a/arch/x86/kernel/cpu/perf_event_amd.c
> +++ b/arch/x86/kernel/cpu/perf_event_amd.c
> @@ -648,24 +648,19 @@ static __initconst const struct x86_pmu amd_pmu = {
>  	.cpu_dead		= amd_pmu_cpu_dead,
>  };
>  
> -static int setup_event_constraints(void)
> +__init int amd_core_pmu_init(void)
>  {
> -	if (boot_cpu_data.x86 == 0x15)
> +	switch (boot_cpu_data.x86) {
> +	case 0x15:
> +		pr_cont("Fam15h ");
>  		x86_pmu.get_event_constraints = amd_get_event_constraints_f15h;
> -	return 0;
> -}
> +		break;
>  
> -static int setup_perfctr_core(void)
> -{
> -	if (!cpu_has_perfctr_core) {
> -		WARN(x86_pmu.get_event_constraints == amd_get_event_constraints_f15h,
> -		     KERN_ERR "Odd, counter constraints enabled but no core perfctrs detected!");
> +	default:
> +		pr_err("core perfctr but no constraints; unknown hardware!\n");
>  		return -ENODEV;
>  	}
>  
> -	WARN(x86_pmu.get_event_constraints == amd_get_event_constraints,
> -	     KERN_ERR "hw perf events core counters need constraints handler!");
> -
>  	/*
>  	 * If core performance counter extensions exists, we must use
>  	 * MSR_F15H_PERF_CTL/MSR_F15H_PERF_CTR msrs. See also

... amd_pmu_addr_offset():

         * If core performance counter extensions exists, we must use
         * MSR_F15H_PERF_CTL/MSR_F15H_PERF_CTR msrs. See also
         * amd_pmu_addr_offset().

> @@ -675,8 +670,7 @@ static int setup_perfctr_core(void)
>  	x86_pmu.perfctr		= MSR_F15H_PERF_CTR;
>  	x86_pmu.num_counters	= AMD64_NUM_COUNTERS_CORE;
>  
> -	printk(KERN_INFO "perf: AMD core performance counters detected\n");
> -
> +	pr_cont("core perfctr, ");
>  	return 0;
>  }
>  
> @@ -688,8 +682,8 @@ __init int amd_pmu_init(void)
>  
>  	x86_pmu = amd_pmu;
>  
> -	setup_event_constraints();
> -	setup_perfctr_core();
> +	if (cpu_has_perfctr_core && amd_core_pmu_init())
> +		return -ENODEV;

Better return result of amd_core_pmu_init().

>  
>  	/* Events are common for all AMDs */
>  	memcpy(hw_cache_event_ids, amd_hw_cache_event_ids,
> 

Re: Drop WARN on AMD lack of perfctrs

[Peter Zijlstra]

On Tue, May 21, 2013 at 10:56:30AM +0200, Robert Richter wrote:
> On 17.05.13 12:57:30, Peter Zijlstra wrote:
> > 
> > So what about something like the below?
> 
> See my comments below, otherwise it looks fine to me.

I've been liberal and read an Ack there, holler if that needs be amended.

> There is the question about core performance counters and its
> constraints on fam16h. Not sure if there are any. Cc'ing Jacob.

Currently the code doesn't work for Fam16h afaict, so I didn't wreck
that did I?

> >  	/*
> >  	 * If core performance counter extensions exists, we must use
> >  	 * MSR_F15H_PERF_CTL/MSR_F15H_PERF_CTR msrs. See also
> 
> ... amd_pmu_addr_offset():
> 
>          * If core performance counter extensions exists, we must use
>          * MSR_F15H_PERF_CTL/MSR_F15H_PERF_CTR msrs. See also
>          * amd_pmu_addr_offset().
> 

Done.

> > @@ -675,8 +670,7 @@ static int setup_perfctr_core(void)
> >  	x86_pmu.perfctr		= MSR_F15H_PERF_CTR;
> >  	x86_pmu.num_counters	= AMD64_NUM_COUNTERS_CORE;
> >  
> > -	printk(KERN_INFO "perf: AMD core performance counters detected\n");
> > -
> > +	pr_cont("core perfctr, ");
> >  	return 0;
> >  }
> >  
> > @@ -688,8 +682,8 @@ __init int amd_pmu_init(void)
> >  
> >  	x86_pmu = amd_pmu;
> >  
> > -	setup_event_constraints();
> > -	setup_perfctr_core();
> > +	if (cpu_has_perfctr_core && amd_core_pmu_init())
> > +		return -ENODEV;
> 
> Better return result of amd_core_pmu_init().

Done.. that was admittedly a tad lazy of me :-)

---
Subject: perf, x86: Rework AMD PMU init code
From: Peter Zijlstra <peterz@infradead.org>
Date: Fri, 17 May 2013 12:57:30 +0200

Josh reported that his QEMU is a bad hardware emulator and trips a
WARN in the AMD PMU init code. He requested the WARN be turned into a
pr_err() or similar.

While there, rework the code a little.

Reported-by: Josh Boyer <jwboyer@redhat.com>
Acked-by: Robert Richter <rric@kernel.org>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 arch/x86/kernel/cpu/perf_event_amd.c |   34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

--- a/arch/x86/kernel/cpu/perf_event_amd.c
+++ b/arch/x86/kernel/cpu/perf_event_amd.c
@@ -648,48 +648,48 @@ static __initconst const struct x86_pmu
 	.cpu_dead		= amd_pmu_cpu_dead,
 };
 
-static int setup_event_constraints(void)
+__init int amd_core_pmu_init(void)
 {
-	if (boot_cpu_data.x86 == 0x15)
+	if (!cpu_has_perfctr_core)
+		return 0;
+
+	switch (boot_cpu_data.x86) {
+	case 0x15:
+		pr_cont("Fam15h ");
 		x86_pmu.get_event_constraints = amd_get_event_constraints_f15h;
-	return 0;
-}
+		break;
 
-static int setup_perfctr_core(void)
-{
-	if (!cpu_has_perfctr_core) {
-		WARN(x86_pmu.get_event_constraints == amd_get_event_constraints_f15h,
-		     KERN_ERR "Odd, counter constraints enabled but no core perfctrs detected!");
+	default:
+		pr_err("core perfctr but no constraints; unknown hardware!\n");
 		return -ENODEV;
 	}
 
-	WARN(x86_pmu.get_event_constraints == amd_get_event_constraints,
-	     KERN_ERR "hw perf events core counters need constraints handler!");
-
 	/*
 	 * If core performance counter extensions exists, we must use
 	 * MSR_F15H_PERF_CTL/MSR_F15H_PERF_CTR msrs. See also
-	 * x86_pmu_addr_offset().
+	 * amd_pmu_addr_offset().
 	 */
 	x86_pmu.eventsel	= MSR_F15H_PERF_CTL;
 	x86_pmu.perfctr		= MSR_F15H_PERF_CTR;
 	x86_pmu.num_counters	= AMD64_NUM_COUNTERS_CORE;
 
-	printk(KERN_INFO "perf: AMD core performance counters detected\n");
-
+	pr_cont("core perfctr, ");
 	return 0;
 }
 
 __init int amd_pmu_init(void)
 {
+	int ret;
+
 	/* Performance-monitoring supported from K7 and later: */
 	if (boot_cpu_data.x86 < 6)
 		return -ENODEV;
 
 	x86_pmu = amd_pmu;
 
-	setup_event_constraints();
-	setup_perfctr_core();
+	ret = amd_core_pmu_init();
+	if (ret)
+		return ret;
 
 	/* Events are common for all AMDs */
 	memcpy(hw_cache_event_ids, amd_hw_cache_event_ids,

Re: Drop WARN on AMD lack of perfctrs

[Robert Richter]

On 21.05.13 13:05:37, Peter Zijlstra wrote:
> Reported-by: Josh Boyer <jwboyer@redhat.com>
> Acked-by: Robert Richter <rric@kernel.org>

Fine with me.

> -static int setup_event_constraints(void)
> +__init int amd_core_pmu_init(void)

Just noticed this should be static int __init.

Thanks,

-Robert

Re: Drop WARN on AMD lack of perfctrs

[Jacob Shin]

On Tue, May 21, 2013 at 01:05:37PM +0200, Peter Zijlstra wrote:
> On Tue, May 21, 2013 at 10:56:30AM +0200, Robert Richter wrote:
> > On 17.05.13 12:57:30, Peter Zijlstra wrote:
> > > 
> > > So what about something like the below?
> > 
> > See my comments below, otherwise it looks fine to me.
> 
> I've been liberal and read an Ack there, holler if that needs be amended.
> 
> > There is the question about core performance counters and its
> > constraints on fam16h. Not sure if there are any. Cc'ing Jacob.
> 
> Currently the code doesn't work for Fam16h afaict, so I didn't wreck
> that did I?

Hi, right, Family 16h does not have perfctr_core, instead it still has
the 4 legacy performance counter registers just like Family 10h, and
so does not have any special constraints as 15h's perfctr_core does.

Thanks!

Acked-by: Jacob Shin <jacob.shin@amd.com>

> 
> > >  	/*
> > >  	 * If core performance counter extensions exists, we must use
> > >  	 * MSR_F15H_PERF_CTL/MSR_F15H_PERF_CTR msrs. See also
> > 
> > ... amd_pmu_addr_offset():
> > 
> >          * If core performance counter extensions exists, we must use
> >          * MSR_F15H_PERF_CTL/MSR_F15H_PERF_CTR msrs. See also
> >          * amd_pmu_addr_offset().
> > 
> 
> Done.
> 
> > > @@ -675,8 +670,7 @@ static int setup_perfctr_core(void)
> > >  	x86_pmu.perfctr		= MSR_F15H_PERF_CTR;
> > >  	x86_pmu.num_counters	= AMD64_NUM_COUNTERS_CORE;
> > >  
> > > -	printk(KERN_INFO "perf: AMD core performance counters detected\n");
> > > -
> > > +	pr_cont("core perfctr, ");
> > >  	return 0;
> > >  }
> > >  
> > > @@ -688,8 +682,8 @@ __init int amd_pmu_init(void)
> > >  
> > >  	x86_pmu = amd_pmu;
> > >  
> > > -	setup_event_constraints();
> > > -	setup_perfctr_core();
> > > +	if (cpu_has_perfctr_core && amd_core_pmu_init())
> > > +		return -ENODEV;
> > 
> > Better return result of amd_core_pmu_init().
> 
> Done.. that was admittedly a tad lazy of me :-)
> 
> ---
> Subject: perf, x86: Rework AMD PMU init code
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Fri, 17 May 2013 12:57:30 +0200
> 
> Josh reported that his QEMU is a bad hardware emulator and trips a
> WARN in the AMD PMU init code. He requested the WARN be turned into a
> pr_err() or similar.
> 
> While there, rework the code a little.
> 
> Reported-by: Josh Boyer <jwboyer@redhat.com>
> Acked-by: Robert Richter <rric@kernel.org>
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> ---
>  arch/x86/kernel/cpu/perf_event_amd.c |   34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> --- a/arch/x86/kernel/cpu/perf_event_amd.c
> +++ b/arch/x86/kernel/cpu/perf_event_amd.c
> @@ -648,48 +648,48 @@ static __initconst const struct x86_pmu
>  	.cpu_dead		= amd_pmu_cpu_dead,
>  };
>  
> -static int setup_event_constraints(void)
> +__init int amd_core_pmu_init(void)
>  {
> -	if (boot_cpu_data.x86 == 0x15)
> +	if (!cpu_has_perfctr_core)
> +		return 0;
> +
> +	switch (boot_cpu_data.x86) {
> +	case 0x15:
> +		pr_cont("Fam15h ");
>  		x86_pmu.get_event_constraints = amd_get_event_constraints_f15h;
> -	return 0;
> -}
> +		break;
>  
> -static int setup_perfctr_core(void)
> -{
> -	if (!cpu_has_perfctr_core) {
> -		WARN(x86_pmu.get_event_constraints == amd_get_event_constraints_f15h,
> -		     KERN_ERR "Odd, counter constraints enabled but no core perfctrs detected!");
> +	default:
> +		pr_err("core perfctr but no constraints; unknown hardware!\n");
>  		return -ENODEV;
>  	}
>  
> -	WARN(x86_pmu.get_event_constraints == amd_get_event_constraints,
> -	     KERN_ERR "hw perf events core counters need constraints handler!");
> -
>  	/*
>  	 * If core performance counter extensions exists, we must use
>  	 * MSR_F15H_PERF_CTL/MSR_F15H_PERF_CTR msrs. See also
> -	 * x86_pmu_addr_offset().
> +	 * amd_pmu_addr_offset().
>  	 */
>  	x86_pmu.eventsel	= MSR_F15H_PERF_CTL;
>  	x86_pmu.perfctr		= MSR_F15H_PERF_CTR;
>  	x86_pmu.num_counters	= AMD64_NUM_COUNTERS_CORE;
>  
> -	printk(KERN_INFO "perf: AMD core performance counters detected\n");
> -
> +	pr_cont("core perfctr, ");
>  	return 0;
>  }
>  
>  __init int amd_pmu_init(void)
>  {
> +	int ret;
> +
>  	/* Performance-monitoring supported from K7 and later: */
>  	if (boot_cpu_data.x86 < 6)
>  		return -ENODEV;
>  
>  	x86_pmu = amd_pmu;
>  
> -	setup_event_constraints();
> -	setup_perfctr_core();
> +	ret = amd_core_pmu_init();
> +	if (ret)
> +		return ret;
>  
>  	/* Events are common for all AMDs */
>  	memcpy(hw_cache_event_ids, amd_hw_cache_event_ids,
> 
> 

[tip:perf/core] perf/x86/amd: Rework AMD PMU init code

[tip-bot for Peter Zijlstra]

Commit-ID:  1b45adcd9a503428e6de6b39bc6892d86c9c1d41
Gitweb:     http://git.kernel.org/tip/1b45adcd9a503428e6de6b39bc6892d86c9c1d41
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Tue, 21 May 2013 13:05:37 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 28 May 2013 09:13:55 +0200

perf/x86/amd: Rework AMD PMU init code

Josh reported that his QEMU is a bad hardware emulator and trips a
WARN in the AMD PMU init code. He requested the WARN be turned into a
pr_err() or similar.

While there, rework the code a little.

Reported-by: Josh Boyer <jwboyer@redhat.com>
Acked-by: Robert Richter <rric@kernel.org>
Acked-by: Jacob Shin <jacob.shin@amd.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20130521110537.GG26912@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/cpu/perf_event_amd.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_amd.c b/arch/x86/kernel/cpu/perf_event_amd.c
index 7e28d94..4cbe032 100644
--- a/arch/x86/kernel/cpu/perf_event_amd.c
+++ b/arch/x86/kernel/cpu/perf_event_amd.c
@@ -648,48 +648,48 @@ static __initconst const struct x86_pmu amd_pmu = {
 	.cpu_dead		= amd_pmu_cpu_dead,
 };
 
-static int setup_event_constraints(void)
+static int __init amd_core_pmu_init(void)
 {
-	if (boot_cpu_data.x86 == 0x15)
+	if (!cpu_has_perfctr_core)
+		return 0;
+
+	switch (boot_cpu_data.x86) {
+	case 0x15:
+		pr_cont("Fam15h ");
 		x86_pmu.get_event_constraints = amd_get_event_constraints_f15h;
-	return 0;
-}
+		break;
 
-static int setup_perfctr_core(void)
-{
-	if (!cpu_has_perfctr_core) {
-		WARN(x86_pmu.get_event_constraints == amd_get_event_constraints_f15h,
-		     KERN_ERR "Odd, counter constraints enabled but no core perfctrs detected!");
+	default:
+		pr_err("core perfctr but no constraints; unknown hardware!\n");
 		return -ENODEV;
 	}
 
-	WARN(x86_pmu.get_event_constraints == amd_get_event_constraints,
-	     KERN_ERR "hw perf events core counters need constraints handler!");
-
 	/*
 	 * If core performance counter extensions exists, we must use
 	 * MSR_F15H_PERF_CTL/MSR_F15H_PERF_CTR msrs. See also
-	 * x86_pmu_addr_offset().
+	 * amd_pmu_addr_offset().
 	 */
 	x86_pmu.eventsel	= MSR_F15H_PERF_CTL;
 	x86_pmu.perfctr		= MSR_F15H_PERF_CTR;
 	x86_pmu.num_counters	= AMD64_NUM_COUNTERS_CORE;
 
-	printk(KERN_INFO "perf: AMD core performance counters detected\n");
-
+	pr_cont("core perfctr, ");
 	return 0;
 }
 
 __init int amd_pmu_init(void)
 {
+	int ret;
+
 	/* Performance-monitoring supported from K7 and later: */
 	if (boot_cpu_data.x86 < 6)
 		return -ENODEV;
 
 	x86_pmu = amd_pmu;
 
-	setup_event_constraints();
-	setup_perfctr_core();
+	ret = amd_core_pmu_init();
+	if (ret)
+		return ret;
 
 	/* Events are common for all AMDs */
 	memcpy(hw_cache_event_ids, amd_hw_cache_event_ids,

Re: Drop WARN on AMD lack of perfctrs

[Josh Boyer]

On Thu, May 16, 2013 at 10:55:58PM +0200, Robert Richter wrote:
> On 16.05.13 20:10:18, Peter Zijlstra wrote:
> > On Thu, May 16, 2013 at 01:55:57PM -0400, Josh Boyer wrote:
> > > Maybe.  But do you really need to dump a stack trace here?  What is a
> > > user supposed to do with that information?  Can they fix the kernel?
> > > Can the fix the CPU?  As far as I can tell, they can't do either.
> > 
> > Send their CPU back to AMD is I suppose the best they can do ;-)
> >  
> > > Is using pr_err with the same message really somehow worse than using
> > > WARN?
> > 
> > I would make it a FW_BUG as well. But yeah, I suppose that is a better option
> > than the WARN_ON. Unless Robert had a different reason...
> 
> iirc the reason was the different msr range that is switched on fam15h
> with a different counter to counter msr offset of 2 instead of 1. The
> code relies on the assumption that the msrs exist on that cpu. Thus
> the warning if not. Also note that code may have changed in 3.10 in
> that area.

Again, what is someone supposed to do with a backtrace from the WARN?
As far as I can see, a user can't really do anything other than report
it and then there's nothing to fix.

The code might have moved in 3.10, but the WARN is still there.

Would you like me to send a patch to get it reduced to pr_err?

josh

Re: Drop WARN on AMD lack of perfctrs

[Robert Richter]

On 16.05.13 17:54:59, Josh Boyer wrote:
> On Thu, May 16, 2013 at 10:55:58PM +0200, Robert Richter wrote:
> > On 16.05.13 20:10:18, Peter Zijlstra wrote:
> > > I would make it a FW_BUG as well. But yeah, I suppose that is a better option
> > > than the WARN_ON. Unless Robert had a different reason...
> > 
> > iirc the reason was the different msr range that is switched on fam15h
> > with a different counter to counter msr offset of 2 instead of 1. The
> > code relies on the assumption that the msrs exist on that cpu. Thus
> > the warning if not. Also note that code may have changed in 3.10 in
> > that area.
> 
> Again, what is someone supposed to do with a backtrace from the WARN?
> As far as I can see, a user can't really do anything other than report
> it and then there's nothing to fix.
> 
> The code might have moved in 3.10, but the WARN is still there.
> 
> Would you like me to send a patch to get it reduced to pr_err?

I wrote the code with the assumption of a certain system layout which
was the check for. Now, in a vm this assumption is no longer valid.
Will change the code in a way this is handled properly.

-Robert

Re: Drop WARN on AMD lack of perfctrs

[Gleb Natapov]

On Thu, May 16, 2013 at 07:51:17PM +0200, Peter Zijlstra wrote:
> On Thu, May 16, 2013 at 11:10:26AM -0400, Josh Boyer wrote:
> > Hi All,
> > 
> > If you boot a KVM guest on an AMD family 15h and specify -cpu host,
> > you'll get the following splat:
> > 
> > [    0.031000] ------------[ cut here ]------------
> > [    0.031000] WARNING: at arch/x86/kernel/cpu/perf_event_amd.c:772
> > amd_pmu_init+0x18c/0x249()
> > [    0.031000] Hardware name: Bochs
> > [    0.031000] Odd, counter constraints enabled but no core perfctrs
> > detected!
> > [    0.031000] Modules linked in:
> > 
> > [    0.031000] Pid: 1, comm: swapper/0 Not tainted
> > 3.9.0-0.rc1.git0.4.fc19.x86_64 #1
> > [    0.031000] Call Trace:
> > [    0.031000]  [<ffffffff81d10c67>] ? amd_pmu_init+0x18c/0x249
> > [    0.031000]  [<ffffffff8105c9a0>] warn_slowpath_common+0x70/0xa0
> > [    0.031000]  [<ffffffff81d106b3>] ? check_bugs+0x2d/0x2d
> > [    0.031000]  [<ffffffff8105ca1c>] warn_slowpath_fmt+0x4c/0x50
> > [    0.031000]  [<ffffffff81d10c67>] amd_pmu_init+0x18c/0x249
> > [    0.031000]  [<ffffffff81d106e7>] init_hw_perf_events+0x34/0x428
> > [    0.031000]  [<ffffffff81d106b3>] ? check_bugs+0x2d/0x2d
> > [    0.031000]  [<ffffffff8100210a>] do_one_initcall+0x10a/0x160
> > [    0.031000]  [<ffffffff81d06fa5>] kernel_init_freeable+0xcf/0x1fa
> > [    0.031000]  [<ffffffff81629800>] ? rest_init+0x80/0x80
> > [    0.031000]  [<ffffffff8162980e>] kernel_init+0xe/0x190
> > [    0.031000]  [<ffffffff8164e22c>] ret_from_fork+0x7c/0xb0
> > [    0.031000]  [<ffffffff81629800>] ? rest_init+0x80/0x80
> > [    0.031000] ---[ end trace a1e57d3cb8668105 ]---
> > 
> > That seems a bit excessive, and it gets picked up by auto-reporting
> > tools like ABRT as a bug.  Can we remove the WARN and just use pr_err or
> > something else instead?
> 
> Robert put that in, I suppose its because the CPUID crap indicates its got perf
> counters but then it doesn't actually have them.
> 
Actually it looks like it is the opposite: CPUID crap indicates
it got no perf, but the code expects this cpu model to have it.

> Clearly this is something that should be fixed in your virt thingy instead.
The only way to fix it is to implement perf virtualization for AMD, but
then it is odd for a guest to try and match CPUIDs with CPU model. This
defeats the purpose of CPUIDs in the first place.

--
			Gleb.

Re: Drop WARN on AMD lack of perfctrs

[Borislav Petkov]

On Thu, May 16, 2013 at 10:31:37PM +0300, Gleb Natapov wrote:
> Actually it looks like it is the opposite: CPUID crap indicates
> it got no perf, but the code expects this cpu model to have it.

Yep, AFAICT the code should look at cpu_has_perfctr_core already in
amd_pmu_init() and exit if the CPUID bit is not set.

Robert?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
-
-