[PATCH] lib: sbi_pmu: Add FW counter index validation when reading high bits on RV64

[James R T]

On Mon, Jan 19, 2026 at 7:57 PM Radim Krčmář
<radim.krcmar at oss.qualcomm.com> wrote:
>
> 2026-01-17T20:50:31+08:00, James Raphael Tiovalen <jamestiotio at gmail.com>:
> > Currently, when we attempt to read the upper 32 bits of a firmware
> > counter on RV64 or higher, we just set `sbiret.value` to 0 without
> > validating the counter index. The SBI specification requires us to set
> > `sbiret.error` to `SBI_ERR_INVALID_PARAM` if the counter index points to
> > a hardware counter or an invalid counter. Add a validation check to
> > ensure compliance with the specification on RV64 or higher.
> >
> > Fixes: 51951d9e9af8 ("lib: sbi_pmu: Implement sbi_pmu_counter_fw_read_hi")
> > Signed-off-by: James Raphael Tiovalen <jamestiotio at gmail.com>
> > ---
>
> While the call is poorly specified and your interpretation is valid,
> I think the intention is to make this function do nothing else than
> return {err, 0} on RV64 as there isn't much reason to complicate the
> implementation.
>

I discovered this when I was writing some PMU SBI tests for
KVM-Unit-Tests and, at least to me, it was unexpected. I understand
the intention behind implementing it in the simplest way possible, but
it did seem to deviate from the specification, at least with my way of
reading it.

> I think always returning {SBI_EINVAL, 0} would be even better than
> the current {SBI_SUCCESS, 0}, but it doesn't matter much since
> legitimate RV64 software shouldn't ever invoke the ecall.
>

If it is better to return {SBI_EINVAL, 0}, then let's do it. That
said, I think we should only return that on an invalid counter index,
not always.

> > diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
> > @@ -227,6 +227,14 @@ static bool pmu_ctr_idx_validate(unsigned long cbase, unsigned long cmask)
> >       return cmask && cbase + sbi_fls(cmask) < total_ctrs;
> >  }
>
> In case of disagreement with the above:
>
> > +int sbi_pmu_is_fw_ctr_idx(uint32_t cidx)
> > +{
>
> This functions gets passed regs->a0, so cidx should be an xlen sized
> type.
> (I understand trying to be consistent, as sbi_pmu_ctr_fw_read has the
>  same bug, but better fix related code when touching it.)
>

Sure, I will include the fix for this in v2 then.

> > +     if (cidx < num_hw_ctrs || cidx >= total_ctrs)
> > +             return SBI_EINVAL;
>
> Please refactor the check in sbi_pmu_ctr_fw_read, and use it instead of
> adding more logic.
>

Got it. Will do that in v2.

> Thanks.

Thank you.

Best regards,
James Raphael Tiovalen