From: Leo Yan <leo.yan@linaro.org> To: German Gomez <german.gomez@arm.com> Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, will@kernel.org, mark.rutland@arm.com, james.clark@arm.com Subject: Re: [RFC PATCH 1/2] perf: arm_spe: Fix consistency of PMSCR register bit CX Date: Sat, 5 Feb 2022 23:39:40 +0800 [thread overview] Message-ID: <20220205153940.GB391033@leoy-ThinkPad-X240s> (raw) In-Reply-To: <20220117124432.3119132-2-german.gomez@arm.com> Hi German, On Mon, Jan 17, 2022 at 12:44:31PM +0000, German Gomez wrote: > The arm_spe_pmu driver will enable the CX bit of the PMSCR register in > order to add CONTEXT packets into the traces if the owner of the perf > event runs with capabilities CAP_PERFMON or CAP_SYS_ADMIN. > > The value of the bit is computed in the arm_spe_event_to_pmscr function > [1], and the check for capabilities happens in [2] in the pmu init > callback. This suggests that the value of the CX bit should remain > consistent for the duration of the perf session. > > However, the function arm_spe_event_to_pmscr may be called later during > the start callback [3] when the "current" process is not the owner of > the perf session, so the CX bit is currently not consistent. Consider > the following example: > > 1. Run a process in the background with capability CAP_SYS_ADMIN in CPU0. > > $ taskset --cpu-list 0 sudo dd if=/dev/random of=/dev/null & > [3] 3806 > > 2. Begin a perf session _without_ capabilities (we shouldn't see CONTEXT packets). > > $ perf record -e arm_spe_0// -C0 -- sleep 1 > $ perf report -D | grep CONTEXT > . 0000000e: 65 df 0e 00 00 CONTEXT 0xedf el2 > . 0000004e: 65 df 0e 00 00 CONTEXT 0xedf el2 > . 0000008e: 65 df 0e 00 00 CONTEXT 0xedf el2 > [...] Could you confirm if you still can reproduce this issue on the latest mainline kernel? I cannot reproduce this issue on the latest mainline kernel, I suspect this is impacted by recent refactoring evlist patches from Ian Rogers (though I am not for this). > As can be seen, the traces begin showing CONTEXT packets when the pid is > 0xedf (3807). This happens because the pmu start callback is run when > the current process is not the owner of the perf session, so the CX > register bit is set. I can image a potential issue is: the "dd" program running in background with capability CAP_SYS_ADMIN on CPU0, and then perf session sends an IPI remotely from any other CPU to CPU0, the dd process (on CPU0) is interrupted to handle ioctl PERF_EVENT_IOC_ENABLE, thus perfmon_capable() returns the capability of dd process, finally it leads to the wrong setting for PMSCR. I reviewed the code and also traced the backtrace for the function arm_spe_pmu_start(), I can confirm that every time perf session will execute below flow: evlist__enable() __evlist__enable() evlist__for_each_cpu() { -> call affinity__set() evsel__enable_cpu() } We can see the macro evlist__for_each_cpu() will extend to invoke evlist__cpu_begin() and affinity__set(); affinity__set() will set CPU affinity to the target CPU, thus perf process will firstly migrate to the target CPU and enable event on the target CPU. This means perf will not send remote IPI and it directly runs on target CPU, and the dd program will not interfere capabilities for perf session. Thanks, Leo > One way to fix this is by caching the value of the CX bit during the > initialization of the PMU event, so that it remains consistent for the > duration of the session. > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/arm_spe_pmu.c?h=v5.16#n275 > [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/arm_spe_pmu.c?h=v5.16#n713 > [3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/arm_spe_pmu.c?h=v5.16#n751 > > Signed-off-by: German Gomez <german.gomez@arm.com> > --- > drivers/perf/arm_spe_pmu.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c > index d44bcc29d..8515bf85c 100644 > --- a/drivers/perf/arm_spe_pmu.c > +++ b/drivers/perf/arm_spe_pmu.c > @@ -57,6 +57,7 @@ struct arm_spe_pmu { > u16 pmsver; > u16 min_period; > u16 counter_sz; > + bool pmscr_cx; > > #define SPE_PMU_FEAT_FILT_EVT (1UL << 0) > #define SPE_PMU_FEAT_FILT_TYP (1UL << 1) > @@ -260,6 +261,7 @@ static const struct attribute_group *arm_spe_pmu_attr_groups[] = { > static u64 arm_spe_event_to_pmscr(struct perf_event *event) > { > struct perf_event_attr *attr = &event->attr; > + struct arm_spe_pmu *spe_pmu = to_spe_pmu(event->pmu); > u64 reg = 0; > > reg |= ATTR_CFG_GET_FLD(attr, ts_enable) << SYS_PMSCR_EL1_TS_SHIFT; > @@ -272,7 +274,7 @@ static u64 arm_spe_event_to_pmscr(struct perf_event *event) > if (!attr->exclude_kernel) > reg |= BIT(SYS_PMSCR_EL1_E1SPE_SHIFT); > > - if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable()) > + if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && spe_pmu->pmscr_cx) > reg |= BIT(SYS_PMSCR_EL1_CX_SHIFT); > > return reg; > @@ -709,10 +711,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event) > !(spe_pmu->features & SPE_PMU_FEAT_FILT_LAT)) > return -EOPNOTSUPP; > > + spe_pmu->pmscr_cx = perfmon_capable(); > reg = arm_spe_event_to_pmscr(event); > if (!perfmon_capable() && > (reg & (BIT(SYS_PMSCR_EL1_PA_SHIFT) | > - BIT(SYS_PMSCR_EL1_CX_SHIFT) | > BIT(SYS_PMSCR_EL1_PCT_SHIFT)))) > return -EACCES; > > -- > 2.25.1 >
WARNING: multiple messages have this Message-ID (diff)
From: Leo Yan <leo.yan@linaro.org> To: German Gomez <german.gomez@arm.com> Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, will@kernel.org, mark.rutland@arm.com, james.clark@arm.com Subject: Re: [RFC PATCH 1/2] perf: arm_spe: Fix consistency of PMSCR register bit CX Date: Sat, 5 Feb 2022 23:39:40 +0800 [thread overview] Message-ID: <20220205153940.GB391033@leoy-ThinkPad-X240s> (raw) In-Reply-To: <20220117124432.3119132-2-german.gomez@arm.com> Hi German, On Mon, Jan 17, 2022 at 12:44:31PM +0000, German Gomez wrote: > The arm_spe_pmu driver will enable the CX bit of the PMSCR register in > order to add CONTEXT packets into the traces if the owner of the perf > event runs with capabilities CAP_PERFMON or CAP_SYS_ADMIN. > > The value of the bit is computed in the arm_spe_event_to_pmscr function > [1], and the check for capabilities happens in [2] in the pmu init > callback. This suggests that the value of the CX bit should remain > consistent for the duration of the perf session. > > However, the function arm_spe_event_to_pmscr may be called later during > the start callback [3] when the "current" process is not the owner of > the perf session, so the CX bit is currently not consistent. Consider > the following example: > > 1. Run a process in the background with capability CAP_SYS_ADMIN in CPU0. > > $ taskset --cpu-list 0 sudo dd if=/dev/random of=/dev/null & > [3] 3806 > > 2. Begin a perf session _without_ capabilities (we shouldn't see CONTEXT packets). > > $ perf record -e arm_spe_0// -C0 -- sleep 1 > $ perf report -D | grep CONTEXT > . 0000000e: 65 df 0e 00 00 CONTEXT 0xedf el2 > . 0000004e: 65 df 0e 00 00 CONTEXT 0xedf el2 > . 0000008e: 65 df 0e 00 00 CONTEXT 0xedf el2 > [...] Could you confirm if you still can reproduce this issue on the latest mainline kernel? I cannot reproduce this issue on the latest mainline kernel, I suspect this is impacted by recent refactoring evlist patches from Ian Rogers (though I am not for this). > As can be seen, the traces begin showing CONTEXT packets when the pid is > 0xedf (3807). This happens because the pmu start callback is run when > the current process is not the owner of the perf session, so the CX > register bit is set. I can image a potential issue is: the "dd" program running in background with capability CAP_SYS_ADMIN on CPU0, and then perf session sends an IPI remotely from any other CPU to CPU0, the dd process (on CPU0) is interrupted to handle ioctl PERF_EVENT_IOC_ENABLE, thus perfmon_capable() returns the capability of dd process, finally it leads to the wrong setting for PMSCR. I reviewed the code and also traced the backtrace for the function arm_spe_pmu_start(), I can confirm that every time perf session will execute below flow: evlist__enable() __evlist__enable() evlist__for_each_cpu() { -> call affinity__set() evsel__enable_cpu() } We can see the macro evlist__for_each_cpu() will extend to invoke evlist__cpu_begin() and affinity__set(); affinity__set() will set CPU affinity to the target CPU, thus perf process will firstly migrate to the target CPU and enable event on the target CPU. This means perf will not send remote IPI and it directly runs on target CPU, and the dd program will not interfere capabilities for perf session. Thanks, Leo > One way to fix this is by caching the value of the CX bit during the > initialization of the PMU event, so that it remains consistent for the > duration of the session. > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/arm_spe_pmu.c?h=v5.16#n275 > [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/arm_spe_pmu.c?h=v5.16#n713 > [3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/arm_spe_pmu.c?h=v5.16#n751 > > Signed-off-by: German Gomez <german.gomez@arm.com> > --- > drivers/perf/arm_spe_pmu.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c > index d44bcc29d..8515bf85c 100644 > --- a/drivers/perf/arm_spe_pmu.c > +++ b/drivers/perf/arm_spe_pmu.c > @@ -57,6 +57,7 @@ struct arm_spe_pmu { > u16 pmsver; > u16 min_period; > u16 counter_sz; > + bool pmscr_cx; > > #define SPE_PMU_FEAT_FILT_EVT (1UL << 0) > #define SPE_PMU_FEAT_FILT_TYP (1UL << 1) > @@ -260,6 +261,7 @@ static const struct attribute_group *arm_spe_pmu_attr_groups[] = { > static u64 arm_spe_event_to_pmscr(struct perf_event *event) > { > struct perf_event_attr *attr = &event->attr; > + struct arm_spe_pmu *spe_pmu = to_spe_pmu(event->pmu); > u64 reg = 0; > > reg |= ATTR_CFG_GET_FLD(attr, ts_enable) << SYS_PMSCR_EL1_TS_SHIFT; > @@ -272,7 +274,7 @@ static u64 arm_spe_event_to_pmscr(struct perf_event *event) > if (!attr->exclude_kernel) > reg |= BIT(SYS_PMSCR_EL1_E1SPE_SHIFT); > > - if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable()) > + if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && spe_pmu->pmscr_cx) > reg |= BIT(SYS_PMSCR_EL1_CX_SHIFT); > > return reg; > @@ -709,10 +711,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event) > !(spe_pmu->features & SPE_PMU_FEAT_FILT_LAT)) > return -EOPNOTSUPP; > > + spe_pmu->pmscr_cx = perfmon_capable(); > reg = arm_spe_event_to_pmscr(event); > if (!perfmon_capable() && > (reg & (BIT(SYS_PMSCR_EL1_PA_SHIFT) | > - BIT(SYS_PMSCR_EL1_CX_SHIFT) | > BIT(SYS_PMSCR_EL1_PCT_SHIFT)))) > return -EACCES; > > -- > 2.25.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-02-05 15:39 UTC|newest] Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-01-17 12:44 [RFC PATCH 0/2] perf: arm_spe: Fix consistency of CONTEXT packets in SPE driver German Gomez 2022-01-17 12:44 ` German Gomez 2022-01-17 12:44 ` [RFC PATCH 1/2] perf: arm_spe: Fix consistency of PMSCR register bit CX German Gomez 2022-01-17 12:44 ` German Gomez 2022-01-18 10:07 ` Will Deacon 2022-01-18 10:07 ` Will Deacon 2022-01-18 14:04 ` German Gomez 2022-01-18 14:04 ` German Gomez 2022-01-19 11:27 ` German Gomez 2022-01-19 11:27 ` German Gomez 2022-01-18 16:28 ` James Clark 2022-01-18 16:28 ` James Clark 2022-02-05 15:39 ` Leo Yan [this message] 2022-02-05 15:39 ` Leo Yan 2022-02-07 12:06 ` German Gomez 2022-02-07 12:06 ` German Gomez 2022-02-08 13:00 ` Leo Yan 2022-02-08 13:00 ` Leo Yan 2022-02-10 17:23 ` German Gomez 2022-02-10 17:23 ` German Gomez 2022-02-11 10:45 ` Leo Yan 2022-02-11 10:45 ` Leo Yan 2022-02-15 14:29 ` German Gomez 2022-02-15 14:29 ` German Gomez 2022-02-16 13:22 ` Leo Yan 2022-02-16 13:22 ` Leo Yan 2022-02-16 15:16 ` German Gomez 2022-02-16 15:16 ` German Gomez 2022-01-17 12:44 ` [RFC PATCH 2/2] perf: arm_spe: Enable CONTEXT packets in SPE traces if the profiler runs in CPU mode German Gomez 2022-01-17 12:44 ` German Gomez 2022-01-17 14:04 ` German Gomez 2022-01-17 14:04 ` German Gomez 2022-01-18 9:52 ` Will Deacon 2022-01-18 9:52 ` Will Deacon 2022-01-18 14:13 ` German Gomez 2022-01-18 14:13 ` German Gomez
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220205153940.GB391033@leoy-ThinkPad-X240s \ --to=leo.yan@linaro.org \ --cc=german.gomez@arm.com \ --cc=james.clark@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.