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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CANXhq0oqeTJfSsDy0VKhfdeOssO7JVtz=XjZcFiLCbgkxK4zqw@mail.gmail.com>
Date: Tue, 21 Jan 2025 16:37:23 +0800
From: Zong Li <zong.li@...ive.com>
To: Robin Murphy <robin.murphy@....com>
Cc: joro@...tes.org, will@...nel.org, tjeznach@...osinc.com, 
	paul.walmsley@...ive.com, palmer@...belt.com, aou@...s.berkeley.edu, 
	luxu.kernel@...edance.com, linux-kernel@...r.kernel.org, 
	iommu@...ts.linux.dev, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH 2/2] iommu/riscv: support HPM and interrupt handling

On Fri, Jan 17, 2025 at 8:45 PM Robin Murphy <robin.murphy@....com> wrote:
>
> On 2025-01-17 2:46 am, Zong Li wrote:
> > On Thu, Jan 16, 2025 at 5:56 AM Robin Murphy <robin.murphy@....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@...ive.com>
> >>> Tested-by: Xu Lu <luxu.kernel@...edance.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)? ;)

Sorry for the misunderstanding earlier; I understand now. I think I
might need to exchange the IRQ request and PMU initialization,
ensuring that the PMU instance is initialized before a PMU interrupt
arrives. Thanks!

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