[PATCH RESEND v7 3/3] drivers/perf: riscv-iommu: protect shared state with a raw spinlock

[Zong Li]

On Fri, Sep 4, 2026 at 4:38 PM Zong Li <zong.li at sifive.com> wrote:
>
> On Wed, Sep 2, 2026 at 4:07 PM Yicong Yang <yang.yicong at picoheart.com> wrote:
> >
> > On 8/28/26 4:58 PM, Zong Li wrote:
> > > Events are bound to one CPU and the interrupt is affine to it, so the
> > > perf callbacks running with interrupts disabled would be enough to
> > > exclude the handler.
> > >
> > > PCI MSI/MSI-X on IMSIC breaks that: the irqchip sets
> > > 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.
> > >
> > > Take a raw spinlock, with interrupts disabled so that the handler can
> > > never interrupt a holder on the same CPU, rather than depending on that
> > > irqchip behaviour. It covers the state which is reachable from both
> > > sides:
> > >
> > >  - IOCOUNTINH is read-modify-written by ->start()/->stop() and is saved
> > >    and restored around the whole handler.
> > >  - pmu->events[] is written by ->del() and read by the handler.
> > >  - hw_perf_event::prev_count is updated by both.
> > >
> > > ->add() and ->del() call the unlocked __riscv_iommu_pmu_start() and
> > > __riscv_iommu_pmu_stop() so the lock is taken once per callback.
> > >
> > > Signed-off-by: Zong Li <zong.li at sifive.com>
> > > ---
> > >  drivers/perf/riscv_iommu_pmu.c | 66 ++++++++++++++++++++++++++++++----
> > >  1 file changed, 60 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/perf/riscv_iommu_pmu.c b/drivers/perf/riscv_iommu_pmu.c
> > > index f6acd56f2f61..ee2f6d1fbece 100644
> > > --- a/drivers/perf/riscv_iommu_pmu.c
> > > +++ b/drivers/perf/riscv_iommu_pmu.c
> > > @@ -101,6 +101,7 @@ struct riscv_iommu_pmu {
> > >       u64 event_cntr_mask;
> > >       struct perf_event *events[RISCV_IOMMU_HPM_COUNTER_NUM];
> > >       DECLARE_BITMAP(used_counters, RISCV_IOMMU_HPM_COUNTER_NUM);
> > > +     raw_spinlock_t lock;
> > >  };
> > >
> > >  #define to_riscv_iommu_pmu(p) (container_of(p, struct riscv_iommu_pmu, pmu))
> > > @@ -485,7 +486,8 @@ static void riscv_iommu_pmu_update(struct perf_event *event)
> > >       local64_add(delta, &event->count);
> > >  }
> > >
> > > -static void riscv_iommu_pmu_start(struct perf_event *event, int flags)
> > > +/* Called with pmu->lock held */
> > > +static void __riscv_iommu_pmu_start(struct perf_event *event, int flags)
> > >  {
> > >       struct riscv_iommu_pmu *pmu = to_riscv_iommu_pmu(event->pmu);
> > >       struct hw_perf_event *hwc = &event->hw;
> > > @@ -500,11 +502,22 @@ static void riscv_iommu_pmu_start(struct perf_event *event, int flags)
> > >       riscv_iommu_pmu_set_period(event);
> > >       riscv_iommu_pmu_set_event(pmu, hwc->idx, hwc->config);
> > >       riscv_iommu_pmu_enable_counter(pmu, hwc->idx);
> > > +}
> > > +
> > > +static void riscv_iommu_pmu_start(struct perf_event *event, int flags)
> > > +{
> > > +     struct riscv_iommu_pmu *pmu = to_riscv_iommu_pmu(event->pmu);
> > > +     unsigned long irqflags;
> > > +
> > > +     raw_spin_lock_irqsave(&pmu->lock, irqflags);
> > > +     __riscv_iommu_pmu_start(event, flags);
> > > +     raw_spin_unlock_irqrestore(&pmu->lock, irqflags);
> >
> > could use guard(raw_spinlock_irqsave)() to help handle the lock release.
> >
> > I'm considering if it'll better to use irq_work or function call to handle the
> > counts update from the interrupt handler, e.g. in the irq handler first check
> > if current cpu is the pmu->on_cpu, if so update the counts, otherwise queue
> > the update work to pmu->on_cpu by irq work or smp_call_function_single_async.
> > In this way we can use pmu's original synchronization mechanism without
> > extra spin locks since the callbacks will always run and synchronized by
> > the same cpu, and the shared interrupt doesn't matter at all.
> >
> > any thoughts on this?
> >
>
> Thank you for your suggestion. It really helped me re-think the impact
> of IRQCHIP_MOVE_DEFERRED.
>
> When an IRQ goes to the wrong CPU, handle_edge_irq() already changes
> the affinity to the correct CPU via irq_ack at this time. Because of
> this, we might only need a simple check at the beginning of the IRQ
> handler. We can just check if the current CPU is the correct one. If
> it is not, we can simply return and do nothing. Since PMIP is still
> asserted, the IRQ will trigger again and go to the correct CPU.

I apologize, I missed one detail. The interrupt won't re-trigger if
the PMIP and OF bits are not cleared.
I will follow your suggestion and use the irq_work mechanism. I will
add this to the updated version.
Thank you for your help

>
> I will drop this patch and make a new, simpler fix.
>
> Thanks again for your help!
>
> > thanks.
> >
> > >
> > >       perf_event_update_userpage(event);
> > >  }
> > >
> > > -static void riscv_iommu_pmu_stop(struct perf_event *event, int flags)
> > > +/* Called with pmu->lock held */
> > > +static void __riscv_iommu_pmu_stop(struct perf_event *event, int flags)
> > >  {
> > >       struct riscv_iommu_pmu *pmu = to_riscv_iommu_pmu(event->pmu);
> > >       struct hw_perf_event *hwc = &event->hw;
> > > @@ -521,13 +534,26 @@ static void riscv_iommu_pmu_stop(struct perf_event *event, int flags)
> > >       hwc->state |= PERF_HES_STOPPED | PERF_HES_UPTODATE;
> > >  }
> > >
> > > +static void riscv_iommu_pmu_stop(struct perf_event *event, int flags)
> > > +{
> > > +     struct riscv_iommu_pmu *pmu = to_riscv_iommu_pmu(event->pmu);
> > > +     unsigned long irqflags;
> > > +
> > > +     raw_spin_lock_irqsave(&pmu->lock, irqflags);
> > > +     __riscv_iommu_pmu_stop(event, flags);
> > > +     raw_spin_unlock_irqrestore(&pmu->lock, irqflags);
> > > +}
> > > +
> > >  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 long irqflags;
> > >       unsigned int idx;
> > >
> > > +     raw_spin_lock_irqsave(&pmu->lock, irqflags);
> > > +
> > >       /* Reserve index zero for iohpmcycles */
> > >       if (is_cycle_event(event->attr.config))
> > >               idx = RISCV_IOMMU_HPM_CYCLE_IDX;
> > > @@ -535,8 +561,10 @@ static int riscv_iommu_pmu_add(struct perf_event *event, int flags)
> > >               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])
> > > +     if (idx == num_counters || pmu->events[idx]) {
> > > +             raw_spin_unlock_irqrestore(&pmu->lock, irqflags);
> > >               return -EAGAIN;
> > > +     }
> > >
> > >       set_bit(idx, pmu->used_counters);
> > >
> > > @@ -546,7 +574,9 @@ static int riscv_iommu_pmu_add(struct perf_event *event, int flags)
> > >       local64_set(&hwc->prev_count, 0);
> > >
> > >       if (flags & PERF_EF_START)
> > > -             riscv_iommu_pmu_start(event, flags);
> > > +             __riscv_iommu_pmu_start(event, flags);
> > > +
> > > +     raw_spin_unlock_irqrestore(&pmu->lock, irqflags);
> > >
> > >       /* Propagate changes to the userspace mapping. */
> > >       perf_event_update_userpage(event);
> > > @@ -556,18 +586,26 @@ static int riscv_iommu_pmu_add(struct perf_event *event, int flags)
> > >
> > >  static void riscv_iommu_pmu_read(struct perf_event *event)
> > >  {
> > > +     struct riscv_iommu_pmu *pmu = to_riscv_iommu_pmu(event->pmu);
> > > +     unsigned long irqflags;
> > > +
> > > +     raw_spin_lock_irqsave(&pmu->lock, irqflags);
> > >       riscv_iommu_pmu_update(event);
> > > +     raw_spin_unlock_irqrestore(&pmu->lock, irqflags);
> > >  }
> > >
> > >  static void riscv_iommu_pmu_del(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 long irqflags;
> > >       int idx = hwc->idx;
> > >
> > > -     riscv_iommu_pmu_stop(event, PERF_EF_UPDATE);
> > > +     raw_spin_lock_irqsave(&pmu->lock, irqflags);
> > > +     __riscv_iommu_pmu_stop(event, PERF_EF_UPDATE);
> > >       pmu->events[idx] = NULL;
> > >       clear_bit(idx, pmu->used_counters);
> > > +     raw_spin_unlock_irqrestore(&pmu->lock, irqflags);
> > >
> > >       perf_event_update_userpage(event);
> > >  }
> > > @@ -635,12 +673,24 @@ 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));
> > > +     unsigned long irqflags;
> > >       u32 ovf, idx, inhibit;
> > >
> > > -     /* Check whether this interrupt is for PMU */
> > > +     /*
> > > +      * Check whether this interrupt is for PMU. Done outside the lock so
> > > +      * that a shared interrupt line is left alone as cheaply as possible.
> > > +      */
> > >       if (!(readl_relaxed(pmu->reg + RISCV_IOMMU_REG_IPSR) & RISCV_IOMMU_IPSR_PMIP))
> > >               return IRQ_NONE;
> > >
> > > +     /*
> > > +      * Hold the lock across the whole sequence below. Stopping the
> > > +      * counters, processing them and restoring the previous inhibit state
> > > +      * has to be atomic against ->start()/->stop(), otherwise a counter
> > > +      * enabled in between would be inhibited again by the restore.
> > > +      */
> > > +     raw_spin_lock_irqsave(&pmu->lock, irqflags);
> > > +
> > >       /* Process PMU IRQ */
> > >       inhibit = riscv_iommu_pmu_stop_all(pmu);
> > >
> > > @@ -672,6 +722,8 @@ static irqreturn_t riscv_iommu_pmu_irq_handler(int irq, void *dev_id)
> > >
> > >       riscv_iommu_pmu_start_all(pmu, inhibit);
> > >
> > > +     raw_spin_unlock_irqrestore(&pmu->lock, irqflags);
> > > +
> > >       return IRQ_HANDLED;
> > >  }
> > >
> > > @@ -735,6 +787,8 @@ static int riscv_iommu_pmu_probe(struct auxiliary_device *auxdev,
> > >
> > >       iommu_pmu->reg = iommu_dev->reg;
> > >
> > > +     raw_spin_lock_init(&iommu_pmu->lock);
> > > +
> > >       /*
> > >        * Counter number and width are hardware-implemented, detect them by
> > >        * writing 1s and reading back which bits stuck.