Re: [PATCH v4 1/2] drivers/perf: riscv-iommu: add risc-v iommu pmu driver

On Wed, Jul 22, 2026 at 5:32 PM Yicong Yang <[email protected]> wrote:
>
> On 7/22/26 1:15 PM, Zong Li wrote:
> > On Mon, Jul 20, 2026 at 3:29 PM Yicong Yang <[email protected]> wrote:
> >>
> >> Hi Zong,
> >>
> >> On 7/14/26 4:36 PM, Zong Li wrote:
> >>> 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.
> >>>
> >>> The RISC-V IOMMU PMU separates the cycle counter from the event counters.
> >>> The cycle counter is not associated with iohpmevt0, so a software-defined
> >>> cycle event is required for the perf subsystem.
> >>>
> >>> The number and width of the counters are hardware-implemented and must
> >>> be detected at runtime.
> >>>
> >>> The performance monitor provides counters with filtering support to
> >>> collect events for specific device ID/process ID, or GSCID/PSCID.
> >>>
> >>> According to RISC-V IOMMU specification Chapter 6:
> >>> Whether an 8 byte access to an IOMMU register is single-copy atomic is
> >>> UNSPECIFIED. Use two separate 4 byte accesses for hardware
> >>> compatibility.
> >>>
> >>> PMU-related definitions are moved into the perf driver, where they are
> >>> used exclusively.
> >>>
> >>> Suggested-by: David Laight <[email protected]>
> >>> Suggested-by: Guo Ren <[email protected]>
> >>> Link: https://lore.kernel.org/linux-riscv/20260618143634.7f3dd6c5@pumpkin/
> >>> Signed-off-by: Zong Li <[email protected]>
> >>> ---
> >>>  drivers/iommu/riscv/iommu-bits.h |  61 ---
> >>>  drivers/perf/Kconfig             |  12 +
> >>>  drivers/perf/Makefile            |   1 +
> >>>  drivers/perf/riscv_iommu_pmu.c   | 722 +++++++++++++++++++++++++++++++
> >>>  4 files changed, 735 insertions(+), 61 deletions(-)
> >>>  create mode 100644 drivers/perf/riscv_iommu_pmu.c
> >>>
>
> [...]
>
> >>> +
> >>> +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, RISCV_IOMMU_HPM_COUNTER_NUM);
> >>> +     u32 ovf, idx;
> >>> +
> >>> +     /* 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 */
> >>> +     riscv_iommu_pmu_stop_all(pmu);
> >>> +
> >>> +     ovf = readl(pmu->reg + RISCV_IOMMU_REG_IOCOUNTOVF);
> >>> +     if (!ovf)
> >>> +             return IRQ_NONE;
> >>> +
> >>> +     bitmap_from_u64(ovf_bitmap, ovf);
> >>> +     for_each_set_bit(idx, ovf_bitmap, pmu->num_counters) {
> >>> +             struct perf_event *event = pmu->events[idx];
> >>> +
> >>> +             if (WARN_ON_ONCE(!event))
> >>> +                     continue;
> >>> +
> >>> +             riscv_iommu_pmu_update(event);
> >>> +             riscv_iommu_pmu_set_period(event);
> >>> +             riscv_iommu_pmu_clear_ovf(event, idx);
> >>> +     }
> >>> +
> >>> +     /* Clear performance monitoring interrupt pending bit */
> >>> +     writel_relaxed(RISCV_IOMMU_IPSR_PMIP, pmu->reg + RISCV_IOMMU_REG_IPSR);
> >>> +
> >>> +     riscv_iommu_pmu_start_all(pmu);
> >>> +
> >>> +     return IRQ_HANDLED;
> >>> +}
> >>> +
> >>> +static unsigned int riscv_iommu_pmu_get_irq_num(struct riscv_iommu_device *iommu)
> >>> +{
> >>> +     /* Reuse ICVEC.CIV mask for all interrupt vectors mapping */
> >>> +     int vec = (iommu->icvec >> (RISCV_IOMMU_INTR_PM * 4)) & RISCV_IOMMU_ICVEC_CIV;
> >>> +
> >>> +     return iommu->irqs[vec];
> >>> +}
> >>> +
> >>> +static int riscv_iommu_pmu_request_irq(struct riscv_iommu_device *iommu,
> >>> +                                    struct riscv_iommu_pmu *pmu)
> >>> +{
> >>> +     return devm_request_irq(iommu->dev, pmu->irq, riscv_iommu_pmu_irq_handler,
> >>> +                             IRQF_NOBALANCING | IRQF_SHARED, dev_name(iommu->dev), pmu);
> >>
> >> need IRQF_NO_THREAD to avoid force thread of the handler.
> >>
> >> it's a PMU exclusive interrupt, reason to make it IRQF_SHARED?
> >
> > PMU interrupt can be shared with the queue's interrupt (such as the
> > command queue, fault queue, and so on). Also, in the IOMMU driver,
> > these queue interrupts are handled as threaded IRQ
> >
>
> you can keep iommu's handler threaded while pmu's still in
> the top half, so it doesn't matter to set IRQF_SHARED.

I notieced that this conflates two independent checks in
__setup_irq(). Both are in kernel/irq/manage.c, on the path taken when
the virq already has an action installed:

         if (!((old->flags & new->flags) & IRQF_SHARED) ||
             (oldtype != (new->flags & IRQF_TRIGGER_MASK)))
                 goto mismatch;

         if ((old->flags & IRQF_ONESHOT) &&
             (new->flags & IRQF_COND_ONESHOT))
                 new->flags |= IRQF_ONESHOT;
         else if ((old->flags ^ new->flags) & IRQF_ONESHOT)
                 goto mismatch;

There is no path anywhere in __setup_irq() that relaxes the
requirement because one requester runs in hard IRQ context and the
other one is threaded. The iommu driver requests its queue interrupts
with IRQF_ONESHOT | IRQF_SHARED, so old->flags has IRQF_SHARED.
Without it in new->flags the AND is zero and we take the mismatch
path, which means -EBUSY.

I will add a comment above the call recording why IRQF_SHARED is
needed, so this does not get dropped again in a later revision.

>
> if so we cannot ensure the count's updated in the atmoic
> context on event->cpu, which is required by the perf core
> to keep serializaiton. e.g. possible for the other users
> bind this irq to another cpu other than event->cpu?
>
>
> >>
> >>> +}
> >>> +
> >>> +static int riscv_iommu_pmu_probe(struct auxiliary_device *auxdev,
> >>> +                              const struct auxiliary_device_id *id)
> >>> +{
> >>> +     struct  riscv_iommu_device *iommu_dev = dev_get_platdata(&auxdev->dev);
> >>> +     struct riscv_iommu_pmu *iommu_pmu;
> >>> +     void __iomem *addr;
> >>> +     char *name;
> >>> +     int ret;
> >>> +
> >>> +     iommu_pmu = devm_kzalloc(&auxdev->dev, sizeof(*iommu_pmu), GFP_KERNEL);
> >>> +     if (!iommu_pmu)
> >>> +             return -ENOMEM;
> >>> +
> >>> +     iommu_pmu->reg = iommu_dev->reg;
> >>> +
> >>> +     /* Counter number and width are hardware-implemented. Detect them by write 1s */
> >>> +     addr = iommu_pmu->reg + RISCV_IOMMU_REG_IOCOUNTINH;
> >>> +     writel(RISCV_IOMMU_IOCOUNTINH_HPM, addr);
> >>> +     iommu_pmu->num_counters = hweight32(readl(addr));
> >>> +
> >>> +     addr = iommu_pmu->reg + RISCV_IOMMU_REG_IOHPMCYCLES;
> >>> +     riscv_iommu_pmu_writeq(RISCV_IOMMU_IOHPMCYCLES_COUNTER, addr);
> >>> +     iommu_pmu->cycle_cntr_mask = riscv_iommu_pmu_readq(addr);
> >>> +
> >>> +     /* Assume the width of all event counters are the same */
> >>> +     addr = iommu_pmu->reg + RISCV_IOMMU_REG_IOHPMCTR_BASE;
> >>> +     riscv_iommu_pmu_writeq(RISCV_IOMMU_IOHPMCTR_COUNTER, addr);
> >>> +     iommu_pmu->event_cntr_mask = riscv_iommu_pmu_readq(addr);
> >>> +
> >>> +     iommu_pmu->pmu = (struct pmu) {
> >>> +             .module         = THIS_MODULE,
> >>> +             .parent         = &auxdev->dev,
> >>> +             .task_ctx_nr    = perf_invalid_context,
> >>> +             .event_init     = riscv_iommu_pmu_event_init,
> >>> +             .add            = riscv_iommu_pmu_add,
> >>> +             .del            = riscv_iommu_pmu_del,
> >>> +             .start          = riscv_iommu_pmu_start,
> >>> +             .stop           = riscv_iommu_pmu_stop,
> >>> +             .read           = riscv_iommu_pmu_read,
> >>> +             .attr_groups    = riscv_iommu_pmu_attr_grps,
> >>> +             .capabilities   = PERF_PMU_CAP_NO_EXCLUDE,
> >>> +     };
> >>> +
> >>> +     auxiliary_set_drvdata(auxdev, iommu_pmu);
> >>> +
> >>> +     name = devm_kasprintf(&auxdev->dev, GFP_KERNEL,
> >>> +                           "riscv_iommu_pmu_%s", dev_name(iommu_dev->dev));
> >>
> >> this name is a bit unfriendly for a platform iommu. perf use ":" as
> >> a special delimiter, so I need to double quote the event like:
> >>
> >>   perf stat -e "'riscv_iommu_pmu_RSCV0004:00'/cycle/"
> >>
> >> possible to make it something like riscv_iommu_pmu_RSCV0004_00?
> >
> > Ok, this will also happen in the PCIe case. Let's find a way to modify
> > it. Or do you have any idea for the name?
> >
>
> maybe just replace ":" with "_" or use the PFN to distinguish
> like arm smmu pmcg. the latter one will eliminate the
> implementation differences (platform or pci device) but may
> not suitable since it's hard to find the connection between
> iommu and it's related pmu as iommu is still named by device
> name.
>
> I see the PCIe based IOMMU is named in the raw BDF encoding
> in fengyu's test [1] but it should be separate by ":", did
> I miss something for the name of PCIe based IOMMU?
>
> [1] https://lore.kernel.org/linux-riscv/[email protected]/T/#m847cd5cba7f53e9191e09e64ef604c7c38fd0173
>
> >>
> >>> +     if (!name) {
> >>> +             dev_err(&auxdev->dev, "Failed to create name riscv_iommu_pmu_%s\n",
> >>> +                     dev_name(iommu_dev->dev));
> >>> +             return -ENOMEM;
> >>> +     }
> >>> +
> >>> +     /* Bind all events to the same cpu context to avoid race enabling */
> >>> +     iommu_pmu->on_cpu = raw_smp_processor_id();
> >>
> >> smp_processor_id() should be enough.
> >
> > Perhaps we should try to get the local CPU of iommu as above.
> >
> >>
> >>> +     iommu_pmu->irq = riscv_iommu_pmu_get_irq_num(iommu_dev);
> >>> +     WARN_ON(irq_set_affinity(iommu_pmu->irq, cpumask_of(iommu_pmu->on_cpu)));
> >>> +
> >>> +     ret = riscv_iommu_pmu_request_irq(iommu_dev, iommu_pmu);
> >>> +     if (ret) {
> >>> +             dev_err(&auxdev->dev, "Failed to request irq %s: %d\n", name, ret);
> >>> +             return ret;
> >>> +     }
> >>> +
> >>> +     ret = cpuhp_state_add_instance_nocalls(cpuhp_state, &iommu_pmu->node);
> >>> +     if (ret) {
> >>> +             dev_err(&auxdev->dev, "Failed to register hotplug %s: %d\n", name, ret);
> >>> +             return ret;
> >>> +     }
> >>> +
> >>> +     ret = perf_pmu_register(&iommu_pmu->pmu, name, -1);
> >>> +     if (ret) {
> >>> +             dev_err(&auxdev->dev, "Failed to registe %s: %d\n", name, ret);
> >>> +             goto err_unregister;
> >>> +     }
> >>> +
> >>> +     dev_info(&auxdev->dev, "%s: Registered with %d counters\n",
> >>> +              name, iommu_pmu->num_counters);
> >>> +
> >>> +     return 0;
> >>> +
> >>> +err_unregister:
> >>> +     cpuhp_state_remove_instance_nocalls(cpuhp_state, &iommu_pmu->node);> +  return ret;
> >>> +}
> >>> +
> >>> +static const struct auxiliary_device_id riscv_iommu_pmu_id_table[] = {
> >>> +     { .name = "iommu.pmu" },
> >>
> >> it's clear and consistent to name it as "riscv-iommu.pmu".
> >> you've already have this "riscv" prefix in other places
> >> like the name of the PMU.
> >
> > Ok. Let's add prefix in the next version
> >
> >>
> >>> +     {}
> >>> +};
> >>> +MODULE_DEVICE_TABLE(auxiliary, riscv_iommu_pmu_id_table);
> >>> +
> >>> +static struct auxiliary_driver iommu_pmu_driver = {
> >>> +     .probe          = riscv_iommu_pmu_probe,
> >>
> >> need ->remove() to clean up the resources like PMU and cpuhp instance
> >> on driver unbind. otherwise you can use devm_add_action_or_reset()
> >> to make them device managed.
> >
> > As suggested by Robin in my v1 version,  we might be inclined to just
> > leave out all the dead cleanup code if the PMU driver is tied to the
> > IOMMU driver and can never realistically be removed. So in my
> > subsequent versions, I didn't add any cleanup code.
> >
>
> ok, thanks for the background.
>
> then need to ensure there will be no possible removal from the
> userspace, e.g. at least set driver::suppress_bind_attrs to
> avoid manual unbind.
>
> in fact it's the auxiliary device tied to the iommu driver
> rather than this auxiliary driver. as only the pmu and
> cpuhp registration lack cleanups (you've already made
> other resources device managed), it'll be easy to do
> the cleanups and support this driver as a module. but it's
> up to you.
>
> thanks.
>
> >>
> >>> +     .id_table       = riscv_iommu_pmu_id_table,
> >>> +};
> >>> +
> >>> +static int __init riscv_iommu_pmu_init(void)
> >>> +{
> >>> +     int ret;
> >>> +
> >>> +     cpuhp_state = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN,
> >>> +                                           "perf/riscv/iommu:online",
> >>> +                                           riscv_iommu_pmu_online_cpu,
> >>> +                                           riscv_iommu_pmu_offline_cpu);
> >>> +     if (cpuhp_state < 0)
> >>> +             return cpuhp_state;
> >>> +
> >>> +     ret = auxiliary_driver_register(&iommu_pmu_driver);
> >>> +     if (ret)
> >>> +             cpuhp_remove_multi_state(cpuhp_state);
> >>> +
> >>> +     return ret;
> >>> +}
> >>> +module_init(riscv_iommu_pmu_init);
> >>
> >> it'll be easy to make it a module for debug and test.
> >>
> >> thanks.

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv