[PATCH 0/2] iommu/amd: Interrupt handling improvements

[PATCH 0/2] iommu/amd: Interrupt handling improvements

[Vasant Hegde]

AMD IOMMU has three different logs/interrupts (Event, PPR and GA).
Currently, the IOMMU driver consolidates interrupts for all three logs to
a single interrupt. This could become a bottleneck when enable GA and PPR
logs simultaneously. Hence separate these interrupt to prevent potential
performance issue.

This patch applies on top of cleanup series:
  https://lore.kernel.org/linux-iommu/20230609090631.6052-1-vasant.hegde@amd.com/T/#t

Vasant Hegde (2):
  iommu/amd: Add separate interrupt handler for PPR and GA log
  iommu/amd: Enable separate interrupt for PPR and GA log

 drivers/iommu/amd/amd_iommu.h       |  3 +
 drivers/iommu/amd/amd_iommu_types.h |  9 +++
 drivers/iommu/amd/init.c            | 48 ++++++++++----
 drivers/iommu/amd/iommu.c           | 98 ++++++++++++++++-------------
 4 files changed, 102 insertions(+), 56 deletions(-)

-- 
2.31.1

[PATCH 1/2] iommu/amd: Add separate interrupt handler for PPR and GA log

[Vasant Hegde]

The AMD IOMMU has three different logs (Event, PPR and GA) and it can be
configured to send separate interrupt for each log type.
  - Event log is used whenever IOMMU reports events like IO_PAGE_FAULT,
    TLB_INV_TIMEOUT, etc,. During normal system operation this log is not
    used actively.

  - GA log is used to record device interrupt requests that could not be
    immediately delivered to the target virtual processor due the fact the
    target was not running. This is actively used when we do device
    passthrough to AVIC enabled guest.

  - PPR log is used to service the page fault request from device in Shared
    Virtual Addressing (SVA) mode where page table is shared by CPU and
    device. In this mode it will generate PPR interrupt frequently.

Currently we have single interrupt to handle all three logs. GA log and
PPR log usage is increasing. Hence, split interrupt handler thread
into three separate interrupt handler function. Following patch enables
separate interrupt for PPR and GA Log.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h |  3 ++
 drivers/iommu/amd/iommu.c     | 98 +++++++++++++++++++----------------
 2 files changed, 57 insertions(+), 44 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 156f57b4f78c..e2857109e966 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -12,6 +12,9 @@
 #include "amd_iommu_types.h"
 
 irqreturn_t amd_iommu_int_thread(int irq, void *data);
+irqreturn_t amd_iommu_int_thread_evtlog(int irq, void *data);
+irqreturn_t amd_iommu_int_thread_pprlog(int irq, void *data);
+irqreturn_t amd_iommu_int_thread_galog(int irq, void *data);
 irqreturn_t amd_iommu_int_handler(int irq, void *data);
 void amd_iommu_apply_erratum_63(struct amd_iommu *iommu, u16 devid);
 void amd_iommu_restart_event_logging(struct amd_iommu *iommu);
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 3c179d548ecd..d427f7e3b869 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -841,57 +841,23 @@ static inline void
 amd_iommu_set_pci_msi_domain(struct device *dev, struct amd_iommu *iommu) { }
 #endif /* !CONFIG_IRQ_REMAP */
 
-#define AMD_IOMMU_INT_MASK	\
-	(MMIO_STATUS_EVT_OVERFLOW_MASK | \
-	 MMIO_STATUS_EVT_INT_MASK | \
-	 MMIO_STATUS_PPR_OVERFLOW_MASK | \
-	 MMIO_STATUS_PPR_INT_MASK | \
-	 MMIO_STATUS_GALOG_OVERFLOW_MASK | \
-	 MMIO_STATUS_GALOG_INT_MASK)
-
-irqreturn_t amd_iommu_int_thread(int irq, void *data)
+static void amd_iommu_handle_irq(void *data, u32 int_mask, u32 overflow_mask,
+				 void (*int_handler)(struct amd_iommu *),
+				 void (*overflow_handler)(struct amd_iommu *))
 {
 	struct amd_iommu *iommu = (struct amd_iommu *) data;
 	u32 status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
+	u32 mask = int_mask | overflow_mask;
 
-	while (status & AMD_IOMMU_INT_MASK) {
+	while (status & mask) {
 		/* Enable interrupt sources again */
-		writel(AMD_IOMMU_INT_MASK,
-			iommu->mmio_base + MMIO_STATUS_OFFSET);
-
-		if (status & MMIO_STATUS_EVT_INT_MASK) {
-			pr_devel("Processing IOMMU Event Log\n");
-			iommu_poll_events(iommu);
-		}
-
-		if (status & (MMIO_STATUS_PPR_INT_MASK |
-			      MMIO_STATUS_PPR_OVERFLOW_MASK)) {
-			pr_devel("Processing IOMMU PPR Log\n");
-			iommu_poll_ppr_log(iommu);
-		}
+		writel(mask, iommu->mmio_base + MMIO_STATUS_OFFSET);
 
-		if (status & MMIO_STATUS_PPR_OVERFLOW_MASK) {
-			pr_info_ratelimited("IOMMU PPR log overflow\n");
-			amd_iommu_restart_ppr_log(iommu);
-		}
+		if (int_handler)
+			int_handler(iommu);
 
-#ifdef CONFIG_IRQ_REMAP
-		if (status & (MMIO_STATUS_GALOG_INT_MASK |
-			      MMIO_STATUS_GALOG_OVERFLOW_MASK)) {
-			pr_devel("Processing IOMMU GA Log\n");
-			iommu_poll_ga_log(iommu);
-		}
-
-		if (status & MMIO_STATUS_GALOG_OVERFLOW_MASK) {
-			pr_info_ratelimited("IOMMU GA Log overflow\n");
-			amd_iommu_restart_ga_log(iommu);
-		}
-#endif
-
-		if (status & MMIO_STATUS_EVT_OVERFLOW_MASK) {
-			pr_info_ratelimited("IOMMU event log overflow\n");
-			amd_iommu_restart_event_logging(iommu);
-		}
+		if ((status & overflow_mask) && overflow_handler)
+			overflow_handler(iommu);
 
 		/*
 		 * Hardware bug: ERBT1312
@@ -908,6 +874,50 @@ irqreturn_t amd_iommu_int_thread(int irq, void *data)
 		 */
 		status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
 	}
+}
+
+irqreturn_t amd_iommu_int_thread_evtlog(int irq, void *data)
+{
+	pr_devel("Processing IOMMU Event Log\n");
+	amd_iommu_handle_irq(data, MMIO_STATUS_EVT_INT_MASK,
+			     MMIO_STATUS_EVT_OVERFLOW_MASK,
+			     iommu_poll_events, amd_iommu_restart_event_logging);
+
+	return IRQ_HANDLED;
+}
+
+irqreturn_t amd_iommu_int_thread_pprlog(int irq, void *data)
+{
+	pr_devel("Processing IOMMU PPR Log\n");
+	amd_iommu_handle_irq(data, MMIO_STATUS_PPR_INT_MASK,
+			     MMIO_STATUS_PPR_OVERFLOW_MASK,
+			     iommu_poll_ppr_log, amd_iommu_restart_ppr_log);
+
+	return IRQ_HANDLED;
+}
+
+irqreturn_t amd_iommu_int_thread_galog(int irq, void *data)
+{
+
+	pr_devel("Processing IOMMU GA Log\n");
+#ifdef CONFIG_IRQ_REMAP
+	amd_iommu_handle_irq(data, MMIO_STATUS_GALOG_INT_MASK,
+			     MMIO_STATUS_GALOG_OVERFLOW_MASK,
+			     iommu_poll_ga_log, amd_iommu_restart_ga_log);
+#else
+	amd_iommu_handle_irq(data, MMIO_STATUS_GALOG_INT_MASK,
+			     MMIO_STATUS_GALOG_OVERFLOW_MASK, NULL, NULL);
+#endif
+
+	return IRQ_HANDLED;
+}
+
+irqreturn_t amd_iommu_int_thread(int irq, void *data)
+{
+	amd_iommu_int_thread_evtlog(irq, data);
+	amd_iommu_int_thread_pprlog(irq, data);
+	amd_iommu_int_thread_galog(irq, data);
+
 	return IRQ_HANDLED;
 }
 
-- 
2.31.1

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

[Vasant Hegde]

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);
+
+	return ret;
+}
+
 static int iommu_init_irq(struct amd_iommu *iommu)
 {
 	int ret;
-- 
2.31.1

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

[Jerry Snitselaar]

On Fri, Jun 09, 2023 at 10:20:24AM +0000, Vasant Hegde wrote:
> The AMD IOMMU has three different logs (Event, PPR and GA) and it can be
> configured to send separate interrupt for each log type.
>   - Event log is used whenever IOMMU reports events like IO_PAGE_FAULT,
>     TLB_INV_TIMEOUT, etc,. During normal system operation this log is not
>     used actively.
> 
>   - GA log is used to record device interrupt requests that could not be
>     immediately delivered to the target virtual processor due the fact the
>     target was not running. This is actively used when we do device
>     passthrough to AVIC enabled guest.
> 
>   - PPR log is used to service the page fault request from device in Shared
>     Virtual Addressing (SVA) mode where page table is shared by CPU and
>     device. In this mode it will generate PPR interrupt frequently.
> 
> Currently we have single interrupt to handle all three logs. GA log and
> PPR log usage is increasing. Hence, split interrupt handler thread
> into three separate interrupt handler function. Following patch enables
> separate interrupt for PPR and GA Log.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu.h |  3 ++
>  drivers/iommu/amd/iommu.c     | 98 +++++++++++++++++++----------------
>  2 files changed, 57 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
> index 156f57b4f78c..e2857109e966 100644
> --- a/drivers/iommu/amd/amd_iommu.h
> +++ b/drivers/iommu/amd/amd_iommu.h
> @@ -12,6 +12,9 @@
>  #include "amd_iommu_types.h"
>  
>  irqreturn_t amd_iommu_int_thread(int irq, void *data);
> +irqreturn_t amd_iommu_int_thread_evtlog(int irq, void *data);
> +irqreturn_t amd_iommu_int_thread_pprlog(int irq, void *data);
> +irqreturn_t amd_iommu_int_thread_galog(int irq, void *data);
>  irqreturn_t amd_iommu_int_handler(int irq, void *data);
>  void amd_iommu_apply_erratum_63(struct amd_iommu *iommu, u16 devid);
>  void amd_iommu_restart_event_logging(struct amd_iommu *iommu);
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 3c179d548ecd..d427f7e3b869 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -841,57 +841,23 @@ static inline void
>  amd_iommu_set_pci_msi_domain(struct device *dev, struct amd_iommu *iommu) { }
>  #endif /* !CONFIG_IRQ_REMAP */
>  
> -#define AMD_IOMMU_INT_MASK	\
> -	(MMIO_STATUS_EVT_OVERFLOW_MASK | \
> -	 MMIO_STATUS_EVT_INT_MASK | \
> -	 MMIO_STATUS_PPR_OVERFLOW_MASK | \
> -	 MMIO_STATUS_PPR_INT_MASK | \
> -	 MMIO_STATUS_GALOG_OVERFLOW_MASK | \
> -	 MMIO_STATUS_GALOG_INT_MASK)
> -
> -irqreturn_t amd_iommu_int_thread(int irq, void *data)
> +static void amd_iommu_handle_irq(void *data, u32 int_mask, u32 overflow_mask,
> +				 void (*int_handler)(struct amd_iommu *),
> +				 void (*overflow_handler)(struct amd_iommu *))
>  {
>  	struct amd_iommu *iommu = (struct amd_iommu *) data;
>  	u32 status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
> +	u32 mask = int_mask | overflow_mask;
>  
> -	while (status & AMD_IOMMU_INT_MASK) {
> +	while (status & mask) {
>  		/* Enable interrupt sources again */
> -		writel(AMD_IOMMU_INT_MASK,
> -			iommu->mmio_base + MMIO_STATUS_OFFSET);
> -
> -		if (status & MMIO_STATUS_EVT_INT_MASK) {
> -			pr_devel("Processing IOMMU Event Log\n");
> -			iommu_poll_events(iommu);
> -		}
> -
> -		if (status & (MMIO_STATUS_PPR_INT_MASK |
> -			      MMIO_STATUS_PPR_OVERFLOW_MASK)) {
> -			pr_devel("Processing IOMMU PPR Log\n");
> -			iommu_poll_ppr_log(iommu);
> -		}
> +		writel(mask, iommu->mmio_base + MMIO_STATUS_OFFSET);
>  
> -		if (status & MMIO_STATUS_PPR_OVERFLOW_MASK) {
> -			pr_info_ratelimited("IOMMU PPR log overflow\n");
> -			amd_iommu_restart_ppr_log(iommu);
> -		}
> +		if (int_handler)
> +			int_handler(iommu);
>  
> -#ifdef CONFIG_IRQ_REMAP
> -		if (status & (MMIO_STATUS_GALOG_INT_MASK |
> -			      MMIO_STATUS_GALOG_OVERFLOW_MASK)) {
> -			pr_devel("Processing IOMMU GA Log\n");
> -			iommu_poll_ga_log(iommu);
> -		}
> -
> -		if (status & MMIO_STATUS_GALOG_OVERFLOW_MASK) {
> -			pr_info_ratelimited("IOMMU GA Log overflow\n");
> -			amd_iommu_restart_ga_log(iommu);
> -		}
> -#endif
> -
> -		if (status & MMIO_STATUS_EVT_OVERFLOW_MASK) {
> -			pr_info_ratelimited("IOMMU event log overflow\n");
> -			amd_iommu_restart_event_logging(iommu);
> -		}
> +		if ((status & overflow_mask) && overflow_handler)
> +			overflow_handler(iommu);
>  
>  		/*
>  		 * Hardware bug: ERBT1312
> @@ -908,6 +874,50 @@ irqreturn_t amd_iommu_int_thread(int irq, void *data)
>  		 */
>  		status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
>  	}
> +}
> +
> +irqreturn_t amd_iommu_int_thread_evtlog(int irq, void *data)
> +{
> +	pr_devel("Processing IOMMU Event Log\n");
> +	amd_iommu_handle_irq(data, MMIO_STATUS_EVT_INT_MASK,
> +			     MMIO_STATUS_EVT_OVERFLOW_MASK,
> +			     iommu_poll_events, amd_iommu_restart_event_logging);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +irqreturn_t amd_iommu_int_thread_pprlog(int irq, void *data)
> +{
> +	pr_devel("Processing IOMMU PPR Log\n");
> +	amd_iommu_handle_irq(data, MMIO_STATUS_PPR_INT_MASK,
> +			     MMIO_STATUS_PPR_OVERFLOW_MASK,
> +			     iommu_poll_ppr_log, amd_iommu_restart_ppr_log);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +irqreturn_t amd_iommu_int_thread_galog(int irq, void *data)
> +{
> +
> +	pr_devel("Processing IOMMU GA Log\n");
> +#ifdef CONFIG_IRQ_REMAP
> +	amd_iommu_handle_irq(data, MMIO_STATUS_GALOG_INT_MASK,
> +			     MMIO_STATUS_GALOG_OVERFLOW_MASK,
> +			     iommu_poll_ga_log, amd_iommu_restart_ga_log);
> +#else
> +	amd_iommu_handle_irq(data, MMIO_STATUS_GALOG_INT_MASK,
> +			     MMIO_STATUS_GALOG_OVERFLOW_MASK, NULL, NULL);
> +#endif

Is the else needed here, since GAEn will only get set if CONFIG_IRQ_REMAP is enabled?

Regards,
Jerry

> +
> +	return IRQ_HANDLED;
> +}
> +
> +irqreturn_t amd_iommu_int_thread(int irq, void *data)
> +{
> +	amd_iommu_int_thread_evtlog(irq, data);
> +	amd_iommu_int_thread_pprlog(irq, data);
> +	amd_iommu_int_thread_galog(irq, data);
> +
>  	return IRQ_HANDLED;
>  }
>  
> -- 
> 2.31.1
> 

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

[Vasant Hegde]

Hi Jerry,


On 6/14/2023 3:08 AM, Jerry Snitselaar wrote:
> On Fri, Jun 09, 2023 at 10:20:24AM +0000, Vasant Hegde wrote:
>> The AMD IOMMU has three different logs (Event, PPR and GA) and it can be
>> configured to send separate interrupt for each log type.
>>   - Event log is used whenever IOMMU reports events like IO_PAGE_FAULT,
>>     TLB_INV_TIMEOUT, etc,. During normal system operation this log is not
>>     used actively.
>>
>>   - GA log is used to record device interrupt requests that could not be
>>     immediately delivered to the target virtual processor due the fact the
>>     target was not running. This is actively used when we do device
>>     passthrough to AVIC enabled guest.
>>
>>   - PPR log is used to service the page fault request from device in Shared
>>     Virtual Addressing (SVA) mode where page table is shared by CPU and
>>     device. In this mode it will generate PPR interrupt frequently.
>>
>> Currently we have single interrupt to handle all three logs. GA log and
>> PPR log usage is increasing. Hence, split interrupt handler thread
>> into three separate interrupt handler function. Following patch enables
>> separate interrupt for PPR and GA Log.
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/amd_iommu.h |  3 ++
>>  drivers/iommu/amd/iommu.c     | 98 +++++++++++++++++++----------------
>>  2 files changed, 57 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
>> index 156f57b4f78c..e2857109e966 100644
>> --- a/drivers/iommu/amd/amd_iommu.h
>> +++ b/drivers/iommu/amd/amd_iommu.h
>> @@ -12,6 +12,9 @@
>>  #include "amd_iommu_types.h"
>>  
>>  irqreturn_t amd_iommu_int_thread(int irq, void *data);
>> +irqreturn_t amd_iommu_int_thread_evtlog(int irq, void *data);
>> +irqreturn_t amd_iommu_int_thread_pprlog(int irq, void *data);
>> +irqreturn_t amd_iommu_int_thread_galog(int irq, void *data);
>>  irqreturn_t amd_iommu_int_handler(int irq, void *data);
>>  void amd_iommu_apply_erratum_63(struct amd_iommu *iommu, u16 devid);
>>  void amd_iommu_restart_event_logging(struct amd_iommu *iommu);
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 3c179d548ecd..d427f7e3b869 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -841,57 +841,23 @@ static inline void
>>  amd_iommu_set_pci_msi_domain(struct device *dev, struct amd_iommu *iommu) { }
>>  #endif /* !CONFIG_IRQ_REMAP */
>>  
>> -#define AMD_IOMMU_INT_MASK	\
>> -	(MMIO_STATUS_EVT_OVERFLOW_MASK | \
>> -	 MMIO_STATUS_EVT_INT_MASK | \
>> -	 MMIO_STATUS_PPR_OVERFLOW_MASK | \
>> -	 MMIO_STATUS_PPR_INT_MASK | \
>> -	 MMIO_STATUS_GALOG_OVERFLOW_MASK | \
>> -	 MMIO_STATUS_GALOG_INT_MASK)
>> -
>> -irqreturn_t amd_iommu_int_thread(int irq, void *data)
>> +static void amd_iommu_handle_irq(void *data, u32 int_mask, u32 overflow_mask,
>> +				 void (*int_handler)(struct amd_iommu *),
>> +				 void (*overflow_handler)(struct amd_iommu *))
>>  {
>>  	struct amd_iommu *iommu = (struct amd_iommu *) data;
>>  	u32 status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
>> +	u32 mask = int_mask | overflow_mask;
>>  
>> -	while (status & AMD_IOMMU_INT_MASK) {
>> +	while (status & mask) {
>>  		/* Enable interrupt sources again */
>> -		writel(AMD_IOMMU_INT_MASK,
>> -			iommu->mmio_base + MMIO_STATUS_OFFSET);
>> -
>> -		if (status & MMIO_STATUS_EVT_INT_MASK) {
>> -			pr_devel("Processing IOMMU Event Log\n");
>> -			iommu_poll_events(iommu);
>> -		}
>> -
>> -		if (status & (MMIO_STATUS_PPR_INT_MASK |
>> -			      MMIO_STATUS_PPR_OVERFLOW_MASK)) {
>> -			pr_devel("Processing IOMMU PPR Log\n");
>> -			iommu_poll_ppr_log(iommu);
>> -		}
>> +		writel(mask, iommu->mmio_base + MMIO_STATUS_OFFSET);
>>  
>> -		if (status & MMIO_STATUS_PPR_OVERFLOW_MASK) {
>> -			pr_info_ratelimited("IOMMU PPR log overflow\n");
>> -			amd_iommu_restart_ppr_log(iommu);
>> -		}
>> +		if (int_handler)
>> +			int_handler(iommu);
>>  
>> -#ifdef CONFIG_IRQ_REMAP
>> -		if (status & (MMIO_STATUS_GALOG_INT_MASK |
>> -			      MMIO_STATUS_GALOG_OVERFLOW_MASK)) {
>> -			pr_devel("Processing IOMMU GA Log\n");
>> -			iommu_poll_ga_log(iommu);
>> -		}
>> -
>> -		if (status & MMIO_STATUS_GALOG_OVERFLOW_MASK) {
>> -			pr_info_ratelimited("IOMMU GA Log overflow\n");
>> -			amd_iommu_restart_ga_log(iommu);
>> -		}
>> -#endif
>> -
>> -		if (status & MMIO_STATUS_EVT_OVERFLOW_MASK) {
>> -			pr_info_ratelimited("IOMMU event log overflow\n");
>> -			amd_iommu_restart_event_logging(iommu);
>> -		}
>> +		if ((status & overflow_mask) && overflow_handler)
>> +			overflow_handler(iommu);
>>  
>>  		/*
>>  		 * Hardware bug: ERBT1312
>> @@ -908,6 +874,50 @@ irqreturn_t amd_iommu_int_thread(int irq, void *data)
>>  		 */
>>  		status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
>>  	}
>> +}
>> +
>> +irqreturn_t amd_iommu_int_thread_evtlog(int irq, void *data)
>> +{
>> +	pr_devel("Processing IOMMU Event Log\n");
>> +	amd_iommu_handle_irq(data, MMIO_STATUS_EVT_INT_MASK,
>> +			     MMIO_STATUS_EVT_OVERFLOW_MASK,
>> +			     iommu_poll_events, amd_iommu_restart_event_logging);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +irqreturn_t amd_iommu_int_thread_pprlog(int irq, void *data)
>> +{
>> +	pr_devel("Processing IOMMU PPR Log\n");
>> +	amd_iommu_handle_irq(data, MMIO_STATUS_PPR_INT_MASK,
>> +			     MMIO_STATUS_PPR_OVERFLOW_MASK,
>> +			     iommu_poll_ppr_log, amd_iommu_restart_ppr_log);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +irqreturn_t amd_iommu_int_thread_galog(int irq, void *data)
>> +{
>> +
>> +	pr_devel("Processing IOMMU GA Log\n");
>> +#ifdef CONFIG_IRQ_REMAP
>> +	amd_iommu_handle_irq(data, MMIO_STATUS_GALOG_INT_MASK,
>> +			     MMIO_STATUS_GALOG_OVERFLOW_MASK,
>> +			     iommu_poll_ga_log, amd_iommu_restart_ga_log);
>> +#else
>> +	amd_iommu_handle_irq(data, MMIO_STATUS_GALOG_INT_MASK,
>> +			     MMIO_STATUS_GALOG_OVERFLOW_MASK, NULL, NULL);
>> +#endif
> 
> Is the else needed here, since GAEn will only get set if CONFIG_IRQ_REMAP is enabled?

In the current flow we clear the the interrupt bits (including GALOG_INT and
GALOG_OVERFLOW) first and then if IRQ_REMAP is enabled then we process GA log.
To match that flow I have added else part.

But you are right. We enable GALOG only when IRQ_REMAP is enabled.
Probably I can drop else part.


-Vasant

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

[Jerry Snitselaar]

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
> 

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

[Vasant Hegde]

Jerry,


On 6/14/2023 10:48 PM, Jerry Snitselaar wrote:
> 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;


.../...

>> +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?

Ack. Will fix it in v2.

Thanks
-Vasant

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

[Joao Martins]

On 09/06/2023 11:20, Vasant Hegde wrote:
> The AMD IOMMU has three different logs (Event, PPR and GA) and it can be
> configured to send separate interrupt for each log type.
>   - Event log is used whenever IOMMU reports events like IO_PAGE_FAULT,
>     TLB_INV_TIMEOUT, etc,. During normal system operation this log is not
>     used actively.
> 
>   - GA log is used to record device interrupt requests that could not be
>     immediately delivered to the target virtual processor due the fact the
>     target was not running. This is actively used when we do device
>     passthrough to AVIC enabled guest.
> 
>   - PPR log is used to service the page fault request from device in Shared
>     Virtual Addressing (SVA) mode where page table is shared by CPU and
>     device. In this mode it will generate PPR interrupt frequently.
> 
> Currently we have single interrupt to handle all three logs. GA log and
> PPR log usage is increasing. Hence, split interrupt handler thread
> into three separate interrupt handler function. Following patch enables
> separate interrupt for PPR and GA Log.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu.h |  3 ++
>  drivers/iommu/amd/iommu.c     | 98 +++++++++++++++++++----------------
>  2 files changed, 57 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
> index 156f57b4f78c..e2857109e966 100644
> --- a/drivers/iommu/amd/amd_iommu.h
> +++ b/drivers/iommu/amd/amd_iommu.h
> @@ -12,6 +12,9 @@
>  #include "amd_iommu_types.h"
>  
>  irqreturn_t amd_iommu_int_thread(int irq, void *data);
> +irqreturn_t amd_iommu_int_thread_evtlog(int irq, void *data);
> +irqreturn_t amd_iommu_int_thread_pprlog(int irq, void *data);
> +irqreturn_t amd_iommu_int_thread_galog(int irq, void *data);
>  irqreturn_t amd_iommu_int_handler(int irq, void *data);
>  void amd_iommu_apply_erratum_63(struct amd_iommu *iommu, u16 devid);
>  void amd_iommu_restart_event_logging(struct amd_iommu *iommu);
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 3c179d548ecd..d427f7e3b869 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -841,57 +841,23 @@ static inline void
>  amd_iommu_set_pci_msi_domain(struct device *dev, struct amd_iommu *iommu) { }
>  #endif /* !CONFIG_IRQ_REMAP */
>  
> -#define AMD_IOMMU_INT_MASK	\
> -	(MMIO_STATUS_EVT_OVERFLOW_MASK | \
> -	 MMIO_STATUS_EVT_INT_MASK | \
> -	 MMIO_STATUS_PPR_OVERFLOW_MASK | \
> -	 MMIO_STATUS_PPR_INT_MASK | \
> -	 MMIO_STATUS_GALOG_OVERFLOW_MASK | \
> -	 MMIO_STATUS_GALOG_INT_MASK)
> -
> -irqreturn_t amd_iommu_int_thread(int irq, void *data)
> +static void amd_iommu_handle_irq(void *data, u32 int_mask, u32 overflow_mask,
> +				 void (*int_handler)(struct amd_iommu *),
> +				 void (*overflow_handler)(struct amd_iommu *))
>  {
>  	struct amd_iommu *iommu = (struct amd_iommu *) data;
>  	u32 status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
> +	u32 mask = int_mask | overflow_mask;
>  
> -	while (status & AMD_IOMMU_INT_MASK) {
> +	while (status & mask) {
>  		/* Enable interrupt sources again */
> -		writel(AMD_IOMMU_INT_MASK,
> -			iommu->mmio_base + MMIO_STATUS_OFFSET);
> -
> -		if (status & MMIO_STATUS_EVT_INT_MASK) {
> -			pr_devel("Processing IOMMU Event Log\n");
> -			iommu_poll_events(iommu);
> -		}
> -
> -		if (status & (MMIO_STATUS_PPR_INT_MASK |
> -			      MMIO_STATUS_PPR_OVERFLOW_MASK)) {
> -			pr_devel("Processing IOMMU PPR Log\n");
> -			iommu_poll_ppr_log(iommu);
> -		}
> +		writel(mask, iommu->mmio_base + MMIO_STATUS_OFFSET);
>  
> -		if (status & MMIO_STATUS_PPR_OVERFLOW_MASK) {
> -			pr_info_ratelimited("IOMMU PPR log overflow\n");
> -			amd_iommu_restart_ppr_log(iommu);
> -		}
> +		if (int_handler)
> +			int_handler(iommu);
>  
> -#ifdef CONFIG_IRQ_REMAP
> -		if (status & (MMIO_STATUS_GALOG_INT_MASK |
> -			      MMIO_STATUS_GALOG_OVERFLOW_MASK)) {
> -			pr_devel("Processing IOMMU GA Log\n");
> -			iommu_poll_ga_log(iommu);
> -		}
> -
> -		if (status & MMIO_STATUS_GALOG_OVERFLOW_MASK) {
> -			pr_info_ratelimited("IOMMU GA Log overflow\n");
> -			amd_iommu_restart_ga_log(iommu);
> -		}
> -#endif
> -
> -		if (status & MMIO_STATUS_EVT_OVERFLOW_MASK) {
> -			pr_info_ratelimited("IOMMU event log overflow\n");
> -			amd_iommu_restart_event_logging(iommu);
> -		}
> +		if ((status & overflow_mask) && overflow_handler)
> +			overflow_handler(iommu);
>  
>  		/*
>  		 * Hardware bug: ERBT1312
> @@ -908,6 +874,50 @@ irqreturn_t amd_iommu_int_thread(int irq, void *data)
>  		 */
>  		status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
>  	}
> +}
> +
> +irqreturn_t amd_iommu_int_thread_evtlog(int irq, void *data)
> +{
> +	pr_devel("Processing IOMMU Event Log\n");

Similar to the overflow series you could probably move this pr_devel inside
amd_iommu_handle_irq with a const string as an added argument (...)

> +	amd_iommu_handle_irq(data, MMIO_STATUS_EVT_INT_MASK,
> +			     MMIO_STATUS_EVT_OVERFLOW_MASK,
> +			     iommu_poll_events, amd_iommu_restart_event_logging);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +irqreturn_t amd_iommu_int_thread_pprlog(int irq, void *data)
> +{
> +	pr_devel("Processing IOMMU PPR Log\n");

here (...)

> +	amd_iommu_handle_irq(data, MMIO_STATUS_PPR_INT_MASK,
> +			     MMIO_STATUS_PPR_OVERFLOW_MASK,
> +			     iommu_poll_ppr_log, amd_iommu_restart_ppr_log);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +irqreturn_t amd_iommu_int_thread_galog(int irq, void *data)
> +{
> +
> +	pr_devel("Processing IOMMU GA Log\n");

here (...)

> +#ifdef CONFIG_IRQ_REMAP
> +	amd_iommu_handle_irq(data, MMIO_STATUS_GALOG_INT_MASK,
> +			     MMIO_STATUS_GALOG_OVERFLOW_MASK,
> +			     iommu_poll_ga_log, amd_iommu_restart_ga_log);
> +#else
> +	amd_iommu_handle_irq(data, MMIO_STATUS_GALOG_INT_MASK,
> +			     MMIO_STATUS_GALOG_OVERFLOW_MASK, NULL, NULL);
> +#endif
> +

As you and Jerry was discussing the else is probably not needed. Although it's
probably easier to just move the ifdef into the function below (...)

> +	return IRQ_HANDLED;
> +}
> +
> +irqreturn_t amd_iommu_int_thread(int irq, void *data)
> +{
> +	amd_iommu_int_thread_evtlog(irq, data);
> +	amd_iommu_int_thread_pprlog(irq, data);
> +	amd_iommu_int_thread_galog(irq, data);
> +

... here. Provided the code was before was behaving similarly and in theory you
wouldn't even setup the galog IRQ .

>  	return IRQ_HANDLED;
>  }
>  

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

[Vasant Hegde]

Hi Joao,

Sorry. I missed to Cc you in V2 version.

Can you please look into
https://lore.kernel.org/linux-iommu/20230619133008.6221-1-vasant.hegde@amd.com/


On 6/20/2023 8:31 PM, Joao Martins wrote:
> 
> 
> On 09/06/2023 11:20, Vasant Hegde wrote:
>> The AMD IOMMU has three different logs (Event, PPR and GA) and it can be
>> configured to send separate interrupt for each log type.
>>   - Event log is used whenever IOMMU reports events like IO_PAGE_FAULT,
>>     TLB_INV_TIMEOUT, etc,. During normal system operation this log is not
>>     used actively.
>>
>>   - GA log is used to record device interrupt requests that could not be
>>     immediately delivered to the target virtual processor due the fact the
>>     target was not running. This is actively used when we do device
>>     passthrough to AVIC enabled guest.
>>
>>   - PPR log is used to service the page fault request from device in Shared
>>     Virtual Addressing (SVA) mode where page table is shared by CPU and
>>     device. In this mode it will generate PPR interrupt frequently.
>>
>> Currently we have single interrupt to handle all three logs. GA log and
>> PPR log usage is increasing. Hence, split interrupt handler thread
>> into three separate interrupt handler function. Following patch enables
>> separate interrupt for PPR and GA Log.
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/amd_iommu.h |  3 ++
>>  drivers/iommu/amd/iommu.c     | 98 +++++++++++++++++++----------------
>>  2 files changed, 57 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
>> index 156f57b4f78c..e2857109e966 100644
>> --- a/drivers/iommu/amd/amd_iommu.h
>> +++ b/drivers/iommu/amd/amd_iommu.h
>> @@ -12,6 +12,9 @@
>>  #include "amd_iommu_types.h"
>>  


.../...

>> @@ -908,6 +874,50 @@ irqreturn_t amd_iommu_int_thread(int irq, void *data)
>>  		 */
>>  		status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
>>  	}
>> +}
>> +
>> +irqreturn_t amd_iommu_int_thread_evtlog(int irq, void *data)
>> +{
>> +	pr_devel("Processing IOMMU Event Log\n");
> 
> Similar to the overflow series you could probably move this pr_devel inside
> amd_iommu_handle_irq with a const string as an added argument (...)

Makes sense. Will fix it.

> 
>> +	amd_iommu_handle_irq(data, MMIO_STATUS_EVT_INT_MASK,
>> +			     MMIO_STATUS_EVT_OVERFLOW_MASK,
>> +			     iommu_poll_events, amd_iommu_restart_event_logging);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +irqreturn_t amd_iommu_int_thread_pprlog(int irq, void *data)
>> +{
>> +	pr_devel("Processing IOMMU PPR Log\n");
> 
> here (...)
> 
>> +	amd_iommu_handle_irq(data, MMIO_STATUS_PPR_INT_MASK,
>> +			     MMIO_STATUS_PPR_OVERFLOW_MASK,
>> +			     iommu_poll_ppr_log, amd_iommu_restart_ppr_log);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +irqreturn_t amd_iommu_int_thread_galog(int irq, void *data)
>> +{
>> +
>> +	pr_devel("Processing IOMMU GA Log\n");
> 
> here (...)
> 
>> +#ifdef CONFIG_IRQ_REMAP
>> +	amd_iommu_handle_irq(data, MMIO_STATUS_GALOG_INT_MASK,
>> +			     MMIO_STATUS_GALOG_OVERFLOW_MASK,
>> +			     iommu_poll_ga_log, amd_iommu_restart_ga_log);
>> +#else
>> +	amd_iommu_handle_irq(data, MMIO_STATUS_GALOG_INT_MASK,
>> +			     MMIO_STATUS_GALOG_OVERFLOW_MASK, NULL, NULL);
>> +#endif
>> +
> 
> As you and Jerry was discussing the else is probably not needed. Although it's
> probably easier to just move the ifdef into the function below (...)

If I move it to below function doesnot work as next patch configures separate
interrupt for GA log which calls this function directly.

-Vasant