| Message ID | 20190528150320.25953-5-raphael.gault@arm.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | arm64: Enable access to pmu registers by user-space | expand |
On Tue, May 28, 2019 at 04:03:17PM +0100, Raphael Gault wrote: > +static int armv8pmu_access_event_idx(struct perf_event *event) > +{ > + if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR)) > + return 0; > + > + /* > + * We remap the cycle counter index to 32 to > + * match the offset applied to the rest of > + * the counter indeces. > + */ > + if (event->hw.idx == ARMV8_IDX_CYCLE_COUNTER) > + return 32; > + > + return event->hw.idx; Is there a guarantee event->hw.idx is never 0? Or should you, just like x86, use +1 here? > +}
Hi Peter, On 5/29/19 10:46 AM, Peter Zijlstra wrote: > On Tue, May 28, 2019 at 04:03:17PM +0100, Raphael Gault wrote: >> +static int armv8pmu_access_event_idx(struct perf_event *event) >> +{ >> + if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR)) >> + return 0; >> + >> + /* >> + * We remap the cycle counter index to 32 to >> + * match the offset applied to the rest of >> + * the counter indeces. >> + */ >> + if (event->hw.idx == ARMV8_IDX_CYCLE_COUNTER) >> + return 32; >> + >> + return event->hw.idx; > > Is there a guarantee event->hw.idx is never 0? Or should you, just like > x86, use +1 here? > You are right, I should use +1 here. Thanks for pointing that out. >> +} Thanks,
On 29/05/2019 11:46, Raphael Gault wrote: > Hi Peter, > > On 5/29/19 10:46 AM, Peter Zijlstra wrote: >> On Tue, May 28, 2019 at 04:03:17PM +0100, Raphael Gault wrote: >>> +static int armv8pmu_access_event_idx(struct perf_event *event) >>> +{ >>> + if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR)) >>> + return 0; >>> + >>> + /* >>> + * We remap the cycle counter index to 32 to >>> + * match the offset applied to the rest of >>> + * the counter indeces. >>> + */ >>> + if (event->hw.idx == ARMV8_IDX_CYCLE_COUNTER) >>> + return 32; >>> + >>> + return event->hw.idx; >> >> Is there a guarantee event->hw.idx is never 0? Or should you, just like >> x86, use +1 here? >> > > You are right, I should use +1 here. Thanks for pointing that out. Isn't that already the case though, since we reserve index 0 for the cycle counter? I'm looking at ARMV8_IDX_TO_COUNTER() here... Robin.
Hi Robin, Hi Peter, On 5/29/19 11:50 AM, Robin Murphy wrote: > On 29/05/2019 11:46, Raphael Gault wrote: >> Hi Peter, >> >> On 5/29/19 10:46 AM, Peter Zijlstra wrote: >>> On Tue, May 28, 2019 at 04:03:17PM +0100, Raphael Gault wrote: >>>> +static int armv8pmu_access_event_idx(struct perf_event *event) >>>> +{ >>>> + if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR)) >>>> + return 0; >>>> + >>>> + /* >>>> + * We remap the cycle counter index to 32 to >>>> + * match the offset applied to the rest of >>>> + * the counter indeces. >>>> + */ >>>> + if (event->hw.idx == ARMV8_IDX_CYCLE_COUNTER) >>>> + return 32; >>>> + >>>> + return event->hw.idx; >>> >>> Is there a guarantee event->hw.idx is never 0? Or should you, just like >>> x86, use +1 here? >>> >> >> You are right, I should use +1 here. Thanks for pointing that out. > > Isn't that already the case though, since we reserve index 0 for the > cycle counter? I'm looking at ARMV8_IDX_TO_COUNTER() here... > Well the current behaviour is correct and takes care of the zero case with the ARMV8_IDX_CYCLE_COUNTER check. But using ARMV8_IDX_TO_COUNTER() and add 1 would also work. However this seems indeed redundant with the current value held in event->hw.idx. > Robin.
On Wed, May 29, 2019 at 01:25:46PM +0100, Raphael Gault wrote: > Hi Robin, Hi Peter, > > On 5/29/19 11:50 AM, Robin Murphy wrote: > > On 29/05/2019 11:46, Raphael Gault wrote: > > > Hi Peter, > > > > > > On 5/29/19 10:46 AM, Peter Zijlstra wrote: > > > > On Tue, May 28, 2019 at 04:03:17PM +0100, Raphael Gault wrote: > > > > > +static int armv8pmu_access_event_idx(struct perf_event *event) > > > > > +{ > > > > > + if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR)) > > > > > + return 0; > > > > > + > > > > > + /* > > > > > + * We remap the cycle counter index to 32 to > > > > > + * match the offset applied to the rest of > > > > > + * the counter indeces. > > > > > + */ > > > > > + if (event->hw.idx == ARMV8_IDX_CYCLE_COUNTER) > > > > > + return 32; > > > > > + > > > > > + return event->hw.idx; > > > > > > > > Is there a guarantee event->hw.idx is never 0? Or should you, just like > > > > x86, use +1 here? > > > > > > > > > > You are right, I should use +1 here. Thanks for pointing that out. > > > > Isn't that already the case though, since we reserve index 0 for the > > cycle counter? I'm looking at ARMV8_IDX_TO_COUNTER() here... > > > > Well the current behaviour is correct and takes care of the zero case with > the ARMV8_IDX_CYCLE_COUNTER check. But using ARMV8_IDX_TO_COUNTER() and add > 1 would also work. However this seems indeed redundant with the current > value held in event->hw.idx. Note that whatever you pick now will become ABI. Also note that the comment/pseudo-code in perf_event_mmap_page suggests to use idx-1 for the actual hardware access.
Hi Peter, On 5/29/19 1:32 PM, Peter Zijlstra wrote: > On Wed, May 29, 2019 at 01:25:46PM +0100, Raphael Gault wrote: >> Hi Robin, Hi Peter, >> >> On 5/29/19 11:50 AM, Robin Murphy wrote: >>> On 29/05/2019 11:46, Raphael Gault wrote: >>>> Hi Peter, >>>> >>>> On 5/29/19 10:46 AM, Peter Zijlstra wrote: >>>>> On Tue, May 28, 2019 at 04:03:17PM +0100, Raphael Gault wrote: >>>>>> +static int armv8pmu_access_event_idx(struct perf_event *event) >>>>>> +{ >>>>>> + if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR)) >>>>>> + return 0; >>>>>> + >>>>>> + /* >>>>>> + * We remap the cycle counter index to 32 to >>>>>> + * match the offset applied to the rest of >>>>>> + * the counter indeces. >>>>>> + */ >>>>>> + if (event->hw.idx == ARMV8_IDX_CYCLE_COUNTER) >>>>>> + return 32; >>>>>> + >>>>>> + return event->hw.idx; >>>>> >>>>> Is there a guarantee event->hw.idx is never 0? Or should you, just like >>>>> x86, use +1 here? >>>>> >>>> >>>> You are right, I should use +1 here. Thanks for pointing that out. >>> >>> Isn't that already the case though, since we reserve index 0 for the >>> cycle counter? I'm looking at ARMV8_IDX_TO_COUNTER() here... >>> >> >> Well the current behaviour is correct and takes care of the zero case with >> the ARMV8_IDX_CYCLE_COUNTER check. But using ARMV8_IDX_TO_COUNTER() and add >> 1 would also work. However this seems indeed redundant with the current >> value held in event->hw.idx. > > Note that whatever you pick now will become ABI. Also note that the > comment/pseudo-code in perf_event_mmap_page suggests to use idx-1 for > the actual hardware access. > Indeed that's true. As for the pseudo-code in perf_event_mmap_page. It is compatible with what I do here. The two approach are only different in form but it is in both case necessary to subtract 1 on the returned value in order to access the correct hardware counter. Thank you,
diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c index 6164d389eed6..3dc1265540df 100644 --- a/arch/arm64/kernel/perf_event.c +++ b/arch/arm64/kernel/perf_event.c @@ -809,6 +809,22 @@ static void armv8pmu_clear_event_idx(struct pmu_hw_events *cpuc, clear_bit(idx - 1, cpuc->used_mask); } +static int armv8pmu_access_event_idx(struct perf_event *event) +{ + if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR)) + return 0; + + /* + * We remap the cycle counter index to 32 to + * match the offset applied to the rest of + * the counter indeces. + */ + if (event->hw.idx == ARMV8_IDX_CYCLE_COUNTER) + return 32; + + return event->hw.idx; +} + /* * Add an event filter to a given event. */ @@ -890,6 +906,8 @@ static int __armv8_pmuv3_map_event(struct perf_event *event, if (armv8pmu_event_is_64bit(event)) event->hw.flags |= ARMPMU_EVT_64BIT; + event->hw.flags |= ARMPMU_EL0_RD_CNTR; + /* Only expose micro/arch events supported by this PMU */ if ((hw_event_id > 0) && (hw_event_id < ARMV8_PMUV3_MAX_COMMON_EVENTS) && test_bit(hw_event_id, armpmu->pmceid_bitmap)) { @@ -1010,6 +1028,8 @@ static int armv8_pmu_init(struct arm_pmu *cpu_pmu) cpu_pmu->set_event_filter = armv8pmu_set_event_filter; cpu_pmu->filter_match = armv8pmu_filter_match; + cpu_pmu->pmu.event_idx = armv8pmu_access_event_idx; + return 0; } @@ -1188,6 +1208,7 @@ void arch_perf_update_userpage(struct perf_event *event, */ freq = arch_timer_get_rate(); userpg->cap_user_time = 1; + userpg->cap_user_rdpmc = !!(event->hw.flags & ARMPMU_EL0_RD_CNTR); clocks_calc_mult_shift(&userpg->time_mult, &shift, freq, NSEC_PER_SEC, 0); diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h index 4641e850b204..3bef390c1069 100644 --- a/include/linux/perf/arm_pmu.h +++ b/include/linux/perf/arm_pmu.h @@ -30,6 +30,8 @@ */ /* Event uses a 64bit counter */ #define ARMPMU_EVT_64BIT 1 +/* Allow access to hardware counter from userspace */ +#define ARMPMU_EL0_RD_CNTR 2 #define HW_OP_UNSUPPORTED 0xFFFF #define C(_x) PERF_COUNT_HW_CACHE_##_x
In order to be able to access the counter directly for userspace, we need to provide the index of the counter using the userpage. We thus need to override the event_idx function to retrieve and convert the perf_event index to armv8 hardware index. Since the arm_pmu driver can be used by any implementation, even if not armv8, two components play a role into making sure the behaviour is correct and consistent with the PMU capabilities: * the ARMPMU_EL0_RD_CNTR flag which denotes the capability to access counter from userspace. * the event_idx call back, which is implemented and initialized by the PMU implementation: if no callback is provided, the default behaviour applies, returning 0 as index value. Signed-off-by: Raphael Gault <raphael.gault@arm.com> --- arch/arm64/kernel/perf_event.c | 21 +++++++++++++++++++++ include/linux/perf/arm_pmu.h | 2 ++ 2 files changed, 23 insertions(+)