| Message ID | 20220623112735.357093-3-geomatsi@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | perf: RISC-V: fix access to the highest available counter | expand |
On Thu, Jun 23, 2022 at 4:27 AM Sergey Matyukevich <geomatsi@gmail.com> wrote: > > From: Sergey Matyukevich <sergey.matyukevich@syntacore.com> > > Both OpenSBI and Linux explicitly assume that there is no hardware counter > with index 1: hardware uses that bit for TM control. So OpenSBI filters > out that index in sanity checks. However its range sanity checks do not > treat that index in a special way. As a result, OpenSBI does not allow > to use the firmware counter with the highest index. Linux perf RISC-V > SBI driver is adapted for the current OpenSBI behavior: it excludes the > highest valid index from the counter mask passed to OpenSBI. > > This patch fixes ranges to re-enable the highest available counter. > > Accompanying OpenSBI fix to accept full mask: > - https://github.com/riscv-software-src/opensbi/pull/260 > > Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com> > --- > drivers/perf/riscv_pmu_sbi.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > index 3e0ea564b9b8..294d4bded59e 100644 > --- a/drivers/perf/riscv_pmu_sbi.c > +++ b/drivers/perf/riscv_pmu_sbi.c > @@ -265,7 +265,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event) > struct sbiret ret; > int idx; > uint64_t cbase = 0; > - uint64_t cmask = GENMASK_ULL(rvpmu->num_counters - 1, 0); > + uint64_t cmask = GENMASK_ULL(rvpmu->num_counters, 0); > unsigned long cflags = 0; > > if (event->attr.exclude_kernel) > @@ -283,7 +283,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event) > } > > idx = ret.value; > - if (idx >= rvpmu->num_counters || !pmu_ctr_list[idx].value) > + if (idx > rvpmu->num_counters || !pmu_ctr_list[idx].value) > return -ENOENT; > > /* Additional sanity check for the counter id */ > @@ -482,7 +482,7 @@ static inline void pmu_sbi_stop_all(struct riscv_pmu *pmu) > * which may include counters that are not enabled yet. > */ > sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP, > - 0, GENMASK_ULL(pmu->num_counters - 1, 0), 0, 0, 0, 0); > + 0, GENMASK_ULL(pmu->num_counters, 0), 0, 0, 0, 0); > } > > static inline void pmu_sbi_stop_hw_ctrs(struct riscv_pmu *pmu) > -- > 2.36.1 > This won't be required to support maximum firmware counter index. With the fix, I proposed in your first patch in kernel and OpenSBI, it should be doable. Both kernel & OpenSBI should use zero indexed counter schemes to avoid confusion. -- Regards, Atish
diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c index 3e0ea564b9b8..294d4bded59e 100644 --- a/drivers/perf/riscv_pmu_sbi.c +++ b/drivers/perf/riscv_pmu_sbi.c @@ -265,7 +265,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event) struct sbiret ret; int idx; uint64_t cbase = 0; - uint64_t cmask = GENMASK_ULL(rvpmu->num_counters - 1, 0); + uint64_t cmask = GENMASK_ULL(rvpmu->num_counters, 0); unsigned long cflags = 0; if (event->attr.exclude_kernel) @@ -283,7 +283,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event) } idx = ret.value; - if (idx >= rvpmu->num_counters || !pmu_ctr_list[idx].value) + if (idx > rvpmu->num_counters || !pmu_ctr_list[idx].value) return -ENOENT; /* Additional sanity check for the counter id */ @@ -482,7 +482,7 @@ static inline void pmu_sbi_stop_all(struct riscv_pmu *pmu) * which may include counters that are not enabled yet. */ sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP, - 0, GENMASK_ULL(pmu->num_counters - 1, 0), 0, 0, 0, 0); + 0, GENMASK_ULL(pmu->num_counters, 0), 0, 0, 0, 0); } static inline void pmu_sbi_stop_hw_ctrs(struct riscv_pmu *pmu)