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: <20250115-9d8193b7c67eeee41d18a7dd@orel>
Date: Wed, 15 Jan 2025 10:44:25 +0100
From: Andrew Jones <ajones@...tanamicro.com>
To: Zong Li <zong.li@...ive.com>
Cc: joro@...tes.org, will@...nel.org, robin.murphy@....com, 
	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 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@...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);
> +	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