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

[Yicong Yang]

On 7/22/26 1:15 PM, Zong Li wrote:
> On Mon, Jul 20, 2026 at 3:29 PM Yicong Yang <yang.yicong@xxxxxxxxxxxxx> 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 <david.laight.linux@xxxxxxxxx>
>>> Suggested-by: Guo Ren <guoren@xxxxxxxxxx>
>>> Link: https://lore.kernel.org/linux-riscv/20260618143634.7f3dd6c5@pumpkin/
>>> Signed-off-by: Zong Li <zong.li@xxxxxxxxxx>
>>> ---
>>>  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.

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/20260714083625.1083606-1-zong.li@xxxxxxxxxx/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.