Re: [PATCH] perf: riscv: Clear PMU pending interrupt through vendor helper

Re: [PATCH] perf: riscv: Clear PMU pending interrupt through vendor helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The global `riscv_pmu_irq_mask` is declared as a 32-bit `unsigned int` but initialized with a 64-bit mask, causing dangerous truncation and sign-extension on RV64 systems.
- [High] A spurious PMU interrupt triggers an early return that leaves the hardware performance counters permanently stopped.
--

--- Patch [1]: [PATCH] perf: riscv: Clear PMU pending interrupt through vendor helper ---
commit 58e4c92109db4c76ba1174701bb0aa79f0e00c65
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

perf: riscv: Clear PMU pending interrupt through vendor helper

When no hardware counters are in use, the PMU overflow handler clears the
pending interrupt directly in CSR_SIP. This patch modifies the early return
to use ALT_SBI_PMU_OVF_CLEAR_PENDING instead to avoid an out-of-width
shift and the wrong CSR on Andes PMU.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 50220f7b46d9b..ff95f5f136030 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -1058,7 +1058,7 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
>  	/* Firmware counter don't support overflow yet */
>  	fidx = find_first_bit(cpu_hw_evt->used_hw_ctrs, RISCV_MAX_COUNTERS);
>  	if (fidx == RISCV_MAX_COUNTERS) {
> -		csr_clear(CSR_SIP, BIT(riscv_pmu_irq_num));
> +		ALT_SBI_PMU_OVF_CLEAR_PENDING(riscv_pmu_irq_mask);

[Severity: High]
This is a pre-existing issue, but does using riscv_pmu_irq_mask here risk
truncation or sign-extension on RV64 systems?

The global riscv_pmu_irq_mask is declared as a 32-bit unsigned int:

drivers/perf/riscv_pmu_sbi.c:
    static unsigned int riscv_pmu_irq_mask;

But it is initialized with a 64-bit mask in pmu_sbi_setup_irqs():

    riscv_pmu_irq_mask = BIT(riscv_pmu_irq_num % BITS_PER_LONG);

On RV64, if the PMU IRQ number results in (num % 64) >= 32, the assigned
value is truncated to 0, which would cause ALT_SBI_PMU_OVF_CLEAR_PENDING to
clear nothing and potentially lead to an unrecoverable interrupt storm.

Additionally, if (num % 64) == 31, the 32-bit value 0x80000000 could be
sign-extended to 64-bits by the compiler when placed in a general-purpose
register for the csrc inline assembly, unintentionally clearing bits 31-63
of the interrupt pending CSR.

Would it be better to change the type of riscv_pmu_irq_mask to unsigned
long?

[Severity: High]
This isn't a bug introduced by this patch, but does the early return for
spurious interrupts leave the hardware performance counters permanently
stopped?

Later in pmu_sbi_ovf_handler(), all active PMU hardware counters are
stopped before reading the overflow status:

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_ovf_handler() {
    ...
    pmu = to_riscv_pmu(event->pmu);
    pmu_sbi_stop_hw_ctrs(pmu);

    if (sbi_pmu_snapshot_available())
        overflow = sdata->ctr_overflow_mask;
    else
        ALT_SBI_PMU_OVERFLOW(overflow);
    ...
    /* No overflow bit is set */
    if (!overflow)
        return IRQ_NONE;
    ...
}

If a spurious interrupt occurs or overflow evaluates to 0, this early exit
returns IRQ_NONE and bypasses the call to pmu_sbi_start_overflow_mask()
at the end of the function.

Could this silently and permanently break performance sampling on that CPU
until manually restarted by userspace?

>  		return IRQ_NONE;
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825151001.2-andes-pmu-clear-caplitmus-rv@iscas.ac.cn?part=1