Re: [PATCH v6 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 4 potential issue(s) to consider:
- [High] Missing spinlock to protect shared PMU state and hardware registers from concurrent IRQ handler execution.
- [Medium] Cycle event filter configurations are silently ignored.
- [Medium] Missing validation for `num_counters == 0` leads to undefined behavior in GENMASK_ULL.
- [Low] Type confusion in `is_cycle_event()` parameters leads to double-extraction of event IDs.
--

--- Patch [1]: [PATCH v6 1/2] drivers/perf: riscv-iommu: add risc-v iommu pmu driver ---
commit c7845a25e4e2953e42707a05131529dc059de52f
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 for the RISC-V IOMMU PMU.
It enables the perf subsystem to read and configure hardware-implemented
performance monitoring and cycle 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..f81e5bb323364
> --- /dev/null
> +++ b/drivers/perf/riscv_iommu_pmu.c
> @@ -0,0 +1,813 @@

[ ... ]

> +static bool is_cycle_event(u64 event)
> +{
> +	return FIELD_GET(RISCV_IOMMU_IOHPMEVT_EVENTID, event) ==
> +	       RISCV_IOMMU_HPMEVENT_CYCLE;
> +}
> +
> +static void riscv_iommu_pmu_set_event(struct riscv_iommu_pmu *pmu, u32 idx,
> +				      u64 value)
> +{
> +	/* There is no associtated IOHPMEVT0 for IOHPMCYCLES */
> +	if (is_cycle_event(value))
> +		return;

[Severity: Medium]
Will this silently ignore filter configurations requested for the cycle
event?

When userspace invokes perf stat with filter configurations applied to
the cycle event, riscv_iommu_pmu_event_init() accepts them, but this code
completely ignores the configuration since the hardware does not support
filtering for cycle counters.

Should the driver reject unsupported filter configurations with -EINVAL 
instead of silently returning here?

> +
> +	/* Event counter start from idx 1 */
> +	riscv_iommu_pmu_writeq(FIELD_GET(RISCV_IOMMU_IOHPMEVT_EVENT, value),
> +			       pmu->reg + RISCV_IOMMU_REG_IOHPMEVT(idx - 1));
> +}
> +
> +static void riscv_iommu_pmu_enable_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 on IOCOUNTINH race with the IRQ handler
updating the same register in riscv_iommu_pmu_stop_all()?

The driver ignores failures from irq_set_affinity() with just a dev_dbg log, 
so the IRQ handler might run on a different CPU, allowing 
riscv_iommu_pmu_stop_all() to preempt or run concurrently with these state 
updates.

Would it be safer to add a spinlock to protect shared PMU state and hardware
registers?

> +}

[ ... ]

> +static u32 riscv_iommu_pmu_stop_all(struct riscv_iommu_pmu *pmu)
> +{
> +	void __iomem *addr = pmu->reg + RISCV_IOMMU_REG_IOCOUNTINH;
> +	u32 inhibit = readl(addr);
> +
> +	writel(GENMASK_ULL(pmu->num_counters - 1, 0), addr);

[Severity: Medium]
Is there a risk of undefined behavior if this evaluates to GENMASK_ULL(-1, 0)?

If the hardware implements the HPM capability but reports 0 counters,
num_counters is initialized to 0 in riscv_iommu_pmu_probe() without
validation.

If an interrupt fires, num_counters - 1 becomes -1, violating the macro's
internal upper bound checks.

> +
> +	return inhibit;
> +}

[ ... ]

> +static int riscv_iommu_pmu_event_init(struct perf_event *event)
> +{
> +	struct riscv_iommu_pmu *pmu = to_riscv_iommu_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct perf_event *sibling;
> +	int total_event_counters = pmu->num_counters - 1;
> +	int counters = 0;

[ ... ]

> +	if (!is_cycle_event(get_event(event)))

[Severity: Low]
Could this result in double-extraction of the event ID?

is_cycle_event() applies FIELD_GET to extract the event ID from a raw config,
but get_event() also applies FIELD_GET. While this might accidentally work if
the bitmask starts at bit 0, it could break if the mask layout changes.

> +		if (++counters > total_event_counters)
> +			return -EINVAL;

[ ... ]

> +static void riscv_iommu_pmu_update(struct perf_event *event)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct riscv_iommu_pmu *pmu = to_riscv_iommu_pmu(event->pmu);
> +	u64 delta, prev, now;
> +	u32 idx = hwc->idx;
> +	u64 counter_mask = idx ? pmu->event_cntr_mask : pmu->cycle_cntr_mask;
> +
> +	do {
> +		prev = local64_read(&hwc->prev_count);
> +		now = riscv_iommu_pmu_get_counter(pmu, idx);
> +	} while (local64_cmpxchg(&hwc->prev_count, prev, now) != prev);

[Severity: High]
How is this data protected from concurrent accesses by the IRQ handler?

local64_cmpxchg() is not multi-CPU atomic. If irq_set_affinity() fails,
riscv_iommu_pmu_irq_handler() could execute concurrently on another CPU,
racing with this update.

> +
> +	delta = (now - prev) & counter_mask;
> +	local64_add(delta, &event->count);
> +}

[ ... ]

> +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;
> +
> +	/* 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);
> +
> +	ovf = readl(pmu->reg + RISCV_IOMMU_REG_IOCOUNTOVF);
> +	if (ovf) {
> +		bitmap_from_u64(ovf_bitmap, ovf);
> +		for_each_set_bit(idx, ovf_bitmap, pmu->num_counters) {
> +			struct perf_event *event = pmu->events[idx];

[Severity: High]
Are accesses to the pmu->events array synchronized against event removal?

If the IRQ handler runs concurrently with riscv_iommu_pmu_del() because
irq_set_affinity() failed to restrict it to a single CPU, this could result
in a use-after-free of the event structures.

> +
> +			/*

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