LinuxLists.cc - [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH

[permalink] [raw]

Subject: Re: [tip:perf/urgent] perf/x86/intel/lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

> > @@ -318,8 +318,11 @@ static void intel_pmu_setup_sw_lbr_filter(struct perf_event *event)
> > if (br_type & PERF_SAMPLE_BRANCH_USER)
> > mask |= X86_BR_USER;
> >
> > - if (br_type & PERF_SAMPLE_BRANCH_KERNEL)
> > + if (br_type & PERF_SAMPLE_BRANCH_KERNEL) {
> > + if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
> > + return -EACCES;
>
> It is probably not too late to amend this patch and remove the "-EACCES":
>
> arch/x86/kernel/cpu/perf_event_intel_lbr.c: In function ‘intel_pmu_setup_sw_lbr_filter’:
> arch/x86/kernel/cpu/perf_event_intel_lbr.c:323:4: warning: ‘return’ with a value, in function returning void [enabled by default]

Oh urgh, looks like I forgot a refresh before posting..

This one actually compiles a defconfig bzImage.

---
Subject: perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
From: Peter Zijlstra <[email protected]>
Date: Fri May 03 14:07:49 CEST 2013

We should always have proper privileges when requesting kernel data.

Cc: Andi Kleen <[email protected]>
Cc: [email protected]
Signed-off-by: Peter Zijlstra <[email protected]>
Link: http://lkml.kernel.org/n/[email protected]
---
arch/x86/kernel/cpu/perf_event_intel_lbr.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)

--- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
@@ -310,7 +310,7 @@ void intel_pmu_lbr_read(void)
* - in case there is no HW filter
* - in case the HW filter has errata or limitations
*/
-static void intel_pmu_setup_sw_lbr_filter(struct perf_event *event)
+static int intel_pmu_setup_sw_lbr_filter(struct perf_event *event)
{
u64 br_type = event->attr.branch_sample_type;
int mask = 0;
@@ -318,8 +318,11 @@ static void intel_pmu_setup_sw_lbr_filte
if (br_type & PERF_SAMPLE_BRANCH_USER)
mask |= X86_BR_USER;

- if (br_type & PERF_SAMPLE_BRANCH_KERNEL)
+ if (br_type & PERF_SAMPLE_BRANCH_KERNEL) {
+ if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
+ return -EACCES;
mask |= X86_BR_KERNEL;
+ }

/* we ignore BRANCH_HV here */

@@ -339,6 +342,8 @@ static void intel_pmu_setup_sw_lbr_filte
* be used by fixup code for some CPU
*/
event->hw.branch_reg.reg = mask;
+
+ return 0;
}

/*
@@ -375,7 +380,7 @@ static int intel_pmu_setup_hw_lbr_filter

int intel_pmu_setup_lbr_filter(struct perf_event *event)
{
- int ret = 0;
+ int ret;

/*
* no LBR on this PMU
@@ -386,7 +391,9 @@ int intel_pmu_setup_lbr_filter(struct pe
/*
* setup SW LBR filter
*/
- intel_pmu_setup_sw_lbr_filter(event);
+ ret = intel_pmu_setup_sw_lbr_filter(event);
+ if (ret)
+ return ret;

/*
* setup HW LBR filter, if any

2013-05-06 09:42:59

by Ingo Molnar

[permalink] [raw]

Subject: Re: [tip:perf/urgent] perf/x86/intel/lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

* Peter Zijlstra <[email protected]> wrote:

> > > @@ -318,8 +318,11 @@ static void intel_pmu_setup_sw_lbr_filter(struct perf_event *event)
> > > if (br_type & PERF_SAMPLE_BRANCH_USER)
> > > mask |= X86_BR_USER;
> > >
> > > - if (br_type & PERF_SAMPLE_BRANCH_KERNEL)
> > > + if (br_type & PERF_SAMPLE_BRANCH_KERNEL) {
> > > + if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
> > > + return -EACCES;
> >
> > It is probably not too late to amend this patch and remove the "-EACCES":
> >
> > arch/x86/kernel/cpu/perf_event_intel_lbr.c: In function ???intel_pmu_setup_sw_lbr_filter???:
> > arch/x86/kernel/cpu/perf_event_intel_lbr.c:323:4: warning: ???return??? with a value, in function returning void [enabled by default]
>
> Oh urgh, looks like I forgot a refresh before posting..
>
> This one actually compiles a defconfig bzImage.
>
> ---
> Subject: perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
> From: Peter Zijlstra <[email protected]>
> Date: Fri May 03 14:07:49 CEST 2013
>
> We should always have proper privileges when requesting kernel data.
>
> Cc: Andi Kleen <[email protected]>
> Cc: [email protected]
> Signed-off-by: Peter Zijlstra <[email protected]>
> Link: http://lkml.kernel.org/n/[email protected]
> ---
> arch/x86/kernel/cpu/perf_event_intel_lbr.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> --- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> @@ -310,7 +310,7 @@ void intel_pmu_lbr_read(void)
> * - in case there is no HW filter
> * - in case the HW filter has errata or limitations
> */
> -static void intel_pmu_setup_sw_lbr_filter(struct perf_event *event)
> +static int intel_pmu_setup_sw_lbr_filter(struct perf_event *event)
> {
> u64 br_type = event->attr.branch_sample_type;
> int mask = 0;
> @@ -318,8 +318,11 @@ static void intel_pmu_setup_sw_lbr_filte
> if (br_type & PERF_SAMPLE_BRANCH_USER)
> mask |= X86_BR_USER;
>
> - if (br_type & PERF_SAMPLE_BRANCH_KERNEL)
> + if (br_type & PERF_SAMPLE_BRANCH_KERNEL) {
> + if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
> + return -EACCES;
> mask |= X86_BR_KERNEL;
> + }
>
> /* we ignore BRANCH_HV here */
>
> @@ -339,6 +342,8 @@ static void intel_pmu_setup_sw_lbr_filte
> * be used by fixup code for some CPU
> */
> event->hw.branch_reg.reg = mask;
> +
> + return 0;
> }
>
> /*
> @@ -375,7 +380,7 @@ static int intel_pmu_setup_hw_lbr_filter
>
> int intel_pmu_setup_lbr_filter(struct perf_event *event)
> {
> - int ret = 0;
> + int ret;
>
> /*
> * no LBR on this PMU
> @@ -386,7 +391,9 @@ int intel_pmu_setup_lbr_filter(struct pe
> /*
> * setup SW LBR filter
> */
> - intel_pmu_setup_sw_lbr_filter(event);
> + ret = intel_pmu_setup_sw_lbr_filter(event);
> + if (ret)
> + return ret;
>
> /*
> * setup HW LBR filter, if any

That looks pretty close to what I did as well.

Thanks,

Ingo

2013-05-15 13:37:24

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

On Fri, May 3, 2013 at 2:11 PM, Peter Zijlstra <[email protected]> wrote:
> We should always have proper privileges when requesting kernel data.
>
> Cc: Andi Kleen <[email protected]>
> Cc: [email protected]
> Signed-off-by: Peter Zijlstra <[email protected]>
> Link: http://lkml.kernel.org/n/[email protected]
> ---
> arch/x86/kernel/cpu/perf_event_intel_lbr.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> --- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> @@ -318,8 +318,11 @@ static void intel_pmu_setup_sw_lbr_filte
> if (br_type & PERF_SAMPLE_BRANCH_USER)
> mask |= X86_BR_USER;
>
> - if (br_type & PERF_SAMPLE_BRANCH_KERNEL)
> + if (br_type & PERF_SAMPLE_BRANCH_KERNEL) {
> + if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
> + return -EACCES;
> mask |= X86_BR_KERNEL;
> + }
>
This will prevent regular users from capturing kernel -> kernel branches.
But it won't prevent users from getting kernel -> user branches. Thus
some kernel address will still be captured. I guess they could be eliminated
by the sw_filter.

When using LBR priv level filtering, the filter applies to the branch target
only.

2013-05-15 14:30:38

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

2013-05-16 09:11:05

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

On Wed, May 15, 2013 at 03:37:22PM +0200, Stephane Eranian wrote:
> On Fri, May 3, 2013 at 2:11 PM, Peter Zijlstra <[email protected]> wrote:
> > We should always have proper privileges when requesting kernel data.
> >
> > Cc: Andi Kleen <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Peter Zijlstra <[email protected]>
> > Link: http://lkml.kernel.org/n/[email protected]
> > ---
> > arch/x86/kernel/cpu/perf_event_intel_lbr.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > --- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> > +++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> > @@ -318,8 +318,11 @@ static void intel_pmu_setup_sw_lbr_filte
> > if (br_type & PERF_SAMPLE_BRANCH_USER)
> > mask |= X86_BR_USER;
> >
> > - if (br_type & PERF_SAMPLE_BRANCH_KERNEL)
> > + if (br_type & PERF_SAMPLE_BRANCH_KERNEL) {
> > + if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
> > + return -EACCES;
> > mask |= X86_BR_KERNEL;
> > + }
> >
> This will prevent regular users from capturing kernel -> kernel branches.
> But it won't prevent users from getting kernel -> user branches. Thus
> some kernel address will still be captured. I guess they could be eliminated
> by the sw_filter.
>
> When using LBR priv level filtering, the filter applies to the branch target
> only.

How about something like the below? It also adds the branch flags Mikey wanted
for PowerPC.

---
arch/x86/kernel/cpu/perf_event_intel_lbr.c | 12 +++++++++---
include/linux/perf_event.h | 10 +++++++---
2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel_lbr.c b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
index d978353..f44d635 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
@@ -585,17 +585,23 @@ intel_pmu_lbr_filter(struct cpu_hw_events *cpuc)

/* if type does not correspond, then discard */
if (type == X86_BR_NONE || (br_sel & type) != type) {
- cpuc->lbr_entries[i].from = 0;
+ cpuc->lbr_entries[i].__delete = 1;
compress = true;
}
+
+ /* hide kernel addresses if we're not privileged */
+ if (!(br_sel & X86_BR_KERNEL) && kernel_ip(from)) {
+ cpuc->lbr_entries[i].from = -1L;
+ cpuc->lbr_entries[i].invalid_from = 1;
+ }
}

if (!compress)
return;

- /* remove all entries with from=0 */
+ /* remove all entries with __delete */
for (i = 0; i < cpuc->lbr_stack.nr; ) {
- if (!cpuc->lbr_entries[i].from) {
+ if (cpuc->lbr_entries[i].__delete) {
j = i;
while (++j < cpuc->lbr_stack.nr)
cpuc->lbr_entries[j-1] = cpuc->lbr_entries[j];
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index f463a46..7acf1c9 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -77,9 +77,13 @@ struct perf_raw_record {
struct perf_branch_entry {
__u64 from;
__u64 to;
- __u64 mispred:1, /* target mispredicted */
- predicted:1,/* target predicted */
- reserved:62;
+ __u64 mispred:1, /* target mispredicted */
+ predicted:1, /* target predicted */
+ invalid_to:1, /* @to isn't to be trusted */
+ invalid_from:1, /* @from isn't to be trusted */
+ reserved:59,
+ __delete:1; /* Implementation; userspace should
+ always see a 0 */
};

/*

2013-05-16 09:18:56

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

On Thu, May 16, 2013 at 11:09:16AM +0200, Peter Zijlstra wrote:
> How about something like the below? It also adds the branch flags Mikey wanted
> for PowerPC.

The asymmetry is unfortunate, but I think its more useful to have the from
kernel branch target than it is not to have it. This way you at least know
there was a kernel entry/exit and where you've continued.

Without either branch to kernel and branch from kernel entries you'd be
wondering WTF happend to your control flow.

> ---
> arch/x86/kernel/cpu/perf_event_intel_lbr.c | 12 +++++++++---
> include/linux/perf_event.h | 10 +++++++---
> 2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/perf_event_intel_lbr.c b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> index d978353..f44d635 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> @@ -585,17 +585,23 @@ intel_pmu_lbr_filter(struct cpu_hw_events *cpuc)
>
> /* if type does not correspond, then discard */
> if (type == X86_BR_NONE || (br_sel & type) != type) {
> - cpuc->lbr_entries[i].from = 0;
> + cpuc->lbr_entries[i].__delete = 1;
> compress = true;
> }
> +
> + /* hide kernel addresses if we're not privileged */
> + if (!(br_sel & X86_BR_KERNEL) && kernel_ip(from)) {
> + cpuc->lbr_entries[i].from = -1L;
> + cpuc->lbr_entries[i].invalid_from = 1;
> + }
> }
>
> if (!compress)
> return;
>
> - /* remove all entries with from=0 */
> + /* remove all entries with __delete */
> for (i = 0; i < cpuc->lbr_stack.nr; ) {
> - if (!cpuc->lbr_entries[i].from) {
> + if (cpuc->lbr_entries[i].__delete) {
> j = i;
> while (++j < cpuc->lbr_stack.nr)
> cpuc->lbr_entries[j-1] = cpuc->lbr_entries[j];
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index f463a46..7acf1c9 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -77,9 +77,13 @@ struct perf_raw_record {
> struct perf_branch_entry {
> __u64 from;
> __u64 to;
> - __u64 mispred:1, /* target mispredicted */
> - predicted:1,/* target predicted */
> - reserved:62;
> + __u64 mispred:1, /* target mispredicted */
> + predicted:1, /* target predicted */
> + invalid_to:1, /* @to isn't to be trusted */
> + invalid_from:1, /* @from isn't to be trusted */
> + reserved:59,
> + __delete:1; /* Implementation; userspace should
> + always see a 0 */
> };
>
> /*
>

2013-05-16 10:09:40

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

Peter Zijlstra <[email protected]> wrote:

> On Wed, May 15, 2013 at 03:37:22PM +0200, Stephane Eranian wrote:
> > On Fri, May 3, 2013 at 2:11 PM, Peter Zijlstra <[email protected]> wrote:
> > > We should always have proper privileges when requesting kernel data.
> > >
> > > Cc: Andi Kleen <[email protected]>
> > > Cc: [email protected]
> > > Signed-off-by: Peter Zijlstra <[email protected]>
> > > Link: http://lkml.kernel.org/n/[email protected]
> > > ---
> > > arch/x86/kernel/cpu/perf_event_intel_lbr.c | 5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > --- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> > > +++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> > > @@ -318,8 +318,11 @@ static void intel_pmu_setup_sw_lbr_filte
> > > if (br_type & PERF_SAMPLE_BRANCH_USER)
> > > mask |= X86_BR_USER;
> > >
> > > - if (br_type & PERF_SAMPLE_BRANCH_KERNEL)
> > > + if (br_type & PERF_SAMPLE_BRANCH_KERNEL) {
> > > + if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
> > > + return -EACCES;
> > > mask |= X86_BR_KERNEL;
> > > + }
> > >
> > This will prevent regular users from capturing kernel -> kernel branches.
> > But it won't prevent users from getting kernel -> user branches. Thus
> > some kernel address will still be captured. I guess they could be eliminated
> > by the sw_filter.
> >
> > When using LBR priv level filtering, the filter applies to the branch target
> > only.
>
> How about something like the below? It also adds the branch flags
> Mikey wanted for PowerPC.
>
> ---
> arch/x86/kernel/cpu/perf_event_intel_lbr.c | 12 +++++++++---
> include/linux/perf_event.h | 10 +++++++---
> 2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/perf_event_intel_lbr.c b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> index d978353..f44d635 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> @@ -585,17 +585,23 @@ intel_pmu_lbr_filter(struct cpu_hw_events *cpuc)
>
> /* if type does not correspond, then discard */
> if (type == X86_BR_NONE || (br_sel & type) != type) {
> - cpuc->lbr_entries[i].from = 0;
> + cpuc->lbr_entries[i].__delete = 1;
> compress = true;
> }
> +
> + /* hide kernel addresses if we're not privileged */
> + if (!(br_sel & X86_BR_KERNEL) && kernel_ip(from)) {
> + cpuc->lbr_entries[i].from = -1L;
> + cpuc->lbr_entries[i].invalid_from = 1;
> + }
> }
>
> if (!compress)
> return;
>
> - /* remove all entries with from=0 */
> + /* remove all entries with __delete */
> for (i = 0; i < cpuc->lbr_stack.nr; ) {
> - if (!cpuc->lbr_entries[i].from) {
> + if (cpuc->lbr_entries[i].__delete) {
> j = i;
> while (++j < cpuc->lbr_stack.nr)
> cpuc->lbr_entries[j-1] = cpuc->lbr_entries[j];
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index f463a46..7acf1c9 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -77,9 +77,13 @@ struct perf_raw_record {
> struct perf_branch_entry {
> __u64 from;
> __u64 to;
> - __u64 mispred:1, /* target mispredicted */
> - predicted:1,/* target predicted */
> - reserved:62;
> + __u64 mispred:1, /* target mispredicted */
> + predicted:1, /* target predicted */
> + invalid_to:1, /* @to isn't to be trusted */
> + invalid_from:1, /* @from isn't to be trusted */

Thanks Peter. One possible issue...

When the kernel has to read the branch from memory, there is no way for
it to know that it's the same one that the HW actually executed. Hence
there's a possibility that the to address is invalid but we can't tell
for sure.

I'm happy to just ignore that and mark calculated to address as valid,
unless you think it would be worthwhile extra information to pass onto
the user?

If we wanted this extra fidelity we could add a possibly_invalid_to:1
flag to your patch but I'm not sure it's worth it to be honest.

mikey

2013-05-16 10:15:22

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

Peter Zijlstra <[email protected]> wrote:

> On Wed, May 15, 2013 at 03:37:22PM +0200, Stephane Eranian wrote:
> > On Fri, May 3, 2013 at 2:11 PM, Peter Zijlstra <[email protected]> wrote:
> > > We should always have proper privileges when requesting kernel data.
> > >
> > > Cc: Andi Kleen <[email protected]>
> > > Cc: [email protected]
> > > Signed-off-by: Peter Zijlstra <[email protected]>
> > > Link: http://lkml.kernel.org/n/[email protected]
> > > ---
> > > arch/x86/kernel/cpu/perf_event_intel_lbr.c | 5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > --- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> > > +++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> > > @@ -318,8 +318,11 @@ static void intel_pmu_setup_sw_lbr_filte
> > > if (br_type & PERF_SAMPLE_BRANCH_USER)
> > > mask |= X86_BR_USER;
> > >
> > > - if (br_type & PERF_SAMPLE_BRANCH_KERNEL)
> > > + if (br_type & PERF_SAMPLE_BRANCH_KERNEL) {
> > > + if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
> > > + return -EACCES;
> > > mask |= X86_BR_KERNEL;
> > > + }
> > >
> > This will prevent regular users from capturing kernel -> kernel branches.
> > But it won't prevent users from getting kernel -> user branches. Thus
> > some kernel address will still be captured. I guess they could be eliminated
> > by the sw_filter.
> >
> > When using LBR priv level filtering, the filter applies to the branch target
> > only.
>
> How about something like the below? It also adds the branch flags
> Mikey wanted for PowerPC.

Peter,

BTW PowerPC also has the ability to filter on conditional branches. Any
chance we could add something like the follow to perf also?

Mikey

diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index fb104e5..891c769 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -157,8 +157,9 @@ enum perf_branch_sample_type {
PERF_SAMPLE_BRANCH_ANY_CALL = 1U << 4, /* any call branch */
PERF_SAMPLE_BRANCH_ANY_RETURN = 1U << 5, /* any return branch */
PERF_SAMPLE_BRANCH_IND_CALL = 1U << 6, /* indirect calls */
+ PERF_SAMPLE_BRANCH_CONDITIONAL = 1U << 7, /* conditional branches */

- PERF_SAMPLE_BRANCH_MAX = 1U << 7, /* non-ABI */
+ PERF_SAMPLE_BRANCH_MAX = 1U << 8, /* non-ABI */
};

#define PERF_SAMPLE_BRANCH_PLM_ALL \
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index cdf58ec..5b0b89d 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -676,6 +676,7 @@ static const struct branch_mode branch_modes[] = {
BRANCH_OPT("any_call", PERF_SAMPLE_BRANCH_ANY_CALL),
BRANCH_OPT("any_ret", PERF_SAMPLE_BRANCH_ANY_RETURN),
BRANCH_OPT("ind_call", PERF_SAMPLE_BRANCH_IND_CALL),
+ BRANCH_OPT("cnd", PERF_SAMPLE_BRANCH_CONDITIONAL),
BRANCH_END
};

2013-05-16 11:17:20

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

On Thu, May 16, 2013 at 08:15:17PM +1000, Michael Neuling wrote:
> Peter,
>
> BTW PowerPC also has the ability to filter on conditional branches. Any
> chance we could add something like the follow to perf also?
>

I don't see an immediate problem with that except that we on x86 need to
implement that in the software filter. Stephane do you see any
fundamental issue with that?

>
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index fb104e5..891c769 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -157,8 +157,9 @@ enum perf_branch_sample_type {
> PERF_SAMPLE_BRANCH_ANY_CALL = 1U << 4, /* any call branch */
> PERF_SAMPLE_BRANCH_ANY_RETURN = 1U << 5, /* any return branch */
> PERF_SAMPLE_BRANCH_IND_CALL = 1U << 6, /* indirect calls */
> + PERF_SAMPLE_BRANCH_CONDITIONAL = 1U << 7, /* conditional branches */
>
> - PERF_SAMPLE_BRANCH_MAX = 1U << 7, /* non-ABI */
> + PERF_SAMPLE_BRANCH_MAX = 1U << 8, /* non-ABI */
> };
>
> #define PERF_SAMPLE_BRANCH_PLM_ALL \
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index cdf58ec..5b0b89d 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -676,6 +676,7 @@ static const struct branch_mode branch_modes[] = {
> BRANCH_OPT("any_call", PERF_SAMPLE_BRANCH_ANY_CALL),
> BRANCH_OPT("any_ret", PERF_SAMPLE_BRANCH_ANY_RETURN),
> BRANCH_OPT("ind_call", PERF_SAMPLE_BRANCH_IND_CALL),
> + BRANCH_OPT("cnd", PERF_SAMPLE_BRANCH_CONDITIONAL),
> BRANCH_END
> };
>

2013-05-16 15:36:15

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

On Thu, May 16, 2013 at 1:16 PM, Peter Zijlstra <[email protected]> wrote:
> On Thu, May 16, 2013 at 08:15:17PM +1000, Michael Neuling wrote:
>> Peter,
>>
>> BTW PowerPC also has the ability to filter on conditional branches. Any
>> chance we could add something like the follow to perf also?
>>
>
> I don't see an immediate problem with that except that we on x86 need to
> implement that in the software filter. Stephane do you see any
> fundamental issue with that?
>
On X86, the LBR cannot filter on conditional in HW. Thus as Peter said, it would
have to be done in SW. I did not add that because I think those branches are
not necessarily useful for tools.

>>
>> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
>> index fb104e5..891c769 100644
>> --- a/include/uapi/linux/perf_event.h
>> +++ b/include/uapi/linux/perf_event.h
>> @@ -157,8 +157,9 @@ enum perf_branch_sample_type {
>> PERF_SAMPLE_BRANCH_ANY_CALL = 1U << 4, /* any call branch */
>> PERF_SAMPLE_BRANCH_ANY_RETURN = 1U << 5, /* any return branch */
>> PERF_SAMPLE_BRANCH_IND_CALL = 1U << 6, /* indirect calls */
>> + PERF_SAMPLE_BRANCH_CONDITIONAL = 1U << 7, /* conditional branches */
>>
>> - PERF_SAMPLE_BRANCH_MAX = 1U << 7, /* non-ABI */
>> + PERF_SAMPLE_BRANCH_MAX = 1U << 8, /* non-ABI */
>> };
>>
>> #define PERF_SAMPLE_BRANCH_PLM_ALL \
>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>> index cdf58ec..5b0b89d 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -676,6 +676,7 @@ static const struct branch_mode branch_modes[] = {
>> BRANCH_OPT("any_call", PERF_SAMPLE_BRANCH_ANY_CALL),
>> BRANCH_OPT("any_ret", PERF_SAMPLE_BRANCH_ANY_RETURN),
>> BRANCH_OPT("ind_call", PERF_SAMPLE_BRANCH_IND_CALL),
>> + BRANCH_OPT("cnd", PERF_SAMPLE_BRANCH_CONDITIONAL),
>> BRANCH_END
>> };
>>

2013-05-17 11:14:28

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

On Thu, May 16, 2013 at 05:36:11PM +0200, Stephane Eranian wrote:
> On Thu, May 16, 2013 at 1:16 PM, Peter Zijlstra <[email protected]> wrote:
> > On Thu, May 16, 2013 at 08:15:17PM +1000, Michael Neuling wrote:
> >> Peter,
> >>
> >> BTW PowerPC also has the ability to filter on conditional branches. Any
> >> chance we could add something like the follow to perf also?
> >>
> >
> > I don't see an immediate problem with that except that we on x86 need to
> > implement that in the software filter. Stephane do you see any
> > fundamental issue with that?
> >
> On X86, the LBR cannot filter on conditional in HW. Thus as Peter said, it would
> have to be done in SW. I did not add that because I think those branches are
> not necessarily useful for tools.

Wouldn't it be mostly conditional branches that are the primary control flow
and can get predicted wrong? I mean, I'm sure someone will miss-predict an
unconditional branch but its not like we care about people with such
afflictions do we?

Anyway, since PPC people thought it worth baking into hardware, presumably they
have a compelling use case. Mikey could you see if you can retrieve that from
someone in the know? It might be interesting.

Also, it looks like its trivial to add to x86, you seem to have already done
all the hard work by having X86_BR_JCC.

The only missing piece would be:

--- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
@@ -337,6 +337,10 @@ static int intel_pmu_setup_sw_lbr_filter

if (br_type & PERF_SAMPLE_BRANCH_IND_CALL)
mask |= X86_BR_IND_CALL;
+
+ if (br_type & PERF_SAMPLE_BRANCH_CONDITIONAL)
+ mask |= X86_BR_JCC;
+
/*
* stash actual user request into reg, it may
* be used by fixup code for some CPU

2013-05-17 11:32:11

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

Peter Zijlstra <[email protected]> wrote:

> On Thu, May 16, 2013 at 05:36:11PM +0200, Stephane Eranian wrote:
> > On Thu, May 16, 2013 at 1:16 PM, Peter Zijlstra <[email protected]> wrote:
> > > On Thu, May 16, 2013 at 08:15:17PM +1000, Michael Neuling wrote:
> > >> Peter,
> > >>
> > >> BTW PowerPC also has the ability to filter on conditional branches. Any
> > >> chance we could add something like the follow to perf also?
> > >>
> > >
> > > I don't see an immediate problem with that except that we on x86 need to
> > > implement that in the software filter. Stephane do you see any
> > > fundamental issue with that?
> > >
> > On X86, the LBR cannot filter on conditional in HW. Thus as Peter said, it would
> > have to be done in SW. I did not add that because I think those branches are
> > not necessarily useful for tools.
>
> Wouldn't it be mostly conditional branches that are the primary control flow
> and can get predicted wrong? I mean, I'm sure someone will miss-predict an
> unconditional branch but its not like we care about people with such
> afflictions do we?

You could mispredict the target address of a computed goto. You'd know
it was taken but not know target address until later in the pipeline.

On this, the POWER8 branch history buffer tells us two things about the
prediction status.
1) if the branch was predicted taken/not taken correctly
2) if the target address was predicted correctly or not (for computed
gotos only)
So we'd actually like more prediction bits too :-D

> Anyway, since PPC people thought it worth baking into hardware,
> presumably they have a compelling use case. Mikey could you see if you
> can retrieve that from someone in the know? It might be interesting.

I don't think we can mispredict a non-conditional non-computed but I'll
have to check with the HW folks.

Mikey

>
> Also, it looks like its trivial to add to x86, you seem to have already done
> all the hard work by having X86_BR_JCC.
>
> The only missing piece would be:
>
> --- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> @@ -337,6 +337,10 @@ static int intel_pmu_setup_sw_lbr_filter
>
> if (br_type & PERF_SAMPLE_BRANCH_IND_CALL)
> mask |= X86_BR_IND_CALL;
> +
> + if (br_type & PERF_SAMPLE_BRANCH_CONDITIONAL)
> + mask |= X86_BR_JCC;
> +
> /*
> * stash actual user request into reg, it may
> * be used by fixup code for some CPU
>

2013-05-17 11:41:13

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

On Fri, May 17, 2013 at 09:32:08PM +1000, Michael Neuling wrote:
> Peter Zijlstra <[email protected]> wrote:

> > Wouldn't it be mostly conditional branches that are the primary control flow
> > and can get predicted wrong? I mean, I'm sure someone will miss-predict an
> > unconditional branch but its not like we care about people with such
> > afflictions do we?
>
> You could mispredict the target address of a computed goto. You'd know
> it was taken but not know target address until later in the pipeline.

Oh right, computed targets could indeed be mis predicted. I was more thinking
about jumps with immediate values.

> On this, the POWER8 branch history buffer tells us two things about the
> prediction status.
> 1) if the branch was predicted taken/not taken correctly
> 2) if the target address was predicted correctly or not (for computed
> gotos only)
> So we'd actually like more prediction bits too :-D

So if I understand this right, 1) maps to the predicted flags we have; 2)
would be new stuff?

We don't really have anything like that on x86, but I suppose if you make the
thing optional and present a 'useful' use-case implemented in userspace code
we could take it :-)

> > Anyway, since PPC people thought it worth baking into hardware,
> > presumably they have a compelling use case. Mikey could you see if you
> > can retrieve that from someone in the know? It might be interesting.
>
> I don't think we can mispredict a non-conditional non-computed but I'll
> have to check with the HW folks.

I was mostly wondering about the use-case for the conditional filter. Stephane
didn't think it useful, clearly your hardware guys thought different :-)

2013-05-17 21:39:49

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

On Fri, May 17, 2013 at 1:39 PM, Peter Zijlstra <[email protected]> wrote:
> On Fri, May 17, 2013 at 09:32:08PM +1000, Michael Neuling wrote:
>> Peter Zijlstra <[email protected]> wrote:
>
>> > Wouldn't it be mostly conditional branches that are the primary control flow
>> > and can get predicted wrong? I mean, I'm sure someone will miss-predict an
>> > unconditional branch but its not like we care about people with such
>> > afflictions do we?
>>
>> You could mispredict the target address of a computed goto. You'd know
>> it was taken but not know target address until later in the pipeline.
>
> Oh right, computed targets could indeed be mis predicted. I was more thinking
> about jumps with immediate values.
>
>> On this, the POWER8 branch history buffer tells us two things about the
>> prediction status.
>> 1) if the branch was predicted taken/not taken correctly
>> 2) if the target address was predicted correctly or not (for computed
>> gotos only)
>> So we'd actually like more prediction bits too :-D
>
> So if I understand this right, 1) maps to the predicted flags we have; 2)
> would be new stuff?
>
> We don't really have anything like that on x86, but I suppose if you make the
> thing optional and present a 'useful' use-case implemented in userspace code
> we could take it :-)
>
>> > Anyway, since PPC people thought it worth baking into hardware,
>> > presumably they have a compelling use case. Mikey could you see if you
>> > can retrieve that from someone in the know? It might be interesting.
>>
>> I don't think we can mispredict a non-conditional non-computed but I'll
>> have to check with the HW folks.
>
> I was mostly wondering about the use-case for the conditional filter. Stephane
> didn't think it useful, clearly your hardware guys thought different :-)

>From my experience talking with compiler people, they care about ALL
the branches
and not the conditional so much. They use LBR to do basic block profiling.

2013-05-17 22:14:55

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

Stephane Eranian <[email protected]> wrote:

> On Fri, May 17, 2013 at 1:39 PM, Peter Zijlstra <[email protected]> wrote:
> > On Fri, May 17, 2013 at 09:32:08PM +1000, Michael Neuling wrote:
> >> Peter Zijlstra <[email protected]> wrote:
> >
> >> > Wouldn't it be mostly conditional branches that are the primary control flow
> >> > and can get predicted wrong? I mean, I'm sure someone will miss-predict an
> >> > unconditional branch but its not like we care about people with such
> >> > afflictions do we?
> >>
> >> You could mispredict the target address of a computed goto. You'd know
> >> it was taken but not know target address until later in the pipeline.
> >
> > Oh right, computed targets could indeed be mis predicted. I was more thinking
> > about jumps with immediate values.
> >
> >> On this, the POWER8 branch history buffer tells us two things about the
> >> prediction status.
> >> 1) if the branch was predicted taken/not taken correctly
> >> 2) if the target address was predicted correctly or not (for computed
> >> gotos only)
> >> So we'd actually like more prediction bits too :-D
> >
> > So if I understand this right, 1) maps to the predicted flags we have; 2)
> > would be new stuff?
> >
> > We don't really have anything like that on x86, but I suppose if you make the
> > thing optional and present a 'useful' use-case implemented in userspace code
> > we could take it :-)
> >
> >> > Anyway, since PPC people thought it worth baking into hardware,
> >> > presumably they have a compelling use case. Mikey could you see if you
> >> > can retrieve that from someone in the know? It might be interesting.
> >>
> >> I don't think we can mispredict a non-conditional non-computed but I'll
> >> have to check with the HW folks.
> >
> > I was mostly wondering about the use-case for the conditional filter. Stephane
> > didn't think it useful, clearly your hardware guys thought different :-)
>
> From my experience talking with compiler people, they care about ALL
> the branches and not the conditional so much. They use LBR to do basic
> block profiling.

OK. I don't have a good handle on what's useful for compilers or JITs
right now. I'm just plumbing through what's possible.

Mikey

2013-05-17 22:59:13

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

On Sat, May 18, 2013 at 12:14 AM, Michael Neuling <[email protected]> wrote:
> Stephane Eranian <[email protected]> wrote:
>
>> On Fri, May 17, 2013 at 1:39 PM, Peter Zijlstra <[email protected]> wrote:
>> > On Fri, May 17, 2013 at 09:32:08PM +1000, Michael Neuling wrote:
>> >> Peter Zijlstra <[email protected]> wrote:
>> >
>> >> > Wouldn't it be mostly conditional branches that are the primary control flow
>> >> > and can get predicted wrong? I mean, I'm sure someone will miss-predict an
>> >> > unconditional branch but its not like we care about people with such
>> >> > afflictions do we?
>> >>
>> >> You could mispredict the target address of a computed goto. You'd know
>> >> it was taken but not know target address until later in the pipeline.
>> >
>> > Oh right, computed targets could indeed be mis predicted. I was more thinking
>> > about jumps with immediate values.
>> >
>> >> On this, the POWER8 branch history buffer tells us two things about the
>> >> prediction status.
>> >> 1) if the branch was predicted taken/not taken correctly
>> >> 2) if the target address was predicted correctly or not (for computed
>> >> gotos only)
>> >> So we'd actually like more prediction bits too :-D
>> >
>> > So if I understand this right, 1) maps to the predicted flags we have; 2)
>> > would be new stuff?
>> >
>> > We don't really have anything like that on x86, but I suppose if you make the
>> > thing optional and present a 'useful' use-case implemented in userspace code
>> > we could take it :-)
>> >
>> >> > Anyway, since PPC people thought it worth baking into hardware,
>> >> > presumably they have a compelling use case. Mikey could you see if you
>> >> > can retrieve that from someone in the know? It might be interesting.
>> >>
>> >> I don't think we can mispredict a non-conditional non-computed but I'll
>> >> have to check with the HW folks.
>> >
>> > I was mostly wondering about the use-case for the conditional filter. Stephane
>> > didn't think it useful, clearly your hardware guys thought different :-)
>>
>> From my experience talking with compiler people, they care about ALL
>> the branches and not the conditional so much. They use LBR to do basic
>> block profiling.
>
> OK. I don't have a good handle on what's useful for compilers or JITs
> right now. I'm just plumbing through what's possible.
>
I understand. It is okay to extend the interface.

2013-05-21 05:41:38

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

Peter Zijlstra <[email protected]> wrote:

> On Thu, May 16, 2013 at 05:36:11PM +0200, Stephane Eranian wrote:
> > On Thu, May 16, 2013 at 1:16 PM, Peter Zijlstra <[email protected]> wrote:
> > > On Thu, May 16, 2013 at 08:15:17PM +1000, Michael Neuling wrote:
> > >> Peter,
> > >>
> > >> BTW PowerPC also has the ability to filter on conditional branches. Any
> > >> chance we could add something like the follow to perf also?
> > >>
> > >
> > > I don't see an immediate problem with that except that we on x86 need to
> > > implement that in the software filter. Stephane do you see any
> > > fundamental issue with that?
> > >
> > On X86, the LBR cannot filter on conditional in HW. Thus as Peter said, it would
> > have to be done in SW. I did not add that because I think those branches are
> > not necessarily useful for tools.
>
> Wouldn't it be mostly conditional branches that are the primary control flow
> and can get predicted wrong? I mean, I'm sure someone will miss-predict an
> unconditional branch but its not like we care about people with such
> afflictions do we?
>
> Anyway, since PPC people thought it worth baking into hardware, presumably they
> have a compelling use case. Mikey could you see if you can retrieve that from
> someone in the know? It might be interesting.
>
> Also, it looks like its trivial to add to x86, you seem to have already done
> all the hard work by having X86_BR_JCC.
>
> The only missing piece would be:

Peter,

Can we add your signed-off-by on this?

We are cleaning up our series for conditional branches and would like to
add this as part of the post.

Mikey

>
> --- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> @@ -337,6 +337,10 @@ static int intel_pmu_setup_sw_lbr_filter
>
> if (br_type & PERF_SAMPLE_BRANCH_IND_CALL)
> mask |= X86_BR_IND_CALL;
> +
> + if (br_type & PERF_SAMPLE_BRANCH_CONDITIONAL)
> + mask |= X86_BR_JCC;
> +
> /*
> * stash actual user request into reg, it may
> * be used by fixup code for some CPU
>

2013-05-21 13:38:49

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

On Tue, May 21, 2013 at 03:41:35PM +1000, Michael Neuling wrote:
> Peter Zijlstra <[email protected]> wrote:
>
> Can we add your signed-off-by on this?
>
> We are cleaning up our series for conditional branches and would like to
> add this as part of the post.

Sure, but its completely untested.. I was hoping Stephane would say
somnething about it since he wrote all that magic ;-)

But yeah, feel free to add my SoB.

2013-05-21 13:46:44

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

On Tue, May 21, 2013 at 10:50 AM, Peter Zijlstra <[email protected]> wrote:
> On Tue, May 21, 2013 at 03:41:35PM +1000, Michael Neuling wrote:
>> Peter Zijlstra <[email protected]> wrote:
>>
>> Can we add your signed-off-by on this?
>>
>> We are cleaning up our series for conditional branches and would like to
>> add this as part of the post.
>
> Sure, but its completely untested.. I was hoping Stephane would say
> somnething about it since he wrote all that magic ;-)
>
Let me take a look at it.

> But yeah, feel free to add my SoB.

2013-05-21 13:55:17

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

On Thu, May 16, 2013 at 12:15 PM, Michael Neuling <[email protected]> wrote:
> Peter Zijlstra <[email protected]> wrote:
>
>> On Wed, May 15, 2013 at 03:37:22PM +0200, Stephane Eranian wrote:
>> > On Fri, May 3, 2013 at 2:11 PM, Peter Zijlstra <[email protected]> wrote:
>> > > We should always have proper privileges when requesting kernel data.
>> > >
>> > > Cc: Andi Kleen <[email protected]>
>> > > Cc: [email protected]
>> > > Signed-off-by: Peter Zijlstra <[email protected]>
>> > > Link: http://lkml.kernel.org/n/[email protected]
>> > > ---
>> > > arch/x86/kernel/cpu/perf_event_intel_lbr.c | 5 ++++-
>> > > 1 file changed, 4 insertions(+), 1 deletion(-)
>> > >
>> > > --- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
>> > > +++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
>> > > @@ -318,8 +318,11 @@ static void intel_pmu_setup_sw_lbr_filte
>> > > if (br_type & PERF_SAMPLE_BRANCH_USER)
>> > > mask |= X86_BR_USER;
>> > >
>> > > - if (br_type & PERF_SAMPLE_BRANCH_KERNEL)
>> > > + if (br_type & PERF_SAMPLE_BRANCH_KERNEL) {
>> > > + if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
>> > > + return -EACCES;
>> > > mask |= X86_BR_KERNEL;
>> > > + }
>> > >
>> > This will prevent regular users from capturing kernel -> kernel branches.
>> > But it won't prevent users from getting kernel -> user branches. Thus
>> > some kernel address will still be captured. I guess they could be eliminated
>> > by the sw_filter.
>> >
>> > When using LBR priv level filtering, the filter applies to the branch target
>> > only.
>>
>> How about something like the below? It also adds the branch flags
>> Mikey wanted for PowerPC.
>
> Peter,
>
> BTW PowerPC also has the ability to filter on conditional branches. Any
> chance we could add something like the follow to perf also?
>
> Mikey
>
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index fb104e5..891c769 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -157,8 +157,9 @@ enum perf_branch_sample_type {
> PERF_SAMPLE_BRANCH_ANY_CALL = 1U << 4, /* any call branch */
> PERF_SAMPLE_BRANCH_ANY_RETURN = 1U << 5, /* any return branch */
> PERF_SAMPLE_BRANCH_IND_CALL = 1U << 6, /* indirect calls */
> + PERF_SAMPLE_BRANCH_CONDITIONAL = 1U << 7, /* conditional branches */
>
I would use PERF_SAMPLE_BRANCH_COND here.

> - PERF_SAMPLE_BRANCH_MAX = 1U << 7, /* non-ABI */
> + PERF_SAMPLE_BRANCH_MAX = 1U << 8, /* non-ABI */
> };
>
> #define PERF_SAMPLE_BRANCH_PLM_ALL \
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index cdf58ec..5b0b89d 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -676,6 +676,7 @@ static const struct branch_mode branch_modes[] = {
> BRANCH_OPT("any_call", PERF_SAMPLE_BRANCH_ANY_CALL),
> BRANCH_OPT("any_ret", PERF_SAMPLE_BRANCH_ANY_RETURN),
> BRANCH_OPT("ind_call", PERF_SAMPLE_BRANCH_IND_CALL),
> + BRANCH_OPT("cnd", PERF_SAMPLE_BRANCH_CONDITIONAL),

use "cond"

> BRANCH_END
> };
>

And if you do this, you also need to update the x86
perf_event_intel_lbr.c mapping
tables to fill out the entries for PERF_SAMPLE_BRANCH_COND:

[PERF_SAMPLE_BRANCH_COND] = LBR_JCC,

And you also need to update intel_pmu_setup_sw_lbr_filter()
to handle the conversion to x86 instructions:

if (br_type & PERF_SAMPLE_BRANCH_COND)
mask |= X86_BR_JCC;

You also need to update the perf-record.txt documentation to list cond
as a possible
branch filter.

2013-05-22 06:44:43

by Anshuman Khandual

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL

On 05/21/2013 07:25 PM, Stephane Eranian wrote:
> On Thu, May 16, 2013 at 12:15 PM, Michael Neuling <[email protected]> wrote:
>> Peter Zijlstra <[email protected]> wrote:
>>
>>> On Wed, May 15, 2013 at 03:37:22PM +0200, Stephane Eranian wrote:
>>>> On Fri, May 3, 2013 at 2:11 PM, Peter Zijlstra <[email protected]> wrote:
>>>>> We should always have proper privileges when requesting kernel data.
>>>>>
>>>>> Cc: Andi Kleen <[email protected]>
>>>>> Cc: [email protected]
>>>>> Signed-off-by: Peter Zijlstra <[email protected]>
>>>>> Link: http://lkml.kernel.org/n/[email protected]
>>>>> ---
>>>>> arch/x86/kernel/cpu/perf_event_intel_lbr.c | 5 ++++-
>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> --- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
>>>>> +++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
>>>>> @@ -318,8 +318,11 @@ static void intel_pmu_setup_sw_lbr_filte
>>>>> if (br_type & PERF_SAMPLE_BRANCH_USER)
>>>>> mask |= X86_BR_USER;
>>>>>
>>>>> - if (br_type & PERF_SAMPLE_BRANCH_KERNEL)
>>>>> + if (br_type & PERF_SAMPLE_BRANCH_KERNEL) {
>>>>> + if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
>>>>> + return -EACCES;
>>>>> mask |= X86_BR_KERNEL;
>>>>> + }
>>>>>
>>>> This will prevent regular users from capturing kernel -> kernel branches.
>>>> But it won't prevent users from getting kernel -> user branches. Thus
>>>> some kernel address will still be captured. I guess they could be eliminated
>>>> by the sw_filter.
>>>>
>>>> When using LBR priv level filtering, the filter applies to the branch target
>>>> only.
>>>
>>> How about something like the below? It also adds the branch flags
>>> Mikey wanted for PowerPC.
>>
>> Peter,
>>
>> BTW PowerPC also has the ability to filter on conditional branches. Any
>> chance we could add something like the follow to perf also?
>>
>> Mikey
>>
>> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
>> index fb104e5..891c769 100644
>> --- a/include/uapi/linux/perf_event.h
>> +++ b/include/uapi/linux/perf_event.h
>> @@ -157,8 +157,9 @@ enum perf_branch_sample_type {
>> PERF_SAMPLE_BRANCH_ANY_CALL = 1U << 4, /* any call branch */
>> PERF_SAMPLE_BRANCH_ANY_RETURN = 1U << 5, /* any return branch */
>> PERF_SAMPLE_BRANCH_IND_CALL = 1U << 6, /* indirect calls */
>> + PERF_SAMPLE_BRANCH_CONDITIONAL = 1U << 7, /* conditional branches */
>>
> I would use PERF_SAMPLE_BRANCH_COND here.
>
>> - PERF_SAMPLE_BRANCH_MAX = 1U << 7, /* non-ABI */
>> + PERF_SAMPLE_BRANCH_MAX = 1U << 8, /* non-ABI */
>> };
>>
>> #define PERF_SAMPLE_BRANCH_PLM_ALL \
>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>> index cdf58ec..5b0b89d 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -676,6 +676,7 @@ static const struct branch_mode branch_modes[] = {
>> BRANCH_OPT("any_call", PERF_SAMPLE_BRANCH_ANY_CALL),
>> BRANCH_OPT("any_ret", PERF_SAMPLE_BRANCH_ANY_RETURN),
>> BRANCH_OPT("ind_call", PERF_SAMPLE_BRANCH_IND_CALL),
>> + BRANCH_OPT("cnd", PERF_SAMPLE_BRANCH_CONDITIONAL),
>
> use "cond"
>
>> BRANCH_END
>> };
>>
>
> And if you do this, you also need to update the x86
> perf_event_intel_lbr.c mapping
> tables to fill out the entries for PERF_SAMPLE_BRANCH_COND:
>
> [PERF_SAMPLE_BRANCH_COND] = LBR_JCC,
>
> And you also need to update intel_pmu_setup_sw_lbr_filter()
> to handle the conversion to x86 instructions:
>
> if (br_type & PERF_SAMPLE_BRANCH_COND)
> mask |= X86_BR_JCC;
>
>
> You also need to update the perf-record.txt documentation to list cond
> as a possible
> branch filter.

Hey Stephane,

I have incorporated all the review comments into the patch series
https://lkml.org/lkml/2013/5/22/51.

Regards
Anshuman

2013-05-22 12:23:23