[RFC PATCH 1/2] perf: arm_spe: Fix consistency of PMSCR register bit CX
German Gomez
german.gomez at arm.com
Wed Feb 16 07:16:01 PST 2022
Hi Leo,
Thanks for the review
On 16/02/2022 13:22, Leo Yan wrote:
> On Tue, Feb 15, 2022 at 02:29:27PM +0000, German Gomez wrote:
>
> [...]
>
>> Thanks for the suggestion. I recorded the following stacktrace:
>>
>> perf-323841 [052] d.... 3996.528812: arm_spe_pmu_setup_aux: (arm_spe_pmu_setup_aux+0x60/0x1c0 [arm_spe_pmu])
>> perf-323841 [052] d.... 3996.528813: <stack trace>
> Yeah, this show arm_spe_pmu_setup_aux() is called in perf process.
>
>> => kprobe_dispatcher
>> => kprobe_breakpoint_handler
>> => call_break_hook
>> => brk_handler
>> => do_debug_exception
>> => el1_dbg
>> => el1h_64_sync_handler
>> => el1h_64_sync
>> => arm_spe_pmu_setup_aux
>> => perf_mmap
>> => mmap_region
>> => do_mmap
>> => vm_mmap_pgoff
>> => ksys_mmap_pgoff
>> => __arm64_sys_mmap
>> => invoke_syscall
>> => el0_svc_common.constprop.0
>> => do_el0_svc
>> => el0_svc
>> => el0t_64_sync_handler
>> => el0t_64_sync
>>
>> So for a v2 I may include something like this:
> The change looks good to me, please see below minor comment.
>
>> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
>> index d44bcc29d..aadec5a0e 100644
>> --- a/drivers/perf/arm_spe_pmu.c
>> +++ b/drivers/perf/arm_spe_pmu.c
>> @@ -45,6 +45,7 @@ struct arm_spe_pmu_buf {
>> int nr_pages;
>> bool snapshot;
>> void *base;
>> + u64 pmscr;
>> };
>>
>> struct arm_spe_pmu {
>> @@ -748,7 +749,7 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags)
>> write_sysreg_s(reg, SYS_PMSICR_EL1);
>> }
>>
>> - reg = arm_spe_event_to_pmscr(event);
>> + reg = ((struct arm_spe_pmu_buf *) perf_get_aux(handle))->pmscr;
>> isb();
>> write_sysreg_s(reg, SYS_PMSCR_EL1);
> Just nitpick (or it's just my preferring coding style), we can define
> a local pointer variable 'buf':
>
> struct arm_spe_pmu_buf *buf = perf_get_aux(handle);
I need to make sure perf_get_aux(..) is called between perf_aux_output_begin and *_end though (so, after arm_spe_perf_aux_output_begin(..)):
buf = perf_get_aux(handle);
reg = buf->pmscr;
isb();
write_sysreg_s(buf, SYS_PMSCR_EL1);
Alternatively, we set the register inside of perf_aux_output_begin. It might be confusing for casual readers because the function handles a case where perf_get_aux(..) returns NULL.
Alternatively, we could also wrap perf_get_aux(..) in a static inline function that returns the correct type and do:
reg = arm_spe_get_aux(handle)->pmscr;
isb();
write_sysreg_s(reg, SYS_PMSCR_EL1);
So that it looks cleaner.
>
> ...
>
> isb();
> write_sysreg_s(buf->pmscr, SYS_PMSCR_EL1);
>
> Thanks,
> Leo
>
>> }
>> @@ -855,6 +856,8 @@ static void *arm_spe_pmu_setup_aux(struct perf_event *event, void **pages,
>> if (!pglist)
>> goto out_free_buf;
>>
>> + buf->pmscr = arm_spe_event_to_pmscr(event);
>> +
>> for (i = 0; i < nr_pages; ++i)
>> pglist[i] = virt_to_page(pages[i]);
>>
>>> Thanks,
>>> Leo
More information about the linux-arm-kernel
mailing list