Re: [PATCH 2/2] iommu/riscv: support HPM and interrupt handling

Robin Murphy
2025-01-17T12:45:14Z

On 2025-01-17 2:46 am, Zong Li wrote:
> On Thu, Jan 16, 2025 at 5:56 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2025-01-15 3:03 am, Zong Li wrote:
>>> Initialize the PMU and uninitialize it when driver is removed.
>>> Interrupt handling is also implemented, and the handler needs
>>> to be a primary handler instead of a threaded function because
>>> pt_regs is empty when threading the IRQ. However, pt_regs is
>>> required by perf_event_overflow.
>>>
>>> Signed-off-by: Zong Li <zong.li@sifive.com>
>>> Tested-by: Xu Lu <luxu.kernel@bytedance.com>
>>> ---
>>> drivers/iommu/riscv/iommu.c | 65 +++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 65 insertions(+)
>>>
>>> diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
>>> index 8a05def774bd..20ae90471484 100644
>>> --- a/drivers/iommu/riscv/iommu.c
>>> +++ b/drivers/iommu/riscv/iommu.c
>>> @@ -552,6 +552,62 @@ static irqreturn_t riscv_iommu_fltq_process(int irq, void *data)
>>> return IRQ_HANDLED;
>>> }
>>>
>>> +/*
>>> + * IOMMU Hardware performance monitor
>>> + */
>>> +
>>> +/* HPM interrupt primary handler */
>>> +static irqreturn_t riscv_iommu_hpm_irq_handler(int irq, void *dev_id)
>>> +{
>>> + struct riscv_iommu_device *iommu = (struct riscv_iommu_device *)dev_id;
>>> +
>>> + /* Clear performance monitoring interrupt pending */
>>> + riscv_iommu_writel(iommu, RISCV_IOMMU_REG_IPSR, RISCV_IOMMU_IPSR_PMIP);
>>> +
>>> + /* Process pmu irq */
>>> + riscv_iommu_pmu_handle_irq(&iommu->pmu);
>>> +
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +/* HPM initialization */
>>> +static int riscv_iommu_hpm_enable(struct riscv_iommu_device *iommu)
>>> +{
>>> + int rc;
>>> +
>>> + if (!(iommu->caps & RISCV_IOMMU_CAPABILITIES_HPM))
>>> + return 0;
>>> +
>>> + /*
>>> + * pt_regs is empty when threading the IRQ, but pt_regs is necessary
>>> + * by perf_event_overflow. Use primary handler instead of thread
>>> + * function for PM IRQ.
>>> + *
>>> + * Set the IRQF_ONESHOT flag because this IRQ might be shared with
>>> + * other threaded IRQs by other queues.
>>> + */
>>> + rc = devm_request_irq(iommu->dev,
>>> + iommu->irqs[riscv_iommu_queue_vec(iommu, RISCV_IOMMU_IPSR_PMIP)],
>>> + riscv_iommu_hpm_irq_handler, IRQF_ONESHOT | IRQF_SHARED, NULL, iommu);
>>
>> Hmm, shared interrupts are tricky for PMUs, since perf requires any IRQ
>> handler touching a PMU is running on pmu->cpu, so you have to be very
>> careful about maintaining affinity and not letting anyone else change it
>> behind your back.
>>
>> The other thing is that if it really is shared, at this point you could
>> now be in riscv_iommu_pmu_handle_irq() dereferencing NULL.
>
> Yes, the PMU IRQ line could be shared with the command queue, fault
> queue, and page-request queue.
> Could you please provide more tips on what is meant by "dereferencing
> NULL in riscv_iommu_pmu_handle_irq()"?
> I don't complete understand what needs to be done there.
> Thanks

In general it's not a great idea to register an IRQ handler before the
data passed to that handler is initialised. What is pointed to by
(&iommu->pmu)->reg + RISCV_IOMMU_REG_IOCOUNTOVF if the IRQ fires right
now (and/or if CONFIG_DEBUG_SHIRQ ever gets fixed)? ;)

(OK, it's not *literally* NULL, but hey...)

Thanks,
Robin.

>>
>>> + if (rc)
>>> + return rc;
>>> +
>>> + return riscv_iommu_pmu_init(&iommu->pmu, iommu->reg, dev_name(iommu->dev));
>>> +}
>>> +
>>> +/* HPM uninitialization */
>>> +static void riscv_iommu_hpm_disable(struct riscv_iommu_device *iommu)
>>> +{
>>> + if (!(iommu->caps & RISCV_IOMMU_CAPABILITIES_HPM))
>>> + return;
>>> +
>>> + devm_free_irq(iommu->dev,
>>> + iommu->irqs[riscv_iommu_queue_vec(iommu, RISCV_IOMMU_IPSR_PMIP)],
>>> + iommu);
>>> +
>>> + riscv_iommu_pmu_uninit(&iommu->pmu);
>>> +}
>>> +
>>> /* Lookup and initialize device context info structure. */
>>> static struct riscv_iommu_dc *riscv_iommu_get_dc(struct riscv_iommu_device *iommu,
>>> unsigned int devid)
>>> @@ -1596,6 +1652,9 @@ void riscv_iommu_remove(struct riscv_iommu_device *iommu)
>>> riscv_iommu_iodir_set_mode(iommu, RISCV_IOMMU_DDTP_IOMMU_MODE_OFF);
>>> riscv_iommu_queue_disable(&iommu->cmdq);
>>> riscv_iommu_queue_disable(&iommu->fltq);
>>> +
>>> + if (iommu->caps & RISCV_IOMMU_CAPABILITIES_HPM)
>>> + riscv_iommu_pmu_uninit(&iommu->pmu);
>>> }
>>>
>>> int riscv_iommu_init(struct riscv_iommu_device *iommu)
>>> @@ -1635,6 +1694,10 @@ int riscv_iommu_init(struct riscv_iommu_device *iommu)
>>> if (rc)
>>> goto err_queue_disable;
>>>
>>> + rc = riscv_iommu_hpm_enable(iommu);
>>> + if (rc)
>>> + goto err_hpm_disable;
>>> +
>>
>> I would leave this until after the whole IOMMU setup has succeeded. The
>> PMU is not critical to IOMMU operation, so at that point an error is not
>> fatal, it just means you don't get a PMU, thus there shouldn't need to
>> be any cleanup outside riscv_iommu_hpm_enable() itself.
>
> Thanks for pointing this out. PMU failure shouldn't cause the entire
> IOMMU to fail, let me modify it in the next version.
>
>>
>> Thanks,
>> Robin.
>>
>>> rc = iommu_device_sysfs_add(&iommu->iommu, NULL, NULL, "riscv-iommu@%s",
>>> dev_name(iommu->dev));
>>> if (rc) {
>>> @@ -1653,6 +1716,8 @@ int riscv_iommu_init(struct riscv_iommu_device *iommu)
>>> err_remove_sysfs:
>>> iommu_device_sysfs_remove(&iommu->iommu);
>>> err_iodir_off:
>>> + riscv_iommu_hpm_disable(iommu);
>>> +err_hpm_disable:
>>> riscv_iommu_iodir_set_mode(iommu, RISCV_IOMMU_DDTP_IOMMU_MODE_OFF);
>>> err_queue_disable:
>>> riscv_iommu_queue_disable(&iommu->fltq);
>>