[PATCH v2 0/2] lib: sbi: pmu: Rework SSE callbacks

[Clément Léger]

On 19/05/2025 23:56, Samuel Holland wrote:
> Hi Clément,
> 
> On 2025-05-19 3:39 AM, Clément Léger wrote:
>> While testing the PMU SSE support in Linux, a few problem were spotted.
>> The first one was due to interrupts happing during some PMU critical
>> sections. Indeed, the PMU core actually disable interrupts when entering
>> these sections. However, while this works when interrupts are targeting
>> the S-mode, this is not the case with SSE. Fixing this required to
>> implement pmu_disable/enable() in Linux [1]. These two callbacks now
>> call sse_event_disable/enable() which now just needs to set/clear
>> MIE/MIP. Second problem was that reported PMU events count were quite
>> low. This proved to be due to some interrupts being losed when
>> reenabling SSE event. We still need to clear the pending PMU interrupts
>> upon disabling otherwise we might get some spurious interrupts when
>> reenabling it.
> 
> Could you clarify what could cause spurious interrupts here? Disabling the SSE
> event should just prevent it from being delivered to S-mode, right? It doesn't
> affect the event's pending status. My reading of the privileged spec is that
> mip.LCOFIP is edge-triggered by the individual hpmcounter OF bits. So if any
> counter is counting, and mip.LCOFIP is cleared without handing the overflow,
> S-mode would (erroneously) lose an overflow event. This could happen if for
> example an overflow occurs and pmu_sse_disable() was called both from within the
> SSE handler.

Hi Samuel,

That's a good point and I must admit I based my comprehension of this on
what the Linux driver was doing. ie: it stops the counters and then
clears MIP (cf this comment from riscv_sbi_pmu.c)

/*
 * Overflow interrupt pending bit should only be cleared after stopping
 * all the counters to avoid any race condition.
 */

So in SSE, after discussing with Atish, we decided to clear MIP.LCOFIP
in ctr_stop() (which makes sense to me). Regarding the MIP.LCOFIP
clearing in sbi_sse_disable(), I actually added it so that we do not end
up with a spurious IRQ as well right after disabling it. Thinking about
it, even if we have an interrupt after that, the sbi_pmu_stop() code in
Linux is stopping all the counters by calling the SBI so that should
clear MIP as well. But I'll take a better look at it just to be sure I'm
not missing anything.

Thanks,

Clément

> 
> If my understanding is correct, to match the edge interrupt semantics (clear
> LCOFIP before handling, as noted in the privileged spec), I think the only place
> that should clear mip.LCOFIP is sbi_pmu_ovf_irq(), not sbi_pmu_ctr_stop() or
> pmu_sse_disable().
> 
> Regards,
> Samuel
> 
>> Link: https://lore.kernel.org/linux-riscv/20250516152355.560448-1-cleger@rivosinc.com/ [1]
>>
>> Clément Léger (2):
>>   lib: sbi: pmu: Add SSE register/unregister() callbacks
>>   lib: sbi: pmu: Remove MIP clearing from pmu_sse_enable()
>>
>>  lib/sbi/sbi_pmu.c | 25 ++++++++++++++++++-------
>>  1 file changed, 18 insertions(+), 7 deletions(-)
>>
>