Re: [PATCH v9 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] CPU hotplug race causes concurrent lockless access to PMU state and registers.
- [High] Deferring to irq_work without clearing IPSR.PMIP causes an interrupt storm on systems using level-triggered wired interrupts.
--

commit 9de4660461d623132294a2de1ef096c0d7941787
Author: Zong Li <zong.li@sifive.com>

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

Add a new driver to support the RISC-V IOMMU PMU. This is an auxiliary
device driver created by the parent RISC-V IOMMU driver to handle hardware
performance monitoring counters.

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..3a1cf366a79fd
> --- /dev/null
> +++ b/drivers/perf/riscv_iommu_pmu.c

[ ... ]

> +static int riscv_iommu_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
> +{
> +	struct riscv_iommu_pmu *iommu_pmu;
> +	unsigned int target_cpu;
> +
> +	iommu_pmu = hlist_entry_safe(node, struct riscv_iommu_pmu, node);
> +
> +	if (READ_ONCE(iommu_pmu->on_cpu) != (int)cpu)
> +		return 0;

[ ... ]

> +	target_cpu = riscv_iommu_pmu_bind_cpu(iommu_pmu, cpu);
> +	if (WARN_ON_ONCE(target_cpu >= nr_cpu_ids)) {
> +		WRITE_ONCE(iommu_pmu->on_cpu, -1);
> +	} else {
> +		WRITE_ONCE(iommu_pmu->on_cpu, target_cpu);
> +		perf_pmu_migrate_context(&iommu_pmu->pmu, cpu, target_cpu);
> +	}

[Severity: High]
Could this lead to a CPU hotplug race condition? By updating the IRQ affinity
and on_cpu pointer before calling perf_pmu_migrate_context(), the IRQ handler
might run on the new CPU while the old CPU is still actively modifying the PMU
state during the migration.

If a PMU overflow interrupt arrives during this window, the interrupt triggers
on target_cpu, causing riscv_iommu_pmu_process_overflow() to execute
concurrently with the perf core invoking pmu->del (via
perf_pmu_migrate_context) on the outgoing CPU. Could this concurrent lockless
access to pmu->events and shared hardware registers cause data corruption or
invalid PMU configurations?

> +
> +	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;
> +	int target_cpu;
> +
> +	/* Check whether this interrupt is for PMU */
> +	if (!(readl_relaxed(pmu->reg + RISCV_IOMMU_REG_IPSR) & RISCV_IOMMU_IPSR_PMIP))
> +		return IRQ_NONE;

[ ... ]

> +	target_cpu = READ_ONCE(pmu->on_cpu);
> +	if (target_cpu != smp_processor_id() && target_cpu >= 0 &&
> +	    cpu_online(target_cpu)) {
> +		irq_work_queue_on(&pmu->work, target_cpu);
> +		return IRQ_HANDLED;
> +	}

[Severity: High]
Will returning IRQ_HANDLED here without clearing the device-level interrupt
status bit (IPSR.PMIP) cause an interrupt storm on systems using
level-triggered wired interrupts?

When the hardirq handler queues an irq_work and returns without clearing the
hardware interrupt bit, the interrupt line remains asserted. Once the CPU
unmasks the line upon return, it seems it would immediately re-enter the
interrupt handler, potentially resulting in an infinite interrupt storm that
could hard lock the CPU.

> +
> +	/*
> +	 * Either this is the bound CPU, or there is no usable one to defer to.
> +	 * In the latter case handle it here anyway: nobody else is going to
> +	 * clear ipsr.pmip, and without that the pmu would stop reporting for
> +	 * good.
> +	 */
> +	riscv_iommu_pmu_process_overflow(pmu);
> +
> +	return IRQ_HANDLED;
> +}

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