[PATCH v2 3/3] lib: sbi_pmu: Fix counter and event info error codes as per SBI v3.0 spec
Anup Patel
anup at brainfault.org
Fri Sep 4 00:56:22 PDT 2026
On Wed, Aug 19, 2026 at 2:31 AM David E. Garcia Porras
<david.garcia at aheadcomputing.com> wrote:
>
> Align the PMU extension implementation with the error codes required
> by the SBI v3.0 specification, chapter 11:
>
> - sbi_pmu_counter_start and sbi_pmu_counter_stop (secs 11.9-11.10,
> tables 39-42): the start_flags/stop_flags bits 2:(XLEN-1) are
> reserved and must be zero, so return SBI_ERR_INVALID_PARAM when any
> reserved flag bit is set. Introduce SBI_PMU_START_FLAGS_MASK and
> SBI_PMU_STOP_FLAGS_MASK for the valid bits of each function.
>
> - sbi_pmu_counter_start and sbi_pmu_counter_stop (tables 40 and 42):
> return SBI_ERR_ALREADY_STARTED / SBI_ERR_ALREADY_STOPPED when the
> set of counters includes a counter which is already started or
> stopped, instead of ignoring the error returned for each counter.
>
> - sbi_pmu_event_get_info (sec 11.14, table 47): the output word must
> indicate whether the event is supported, but firmware events were
> only matched against the hardware event map and were always
> reported as unsupported. Report a validated firmware event as
> supported.
>
> Signed-off-by: David E. Garcia Porras <david.garcia at aheadcomputing.com>
> ---
> include/sbi/sbi_ecall_interface.h | 12 ++++++
> lib/sbi/sbi_pmu.c | 61 ++++++++++++++++++++-----------
> 2 files changed, 52 insertions(+), 21 deletions(-)
>
> diff --git a/include/sbi/sbi_ecall_interface.h b/include/sbi/sbi_ecall_interface.h
> index bfde25d0..fd4e77ca 100644
> --- a/include/sbi/sbi_ecall_interface.h
> +++ b/include/sbi/sbi_ecall_interface.h
> @@ -306,10 +306,22 @@ struct sbi_pmu_event_info {
> /* Flags defined for counter start function */
> #define SBI_PMU_START_FLAG_SET_INIT_VALUE (1 << 0)
> #define SBI_PMU_START_FLAG_INIT_FROM_SNAPSHOT (1 << 1)
> +/* Start flags valid mask */
> +#define SBI_PMU_START_FLAGS_MASK \
> + ( \
> + SBI_PMU_START_FLAG_SET_INIT_VALUE | \
> + SBI_PMU_START_FLAG_INIT_FROM_SNAPSHOT \
> + )
>
> /* Flags defined for counter stop function */
> #define SBI_PMU_STOP_FLAG_RESET (1 << 0)
> #define SBI_PMU_STOP_FLAG_TAKE_SNAPSHOT (1 << 1)
> +/* Stop flags valid mask */
> +#define SBI_PMU_STOP_FLAGS_MASK \
> + ( \
> + SBI_PMU_STOP_FLAG_RESET | \
> + SBI_PMU_STOP_FLAG_TAKE_SNAPSHOT \
> + )
>
> /* SBI function IDs for DBCN extension */
> #define SBI_EXT_DBCN_CONSOLE_WRITE 0x0
> diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
> index 676de9aa..0c62bde0 100644
> --- a/lib/sbi/sbi_pmu.c
> +++ b/lib/sbi/sbi_pmu.c
> @@ -574,6 +574,9 @@ int sbi_pmu_ctr_start(unsigned long cbase, unsigned long cmask,
> if (!pmu_ctr_idx_validate(cbase, cmask))
> return ret;
>
> + if (flags & ~SBI_PMU_START_FLAGS_MASK)
> + return SBI_ERR_INVALID_PARAM;
> +
> if (flags & SBI_PMU_STOP_FLAG_TAKE_SNAPSHOT)
> return SBI_ENO_SHMEM;
>
> @@ -592,6 +595,8 @@ int sbi_pmu_ctr_start(unsigned long cbase, unsigned long cmask,
> : 0x0;
> ret = pmu_ctr_start_fw(phs, cidx, event_code, edata,
> ival, bUpdate);
> + if (ret)
> + return ret;
> } else {
> if (cidx >= 3) {
> struct sbi_pmu_hw_event_config *ev_cfg =
> @@ -605,6 +610,8 @@ int sbi_pmu_ctr_start(unsigned long cbase, unsigned long cmask,
> return ret;
> }
> ret = pmu_ctr_start_hw(cidx, ival, bUpdate);
> + if (ret)
> + return ret;
> }
> }
>
> @@ -693,6 +700,9 @@ int sbi_pmu_ctr_stop(unsigned long cbase, unsigned long cmask,
> if (!pmu_ctr_idx_validate(cbase, cmask))
> return ret;
>
> + if (flag & ~SBI_PMU_STOP_FLAGS_MASK)
> + return SBI_ERR_INVALID_PARAM;
> +
> if (flag & SBI_PMU_STOP_FLAG_TAKE_SNAPSHOT)
> return SBI_ENO_SHMEM;
>
> @@ -708,6 +718,9 @@ int sbi_pmu_ctr_stop(unsigned long cbase, unsigned long cmask,
> else
> ret = pmu_ctr_stop_hw(cidx);
>
> + if(ret)
> + return ret;
> +
> if (cidx > (CSR_INSTRET - CSR_CYCLE) && flag & SBI_PMU_STOP_FLAG_RESET) {
> phs->active_events[cidx] = SBI_PMU_EVENT_IDX_INVALID;
> pmu_reset_hw_mhpmevent(cidx);
> @@ -1105,30 +1118,36 @@ int sbi_pmu_event_get_info(unsigned long shmem_phys_lo, unsigned long shmem_phys
> if (event_type < 0) {
> einfo[i].output = 0;
> } else {
> - for (j = 0; j < num_hw_events; j++) {
> - temp = &hw_event_map[j];
> - /* For raw events, event data is used as the select value */
> - if (event_idx == SBI_PMU_EVENT_RAW_IDX ||
> - event_idx == SBI_PMU_EVENT_RAW_V2_IDX) {
> - /*
> - * Only a raw event map entry carries a
> - * meaningful select/select_mask pair, so
> - * skip any entry which does not cover the
> - * raw event index.
> - */
> - if (temp->start_idx > event_idx ||
> - event_idx > temp->end_idx)
> - continue;
> - /* just match the selector */
> - if (temp->select == (einfo[i].event_data &
> - temp->select_mask)) {
> + if (event_type == SBI_PMU_EVENT_TYPE_FW) {
> + /* pmu_event_validate() already confirmed this event is valid; counter support is checked later by cfg_match/find_fw. */
> + einfo[i].output = 1;
> + continue;
> + } else {
> + for (j = 0; j < num_hw_events; j++) {
> + temp = &hw_event_map[j];
> + /* For raw events, event data is used as the select value */
> + if (event_idx == SBI_PMU_EVENT_RAW_IDX ||
> + event_idx == SBI_PMU_EVENT_RAW_V2_IDX) {
> + /*
> + * Only a raw event map entry carries a
> + * meaningful select/select_mask pair, so
> + * skip any entry which does not cover the
> + * raw event index.
> + */
> + if (temp->start_idx > event_idx ||
> + event_idx > temp->end_idx)
> + continue;
> + /* just match the selector */
> + if (temp->select == (einfo[i].event_data &
> + temp->select_mask)) {
> + found = true;
> + break;
> + }
> + } else if (temp->start_idx <= event_idx &&
> + event_idx <= temp->end_idx) {
This could have been a much simpler change by using an if-else ladder
like below:
if (event_type < 0) {
einfo[i].output = 0;
} else if (event_type == SBI_PMU_EVENT_TYPE_FW) {
einfo[i].output = 1;
} else {
<snip>
}
I will take care of this at the time of merging this patch.
> found = true;
> break;
> }
> - } else if (temp->start_idx <= event_idx &&
> - event_idx <= temp->end_idx) {
> - found = true;
> - break;
> }
> }
> if (found)
> --
> 2.43.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
Reviewed-by: Anup Patel <anup at brainfault.org>
Applied this patch to the riscv/opensbi repo.
Thanks,
Anup
More information about the opensbi
mailing list