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

Andrew Jones
2025-01-15T09:44:29Z

On Wed, Jan 15, 2025 at 11:03:06AM +0800, 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);
> + 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);

This should be

riscv_iommu_hpm_disable(iommu);

as the cover letter said it would be.

> }
>
> 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;

This should be

goto err_iodir_off;

And the next goto (under sysfs add) should be

goto err_hpm_disable;

> +
> 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);

This should be

err_remove_sysfs:
...
err_hpm_disable:
...
err_iodir_off:
...
err_queue_disable:
...

Thanks,
drew