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

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

[Michael Neuling]

Peter Zijlstra <peterz@infradead.org> 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 <a.p.zijlstra@chello.nl> wrote:
> > > We should always have proper privileges when requesting kernel data.
> > >
> > > Cc: Andi Kleen <ak@linux.intel.com>
> > > Cc: eranian@google.com
> > > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > > Link: http://lkml.kernel.org/n/tip-v0x9ky3ahzr6nm3c6ilwrili@git.kernel.org
> > > ---
> > >  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

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

[Michael Neuling]

Peter Zijlstra <peterz@infradead.org> 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 <a.p.zijlstra@chello.nl> wrote:
> > > We should always have proper privileges when requesting kernel data.
> > >
> > > Cc: Andi Kleen <ak@linux.intel.com>
> > > Cc: eranian@google.com
> > > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > > Link: http://lkml.kernel.org/n/tip-v0x9ky3ahzr6nm3c6ilwrili@git.kernel.org
> > > ---
> > >  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
 };
 

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

[Peter Zijlstra]

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
>  };
>  

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

[Stephane Eranian]

On Thu, May 16, 2013 at 1:16 PM, Peter Zijlstra <peterz@infradead.org> 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
>>  };
>>

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

[Peter Zijlstra]

On Thu, May 16, 2013 at 05:36:11PM +0200, Stephane Eranian wrote:
> On Thu, May 16, 2013 at 1:16 PM, Peter Zijlstra <peterz@infradead.org> 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

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

[Michael Neuling]

Peter Zijlstra <peterz@infradead.org> 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 <peterz@infradead.org> 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
> 

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

[Peter Zijlstra]

On Fri, May 17, 2013 at 09:32:08PM +1000, Michael Neuling wrote:
> Peter Zijlstra <peterz@infradead.org> 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 :-)

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

[Stephane Eranian]

On Fri, May 17, 2013 at 1:39 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, May 17, 2013 at 09:32:08PM +1000, Michael Neuling wrote:
>> Peter Zijlstra <peterz@infradead.org> 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.

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

[Michael Neuling]

Stephane Eranian <eranian@google.com> wrote:

> On Fri, May 17, 2013 at 1:39 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Fri, May 17, 2013 at 09:32:08PM +1000, Michael Neuling wrote:
> >> Peter Zijlstra <peterz@infradead.org> 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

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

[Stephane Eranian]

On Sat, May 18, 2013 at 12:14 AM, Michael Neuling <mikey@neuling.org> wrote:
> Stephane Eranian <eranian@google.com> wrote:
>
>> On Fri, May 17, 2013 at 1:39 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Fri, May 17, 2013 at 09:32:08PM +1000, Michael Neuling wrote:
>> >> Peter Zijlstra <peterz@infradead.org> 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.

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

[Michael Neuling]

Peter Zijlstra <peterz@infradead.org> 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 <peterz@infradead.org> 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
> 

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

[Peter Zijlstra]

On Tue, May 21, 2013 at 03:41:35PM +1000, Michael Neuling wrote:
> Peter Zijlstra <peterz@infradead.org> 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.

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

[Stephane Eranian]

On Tue, May 21, 2013 at 10:50 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, May 21, 2013 at 03:41:35PM +1000, Michael Neuling wrote:
>> Peter Zijlstra <peterz@infradead.org> 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.

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

[Stephane Eranian]

On Thu, May 16, 2013 at 12:15 PM, Michael Neuling <mikey@neuling.org> wrote:
> Peter Zijlstra <peterz@infradead.org> 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 <a.p.zijlstra@chello.nl> wrote:
>> > > We should always have proper privileges when requesting kernel data.
>> > >
>> > > Cc: Andi Kleen <ak@linux.intel.com>
>> > > Cc: eranian@google.com
>> > > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
>> > > Link: http://lkml.kernel.org/n/tip-v0x9ky3ahzr6nm3c6ilwrili@git.kernel.org
>> > > ---
>> > >  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.

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

[Anshuman Khandual]

On 05/21/2013 07:25 PM, Stephane Eranian wrote:
> On Thu, May 16, 2013 at 12:15 PM, Michael Neuling <mikey@neuling.org> wrote:
>> Peter Zijlstra <peterz@infradead.org> 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 <a.p.zijlstra@chello.nl> wrote:
>>>>> We should always have proper privileges when requesting kernel data.
>>>>>
>>>>> Cc: Andi Kleen <ak@linux.intel.com>
>>>>> Cc: eranian@google.com
>>>>> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
>>>>> Link: http://lkml.kernel.org/n/tip-v0x9ky3ahzr6nm3c6ilwrili@git.kernel.org
>>>>> ---
>>>>>  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

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

[Stephane Eranian]

Hi,



On Wed, May 22, 2013 at 8:43 AM, Anshuman Khandual
<khandual@linux.vnet.ibm.com> wrote:
> On 05/21/2013 07:25 PM, Stephane Eranian wrote:
>> On Thu, May 16, 2013 at 12:15 PM, Michael Neuling <mikey@neuling.org> wrote:
>>> Peter Zijlstra <peterz@infradead.org> 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 <a.p.zijlstra@chello.nl> wrote:
>>>>>> We should always have proper privileges when requesting kernel data.
>>>>>>
>>>>>> Cc: Andi Kleen <ak@linux.intel.com>
>>>>>> Cc: eranian@google.com
>>>>>> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
>>>>>> Link: http://lkml.kernel.org/n/tip-v0x9ky3ahzr6nm3c6ilwrili@git.kernel.org
>>>>>> ---
>>>>>>  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.
>
I don't see how you can compile Patch 3/5:

+ BRANCH_OPT("cond", PERF_SAMPLE_BRANCH_CONDITIONAL),

Needs to be PERF_SAMPLE_BRANCH_COND.

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

[Anshuman Khandual]

On 05/22/2013 05:53 PM, Stephane Eranian wrote:
> Hi,
> 
> 
> 
> On Wed, May 22, 2013 at 8:43 AM, Anshuman Khandual
> <khandual@linux.vnet.ibm.com> wrote:
>> On 05/21/2013 07:25 PM, Stephane Eranian wrote:
>>> On Thu, May 16, 2013 at 12:15 PM, Michael Neuling <mikey@neuling.org> wrote:
>>>> Peter Zijlstra <peterz@infradead.org> 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 <a.p.zijlstra@chello.nl> wrote:
>>>>>>> We should always have proper privileges when requesting kernel data.
>>>>>>>
>>>>>>> Cc: Andi Kleen <ak@linux.intel.com>
>>>>>>> Cc: eranian@google.com
>>>>>>> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
>>>>>>> Link: http://lkml.kernel.org/n/tip-v0x9ky3ahzr6nm3c6ilwrili@git.kernel.org
>>>>>>> ---
>>>>>>>  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.
>>
> I don't see how you can compile Patch 3/5:
> 
> + BRANCH_OPT("cond", PERF_SAMPLE_BRANCH_CONDITIONAL),
> 
> Needs to be PERF_SAMPLE_BRANCH_COND.
> 

Ahh, sorry missed it, will fix it.