lkml.org 
[lkml]   [2010]   [Mar]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 0/3] perf/core, x86: unify perfctr bitmasks
On 30.03.10 17:00:55, Peter Zijlstra wrote:
> On Tue, 2010-03-30 at 15:53 +0200, Stephane Eranian wrote:
> > > So, there are some ARCH_PERFMON_EVENTSEL_* masks that are Intel only,
> > > which is true for ARCH_PERFMON_EVENTSEL_ANY. And indead, the code
> > > should be checked for this. ARCH_PERFMON_EVENTSEL_ANY is always
> > > cleared on AMD cpus, so this code is ok. Actually the bit is cleared
> >
> > Until AMD uses that bit too and you won't notice this test. This is a security
> > check specific to Intel and it should be in an Intel-specific function.
> >
> > > for *all* cpus in x86_pmu_raw_event(), the code was and is broken for
> > > this.
> > >
> > Yes, needs to be authorized for any perfmon v3 and later revisions.
>
> So how about something like this on top of Robert's patches?
>
> ---
> arch/x86/kernel/cpu/perf_event.c | 16 ++++++----------
> arch/x86/kernel/cpu/perf_event_amd.c | 5 +++--
> arch/x86/kernel/cpu/perf_event_intel.c | 15 ++++++++++++++-
> arch/x86/kernel/cpu/perf_event_p4.c | 5 +++--
> 4 files changed, 26 insertions(+), 15 deletions(-)
>
> Index: linux-2.6/arch/x86/kernel/cpu/perf_event.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/cpu/perf_event.c
> +++ linux-2.6/arch/x86/kernel/cpu/perf_event.c
> @@ -201,7 +201,7 @@ struct x86_pmu {
> unsigned eventsel;
> unsigned perfctr;
> u64 (*event_map)(int);
> - u64 (*raw_event)(u64);
> + int (*raw_event)(struct perf_event *event);
> int max_events;
> int num_counters;
> int num_counters_fixed;
> @@ -445,9 +445,10 @@ static int x86_hw_config(struct perf_eve
> return 0;
> }
>
> -static u64 x86_pmu_raw_event(u64 hw_event)
> +static int x86_pmu_raw_event(struct perf_event *event)
> {
> - return hw_event & X86_RAW_EVENT_MASK;
> + event->hw.config |= event->attr.config & X86_RAW_EVENT_MASK;

This still clears the ANY bit. See below.

> + return 0;
> }
>
> /*
> @@ -511,13 +512,8 @@ static int __hw_perf_event_init(struct p
> /*
> * Raw hw_event type provide the config in the hw_event structure
> */
> - if (attr->type == PERF_TYPE_RAW) {
> - hwc->config |= x86_pmu.raw_event(attr->config);
> - if ((hwc->config & ARCH_PERFMON_EVENTSEL_ANY) &&
> - perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN))
> - return -EACCES;
> - return 0;
> - }
> + if (attr->type == PERF_TYPE_RAW)
> + return x86_pmu.raw_event(event);

We could go on with this and move this into *_hw_config(), but so far
this change is fine to me. This would then remove .raw_event() at all.

[...]

> Index: linux-2.6/arch/x86/kernel/cpu/perf_event_intel.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/cpu/perf_event_intel.c
> +++ linux-2.6/arch/x86/kernel/cpu/perf_event_intel.c
> @@ -758,6 +758,19 @@ intel_get_event_constraints(struct cpu_h
> return x86_get_event_constraints(cpuc, event);
> }
>
> +static int intel_pmu_raw_event(struct perf_event *event)
> +{
> + int ret = x86_pmu_raw_event(event);
> + if (ret)
> + return ret;
> +
> + if ((event->hw.config & ARCH_PERFMON_EVENTSEL_ANY) &&
> + perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN))
> + return -EACCES;
> +
> + return 0;

Maybe this?

if (!(event->attr.config & ARCH_PERFMON_EVENTSEL_ANY))
return 0;
if (perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN))
return -EACCES;
event->hw.config |= ARCH_PERFMON_EVENTSEL_ANY;
return 0;

-Robert

--
Advanced Micro Devices, Inc.
Operating System Research Center
email: robert.richter@amd.com



\
 
 \ /
  Last update: 2010-03-30 18:03    [W:0.637 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site