Re: [tip:perfcounters/core] perf_counter, x86: remove X86_FEATURE_ARCH_PERFMON flag for AMD cpus

[Jaswinder Singh Rajput <jaswinder@kernel.org>]

Hello Robert,

On Wed, 2009-04-29 at 13:03 +0000, tip-bot for Robert Richter wrote:
> Commit-ID:  da1a776be1ac7f78bb30ececbec4c1383163b079
> Gitweb:     http://git.kernel.org/tip/da1a776be1ac7f78bb30ececbec4c1383163b079
> Author:     Robert Richter <robert.richter@amd.com>
> AuthorDate: Wed, 29 Apr 2009 12:46:58 +0200
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Wed, 29 Apr 2009 14:51:00 +0200
> 
> perf_counter, x86: remove X86_FEATURE_ARCH_PERFMON flag for AMD cpus
> 
> X86_FEATURE_ARCH_PERFMON is an Intel hardware feature that does not
> work on AMD CPUs. The flag is now only used in Intel specific code
> (especially initialization).
> 

I think X86_FEATURE_ARCH_PERFMON should be for all x86 cpus which
support performance monitor.

#define X86_FEATURE_ARCH_PERFMON (3*32+11) /* Intel Architectural PerfMon */
should be replaced by
#define X86_FEATURE_ARCH_PERFMON (3*32+11) /* Performace Monitor */

All x86 cpus which support performance monitor should set this feature.

> [ Impact: refactor code ]
> 
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

ack by Peter ;-)

> Cc: Paul Mackerras <paulus@samba.org>
> LKML-Reference: <1241002046-8832-2-git-send-email-robert.richter@amd.com>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> 
> ---
>  arch/x86/kernel/cpu/amd.c          |    4 ----
>  arch/x86/kernel/cpu/perf_counter.c |    6 +++---
>  2 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index fd69c51..7e4a459 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -420,10 +420,6 @@ static void __cpuinit init_amd(struct cpuinfo_x86 *c)
>  	if (c->x86 >= 6)
>  		set_cpu_cap(c, X86_FEATURE_FXSAVE_LEAK);
>  
> -	/* Enable Performance counter for K7 and later */
> -	if (c->x86 > 6 && c->x86 <= 0x11)
> -		set_cpu_cap(c, X86_FEATURE_ARCH_PERFMON);
> -


This should not be removed. This checks whether performance monitor
support is enable for AMD or not.

I think AMD will also support performance monitor for future models so:

	/* Enable Performance counter for K7 and later */
	if (c->x86 > 6)
		set_cpu_cap(c, X86_FEATURE_ARCH_PERFMON);


> diff --git a/arch/x86/kernel/cpu/perf_counter.c b/arch/x86/kernel/cpu/perf_counter.c
> index 0fcbaab..7d0f81d 100644
> --- a/arch/x86/kernel/cpu/perf_counter.c
> +++ b/arch/x86/kernel/cpu/perf_counter.c
> @@ -949,6 +949,9 @@ static struct pmc_x86_ops *pmc_intel_init(void)
>  	unsigned int unused;
>  	unsigned int ebx;
>  
> +	if (!cpu_has(&boot_cpu_data, X86_FEATURE_ARCH_PERFMON))
> +		return NULL;
> +
>  	/*
>  	 * Check whether the Architectural PerfMon supports
>  	 * Branch Misses Retired Event or not.
> @@ -987,9 +990,6 @@ static struct pmc_x86_ops *pmc_amd_init(void)
>  
>  void __init init_hw_perf_counters(void)
>  {
> -	if (!cpu_has(&boot_cpu_data, X86_FEATURE_ARCH_PERFMON))
> -		return;
> -

This is not safe for AMD. why you move this to intel only ? this checks
whether performance monitor is enable or disable.

For AMD, Is performance monitor is supported for < K7. where are you
checking it.

--
JSR