Re: [PATCH v10 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 3 potential issue(s) to consider:
- [High] The driver calls `disable_irq()` on an `IRQF_SHARED` interrupt line during CPU hotplug, temporarily disabling all shared IOMMU queues.
- [High] Data race during CPU hotplug due to a lack of PMU locking and missing `event->cpu` verification, leading to concurrent modification of shared driver state.
- [Medium] Unsafe implementation of the hi-lo-hi 64-bit counter read pattern, failing to account for hypervisor preemption.
--

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

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

This commit adds a new auxiliary device driver for the RISC-V IOMMU PMU.
The performance monitor provides counters with filtering support and separates
the cycle counter from the event 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
> --- /dev/null
> +++ b/drivers/perf/riscv_iommu_pmu.c
[ ... ]
> +static u64 riscv_iommu_pmu_get_counter(struct riscv_iommu_pmu *pmu, u32 idx)
> +{
> +	void __iomem *addr = pmu->reg + RISCV_IOMMU_REG_IOHPMCTR(idx);
> +	u64 value, counter_mask = idx ? pmu->event_cntr_mask : pmu->cycle_cntr_mask;
> +	u32 hi, lo, hi_again;
> +
> +	hi = readl(addr + 4);
> +	lo = readl(addr);
> +	hi_again = readl(addr + 4);
> +
> +	if (hi_again != hi) {
> +		hi = hi_again;
> +		lo = readl(addr);
> +	}

[Severity: Medium]
Could a hypervisor preempt the vCPU inside this if block before lo is re-read?

In a virtualized environment, if the vCPU is scheduled out for an extended
period before reading the low word again, the hardware counter might wrap
a second time. This would pair a deeply stale high word with a newly wrapped
low word, resulting in a corrupted counter value being returned.

Would a standard do-while loop prevent this issue?

[ ... ]
> +static void riscv_iommu_pmu_disable_counter(struct riscv_iommu_pmu *pmu, u32 idx)
> +{
> +	void __iomem *addr = pmu->reg + RISCV_IOMMU_REG_IOCOUNTINH;
> +	u32 value = readl(addr);
> +
> +	writel(value | BIT(idx), addr);
> +}

[Severity: High]
Can this lockless read-modify-write of the IOCOUNTINH register corrupt the
hardware counter enables?

If this is executed concurrently across multiple CPUs during a hotplug race,
concurrent modifications to the shared driver state could be lost.

[ ... ]
> +static int riscv_iommu_pmu_add(struct perf_event *event, int flags)
> +{
> +	struct riscv_iommu_pmu *pmu = to_riscv_iommu_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	unsigned int num_counters = pmu->num_counters;
> +	unsigned int idx;
> +
> +	/* Reserve index zero for iohpmcycles */
> +	if (is_cycle_event(event->attr.config))
> +		idx = RISCV_IOMMU_HPM_CYCLE_IDX;
> +	else
> +		idx = find_next_zero_bit(pmu->used_counters, num_counters, 1);
> +
> +	/* All event counters or cycle counter are in use */
> +	if (idx == num_counters || pmu->events[idx])
> +		return -EAGAIN;
> +
> +	set_bit(idx, pmu->used_counters);
> +
> +	pmu->events[idx] = event;

[Severity: High]
Does this code safely handle concurrent allocations during CPU hotplug?

Since the driver appears to rely exclusively on the per-CPU perf_event_context
lock without a PMU-level spinlock or event->cpu verification,
riscv_iommu_pmu_add() might execute on two CPUs simultaneously during a hotplug
window.

If task A is preempted before installation and migrated, while task B opens an
event on the new CPU, these lockless find_next_zero_bit() and set_bit()
operations could race. This would allow two different events to allocate the
same hardware counter.

[ ... ]
> +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;
> +
> +	/*
> +	 * Masking the irq for the whole selection + migration sequence below
> +	 * removes the need to reason about exactly when in that window an
> +	 * interrupt could fire - none can, until enable_irq() at the end.
> +	 * This also avoid CPU hotplug race condition
> +	 */
> +	disable_irq(iommu_pmu->irq);

[Severity: High]
Is it safe to call disable_irq() on this interrupt line?

Because the PMU driver requests this interrupt with IRQF_SHARED, it shares the
hardware vector with the main RISC-V IOMMU driver's fault and command queues.

Calling disable_irq() physically masks the interrupt at the interrupt controller
level, which will stop all critical IOMMU interrupts from being delivered during
CPU hotplug. This could stall IOMMU command completions and device page faults,
potentially leading to severe I/O timeouts for attached PCIe devices.

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