[PATCH] drivers/perf: riscv: Keep the fixed counter counting

[PATCH] drivers/perf: riscv: Keep the fixed counter counting

[cp0613]

From: Chen Pei <cp0613@linux.alibaba.com>

The RISC-V SBI PMU driver disables all PMU counters during initialization
via pmu_sbi_stop_all. For fixed counters CYCLE, TIME and INSTRET, this is
unnecessary for the following two reasons:

1. Some kernel driver code may directly read CYCLE and INSTRET to perform
   simple performance analysis.
2. In legacy mode, user space directly reads CYCLE and INSTRET. (echo 2 >
   /proc/sys/kernel/perf_user_access)

Therefore, We keep counting CYCLE, TIME and INSTRET.

Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>
---
 drivers/perf/riscv_pmu_sbi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index 7dd282da67ce..93aaab324443 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -899,6 +899,9 @@ static int pmu_sbi_get_ctrinfo(int nctr, unsigned long *mask)
 
 static inline void pmu_sbi_stop_all(struct riscv_pmu *pmu)
 {
+	/* We keep counting CYCLE, TIME and INSTRET. */
+	pmu->cmask &= ~0x7;
+
 	/*
 	 * No need to check the error because we are disabling all the counters
 	 * which may include counters that are not enabled yet.
-- 
2.50.1

Re: [PATCH] drivers/perf: riscv: Keep the fixed counter counting

[Atish Patra]

On 1/31/26 3:24 AM, cp0613@linux.alibaba.com wrote:
> From: Chen Pei <cp0613@linux.alibaba.com>
>
> The RISC-V SBI PMU driver disables all PMU counters during initialization
> via pmu_sbi_stop_all. For fixed counters CYCLE, TIME and INSTRET, this is
> unnecessary for the following two reasons:
>
> 1. Some kernel driver code may directly read CYCLE and INSTRET to perform
>     simple performance analysis.

Is this for some debugging purpose to read the instret/cycle count at 
boot time or real use case for driver performance analysis ?

If it is the latter, that will be problematic for various reasons such 
as context switching will lead to inaccurate numbers.

> 2. In legacy mode, user space directly reads CYCLE and INSTRET. (echo 2 >
>     /proc/sys/kernel/perf_user_access)
>
> Therefore, We keep counting CYCLE, TIME and INSTRET.
>
> Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>
> ---
>   drivers/perf/riscv_pmu_sbi.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 7dd282da67ce..93aaab324443 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -899,6 +899,9 @@ static int pmu_sbi_get_ctrinfo(int nctr, unsigned long *mask)
>   
>   static inline void pmu_sbi_stop_all(struct riscv_pmu *pmu)
>   {
> +	/* We keep counting CYCLE, TIME and INSTRET. */
> +	pmu->cmask &= ~0x7;
> +

This is incorrect. The cmask should be set based on the perf_user_access 
value. We should not continue counting the CYCLE/INSTRET when legacy 
mode is not set. if (sysctl_perf_user_access == SYSCTL_LEGACY) 
csr_write(CSR_SCOUNTEREN, 0x7); else csr_write(CSR_SCOUNTEREN, 0x2);

>   	/*
>   	 * No need to check the error because we are disabling all the counters
>   	 * which may include counters that are not enabled yet.

Re: [PATCH] drivers/perf: riscv: Keep the fixed counter counting

[qingwei hu]

> On Feb 4, 2026, at 17:17, Atish Patra <atish.patra@linux.dev> wrote:
> 
> 
> On 1/31/26 3:24 AM, cp0613@linux.alibaba.com wrote:
>> From: Chen Pei <cp0613@linux.alibaba.com>
>> 
>> The RISC-V SBI PMU driver disables all PMU counters during initialization
>> via pmu_sbi_stop_all. For fixed counters CYCLE, TIME and INSTRET, this is
>> unnecessary for the following two reasons:
>> 
>> 1. Some kernel driver code may directly read CYCLE and INSTRET to perform
>>    simple performance analysis.
> 
> Is this for some debugging purpose to read the instret/cycle count at boot time or real use case for driver performance analysis ?
> 
> If it is the latter, that will be problematic for various reasons such as context switching will lead to inaccurate numbers.

Hello Atish.

Besides boot time scenarios and performance analysis, there is another case
where user mode needs to access the cycle counter via rdcycle.

> 
>> 2. In legacy mode, user space directly reads CYCLE and INSTRET. (echo 2 >
>>    /proc/sys/kernel/perf_user_access)
>> 
>> Therefore, We keep counting CYCLE, TIME and INSTRET.
>> 
>> Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>
>> ---
>>  drivers/perf/riscv_pmu_sbi.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
>> index 7dd282da67ce..93aaab324443 100644
>> --- a/drivers/perf/riscv_pmu_sbi.c
>> +++ b/drivers/perf/riscv_pmu_sbi.c
>> @@ -899,6 +899,9 @@ static int pmu_sbi_get_ctrinfo(int nctr, unsigned long *mask)
>>    static inline void pmu_sbi_stop_all(struct riscv_pmu *pmu)
>>  {
>> + /* We keep counting CYCLE, TIME and INSTRET. */
>> + pmu->cmask &= ~0x7;
>> +
> 
> This is incorrect. The cmask should be set based on the perf_user_access value. We should not continue counting the CYCLE/INSTRET when legacy mode is not set. if (sysctl_perf_user_access == SYSCTL_LEGACY) csr_write(CSR_SCOUNTEREN, 0x7); else csr_write(CSR_SCOUNTEREN, 0x2);

Legacy mode refers to the perf framework. When perf_user_access is not in
legacy mode, the application may need to use rdcycle.

As discussed in [1], the rdcycle is necessary, and applications such as
DPDK should use rdcycle in user mode.

So how to use the rdcycle when perf_user_access is not in legacy mode?

[1] https://lists.riscv.org/g/tech-privileged/topic/should_rdcycle_be_deprecated/115162737

Best Regards,
Qingwei Hu

> 
>>   /*
>>    * No need to check the error because we are disabling all the counters
>>    * which may include counters that are not enabled yet.
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] drivers/perf: riscv: Keep the fixed counter counting

[Atish Patra]

On 2/4/26 5:13 AM, qingwei hu wrote:
>> On Feb 4, 2026, at 17:17, Atish Patra <atish.patra@linux.dev> wrote:
>>
>>
>> On 1/31/26 3:24 AM, cp0613@linux.alibaba.com wrote:
>>> From: Chen Pei <cp0613@linux.alibaba.com>
>>>
>>> The RISC-V SBI PMU driver disables all PMU counters during initialization
>>> via pmu_sbi_stop_all. For fixed counters CYCLE, TIME and INSTRET, this is
>>> unnecessary for the following two reasons:
>>>
>>> 1. Some kernel driver code may directly read CYCLE and INSTRET to perform
>>>     simple performance analysis.
>> Is this for some debugging purpose to read the instret/cycle count at boot time or real use case for driver performance analysis ?
>>
>> If it is the latter, that will be problematic for various reasons such as context switching will lead to inaccurate numbers.
> Hello Atish.
>
> Besides boot time scenarios and performance analysis, there is another case
> where user mode needs to access the cycle counter via rdcycle.
>
>>> 2. In legacy mode, user space directly reads CYCLE and INSTRET. (echo 2 >
>>>     /proc/sys/kernel/perf_user_access)
>>>
>>> Therefore, We keep counting CYCLE, TIME and INSTRET.
>>>
>>> Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>
>>> ---
>>>   drivers/perf/riscv_pmu_sbi.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
>>> index 7dd282da67ce..93aaab324443 100644
>>> --- a/drivers/perf/riscv_pmu_sbi.c
>>> +++ b/drivers/perf/riscv_pmu_sbi.c
>>> @@ -899,6 +899,9 @@ static int pmu_sbi_get_ctrinfo(int nctr, unsigned long *mask)
>>>     static inline void pmu_sbi_stop_all(struct riscv_pmu *pmu)
>>>   {
>>> + /* We keep counting CYCLE, TIME and INSTRET. */
>>> + pmu->cmask &= ~0x7;
>>> +
>> This is incorrect. The cmask should be set based on the perf_user_access value. We should not continue counting the CYCLE/INSTRET when legacy mode is not set. if (sysctl_perf_user_access == SYSCTL_LEGACY) csr_write(CSR_SCOUNTEREN, 0x7); else csr_write(CSR_SCOUNTEREN, 0x2);
> Legacy mode refers to the perf framework. When perf_user_access is not in
> legacy mode, the application may need to use rdcycle.
>
> As discussed in [1], the rdcycle is necessary, and applications such as
> DPDK should use rdcycle in user mode.
>
> So how to use the rdcycle when perf_user_access is not in legacy mode?
>
> [1] https://lists.riscv.org/g/tech-privileged/topic/should_rdcycle_be_deprecated/115162737

Based on this thread, it was very clear that the dpdk use case of 
rdcycle is incorrect.

We also had many discussions why enabling rdcycle without perf is a bad 
idea. That's the reason
why we added the *sysctl_perf_user_access* legacy to preserve legacy 
usage. Eventually, it is expected that folks would move away from 
rdcycle usage which was incorrect to begin with.

Let me know if you want me dig up the old threads about the discussion.

> Best Regards,
> Qingwei Hu
>
>>>    /*
>>>     * No need to check the error because we are disabling all the counters
>>>     * which may include counters that are not enabled yet.
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>

Re: [External] [PATCH] drivers/perf: riscv: Keep the fixed counter counting

[Qingwei Hu]

> On Feb 8, 2026, at 17:32, Atish Patra <atish.patra@linux.dev> wrote:
> 
> 
> On 2/4/26 5:13 AM, qingwei hu wrote:
>>> On Feb 4, 2026, at 17:17, Atish Patra <atish.patra@linux.dev> wrote:
>>> 
>>> 
>>> On 1/31/26 3:24 AM, cp0613@linux.alibaba.com wrote:
>>>> From: Chen Pei <cp0613@linux.alibaba.com>
>>>> 
>>>> The RISC-V SBI PMU driver disables all PMU counters during initialization
>>>> via pmu_sbi_stop_all. For fixed counters CYCLE, TIME and INSTRET, this is
>>>> unnecessary for the following two reasons:
>>>> 
>>>> 1. Some kernel driver code may directly read CYCLE and INSTRET to perform
>>>>    simple performance analysis.
>>> Is this for some debugging purpose to read the instret/cycle count at boot time or real use case for driver performance analysis ?
>>> 
>>> If it is the latter, that will be problematic for various reasons such as context switching will lead to inaccurate numbers.
>> Hello Atish.
>> 
>> Besides boot time scenarios and performance analysis, there is another case
>> where user mode needs to access the cycle counter via rdcycle.
>> 
>>>> 2. In legacy mode, user space directly reads CYCLE and INSTRET. (echo 2 >
>>>>    /proc/sys/kernel/perf_user_access)
>>>> 
>>>> Therefore, We keep counting CYCLE, TIME and INSTRET.
>>>> 
>>>> Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>
>>>> ---
>>>>  drivers/perf/riscv_pmu_sbi.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>> 
>>>> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
>>>> index 7dd282da67ce..93aaab324443 100644
>>>> --- a/drivers/perf/riscv_pmu_sbi.c
>>>> +++ b/drivers/perf/riscv_pmu_sbi.c
>>>> @@ -899,6 +899,9 @@ static int pmu_sbi_get_ctrinfo(int nctr, unsigned long *mask)
>>>>    static inline void pmu_sbi_stop_all(struct riscv_pmu *pmu)
>>>>  {
>>>> + /* We keep counting CYCLE, TIME and INSTRET. */
>>>> + pmu->cmask &= ~0x7;
>>>> +
>>> This is incorrect. The cmask should be set based on the perf_user_access value. We should not continue counting the CYCLE/INSTRET when legacy mode is not set. if (sysctl_perf_user_access == SYSCTL_LEGACY) csr_write(CSR_SCOUNTEREN, 0x7); else csr_write(CSR_SCOUNTEREN, 0x2);
>> Legacy mode refers to the perf framework. When perf_user_access is not in
>> legacy mode, the application may need to use rdcycle.
>> 
>> As discussed in [1], the rdcycle is necessary, and applications such as
>> DPDK should use rdcycle in user mode.
>> 
>> So how to use the rdcycle when perf_user_access is not in legacy mode?
>> 
>> [1] https://lists.riscv.org/g/tech-privileged/topic/should_rdcycle_be_deprecated/115162737
> 
> Based on this thread, it was very clear that the dpdk use case of rdcycle is incorrect.
> 
> We also had many discussions why enabling rdcycle without perf is a bad idea. That's the reason
> why we added the *sysctl_perf_user_access* legacy to preserve legacy usage. Eventually, it is expected that folks would move away from rdcycle usage which was incorrect to begin with.
> 
> Let me know if you want me dig up the old threads about the discussion.

Hi Atish, thanks for your reply.

I think I found the earlier discussion thread:

https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/REWcwYnzsKE

could you please confirm this is the one you were referring to?

In that thread, you mentioned you had sent a patch to restore the original
behavior by enabling the CY bit in scounteren. However, looking at the
current code/behavior, setting CY (and IR) in scounteren seems to only
control U-mode visibility/permission to read the counters (and access
related CSRs). The cycle counter itself still doesn’t appear to increment,
which makes me suspect mcountinhibit.CY might still be set.

So I’m wondering whether we also need to explicitly un-inhibit the cycle
counter (ensure mcountinhibit.CY = 0) in the legacy path by sbi call, if
the intent is to preserve the historical rdcycle behavior.

>> Best Regards,
>> Qingwei Hu
>> 
>>>>   /*
>>>>    * No need to check the error because we are disabling all the counters
>>>>    * which may include counters that are not enabled yet.
>>> _______________________________________________
>>> linux-riscv mailing list
>>> linux-riscv@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] drivers/perf: riscv: Keep the fixed counter counting

[cp0613]

On Wed, 4 Feb 2026 01:17:25 -0800, atish.patra@linux.dev wrote:

> > The RISC-V SBI PMU driver disables all PMU counters during initialization
> > via pmu_sbi_stop_all. For fixed counters CYCLE, TIME and INSTRET, this is
> > unnecessary for the following two reasons:
> >
> > 1. Some kernel driver code may directly read CYCLE and INSTRET to perform
> >     simple performance analysis.
> 
> Is this for some debugging purpose to read the instret/cycle count at 
> boot time or real use case for driver performance analysis ?
> 
> If it is the latter, that will be problematic for various reasons such 
> as context switching will lead to inaccurate numbers.

Hi Atish,

Thanks for the reminder, but I might not be able to provide specific scenarios
due to our niche usage. Therefore, let's just discuss the legacy usage of
sysctl_perf_user_access.

> > 2. In legacy mode, user space directly reads CYCLE and INSTRET. (echo 2 >
> >     /proc/sys/kernel/perf_user_access)
> >
> > Therefore, We keep counting CYCLE, TIME and INSTRET.
> >
> > Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>
> > ---
> >   drivers/perf/riscv_pmu_sbi.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> > index 7dd282da67ce..93aaab324443 100644
> > --- a/drivers/perf/riscv_pmu_sbi.c
> > +++ b/drivers/perf/riscv_pmu_sbi.c
> > @@ -899,6 +899,9 @@ static int pmu_sbi_get_ctrinfo(int nctr, unsigned long *mask)
> >   
> >   static inline void pmu_sbi_stop_all(struct riscv_pmu *pmu)
> >   {
> > + /* We keep counting CYCLE, TIME and INSTRET. */
> > + pmu->cmask &= ~0x7;
> > +
> 
> This is incorrect. The cmask should be set based on the perf_user_access 
> value. We should not continue counting the CYCLE/INSTRET when legacy 
> mode is not set. if (sysctl_perf_user_access == SYSCTL_LEGACY) 
> csr_write(CSR_SCOUNTEREN, 0x7); else csr_write(CSR_SCOUNTEREN, 0x2);

I have a slightly different understanding here. Regarding perf_user_access, I think
it is only used to control user mode access permissions to CYCLE, TIME, and INSTRET
(via SCOUNTEREN), but whether the counters themselves are in a counting state is
another issue (via MCOUNTINHIBIT). I think the cmask in pmu_sbi_stop_all should
represent a counter to stop the counting, rather than a permission configuration.

The problem we are currently encountering is that even when switching to LEGACY mode,
CYCLE and INSTRET are not counting, so we want to change this default behavior so
that these three fixed counters are always in counting.

Sorry for the late reply. I've reviewed some previous related discussions, and it
seems that using time is more reasonable between cycle and time. However, for some
small code snippets, there is a need to use cycle and instant (at least instant),
so keeping them constantly counting doesn't seem to have any downsides.

Thanks,
Pei

> >    /*
> >     * No need to check the error because we are disabling all the counters
> >     * which may include counters that are not enabled yet.

Re: [PATCH] drivers/perf: riscv: Keep the fixed counter counting

[Atish Patra]

On 2/9/26 4:36 AM, cp0613@linux.alibaba.com wrote:
> On Wed, 4 Feb 2026 01:17:25 -0800, atish.patra@linux.dev wrote:
>
>>> The RISC-V SBI PMU driver disables all PMU counters during initialization
>>> via pmu_sbi_stop_all. For fixed counters CYCLE, TIME and INSTRET, this is
>>> unnecessary for the following two reasons:
>>>
>>> 1. Some kernel driver code may directly read CYCLE and INSTRET to perform
>>>      simple performance analysis.
>> Is this for some debugging purpose to read the instret/cycle count at
>> boot time or real use case for driver performance analysis ?
>>
>> If it is the latter, that will be problematic for various reasons such
>> as context switching will lead to inaccurate numbers.
> Hi Atish,
>
> Thanks for the reminder, but I might not be able to provide specific scenarios
> due to our niche usage. Therefore, let's just discuss the legacy usage of
> sysctl_perf_user_access.
>
>>> 2. In legacy mode, user space directly reads CYCLE and INSTRET. (echo 2 >
>>>      /proc/sys/kernel/perf_user_access)
>>>
>>> Therefore, We keep counting CYCLE, TIME and INSTRET.
>>>
>>> Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>
>>> ---
>>>    drivers/perf/riscv_pmu_sbi.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
>>> index 7dd282da67ce..93aaab324443 100644
>>> --- a/drivers/perf/riscv_pmu_sbi.c
>>> +++ b/drivers/perf/riscv_pmu_sbi.c
>>> @@ -899,6 +899,9 @@ static int pmu_sbi_get_ctrinfo(int nctr, unsigned long *mask)
>>>    
>>>    static inline void pmu_sbi_stop_all(struct riscv_pmu *pmu)
>>>    {
>>> + /* We keep counting CYCLE, TIME and INSTRET. */
>>> + pmu->cmask &= ~0x7;
>>> +
>> This is incorrect. The cmask should be set based on the perf_user_access
>> value. We should not continue counting the CYCLE/INSTRET when legacy
>> mode is not set. if (sysctl_perf_user_access == SYSCTL_LEGACY)
>> csr_write(CSR_SCOUNTEREN, 0x7); else csr_write(CSR_SCOUNTEREN, 0x2);
> I have a slightly different understanding here. Regarding perf_user_access, I think
> it is only used to control user mode access permissions to CYCLE, TIME, and INSTRET
> (via SCOUNTEREN), but whether the counters themselves are in a counting state is
> another issue (via MCOUNTINHIBIT). I think the cmask in pmu_sbi_stop_all should
> represent a counter to stop the counting, rather than a permission configuration.
>
> The problem we are currently encountering is that even when switching to LEGACY mode,
> CYCLE and INSTRET are not counting, so we want to change this default behavior so
> that these three fixed counters are always in counting.

Ahh. We should just enable counting when you user turn on the legacy 
mode not the default
behavior.

> Sorry for the late reply. I've reviewed some previous related discussions, and it
> seems that using time is more reasonable between cycle and time. However, for some
> small code snippets, there is a need to use cycle and instant (at least instant),
> so keeping them constantly counting doesn't seem to have any downsides.

This thread slipped through cracks unfortunately. Apologies for the late 
reply as well.
I understand the usage which is quick debugging purpose only. So we 
should not introduce
side channel exploits in production code for that purpose.
We keep revisiting topic even after numerous discussion in the past.

We must ensure that legacy mode is the only way where this is enabled.

> Thanks,
> Pei
>
>>>     /*
>>>      * No need to check the error because we are disabling all the counters
>>>      * which may include counters that are not enabled yet.