[PATCH 1/1] lib: pmu: allow to use the highest available counter

[PATCH 1/1] lib: pmu: allow to use the highest available counter

[Sergey Matyukevich]

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@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 at 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))
 		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

[PATCH 1/1] lib: pmu: allow to use the highest available counter

[Anup Patel]

On Thu, Jun 23, 2022 at 7:43 PM Sergey Matyukevich <geomatsi@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@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 at 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() ?

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

[PATCH 1/1] lib: pmu: allow to use the highest available counter

[Atish Patra]

On Thu, Jun 23, 2022 at 8:31 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Thu, Jun 23, 2022 at 7:43 PM Sergey Matyukevich <geomatsi@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@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 at 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.

> 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

[PATCH 1/1] lib: pmu: allow to use the highest available counter

[Anup Patel]

Hi Atish and Sergey,

On Thu, Jun 23, 2022 at 11:29 PM Atish Patra <atishp@atishpatra.org> wrote:
>
> On Thu, Jun 23, 2022 at 8:31 AM Anup Patel <anup@brainfault.org> wrote:
> >
> > On Thu, Jun 23, 2022 at 7:43 PM Sergey Matyukevich <geomatsi@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@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 at 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

[PATCH 1/1] lib: pmu: allow to use the highest available counter

[Sergey Matyukevich]

Hello,

> > > 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@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 at 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

Indeed, this is simpler. But as I mentioned in my reply to v2, this way
you report incorrect number of hw counters to kernel just to get the
correct mask of counters in response. And that behavior will have to
be followed by all the other SBI implementations. 

Current OpenSBI implementation implies the only gap in counters: index 1.
So my approach in v1 version of this patch was as follows:
- send the accurate number of counters to the kernel (hw + fw)
- fix range checks to take into account gap for index 1
This way OpenSBI just sends correct information to the kernel and
expects (and verifies) correct data in response. Accompanying kernel
fixes are the first two patches in the kernel series, see:
https://lore.kernel.org/linux-riscv/20220623112735.357093-1-geomatsi at gmail.com/T/#m4811c83cf767cdaaeb31e2530d4a51034fcf6c3c

The 3rd kernel patch switches from array to IDR. With that change kernel
driver will support any gaps in counters, not only index 1. So OpenSBI
or any other SBI implementation will be able to exclude some hw or fw
counters if needed without changing kernel driver. So far I can see
two possible examples where it could be useful:
- platform specific quirks in OpenSBI to exclude non-functional counters
- keep some hw counters for internal use in OpenSBI or any other SBI implementation

Regards,
Sergey

[PATCH 1/1] lib: pmu: allow to use the highest available counter

[Anup Patel]

On Fri, Jun 24, 2022 at 1:18 PM Sergey Matyukevich <geomatsi@gmail.com> wrote:
>
> Hello,
>
> > > > 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@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 at 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
>
> Indeed, this is simpler. But as I mentioned in my reply to v2, this way
> you report incorrect number of hw counters to kernel just to get the
> correct mask of counters in response. And that behavior will have to
> be followed by all the other SBI implementations.

I don't think this patch reports an incorrect number of counters because
the time CSR is indeed a counter which counts timer ticks.

Just like cycle and instret CSRs, the time CSR is also a fixed counter
which cannot count any other HW event.

The only special thing about time CSR is that as-per RISC-V privilege
specification the time CSR is always counting and there is no way to
halt/resume it because it is an alias of platform-level free-running
mtime counter.

It's totally up to the SBI implementation whether it wants to expose
fixed HW counters (cycle, time, and insret) as SBI PMU counters
or not. In fact, the SBI PMU specification defines counter_idx as
logical counter with no relation to HW counter numbering.

A perfectly valid SBI implementation is allowed to do various things:
1) Intermix HW counters and FW counters (e.g. counter_idx = 0 is
    FW counter but counter_idx = 1 is HW counter)
2) Make a particular HW counter unusable through SBI PMU due
    to some errata (e.g. If mhpmcounter3 is broken then platform
    will not set bit[3] in any of the counter-mask provided in the
   "riscv,pmu" DT node which is parsed by OpenSBI). The
   sbi_pmu_config_matching() will never select a counter_idx
   which is not part of any counter-mask provided by the platform.
3) Create on-demand mapping of counter_idx to HW counters
   because SBI implementation is doing trap-n-emulate of
   counters. This is particularly true for hypervisors like KVM
   when counters are shared between host and guest so the
   hypervisor will on-demand allocate a HW counter and map
   it to a logical counter_idx seen by the guest. (Note: same
   approach is taken by KVM ARM)

>
> Current OpenSBI implementation implies the only gap in counters: index 1.
> So my approach in v1 version of this patch was as follows:
> - send the accurate number of counters to the kernel (hw + fw)
> - fix range checks to take into account gap for index 1
> This way OpenSBI just sends correct information to the kernel and
> expects (and verifies) correct data in response. Accompanying kernel
> fixes are the first two patches in the kernel series, see:
> https://lore.kernel.org/linux-riscv/20220623112735.357093-1-geomatsi at gmail.com/T/#m4811c83cf767cdaaeb31e2530d4a51034fcf6c3c
>
> The 3rd kernel patch switches from array to IDR. With that change kernel
> driver will support any gaps in counters, not only index 1. So OpenSBI
> or any other SBI implementation will be able to exclude some hw or fw
> counters if needed without changing kernel driver. So far I can see
> two possible examples where it could be useful:
> - platform specific quirks in OpenSBI to exclude non-functional counters
> - keep some hw counters for internal use in OpenSBI or any other SBI implementation

I think there is some confusion with regards to counter_idx.

As-per SBI spec, the counter_idx is a logical number so if there are N
counters then 0 <= counter_idx < N. There are no gaps in counter_idx
numbering. Of course, an SBI implementation might intentionally never
configure a particular counter_idx because it is special or broken.

Regards,
Anup

>
> Regards,
> Sergey
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 1/1] lib: pmu: allow to use the highest available counter

[Sergey Matyukevich]

Hi Anup,

> > > > > 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@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 at 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
> >
> > Indeed, this is simpler. But as I mentioned in my reply to v2, this way
> > you report incorrect number of hw counters to kernel just to get the
> > correct mask of counters in response. And that behavior will have to
> > be followed by all the other SBI implementations.
> 
> I don't think this patch reports an incorrect number of counters because
> the time CSR is indeed a counter which counts timer ticks.
> 
> Just like cycle and instret CSRs, the time CSR is also a fixed counter
> which cannot count any other HW event.
> 
> The only special thing about time CSR is that as-per RISC-V privilege
> specification the time CSR is always counting and there is no way to
> halt/resume it because it is an alias of platform-level free-running
> mtime counter.
> 
> It's totally up to the SBI implementation whether it wants to expose
> fixed HW counters (cycle, time, and insret) as SBI PMU counters
> or not. In fact, the SBI PMU specification defines counter_idx as
> logical counter with no relation to HW counter numbering.
> 
> A perfectly valid SBI implementation is allowed to do various things:
> 1) Intermix HW counters and FW counters (e.g. counter_idx = 0 is
>     FW counter but counter_idx = 1 is HW counter)
> 2) Make a particular HW counter unusable through SBI PMU due
>     to some errata (e.g. If mhpmcounter3 is broken then platform
>     will not set bit[3] in any of the counter-mask provided in the
>    "riscv,pmu" DT node which is parsed by OpenSBI). The
>    sbi_pmu_config_matching() will never select a counter_idx
>    which is not part of any counter-mask provided by the platform.
> 3) Create on-demand mapping of counter_idx to HW counters
>    because SBI implementation is doing trap-n-emulate of
>    counters. This is particularly true for hypervisors like KVM
>    when counters are shared between host and guest so the
>    hypervisor will on-demand allocate a HW counter and map
>    it to a logical counter_idx seen by the guest. (Note: same
>    approach is taken by KVM ARM)
> 
> >
> > Current OpenSBI implementation implies the only gap in counters: index 1.
> > So my approach in v1 version of this patch was as follows:
> > - send the accurate number of counters to the kernel (hw + fw)
> > - fix range checks to take into account gap for index 1
> > This way OpenSBI just sends correct information to the kernel and
> > expects (and verifies) correct data in response. Accompanying kernel
> > fixes are the first two patches in the kernel series, see:
> > https://lore.kernel.org/linux-riscv/20220623112735.357093-1-geomatsi at gmail.com/T/#m4811c83cf767cdaaeb31e2530d4a51034fcf6c3c
> >
> > The 3rd kernel patch switches from array to IDR. With that change kernel
> > driver will support any gaps in counters, not only index 1. So OpenSBI
> > or any other SBI implementation will be able to exclude some hw or fw
> > counters if needed without changing kernel driver. So far I can see
> > two possible examples where it could be useful:
> > - platform specific quirks in OpenSBI to exclude non-functional counters
> > - keep some hw counters for internal use in OpenSBI or any other SBI implementation
> 
> I think there is some confusion with regards to counter_idx.
> 
> As-per SBI spec, the counter_idx is a logical number so if there are N
> counters then 0 <= counter_idx < N. There are no gaps in counter_idx
> numbering. Of course, an SBI implementation might intentionally never
> configure a particular counter_idx because it is special or broken.

Thanks for clarifications! So, in brief, it is OK for valid SBI
implementation to do the following:

- to report _total_ number of counters (hw + fw), including special
  ones (like timer) or broken ones or reserved ones
- sbi_pmu_ctr_cfg_match should never send special/reserved/broken
  counters to OS

That makes sense to me. So v2 changes suggested by Atish look reasonable.
But I guess commit message needs to be clarified. 

I will shortly send you v3 version of the OpenSBI patch with updated
commit message. I will also send v2 of the kernel patches. 

Regards,
Sergey

[PATCH 1/1] lib: pmu: allow to use the highest available counter

[Anup Patel]

On Fri, Jun 24, 2022 at 3:31 PM Sergey Matyukevich <geomatsi@gmail.com> wrote:
>
> Hi Anup,
>
> > > > > > 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@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 at 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
> > >
> > > Indeed, this is simpler. But as I mentioned in my reply to v2, this way
> > > you report incorrect number of hw counters to kernel just to get the
> > > correct mask of counters in response. And that behavior will have to
> > > be followed by all the other SBI implementations.
> >
> > I don't think this patch reports an incorrect number of counters because
> > the time CSR is indeed a counter which counts timer ticks.
> >
> > Just like cycle and instret CSRs, the time CSR is also a fixed counter
> > which cannot count any other HW event.
> >
> > The only special thing about time CSR is that as-per RISC-V privilege
> > specification the time CSR is always counting and there is no way to
> > halt/resume it because it is an alias of platform-level free-running
> > mtime counter.
> >
> > It's totally up to the SBI implementation whether it wants to expose
> > fixed HW counters (cycle, time, and insret) as SBI PMU counters
> > or not. In fact, the SBI PMU specification defines counter_idx as
> > logical counter with no relation to HW counter numbering.
> >
> > A perfectly valid SBI implementation is allowed to do various things:
> > 1) Intermix HW counters and FW counters (e.g. counter_idx = 0 is
> >     FW counter but counter_idx = 1 is HW counter)
> > 2) Make a particular HW counter unusable through SBI PMU due
> >     to some errata (e.g. If mhpmcounter3 is broken then platform
> >     will not set bit[3] in any of the counter-mask provided in the
> >    "riscv,pmu" DT node which is parsed by OpenSBI). The
> >    sbi_pmu_config_matching() will never select a counter_idx
> >    which is not part of any counter-mask provided by the platform.
> > 3) Create on-demand mapping of counter_idx to HW counters
> >    because SBI implementation is doing trap-n-emulate of
> >    counters. This is particularly true for hypervisors like KVM
> >    when counters are shared between host and guest so the
> >    hypervisor will on-demand allocate a HW counter and map
> >    it to a logical counter_idx seen by the guest. (Note: same
> >    approach is taken by KVM ARM)
> >
> > >
> > > Current OpenSBI implementation implies the only gap in counters: index 1.
> > > So my approach in v1 version of this patch was as follows:
> > > - send the accurate number of counters to the kernel (hw + fw)
> > > - fix range checks to take into account gap for index 1
> > > This way OpenSBI just sends correct information to the kernel and
> > > expects (and verifies) correct data in response. Accompanying kernel
> > > fixes are the first two patches in the kernel series, see:
> > > https://lore.kernel.org/linux-riscv/20220623112735.357093-1-geomatsi at gmail.com/T/#m4811c83cf767cdaaeb31e2530d4a51034fcf6c3c
> > >
> > > The 3rd kernel patch switches from array to IDR. With that change kernel
> > > driver will support any gaps in counters, not only index 1. So OpenSBI
> > > or any other SBI implementation will be able to exclude some hw or fw
> > > counters if needed without changing kernel driver. So far I can see
> > > two possible examples where it could be useful:
> > > - platform specific quirks in OpenSBI to exclude non-functional counters
> > > - keep some hw counters for internal use in OpenSBI or any other SBI implementation
> >
> > I think there is some confusion with regards to counter_idx.
> >
> > As-per SBI spec, the counter_idx is a logical number so if there are N
> > counters then 0 <= counter_idx < N. There are no gaps in counter_idx
> > numbering. Of course, an SBI implementation might intentionally never
> > configure a particular counter_idx because it is special or broken.
>
> Thanks for clarifications! So, in brief, it is OK for valid SBI
> implementation to do the following:
>
> - to report _total_ number of counters (hw + fw), including special
>   ones (like timer) or broken ones or reserved ones

Yes, as long as SBI implementation controls what counters can be
configured successfully.

> - sbi_pmu_ctr_cfg_match should never send special/reserved/broken
>   counters to OS

Yes, sbi_pmu_counter_config_matching() should:
1) Never select broken or reserved or special counters
2) Select fixed counters (such as cycle or insret) only for specific PMU event

>
> That makes sense to me. So v2 changes suggested by Atish look reasonable.
> But I guess commit message needs to be clarified.
>
> I will shortly send you v3 version of the OpenSBI patch with updated
> commit message. I will also send v2 of the kernel patches.

Sure, thanks.

Also, thanks for catching this issue.

>
> Regards,
> Sergey

Regards,
Anup