[PATCH v16 0/2] RISC-V IOMMU HPM support

[Zong Li]

On Sat, Nov 7, 2026 at 8:37 PM Yicong Yang <yang.yicong at picoheart.com> wrote:
>
> On 10/7/26 3:30 PM, Zong Li wrote:
> > On Tue, Oct 6, 2026 at 5:03 PM Zong Li <zong.li at sifive.com> wrote:
> >>
> >> This series implements support for the RISC-V IOMMU hardware performance
> >> monitor.
> >>
> >> The RISC-V IOMMU PMU driver is implemented as an auxiliary device driver
> >> created by the parent RISC-V IOMMU driver. Therefore, the child driver
> >> can obtain resources and information from the parent device, such as
> >> the MMIO base address and IRQ number.
> >>
> >> It seems sashiko-bot is giving conflicting advice about whether to add
> >> a lock to ->read(). No matter if we add it or not, the sashiko-bot always
> >> suggests the opposite. Looking at no lock case again, sashiko-bot
> >> concerns about a BPF program calling bpf_perf_event_read() from NMI
> >> context and self-deadlocking against the IRQ handler, but that scenario
> >> cannot occur here:
> >>
> >>   - bpf_perf_event_read() goes through perf_event_read_local(), which
> >>     refuses to call ->read() unless event_cpu == smp_processor_id()
> >>     (kernel/events/core.c). All events on this PMU are CPU-bound and
> >>     non-task-bound (->event_init() forces event->cpu = pmu->on_cpu and
> >>     rejects sampling events), so PERF_ATTACH_TASK is never set.
> >>   - More fundamentally, perf overflow interrupts are not NMIs on RISC-V,
> >>     and every pmu->lock critical section in this driver is held under
> >>     raw_spin_lock_irqsave(). There is no context that can interrupt them
> >>     and re-enter ->read().
> >>
> >> No other driver under drivers/perf/ uses a trylock or in_nmi() check in
> >> its ->read() callback, so dropping it also brings this driver in line
> >> with the rest.
> >>
> >> sashiko-bot also reported that migration calling trigger a NULL pointer
> >> dereference. I don't think this can happen on current mainline.
> >> perf_pmu_migrate_context() does not touch pmu->cpu_pmu_context unless it
> >> actually finds events to migrate. This used to be a real hazard: before
> >> commit bd2756811766 ("perf: Rewrite core context handling", v6.2-rc1)
> >> the first two lines were per_cpu_ptr(pmu->pmu_cpu_context, ...), which
> >> did dereference a pmu-owned pointer unconditionally. That is also why
> >> several in-tree drivers pre-seed thier ->cpu field with a real CPU number
> >> rather than a sentinel, and so do call perf_pmu_migrate_context() in
> >> this window, without any reported oops.
> >>
>
> [...]
>
> >
> > Hi all
> >
> > In the v16, sashiko-bot suggested to add a pmu->lock in ->read()
> > again, let me address this, and explain why I would prefer to leave it
> > as a documented limitation in this series rather than respin.
> >
> > What I would like to push back on is the suggested fix, because every
> > point fix in this area has already been reviewed and rejected in an
> > earlier round of this series:
> >
> >   - Taking pmu->lock in ->read() (which is literally the diff
> > suggested here) was in v7-resend. It was rejected: a BPF program
> > reading an IOMMU PMU event from NMI context via bpf_perf_event_read()
> > invokes pmu->read(), and if that NMI lands on a CPU already holding
> > pmu->lock in the IRQ handler or in ->add(), the read callback spins
> > forever.
> >   - raw_spin_trylock_irqsave() in ->read() was the v13 answer to that.
> > It was rejected in v14: on cross-CPU contention the trylock fails,
> > ->read() returns without updating, and userspace silently gets a stale
> > count.
> >   - The current lockless form is what v15 and v16 are objecting to.
> >
> > So the three obvious shapes for ->read() have each been rejected, in
> > each case for a valid reason. That is a strong hint that ->read() is
> > the wrong place to fix this: the actual root cause is that the
> > overflow handler may run off the bound CPU at all.
> >
> > Worth noting that no PMU driver under drivers/perf/ (including all the
> > hisilicon ones) or arch/x86/events/intel/uncore.c takes a spinlock in
> > its ->read() callback -- not even on x86, which does have NMI-context
> > perf events. They don't need to, because they pin the overflow
> > interrupt to on_cpu with irq_set_affinity(), so the handler and the
> > perf callbacks all run on the same CPU, and ->read() is already
> > invoked there with interrupts disabled (smp_call_function_single()
> > from perf_event_read(), or the event_cpu == smp_processor_id() check
> > in perf_event_read_local()). Same-CPU plus interrupts-disabled is the
> > entire synchronisation mechanism, and it is why the
> > prev_count/hardware pair cannot be observed half-updated.
>
> that's how the event core's synchronization is designed and expect the
> PMU drivers to follow. so the race is true if pmu::read() is not protected
> in the current implementation.
> >
> > RISC-V IOMMU PMU driver does the same irq_set_affinity() call, but on
> > IMSIC the move can be deferred, so the guarantee is weaker. Hence the
> > race. On the scope of the impact, to explain why I think this can
> > wait:
> >   - The consequence is confined to a reported count. prev_count and
> > event->count are local64_t and the cmpxchg cannot tear, so there is no
> > memory-safety issue, no oops, and no effect on IOMMU operation itself.
> >   - The window requires an affinity change to be in flight.
> > riscv_iommu_pmu_bind_cpu() is only called from the CPU hotplug online
> > and offline callbacks, so a deferred move is only pending during CPU
> > hotplug. In steady state the interrupt is already on on_cpu, ->read()
> > runs on that same CPU with interrupts disabled, and the window does
> > not exist. This is a transient during hotplug, not the common case.
>
> so the problematic part is that the counter update from the overflow
> interrupt is not guaranteed to run on the event->cpu for the first
> even if the affinity is set to event->cpu, due to IMSIC's affinity
> move deferral. this should only affect IOMMU implemented as PCIe
> device, not platform device with either wired interrupt or MSI.
>
> what was the problem of using irq work to queue the counter update
> on the event->cpu? I see it was removed in v10 due to IRQCHIP_MOVE_DEFERRED
> which confused me. with irq work we can ensure the counter update
> on event->cpu no matter which cpu take the interrupt. was there
> other issues with the irq work?

Yes, as I mentioned at the end of the cover letter, I might prefer to
send a separate patch to address this issue. In that new patch, I am
trying to use irq_work to solve it. The main difference from the
irq_work implementation in v9 is that the previous approach was too
simplistic. The new patch will take more scenarios and corner cases
into consideration and address them properly.

>
> thanks.
>
> >
> > That is a structural change to the interrupt path with several
> > interacting constraints, each of which has already caused a regression
> > in this series once. I would rather not fold it into this series at
> > this point, both because it deserves review on its own and because
> > getting it wrong again in a hurry is worse than a documented
> > counter-accuracy limitation during hotplug. So my proposal is: leave
> > the code as-is for this series, add a comment on
> > riscv_iommu_pmu_read() recording the limitation and why the three
> > obvious fixes are not used, and address it in a follow-up patchset.
> >
> > Please let me know if you want me to do more for this series.
> > Thanks!
> >
> >> Zong Li (2):
> >>   drivers/perf: riscv-iommu: add risc-v iommu pmu driver
> >>   iommu/riscv: create a auxiliary device for HPM
> >>
> >>  drivers/iommu/riscv/Kconfig      |    1 +
> >>  drivers/iommu/riscv/iommu-bits.h |   61 --
> >>  drivers/iommu/riscv/iommu.c      |   58 ++
> >>  drivers/iommu/riscv/iommu.h      |    4 +
> >>  drivers/perf/Kconfig             |   12 +
> >>  drivers/perf/Makefile            |    1 +
> >>  drivers/perf/riscv_iommu_pmu.c   | 1102 ++++++++++++++++++++++++++++++
> >>  7 files changed, 1178 insertions(+), 61 deletions(-)
> >>  create mode 100644 drivers/perf/riscv_iommu_pmu.c
> >>
> >> --
> >> 2.43.7
> >>