[PATCH 1/1] lib: pmu: allow to use the highest available counter
Anup Patel
anup at brainfault.org
Thu Jun 23 20:40:13 PDT 2022
Hi Atish and Sergey,
On Thu, Jun 23, 2022 at 11:29 PM Atish Patra <atishp at atishpatra.org> wrote:
>
> On Thu, Jun 23, 2022 at 8:31 AM Anup Patel <anup at brainfault.org> wrote:
> >
> > On Thu, Jun 23, 2022 at 7:43 PM Sergey Matyukevich <geomatsi at gmail.com> wrote:
> > >
> > > Both OpenSBI and OS 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 the 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. This change modifies
> > > range checks to allow access to the highest index firmware counter.
> > >
> > > The simple test is to make sure that there is no counter multiplexing
> > > in the following command:
> > >
> > > $ perf stat -e \
> > > r8000000000000000,r8000000000000001,r8000000000000002,r8000000000000003, \
> > > r8000000000000004,r8000000000000005,r8000000000000006,r8000000000000007, \
> > > r8000000000000008,r8000000000000009,r800000000000000a,r800000000000000b, \
> > > r800000000000000c,r800000000000000d,r800000000000000e,r800000000000000f \
> > > ls
> > >
> > > Signed-off-by: Sergey Matyukevich <geomatsi at gmail.com>
> > >
> > > ---
> > >
> > > Note that perf RISC-V SBI Linux driver is adapted for the current OpenSBI
> > > behavior: it passes counters mask with exluded highest valid index. So the
> > > accompanying fix is also required for Linux, see the patches posted to
> > > the risc-v mailing list:
> > >
> > > https://lore.kernel.org/linux-riscv/20220623112735.357093-1-geomatsi@gmail.com/T/#m4811c83cf767cdaaeb31e2530d4a51034fcf6c3c
> > >
> > > lib/sbi/sbi_pmu.c | 18 +++++++++---------
> > > 1 file changed, 9 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
> > > index 3ecf536..b386d33 100644
> > > --- a/lib/sbi/sbi_pmu.c
> > > +++ b/lib/sbi/sbi_pmu.c
> > > @@ -115,7 +115,7 @@ static int pmu_ctr_validate(uint32_t cidx, uint32_t *event_idx_code)
> > >
> > > event_idx_val = active_events[hartid][cidx];
> > >
> > > - if (cidx >= total_ctrs || (event_idx_val == SBI_PMU_EVENT_IDX_INVALID))
> > > + if (cidx > total_ctrs || (event_idx_val == SBI_PMU_EVENT_IDX_INVALID))
> >
> > Instead of changing all comparisons of total_ctrs, why not set
> > "num_hw_ctrs = sbi_hart_mhpm_count(scratch) + 3;" in sbi_pmu_init() ?
> >
>
> Yes. That is simpler. We need a few other fixes along with that as
> well. Here is the diff
>
> diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
> index 3ecf536002a4..a532746c74be 100644
> --- a/lib/sbi/sbi_pmu.c
> +++ b/lib/sbi/sbi_pmu.c
> @@ -594,7 +594,7 @@ static int pmu_ctr_find_fw(unsigned long cbase,
> unsigned long cmask, u32 hartid)
> unsigned long ctr_mask = cmask << cbase;
>
> if (cbase <= num_hw_ctrs)
> - fw_base = num_hw_ctrs + 1;
> + fw_base = num_hw_ctrs;
> else
> fw_base = cbase;
>
> @@ -694,7 +694,7 @@ int sbi_pmu_ctr_get_info(uint32_t cidx, unsigned
> long *ctr_info)
> return SBI_EINVAL;
>
> /* We have 31 HW counters with 31 being the last index(MHPMCOUNTER31) */
> - if (cidx <= num_hw_ctrs) {
> + if (cidx < num_hw_ctrs) {
> cinfo.type = SBI_PMU_CTR_TYPE_HW;
> cinfo.csr = CSR_CYCLE + cidx;
> /* mcycle & minstret are always 64 bit */
> @@ -749,7 +749,7 @@ int sbi_pmu_init(struct sbi_scratch *scratch, bool
> cold_boot)
> sbi_platform_pmu_init(plat);
>
> /* mcycle & minstret is available always */
> - num_hw_ctrs = sbi_hart_mhpm_count(scratch) + 2;
> + num_hw_ctrs = sbi_hart_mhpm_count(scratch) + 3;
> total_ctrs = num_hw_ctrs + SBI_PMU_FW_CTR_MAX;
> }
>
> As the counters are zero indexed, the firmware counter should start
> from num_hw_ctrs
> E.g. If a hardware has 8 programmable counters, num_hw_ctrs will be set to 11.
> with counter value (0-10). Firmware counters should range from 11-26.
Can one of you quickly send a v2 (preferably today) so that I can include it for
OpenSBI v1.1 release ?
Regards,
Anup
>
> > Regards,
> > Anup
> >
> > > return SBI_EINVAL;
> > >
> > > event_idx_type = get_cidx_type(event_idx_val);
> > > @@ -347,13 +347,13 @@ int sbi_pmu_ctr_start(unsigned long cbase, unsigned long cmask,
> > > int ret = SBI_EINVAL;
> > > bool bUpdate = FALSE;
> > >
> > > - if (sbi_fls(ctr_mask) >= total_ctrs)
> > > + if (sbi_fls(ctr_mask) > total_ctrs)
> > > return ret;
> > >
> > > if (flags & SBI_PMU_START_FLAG_SET_INIT_VALUE)
> > > bUpdate = TRUE;
> > >
> > > - for_each_set_bit_from(cbase, &ctr_mask, total_ctrs) {
> > > + for_each_set_bit_from(cbase, &ctr_mask, total_ctrs + 1) {
> > > event_idx_type = pmu_ctr_validate(cbase, &event_code);
> > > if (event_idx_type < 0)
> > > /* Continue the start operation for other counters */
> > > @@ -423,10 +423,10 @@ int sbi_pmu_ctr_stop(unsigned long cbase, unsigned long cmask,
> > > uint32_t event_code;
> > > unsigned long ctr_mask = cmask << cbase;
> > >
> > > - if (sbi_fls(ctr_mask) >= total_ctrs)
> > > + if (sbi_fls(ctr_mask) > total_ctrs)
> > > return SBI_EINVAL;
> > >
> > > - for_each_set_bit_from(cbase, &ctr_mask, total_ctrs) {
> > > + for_each_set_bit_from(cbase, &ctr_mask, total_ctrs + 1) {
> > > event_idx_type = pmu_ctr_validate(cbase, &event_code);
> > > if (event_idx_type < 0)
> > > /* Continue the stop operation for other counters */
> > > @@ -598,7 +598,7 @@ static int pmu_ctr_find_fw(unsigned long cbase, unsigned long cmask, u32 hartid)
> > > else
> > > fw_base = cbase;
> > >
> > > - for (i = fw_base; i < total_ctrs; i++)
> > > + for (i = fw_base; i <= total_ctrs; i++)
> > > if ((active_events[hartid][i] == SBI_PMU_EVENT_IDX_INVALID) &&
> > > ((1UL << i) & ctr_mask))
> > > return i;
> > > @@ -618,7 +618,7 @@ int sbi_pmu_ctr_cfg_match(unsigned long cidx_base, unsigned long cidx_mask,
> > > unsigned long tmp = cidx_mask << cidx_base;
> > >
> > > /* Do a basic sanity check of counter base & mask */
> > > - if (sbi_fls(tmp) >= total_ctrs || event_type >= SBI_PMU_EVENT_TYPE_MAX)
> > > + if (sbi_fls(tmp) > total_ctrs || event_type >= SBI_PMU_EVENT_TYPE_MAX)
> > > return SBI_EINVAL;
> > >
> > > if (flags & SBI_PMU_CFG_FLAG_SKIP_MATCH) {
> > > @@ -690,7 +690,7 @@ int sbi_pmu_ctr_get_info(uint32_t cidx, unsigned long *ctr_info)
> > > struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
> > >
> > > /* Sanity check. Counter1 is not mapped at all */
> > > - if (cidx >= total_ctrs || cidx == 1)
> > > + if (cidx > total_ctrs || cidx == 1)
> > > return SBI_EINVAL;
> > >
> > > /* We have 31 HW counters with 31 being the last index(MHPMCOUNTER31) */
> > > @@ -719,7 +719,7 @@ static void pmu_reset_event_map(u32 hartid)
> > > int j;
> > >
> > > /* Initialize the counter to event mapping table */
> > > - for (j = 3; j < total_ctrs; j++)
> > > + for (j = 3; j <= total_ctrs; j++)
> > > active_events[hartid][j] = SBI_PMU_EVENT_IDX_INVALID;
> > > for (j = 0; j < SBI_PMU_FW_CTR_MAX; j++)
> > > sbi_memset(&fw_event_map[hartid][j], 0,
> > > --
> > > 2.36.1
> > >
> > >
> > > --
> > > opensbi mailing list
> > > opensbi at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/opensbi
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
>
>
>
> --
> Regards,
> Atish
More information about the opensbi
mailing list