Re: [PATCH] Allocate DMAR fault interrupts locally

[Jacob Pan <jacob.jun.pan@linux.intel.com>]

Hi Thomas,

On Sun, 24 Mar 2024 22:05:35 +0100, Thomas Gleixner <tglx@linutronix.de>
wrote:

I sent out a tested patch based on your code, I made a few changes below.

> -- a/drivers/iommu/intel/dmar.c
> +++ b/drivers/iommu/intel/dmar.c
> @@ -1182,7 +1182,6 @@ static void free_iommu(struct intel_iomm
>  			iommu->pr_irq = 0;
>  		}
>  		free_irq(iommu->irq, iommu);
> -		dmar_free_hwirq(iommu->irq);
>  		iommu->irq = 0;
>  	}
>  
> @@ -1956,17 +1955,21 @@ void dmar_msi_mask(struct irq_data *data
>  	raw_spin_unlock_irqrestore(&iommu->register_lock, flag);
>  }
>  
> -void dmar_msi_write(int irq, struct msi_msg *msg)
> +static void dmar_iommu_msi_write(struct intel_iommu *iommu, struct
> msi_msg *msg) {
> -	struct intel_iommu *iommu = irq_get_handler_data(irq);
>  	int reg = dmar_msi_reg(iommu, irq);
> -	unsigned long flag;
> +	unsigned long flags;
>  
> -	raw_spin_lock_irqsave(&iommu->register_lock, flag);
> +	raw_spin_lock_irqsave(&iommu->register_lock, flags);
>  	writel(msg->data, iommu->reg + reg + 4);
>  	writel(msg->address_lo, iommu->reg + reg + 8);
>  	writel(msg->address_hi, iommu->reg + reg + 12);
> -	raw_spin_unlock_irqrestore(&iommu->register_lock, flag);
> +	raw_spin_unlock_irqrestore(&iommu->register_lock, flags);
> +}
> +
> +void dmar_msi_write(int irq, struct msi_msg *msg)
> +{
> +	dmar_iommu_msi_write(irq_get_handler_data(irq), msg);
page request and perfmon irqs also use this function, where irq !=
iommu->irq. In this case, fault irq got overwritten by other dmar irq
sources. So we need to pass in irq as well.

>  }
>  
>  void dmar_msi_read(int irq, struct msi_msg *msg)
> @@ -2100,23 +2103,37 @@ irqreturn_t dmar_fault(int irq, void *de
>  
>  int dmar_set_interrupt(struct intel_iommu *iommu)
>  {
> -	int irq, ret;
> +	static int dmar_irq;
> +	int ret;
>  
> -	/*
> -	 * Check if the fault interrupt is already initialized.
> -	 */
> +	/* Don't initialize it twice for a given iommu */
>  	if (iommu->irq)
>  		return 0;
>  
> -	irq = dmar_alloc_hwirq(iommu->seq_id, iommu->node, iommu);
> -	if (irq > 0) {
> -		iommu->irq = irq;
> +	/*
> +	 * There is one shared interrupt for all IOMMUs to prevent vector
> +	 * exhaustion.
> +	 */
> +	if (!dmar_irq) {
> +		int irq = dmar_alloc_hwirq(iommu->seq_id, iommu->node,
> iommu); +
> +		if (irq <= 0) {
> +			pr_err("No free IRQ vectors\n");
> +			return -EINVAL;
> +		}
> +		dmar_irq = irq;
Need to store the fault irq.
iommu->irq = irq

>  	} else {
> -		pr_err("No free IRQ vectors\n");
> -		return -EINVAL;
> +		struct msi_msg msg;
> +
> +		/*
> +		 * Get the MSI message from the shared interrupt and
> write
> +		 * it to the iommu MSI registers.
> +		 */
> +		dmar_msi_read(dmar_irq, &msg);
> +		dmar_iommu_msi_write(iommu, &msg);
>  	}
>  
> -	ret = request_irq(irq, dmar_fault, IRQF_NO_THREAD, iommu->name,
> iommu);
> +	ret = request_irq(dmar_irq, dmar_fault, IRQF_NO_THREAD |
> IRQF_SHARED, iommu->name, iommu); if (ret)
I added IRQF_NOBALANCING, ok? it makes things simple by not reprogramming
all the DMAR msi controls.

>  		pr_err("Can't request irq\n");
>  	return ret;


Thanks,

Jacob