Re: [PATCH v9 4/5] perf: RISC-V: add support for SSE event

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Skipping `pmu_sbi_start_overflow_mask()` when an event is throttled leaves all active PMU counters permanently stopped in hardware.
- [High] Missing unregistration of the SSE event on PMU teardown or probe failure causes a memory leak and use-after-free.
- [Medium] Executing synchronous SBI ecalls in `pmu_enable` and `pmu_disable` introduces severe context switch latency.
- [Medium] The `RISCV_PMU_SBI_SSE` config lacks a required dependency on `RISCV_PMU_SBI`, violating Kconfig consistency.
--

commit 81e6d410a510416892cbd91339e3c8f19af5b1b2
Author: Clément Léger <cleger@rivosinc.com>

perf: RISC-V: add support for SSE event

This commit adds support for delivering local PMU interrupts via SSE
events. It registers an SSE handler for the local PMU event, honors
throttle returns from perf core, and adds a new RISCV_PMU_SBI_SSE
Kconfig option.

> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> index ab90932fc2d01..a92846bcffaca 100644
> --- a/drivers/perf/Kconfig
> +++ b/drivers/perf/Kconfig
> @@ -105,6 +105,16 @@ config RISCV_PMU_SBI
>  	  full perf feature support i.e. counter overflow, privilege mode
>  	  filtering, counter configuration.
>  
> +config RISCV_PMU_SBI_SSE
> +	depends on RISCV_PMU && RISCV_SBI_SSE

Should this depend on RISCV_PMU_SBI instead of RISCV_PMU? 

The implementation for pmu_sbi_setup_sse() is located in riscv_pmu_sbi.c,
which is only compiled if RISCV_PMU_SBI is enabled. If a user enables
RISCV_PMU and RISCV_SBI_SSE but leaves RISCV_PMU_SBI disabled, it seems
like this feature would be selectable but its functions would do nothing.

> +	bool "RISC-V PMU SSE events"

[ ... ]

> diff --git a/drivers/perf/riscv_pmu.c b/drivers/perf/riscv_pmu.c
> index 8e3cd0f35336f..a48e286d9394f 100644
> --- a/drivers/perf/riscv_pmu.c
> +++ b/drivers/perf/riscv_pmu.c
> @@ -254,6 +255,24 @@ void riscv_pmu_start(struct perf_event *event, int flags)
>  	perf_event_update_userpage(event);
>  }
>  
> +#ifdef CONFIG_RISCV_PMU_SBI_SSE
> +static void riscv_pmu_disable(struct pmu *pmu)
> +{
> +	struct riscv_pmu *rvpmu = to_riscv_pmu(pmu);
> +
> +	if (rvpmu->sse_evt)
> +		sse_event_disable_local(rvpmu->sse_evt);
> +}

Does mapping pmu_disable and pmu_enable to sse_event_disable_local and
sse_event_enable_local introduce significant context switch latency?

Since the perf core frequently invokes these callbacks during task
context switches, performing a synchronous SBI ecall to trap into the
machine-mode firmware on every switch might introduce a massive
performance regression.

> +
> +static void riscv_pmu_enable(struct pmu *pmu)
> +{
> +	struct riscv_pmu *rvpmu = to_riscv_pmu(pmu);
> +
> +	if (rvpmu->sse_evt)
> +		sse_event_enable_local(rvpmu->sse_evt);
> +}
> +#endif

[ ... ]

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 385af5e6e6d0d..e6f5a619a2c08 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -1133,18 +1136,65 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
>  			 * TODO: We will need to stop the guest counters once
>  			 * virtualization support is added.
>  			 */
> -			perf_event_overflow(event, &data, regs);
> +			ev_overflow |= perf_event_overflow(event, &data, regs);
>  		}
>  		/* Reset the state as we are going to start the counter after the loop */
>  		hw_evt->state = 0;
>  	}
>  
> -	pmu_sbi_start_overflow_mask(pmu, overflowed_ctrs);
> +	if (!ev_overflow || !from_sse)
> +		pmu_sbi_start_overflow_mask(pmu, overflowed_ctrs);

When perf_event_overflow() throttles an event and returns a non-zero value,
ev_overflow is set to 1. For SSE events, won't this cause the condition to
evaluate to false and completely skip calling pmu_sbi_start_overflow_mask()?

Since pmu_sbi_stop_hw_ctrs() is called at the beginning of the handler,
it seems like skipping this would leave all active hardware counters
permanently stopped in hardware, with the perf core unaware because the
event state was just reset inside the loop.

> +
>  	perf_sample_event_took(sched_clock() - start_clock);
>  
>  	return IRQ_HANDLED;
>  }

[ ... ]

> +#ifdef CONFIG_RISCV_PMU_SBI_SSE
> +static int pmu_sbi_ovf_sse_handler(u32 evt, void *arg, struct pt_regs *regs)
> +{
> +	struct cpu_hw_events __percpu *hw_events = arg;
> +	struct cpu_hw_events *hw_event = raw_cpu_ptr(hw_events);
> +
> +	pmu_sbi_ovf_handler(hw_event, regs, true);
> +
> +	return 0;
> +}
> +
> +static int pmu_sbi_setup_sse(struct riscv_pmu *pmu)
> +{
> +	int ret;
> +	struct sse_event *evt;
> +	struct cpu_hw_events __percpu *hw_events = pmu->hw_events;
> +
> +	evt = sse_event_register(SBI_SSE_EVENT_LOCAL_PMU_OVERFLOW, 0,
> +				 pmu_sbi_ovf_sse_handler, hw_events);

Is there a corresponding sse_event_unregister() call missing in the
PMU teardown or error cleanup paths?

If the PMU device probe fails after registration, or if the driver is
subsequently unbound, won't the registered SSE event remain active in the
firmware? A subsequent hardware overflow might trigger the handler and
dereference the freed hw_events pointer, causing a use-after-free.

> +	if (IS_ERR(evt))
> +		return PTR_ERR(evt);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778331862.git.zhangzhanpeng.jasper@bytedance.com?part=4