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

Jaswinder Singh Rajput
2009-05-10T05:04:31Z

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