Re: [PATCH v8 1/2] drivers/perf: riscv-iommu: add risc-v iommu pmu driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PMU interrupt handler returns IRQ_HANDLED on CPU mismatch without clearing the device-level interrupt pending bit, leading to permanently lost MSIs or IRQ storms.
- [Medium] event_init() accepts event groups with multiple cycle events, which can never be scheduled.
--

commit 20eb6219411a8b8c6c0492ff1ba61f7e29cbb9b0
Author: Zong Li <zong.li@sifive.com>

    drivers/perf: riscv-iommu: add risc-v iommu pmu driver

This patch adds a new auxiliary device driver to support the RISC-V IOMMU
Performance Monitor Unit (PMU). The driver manages PMU event counters,
handles counter detection at runtime, and exposes standard perf interfaces
for counting events like translated requests, TLB misses, and clock cycles.

Link: https://lore.kernel.org/linux-riscv/20260618143634.7f3dd6c5@pumpkin/

> diff --git a/drivers/perf/riscv_iommu_pmu.c b/drivers/perf/riscv_iommu_pmu.c
> new file mode 100644
> index 0000000000000..0c30e26c1f0ce
> --- /dev/null
> +++ b/drivers/perf/riscv_iommu_pmu.c
[ ... ]
> +static int riscv_iommu_pmu_event_init(struct perf_event *event)
> +{
[ ... ]
> +	/*
> +	 * Account for this event itself first. It has to be done before the
> +	 * check below, otherwise an event which is on its own would never be
> +	 * matched against the number of counters the hardware implements.
> +	 */
> +	if (!is_cycle_event(event->attr.config))
> +		if (++counters > total_event_counters)
> +			return -EINVAL;
> +
> +	if (event->group_leader == event)
> +		return 0;
> +
> +	if (!is_software_event(event->group_leader)) {
> +		if (event->group_leader->pmu != event->pmu)
> +			return -EINVAL;
> +
> +		if (!is_cycle_event(event->group_leader->attr.config))
> +			if (++counters > total_event_counters)
> +				return -EINVAL;
> +	}
> +
> +	for_each_sibling_event(sibling, event->group_leader) {
> +		if (is_software_event(sibling))
> +			continue;
> +
> +		if (sibling->pmu != event->pmu)
> +			return -EINVAL;
> +
> +		if (is_cycle_event(sibling->attr.config))
> +			continue;

[Severity: Medium]
Does this logic allow event groups to be created with multiple cycle events?

Because ++counters is explicitly skipped for cycle events, we don't verify if
more than one cycle event is present in the group. Since the hardware only
has one cycle counter (index 0), a group with two cycle events will continually
fail with -EAGAIN in riscv_iommu_pmu_add() and can never be successfully
scheduled. 

Should we track the total number of cycle events in the group to reject
unsatisfiable groups at creation time here in riscv_iommu_pmu_event_init()?

> +
> +		if (++counters > total_event_counters)
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static irqreturn_t riscv_iommu_pmu_irq_handler(int irq, void *dev_id)
> +{
> +	struct riscv_iommu_pmu *pmu = (struct riscv_iommu_pmu *)dev_id;
> +	DECLARE_BITMAP(ovf_bitmap, BITS_PER_TYPE(u64));
> +	u32 ovf, idx, inhibit;
> +
> +	/*
> +	 * PCI MSI/MSI-X on IMSIC will set IRQCHIP_MOVE_DEFERRED, so
> +	 * irq_set_affinity() reports success while only recording the request,
> +	 * and the move is applied in interrupt context upon the next device
> +	 * interrupt. Until then the interrupt is still routed to the CPU IMSIC
> +	 * picked initially, so the first overflow interrupt can run concurrently
> +	 * with the perf callbacks on the CPU the events are bound to
> +	 *
> +	 * A mismatch means the irqchip still had an affinity change pending for
> +	 * this line. However, handle_edge_irq() has just completed it through
> +	 * ->irq_ack(), so leave the overflow and PMIP bits alone and let the
> +	 * interrupt be redelivered on the right CPU.
> +	 *
> +	 * IRQ_HANDLED rather than IRQ_NONE: the latter feeds the spurious
> +	 * detector in note_interrupt(), which disables the whole line once
> +	 * enough interrupts go unhandled, and this line may be shared with the
> +	 * iommu command and fault queues.
> +	 */
> +	if (smp_processor_id() != READ_ONCE(pmu->on_cpu))
> +		return IRQ_HANDLED;

[Severity: High]
Will returning IRQ_HANDLED here without clearing the PMIP bit in the device
leave the hardware interrupt permanently pending?

If the PMU interrupt fires while smp_processor_id() != pmu->on_cpu (which
could happen during an IRQ affinity change or from a shared device), this
early return bypasses the device-level interrupt clearing. 

For edge-triggered MSIs, the device might never send another interrupt because
the pending bit remains asserted, permanently killing the PMU. If
level-triggered, it could cause an infinite IRQ storm on the wrong CPU.

Should we check and clear the PMIP bit even when bouncing the interrupt?

> +
> +	/* Check whether this interrupt is for PMU */
> +	if (!(readl_relaxed(pmu->reg + RISCV_IOMMU_REG_IPSR) & RISCV_IOMMU_IPSR_PMIP))
> +		return IRQ_NONE;
> +
> +	/* Process PMU IRQ */
> +	inhibit = riscv_iommu_pmu_stop_all(pmu);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904092136.113321-1-zong.li@sifive.com?part=1