diff mbox series

[v2,3/3] lib: sbi_pmu: Fix counter and event info error codes as per SBI v3.0 spec

Message ID 20260818210023.466462-4-david.garcia@aheadcomputing.com
State Accepted
Headers show
Series lib: sbi_pmu: SBI v3.0 PMU error code fixes | expand

Commit Message

David E. Garcia Porras Aug. 18, 2026, 9 p.m. UTC 
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@aheadcomputing.com>
---
 include/sbi/sbi_ecall_interface.h | 12 ++++++
 lib/sbi/sbi_pmu.c                 | 61 ++++++++++++++++++++-----------
 2 files changed, 52 insertions(+), 21 deletions(-)

Comments

Anup Patel Sept. 4, 2026, 7:56 a.m. UTC | #1 
On Wed, Aug 19, 2026 at 2:31 AM David E. Garcia Porras
<david.garcia@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@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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

Reviewed-by: Anup Patel <anup@brainfault.org>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup
diff mbox series

Patch

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) {
 						found = true;
 						break;
 					}
-				} else if (temp->start_idx <= event_idx &&
-					   event_idx <= temp->end_idx) {
-					found = true;
-					break;
 				}
 			}
 			if (found)