* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
@ 2013-05-16 10:15 ` Michael Neuling
0 siblings, 0 replies; 56+ messages in thread
From: Michael Neuling @ 2013-05-16 10:15 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: ak, LKML, Stephane Eranian, Linux PPC dev, Ingo Molnar
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
};
^ permalink raw reply related [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
2013-05-16 10:15 ` Michael Neuling
@ 2013-05-16 11:16 ` Peter Zijlstra
-1 siblings, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2013-05-16 11:16 UTC (permalink / raw)
To: Michael Neuling
Cc: Stephane Eranian, Ingo Molnar, LKML, ak, michael, benh, Linux PPC dev
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
> };
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
@ 2013-05-16 11:16 ` Peter Zijlstra
0 siblings, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2013-05-16 11:16 UTC (permalink / raw)
To: Michael Neuling; +Cc: ak, LKML, Stephane Eranian, Linux PPC dev, Ingo Molnar
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
> };
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
2013-05-16 11:16 ` Peter Zijlstra
@ 2013-05-16 15:36 ` Stephane Eranian
-1 siblings, 0 replies; 56+ messages in thread
From: Stephane Eranian @ 2013-05-16 15:36 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Michael Neuling, Ingo Molnar, LKML, ak, Michael Ellerman, benh,
Linux PPC dev
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
>> };
>>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
@ 2013-05-16 15:36 ` Stephane Eranian
0 siblings, 0 replies; 56+ messages in thread
From: Stephane Eranian @ 2013-05-16 15:36 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Michael Neuling, ak, LKML, Linux PPC dev, Ingo Molnar
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
>> };
>>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
2013-05-16 15:36 ` Stephane Eranian
@ 2013-05-17 11:12 ` Peter Zijlstra
-1 siblings, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2013-05-17 11:12 UTC (permalink / raw)
To: Stephane Eranian
Cc: Michael Neuling, Ingo Molnar, LKML, ak, Michael Ellerman, benh,
Linux PPC dev
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
@ 2013-05-17 11:12 ` Peter Zijlstra
0 siblings, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2013-05-17 11:12 UTC (permalink / raw)
To: Stephane Eranian; +Cc: Michael Neuling, ak, LKML, Linux PPC dev, Ingo Molnar
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
2013-05-17 11:12 ` Peter Zijlstra
@ 2013-05-17 11:32 ` Michael Neuling
-1 siblings, 0 replies; 56+ messages in thread
From: Michael Neuling @ 2013-05-17 11:32 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Stephane Eranian, Ingo Molnar, LKML, ak, Michael Ellerman, benh,
Linux PPC dev
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
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
@ 2013-05-17 11:32 ` Michael Neuling
0 siblings, 0 replies; 56+ messages in thread
From: Michael Neuling @ 2013-05-17 11:32 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: ak, LKML, Stephane Eranian, Linux PPC dev, Ingo Molnar
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
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
2013-05-17 11:32 ` Michael Neuling
@ 2013-05-17 11:39 ` Peter Zijlstra
-1 siblings, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2013-05-17 11:39 UTC (permalink / raw)
To: Michael Neuling
Cc: Stephane Eranian, Ingo Molnar, LKML, ak, Michael Ellerman, benh,
Linux PPC dev
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 :-)
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
@ 2013-05-17 11:39 ` Peter Zijlstra
0 siblings, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2013-05-17 11:39 UTC (permalink / raw)
To: Michael Neuling; +Cc: ak, LKML, Stephane Eranian, Linux PPC dev, Ingo Molnar
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 :-)
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
2013-05-17 11:39 ` Peter Zijlstra
@ 2013-05-17 21:39 ` Stephane Eranian
-1 siblings, 0 replies; 56+ messages in thread
From: Stephane Eranian @ 2013-05-17 21:39 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Michael Neuling, Ingo Molnar, LKML, ak, Michael Ellerman, benh,
Linux PPC dev
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.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
@ 2013-05-17 21:39 ` Stephane Eranian
0 siblings, 0 replies; 56+ messages in thread
From: Stephane Eranian @ 2013-05-17 21:39 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Michael Neuling, ak, LKML, Linux PPC dev, Ingo Molnar
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.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
2013-05-17 21:39 ` Stephane Eranian
@ 2013-05-17 22:14 ` Michael Neuling
-1 siblings, 0 replies; 56+ messages in thread
From: Michael Neuling @ 2013-05-17 22:14 UTC (permalink / raw)
To: Stephane Eranian
Cc: Peter Zijlstra, Ingo Molnar, LKML, ak, Michael Ellerman, benh,
Linux PPC dev
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
@ 2013-05-17 22:14 ` Michael Neuling
0 siblings, 0 replies; 56+ messages in thread
From: Michael Neuling @ 2013-05-17 22:14 UTC (permalink / raw)
To: Stephane Eranian; +Cc: ak, Peter Zijlstra, LKML, Linux PPC dev, Ingo Molnar
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
2013-05-17 22:14 ` Michael Neuling
@ 2013-05-17 22:59 ` Stephane Eranian
-1 siblings, 0 replies; 56+ messages in thread
From: Stephane Eranian @ 2013-05-17 22:59 UTC (permalink / raw)
To: Michael Neuling
Cc: Peter Zijlstra, Ingo Molnar, LKML, ak, Michael Ellerman, benh,
Linux PPC dev
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.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
@ 2013-05-17 22:59 ` Stephane Eranian
0 siblings, 0 replies; 56+ messages in thread
From: Stephane Eranian @ 2013-05-17 22:59 UTC (permalink / raw)
To: Michael Neuling; +Cc: ak, Peter Zijlstra, LKML, Linux PPC dev, Ingo Molnar
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.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
2013-05-17 11:12 ` Peter Zijlstra
@ 2013-05-21 5:41 ` Michael Neuling
-1 siblings, 0 replies; 56+ messages in thread
From: Michael Neuling @ 2013-05-21 5:41 UTC (permalink / raw)
To: Peter Zijlstra
Cc: anshuman Stephane Eranian, Ingo Molnar, LKML, ak,
Michael Ellerman, benh, Linux PPC dev
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
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
@ 2013-05-21 5:41 ` Michael Neuling
0 siblings, 0 replies; 56+ messages in thread
From: Michael Neuling @ 2013-05-21 5:41 UTC (permalink / raw)
To: Peter Zijlstra
Cc: ak, LKML, anshuman Stephane Eranian, Linux PPC dev, Ingo Molnar
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
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
2013-05-21 5:41 ` Michael Neuling
@ 2013-05-21 8:50 ` Peter Zijlstra
-1 siblings, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2013-05-21 8:50 UTC (permalink / raw)
To: Michael Neuling
Cc: anshuman Stephane Eranian, Ingo Molnar, LKML, ak,
Michael Ellerman, benh, Linux PPC dev
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.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
@ 2013-05-21 8:50 ` Peter Zijlstra
0 siblings, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2013-05-21 8:50 UTC (permalink / raw)
To: Michael Neuling
Cc: ak, LKML, anshuman Stephane Eranian, Linux PPC dev, Ingo Molnar
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.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
2013-05-21 8:50 ` Peter Zijlstra
@ 2013-05-21 13:46 ` Stephane Eranian
-1 siblings, 0 replies; 56+ messages in thread
From: Stephane Eranian @ 2013-05-21 13:46 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Michael Neuling, Ingo Molnar, LKML, ak, Michael Ellerman, benh,
Linux PPC dev
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.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
@ 2013-05-21 13:46 ` Stephane Eranian
0 siblings, 0 replies; 56+ messages in thread
From: Stephane Eranian @ 2013-05-21 13:46 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Michael Neuling, ak, LKML, Linux PPC dev, Ingo Molnar
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.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
2013-05-16 10:15 ` Michael Neuling
@ 2013-05-21 13:55 ` Stephane Eranian
-1 siblings, 0 replies; 56+ messages in thread
From: Stephane Eranian @ 2013-05-21 13:55 UTC (permalink / raw)
To: Michael Neuling
Cc: Peter Zijlstra, Ingo Molnar, LKML, ak, Michael Ellerman, benh,
Linux PPC dev
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.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
@ 2013-05-21 13:55 ` Stephane Eranian
0 siblings, 0 replies; 56+ messages in thread
From: Stephane Eranian @ 2013-05-21 13:55 UTC (permalink / raw)
To: Michael Neuling; +Cc: ak, Peter Zijlstra, LKML, Linux PPC dev, Ingo Molnar
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.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
2013-05-21 13:55 ` Stephane Eranian
(?)
@ 2013-05-22 6:43 ` Anshuman Khandual
2013-05-22 12:23 ` Stephane Eranian
-1 siblings, 1 reply; 56+ messages in thread
From: Anshuman Khandual @ 2013-05-22 6:43 UTC (permalink / raw)
To: Stephane Eranian
Cc: Michael Neuling, ak, Peter Zijlstra, LKML, Linux PPC dev, Ingo Molnar
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
2013-05-22 6:43 ` Anshuman Khandual
@ 2013-05-22 12:23 ` Stephane Eranian
2013-05-22 14:51 ` Anshuman Khandual
0 siblings, 1 reply; 56+ messages in thread
From: Stephane Eranian @ 2013-05-22 12:23 UTC (permalink / raw)
To: Anshuman Khandual
Cc: Michael Neuling, ak, Peter Zijlstra, LKML, Linux PPC dev, Ingo Molnar
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.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/3] perf, x86, lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
2013-05-22 12:23 ` Stephane Eranian
@ 2013-05-22 14:51 ` Anshuman Khandual
0 siblings, 0 replies; 56+ messages in thread
From: Anshuman Khandual @ 2013-05-22 14:51 UTC (permalink / raw)
To: Stephane Eranian
Cc: Michael Neuling, ak, Peter Zijlstra, LKML, Linux PPC dev, Ingo Molnar
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.
^ permalink raw reply [flat|nested] 56+ messages in thread