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);
}
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;
+
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);
--
2.17.1On 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.
> + 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,
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);
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
>
> > + 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);
>
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);
>>
On Fri, Jan 17, 2025 at 8:45 PM Robin Murphy <robin.murphy@arm.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@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)? ;)
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);
> >>
>
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
On Wed, Jan 15, 2025 at 5:44 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> 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.
Yes, I guess I messed that up. Thanks
>
> > }
> >
> > 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;
>
Yes, thanks for catching that.
> > +
> > 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:
> ...
>
Let me try to fix them in the next version if necessary. As Robin
mentioned, we might not need the HPM cleaner and can simply enable HPM
at the end. Thanks you for the review and catching these.
>
> Thanks,
> drew
© 2016 - 2025 Red Hat, Inc.