Re: [PATCH 2/2] iommu/amd: Enable separate interrupt for PPR and GA log

[Jerry Snitselaar <jsnitsel@redhat.com>]

On Fri, Jun 09, 2023 at 10:20:25AM +0000, Vasant Hegde wrote:
> AMD IOMMU supports separate interrupt for event, ppr and ga log. It has
> separate interrupt control register for same. So far we were using
> single interrupt to handle all three interrupts.
> 
> Add separate interrupt for event, ppr and ga log. `hwirq` is
> set to INTCAPXT register offset. We will use hwirq to [un]mask irqs.
> 
> Also add support for irq naming. It will display proper name for irq
> (AMD-Vi<X>-[Evt/PPR/GA]) instead of generic name (AMD-Vi).
> 
> Note that this patch changes interrupt handling only in IOMMU x2apic mode
> (MMIO 0x18[IntCapXTEn]=1). In legacy mode it will continue to use single
> MSI interrupt.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> Reviewed-by: Alexey Kardashevskiy <aik@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu_types.h |  9 ++++++
>  drivers/iommu/amd/init.c            | 48 +++++++++++++++++++++--------
>  2 files changed, 45 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index a993e1bdb70b..65e18b590a54 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -704,12 +704,21 @@ struct amd_iommu {
>  	/* event buffer virtual address */
>  	u8 *evt_buf;
>  
> +	/* Name for event log interrupt */
> +	unsigned char evt_irq_name[16];
> +
>  	/* Base of the PPR log, if present */
>  	u8 *ppr_log;
>  
> +	/* Name for PPR log interrupt */
> +	unsigned char ppr_irq_name[16];
> +
>  	/* Base of the GA log, if present */
>  	u8 *ga_log;
>  
> +	/* Name for GA log interrupt */
> +	unsigned char ga_irq_name[16];
> +
>  	/* Tail of the GA log, if present */
>  	u8 *ga_log_tail;
>  
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 9908cd4f1c31..a174426da088 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -2331,6 +2331,7 @@ static int intcapxt_irqdomain_alloc(struct irq_domain *domain, unsigned int virq
>  		struct irq_data *irqd = irq_domain_get_irq_data(domain, i);
>  
>  		irqd->chip = &intcapxt_controller;
> +		irqd->hwirq = info->hwirq;
>  		irqd->chip_data = info->data;
>  		__irq_set_handler(i, handle_edge_irq, 0, "edge");
>  	}
> @@ -2357,22 +2358,14 @@ static void intcapxt_unmask_irq(struct irq_data *irqd)
>  	xt.destid_0_23 = cfg->dest_apicid & GENMASK(23, 0);
>  	xt.destid_24_31 = cfg->dest_apicid >> 24;
>  
> -	/**
> -	 * Current IOMMU implementation uses the same IRQ for all
> -	 * 3 IOMMU interrupts.
> -	 */
> -	writeq(xt.capxt, iommu->mmio_base + MMIO_INTCAPXT_EVT_OFFSET);
> -	writeq(xt.capxt, iommu->mmio_base + MMIO_INTCAPXT_PPR_OFFSET);
> -	writeq(xt.capxt, iommu->mmio_base + MMIO_INTCAPXT_GALOG_OFFSET);
> +	writeq(xt.capxt, iommu->mmio_base + irqd->hwirq);
>  }
>  
>  static void intcapxt_mask_irq(struct irq_data *irqd)
>  {
>  	struct amd_iommu *iommu = irqd->chip_data;
>  
> -	writeq(0, iommu->mmio_base + MMIO_INTCAPXT_EVT_OFFSET);
> -	writeq(0, iommu->mmio_base + MMIO_INTCAPXT_PPR_OFFSET);
> -	writeq(0, iommu->mmio_base + MMIO_INTCAPXT_GALOG_OFFSET);
> +	writeq(0, iommu->mmio_base + irqd->hwirq);
>  }
>  
>  
> @@ -2435,7 +2428,8 @@ static struct irq_domain *iommu_get_irqdomain(void)
>  	return iommu_irqdomain;
>  }
>  
> -static int iommu_setup_intcapxt(struct amd_iommu *iommu)
> +static int __iommu_setup_intcapxt(struct amd_iommu *iommu, const char *devname,
> +				  int hwirq, irq_handler_t thread_fn)
>  {
>  	struct irq_domain *domain;
>  	struct irq_alloc_info info;
> @@ -2449,6 +2443,7 @@ static int iommu_setup_intcapxt(struct amd_iommu *iommu)
>  	init_irq_alloc_info(&info, NULL);
>  	info.type = X86_IRQ_ALLOC_TYPE_AMDVI;
>  	info.data = iommu;
> +	info.hwirq = hwirq;
>  
>  	irq = irq_domain_alloc_irqs(domain, 1, node, &info);
>  	if (irq < 0) {
> @@ -2457,7 +2452,7 @@ static int iommu_setup_intcapxt(struct amd_iommu *iommu)
>  	}
>  
>  	ret = request_threaded_irq(irq, amd_iommu_int_handler,
> -				   amd_iommu_int_thread, 0, "AMD-Vi", iommu);
> +				   thread_fn, 0, devname, iommu);
>  	if (ret) {
>  		irq_domain_free_irqs(irq, 1);
>  		irq_domain_remove(domain);
> @@ -2467,6 +2462,35 @@ static int iommu_setup_intcapxt(struct amd_iommu *iommu)
>  	return 0;
>  }
>  
> +static int iommu_setup_intcapxt(struct amd_iommu *iommu)
> +{
> +	int ret;
> +
> +	snprintf(iommu->evt_irq_name, sizeof(iommu->evt_irq_name),
> +		 "AMD-Vi%d-Evt", iommu->index);
> +	ret = __iommu_setup_intcapxt(iommu, iommu->evt_irq_name,
> +				     MMIO_INTCAPXT_EVT_OFFSET,
> +				     amd_iommu_int_thread_evtlog);
> +	if (ret)
> +		return ret;
> +
> +	snprintf(iommu->ppr_irq_name, sizeof(iommu->ppr_irq_name),
> +		 "AMD-Vi%d-PPR", iommu->index);
> +	ret = __iommu_setup_intcapxt(iommu, iommu->ppr_irq_name,
> +				     MMIO_INTCAPXT_PPR_OFFSET,
> +				     amd_iommu_int_thread_pprlog);
> +	if (ret)
> +		return ret;
> +
> +	snprintf(iommu->ga_irq_name, sizeof(iommu->ga_irq_name),
> +		 "AMD-Vi%d-GA", iommu->index);
> +	ret = __iommu_setup_intcapxt(iommu, iommu->ga_irq_name,
> +				     MMIO_INTCAPXT_GALOG_OFFSET,
> +				     amd_iommu_int_thread_galog);
> +

Related to the question with the first patch, should this only
get set up if CONFIG_IRQ_REMAP is enabled?


> +	return ret;
> +}
> +
>  static int iommu_init_irq(struct amd_iommu *iommu)
>  {
>  	int ret;
> -- 
> 2.31.1
>