[PATCH] iommu/amd: Add workaround to propery clearing IOMMU status register

[PATCH] iommu/amd: Add workaround to propery clearing IOMMU status register

[suravee.suthikulpanit]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

The IOMMU interrupt handling in bottom half must clear the PPR log interrupt
and event log interrupt bits to re-enable the interrupt. This is done by
writing 1 to the memory mapped register to clear the bit. Due to hardware bug,
if the driver tries to clear this bit while the IOMMU hardware also setting
this bit, the conflict will result with the bit being set. If the interrupt
handling code does not make sure to clear this bit, subsequent changes in the
event/PPR logs will no longer generating interrupts, and would result if
buffer overflow. After clearing the bits, the driver must read back
the register to verify.

In the current interrupt handling scheme, there are as many threads as
the number of IOMMUs. Each thread is created and assigned to an IOMMU at
the time of registering interrupt handlers (request_threaded_irq).
When an IOMMU HW generates an interrupt, the irq handler (top half) wakes up
the corresponding thread to process event and PPR logs of all IOMMUs
starting from the 1st IOMMU.

In the system with multiple IOMMU,this handling scheme complicates the
synchronization of the IOMMU data structures and status registers as
there could be multiple threads competing for the same IOMMU while
the other IOMMU could be left unhandled.

In order to simplify the implementation of the workaround, this patch is
proposing a different interrupt handling scheme by having each thread only
managing interrupts of the corresponding IOMMU. This can be achieved by passing
the struct amd_iommu when registering the interrupt handlers. This structure is
unique for each IOMMU and can be used by the bottom half thread to identify
the IOMMU to be handled instead of calling for_each_iommu.  Besides this also
eliminate the needs to lock the IOMMU for processing event and PPR logs.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd_iommu.c      |   59 ++++++++++++++++++++--------------------
 drivers/iommu/amd_iommu_init.c |    2 +-
 2 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index e5db3e5..3548d63 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -803,12 +803,6 @@ retry:
 static void iommu_poll_events(struct amd_iommu *iommu)
 {
 	u32 head, tail;
-	unsigned long flags;
-
-	/* enable event interrupts again */
-	writel(MMIO_STATUS_EVT_INT_MASK, iommu->mmio_base + MMIO_STATUS_OFFSET);
-
-	spin_lock_irqsave(&iommu->lock, flags);
 
 	head = readl(iommu->mmio_base + MMIO_EVT_HEAD_OFFSET);
 	tail = readl(iommu->mmio_base + MMIO_EVT_TAIL_OFFSET);
@@ -819,8 +813,6 @@ static void iommu_poll_events(struct amd_iommu *iommu)
 	}
 
 	writel(head, iommu->mmio_base + MMIO_EVT_HEAD_OFFSET);
-
-	spin_unlock_irqrestore(&iommu->lock, flags);
 }
 
 static void iommu_handle_ppr_entry(struct amd_iommu *iommu, u64 *raw)
@@ -845,17 +837,11 @@ static void iommu_handle_ppr_entry(struct amd_iommu *iommu, u64 *raw)
 
 static void iommu_poll_ppr_log(struct amd_iommu *iommu)
 {
-	unsigned long flags;
 	u32 head, tail;
 
 	if (iommu->ppr_log == NULL)
 		return;
 
-	/* enable ppr interrupts again */
-	writel(MMIO_STATUS_PPR_INT_MASK, iommu->mmio_base + MMIO_STATUS_OFFSET);
-
-	spin_lock_irqsave(&iommu->lock, flags);
-
 	head = readl(iommu->mmio_base + MMIO_PPR_HEAD_OFFSET);
 	tail = readl(iommu->mmio_base + MMIO_PPR_TAIL_OFFSET);
 
@@ -891,34 +877,49 @@ static void iommu_poll_ppr_log(struct amd_iommu *iommu)
 		head = (head + PPR_ENTRY_SIZE) % PPR_LOG_SIZE;
 		writel(head, iommu->mmio_base + MMIO_PPR_HEAD_OFFSET);
 
-		/*
-		 * Release iommu->lock because ppr-handling might need to
-		 * re-acquire it
-		 */
-		spin_unlock_irqrestore(&iommu->lock, flags);
-
 		/* Handle PPR entry */
 		iommu_handle_ppr_entry(iommu, entry);
 
-		spin_lock_irqsave(&iommu->lock, flags);
-
 		/* Refresh ring-buffer information */
 		head = readl(iommu->mmio_base + MMIO_PPR_HEAD_OFFSET);
 		tail = readl(iommu->mmio_base + MMIO_PPR_TAIL_OFFSET);
 	}
-
-	spin_unlock_irqrestore(&iommu->lock, flags);
 }
 
 irqreturn_t amd_iommu_int_thread(int irq, void *data)
 {
-	struct amd_iommu *iommu;
+	struct amd_iommu *iommu = (struct amd_iommu *) data;
+	u32 status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
 
-	for_each_iommu(iommu) {
-		iommu_poll_events(iommu);
-		iommu_poll_ppr_log(iommu);
-	}
+	while (status & (MMIO_STATUS_EVT_INT_MASK | MMIO_STATUS_PPR_INT_MASK)) {
+		if (status & MMIO_STATUS_EVT_INT_MASK) {
+			pr_devel("AMD-Vi: Processing IOMMU Event Log\n");
+			iommu_poll_events(iommu);
+		}
 
+		if (status & MMIO_STATUS_PPR_INT_MASK) {
+			pr_devel("AMD-Vi: Processing IOMMU PPR Log\n");
+			iommu_poll_ppr_log(iommu);
+		}
+
+		/* Enable EVT and PPR interrupts again */
+		writel((MMIO_STATUS_EVT_INT_MASK | MMIO_STATUS_PPR_INT_MASK),
+			iommu->mmio_base + MMIO_STATUS_OFFSET);
+
+		/*
+		 * Hardware bug: When re-enabling interrupt (by writing 1
+		 * to clear the bit), the hardware might also try to set
+		 * the interrupt bit in the event status register.
+		 * In this scenario, the bit will be set, and disable
+		 * subsequent interrupts.
+		 *
+		 * Workaround: The IOMMU driver should read back the
+		 * status register and check if the interrupt bits are cleared.
+		 * If not, driver will need to go through the interrupt handler
+		 * again and re-clear the bits
+		 */
+		status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
+	}
 	return IRQ_HANDLED;
 }
 
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index e3c2d74..e4120bb 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1275,7 +1275,7 @@ static int iommu_setup_msi(struct amd_iommu *iommu)
 				 amd_iommu_int_handler,
 				 amd_iommu_int_thread,
 				 0, "AMD-Vi",
-				 iommu->dev);
+				 iommu);
 
 	if (r) {
 		pci_disable_msi(iommu->dev);
-- 
1.7.10.4

Re: [PATCH] iommu/amd: Add workaround to propery clearing IOMMU status register

[Suravee Suthikulanit]

Joerg,

Do you have any feedback about this patch?

Thanks,

Suravee

On 4/3/2013 6:19 PM, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> The IOMMU interrupt handling in bottom half must clear the PPR log interrupt
> and event log interrupt bits to re-enable the interrupt. This is done by
> writing 1 to the memory mapped register to clear the bit. Due to hardware bug,
> if the driver tries to clear this bit while the IOMMU hardware also setting
> this bit, the conflict will result with the bit being set. If the interrupt
> handling code does not make sure to clear this bit, subsequent changes in the
> event/PPR logs will no longer generating interrupts, and would result if
> buffer overflow. After clearing the bits, the driver must read back
> the register to verify.
>
> In the current interrupt handling scheme, there are as many threads as
> the number of IOMMUs. Each thread is created and assigned to an IOMMU at
> the time of registering interrupt handlers (request_threaded_irq).
> When an IOMMU HW generates an interrupt, the irq handler (top half) wakes up
> the corresponding thread to process event and PPR logs of all IOMMUs
> starting from the 1st IOMMU.
>
> In the system with multiple IOMMU,this handling scheme complicates the
> synchronization of the IOMMU data structures and status registers as
> there could be multiple threads competing for the same IOMMU while
> the other IOMMU could be left unhandled.
>
> In order to simplify the implementation of the workaround, this patch is
> proposing a different interrupt handling scheme by having each thread only
> managing interrupts of the corresponding IOMMU. This can be achieved by passing
> the struct amd_iommu when registering the interrupt handlers. This structure is
> unique for each IOMMU and can be used by the bottom half thread to identify
> the IOMMU to be handled instead of calling for_each_iommu.  Besides this also
> eliminate the needs to lock the IOMMU for processing event and PPR logs.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>   drivers/iommu/amd_iommu.c      |   59 ++++++++++++++++++++--------------------
>   drivers/iommu/amd_iommu_init.c |    2 +-
>   2 files changed, 31 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index e5db3e5..3548d63 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -803,12 +803,6 @@ retry:
>   static void iommu_poll_events(struct amd_iommu *iommu)
>   {
>   	u32 head, tail;
> -	unsigned long flags;
> -
> -	/* enable event interrupts again */
> -	writel(MMIO_STATUS_EVT_INT_MASK, iommu->mmio_base + MMIO_STATUS_OFFSET);
> -
> -	spin_lock_irqsave(&iommu->lock, flags);
>   
>   	head = readl(iommu->mmio_base + MMIO_EVT_HEAD_OFFSET);
>   	tail = readl(iommu->mmio_base + MMIO_EVT_TAIL_OFFSET);
> @@ -819,8 +813,6 @@ static void iommu_poll_events(struct amd_iommu *iommu)
>   	}
>   
>   	writel(head, iommu->mmio_base + MMIO_EVT_HEAD_OFFSET);
> -
> -	spin_unlock_irqrestore(&iommu->lock, flags);
>   }
>   
>   static void iommu_handle_ppr_entry(struct amd_iommu *iommu, u64 *raw)
> @@ -845,17 +837,11 @@ static void iommu_handle_ppr_entry(struct amd_iommu *iommu, u64 *raw)
>   
>   static void iommu_poll_ppr_log(struct amd_iommu *iommu)
>   {
> -	unsigned long flags;
>   	u32 head, tail;
>   
>   	if (iommu->ppr_log == NULL)
>   		return;
>   
> -	/* enable ppr interrupts again */
> -	writel(MMIO_STATUS_PPR_INT_MASK, iommu->mmio_base + MMIO_STATUS_OFFSET);
> -
> -	spin_lock_irqsave(&iommu->lock, flags);
> -
>   	head = readl(iommu->mmio_base + MMIO_PPR_HEAD_OFFSET);
>   	tail = readl(iommu->mmio_base + MMIO_PPR_TAIL_OFFSET);
>   
> @@ -891,34 +877,49 @@ static void iommu_poll_ppr_log(struct amd_iommu *iommu)
>   		head = (head + PPR_ENTRY_SIZE) % PPR_LOG_SIZE;
>   		writel(head, iommu->mmio_base + MMIO_PPR_HEAD_OFFSET);
>   
> -		/*
> -		 * Release iommu->lock because ppr-handling might need to
> -		 * re-acquire it
> -		 */
> -		spin_unlock_irqrestore(&iommu->lock, flags);
> -
>   		/* Handle PPR entry */
>   		iommu_handle_ppr_entry(iommu, entry);
>   
> -		spin_lock_irqsave(&iommu->lock, flags);
> -
>   		/* Refresh ring-buffer information */
>   		head = readl(iommu->mmio_base + MMIO_PPR_HEAD_OFFSET);
>   		tail = readl(iommu->mmio_base + MMIO_PPR_TAIL_OFFSET);
>   	}
> -
> -	spin_unlock_irqrestore(&iommu->lock, flags);
>   }
>   
>   irqreturn_t amd_iommu_int_thread(int irq, void *data)
>   {
> -	struct amd_iommu *iommu;
> +	struct amd_iommu *iommu = (struct amd_iommu *) data;
> +	u32 status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
>   
> -	for_each_iommu(iommu) {
> -		iommu_poll_events(iommu);
> -		iommu_poll_ppr_log(iommu);
> -	}
> +	while (status & (MMIO_STATUS_EVT_INT_MASK | MMIO_STATUS_PPR_INT_MASK)) {
> +		if (status & MMIO_STATUS_EVT_INT_MASK) {
> +			pr_devel("AMD-Vi: Processing IOMMU Event Log\n");
> +			iommu_poll_events(iommu);
> +		}
>   
> +		if (status & MMIO_STATUS_PPR_INT_MASK) {
> +			pr_devel("AMD-Vi: Processing IOMMU PPR Log\n");
> +			iommu_poll_ppr_log(iommu);
> +		}
> +
> +		/* Enable EVT and PPR interrupts again */
> +		writel((MMIO_STATUS_EVT_INT_MASK | MMIO_STATUS_PPR_INT_MASK),
> +			iommu->mmio_base + MMIO_STATUS_OFFSET);
> +
> +		/*
> +		 * Hardware bug: When re-enabling interrupt (by writing 1
> +		 * to clear the bit), the hardware might also try to set
> +		 * the interrupt bit in the event status register.
> +		 * In this scenario, the bit will be set, and disable
> +		 * subsequent interrupts.
> +		 *
> +		 * Workaround: The IOMMU driver should read back the
> +		 * status register and check if the interrupt bits are cleared.
> +		 * If not, driver will need to go through the interrupt handler
> +		 * again and re-clear the bits
> +		 */
> +		status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
> +	}
>   	return IRQ_HANDLED;
>   }
>   
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index e3c2d74..e4120bb 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -1275,7 +1275,7 @@ static int iommu_setup_msi(struct amd_iommu *iommu)
>   				 amd_iommu_int_handler,
>   				 amd_iommu_int_thread,
>   				 0, "AMD-Vi",
> -				 iommu->dev);
> +				 iommu);
>   
>   	if (r) {
>   		pci_disable_msi(iommu->dev);

Re: [PATCH] iommu/amd: Add workaround to propery clearing IOMMU status register

[Joerg Roedel]

On Wed, Apr 03, 2013 at 06:19:04PM -0500, Suthikulpanit, Suravee wrote:
> In the system with multiple IOMMU,this handling scheme complicates the
> synchronization of the IOMMU data structures and status registers as
> there could be multiple threads competing for the same IOMMU while
> the other IOMMU could be left unhandled.

I really like that change in the interrupt handling code to pull the
different IOMMUs independently. But its problematic to change that in
the same patch that adds an erratum workaround.

Please move that erratum fix into a seperate small patch that we can
backport to -stable and resend.


	Joerg

Re: [PATCH] iommu/amd: Add workaround to propery clearing IOMMU status register

[Suravee Suthikulanit]

On 4/9/2013 4:49 AM, Joerg Roedel wrote:
> On Wed, Apr 03, 2013 at 06:19:04PM -0500, Suthikulpanit, Suravee wrote:
>> In the system with multiple IOMMU,this handling scheme complicates the
>> synchronization of the IOMMU data structures and status registers as
>> there could be multiple threads competing for the same IOMMU while
>> the other IOMMU could be left unhandled.
> I really like that change in the interrupt handling code to pull the
> different IOMMUs independently. But its problematic to change that in
> the same patch that adds an erratum workaround.
>
> Please move that erratum fix into a seperate small patch that we can
> backport to -stable and resend.
>
>
> 	Joerg
Joerg,

The reason I implemented the "per-thread IOMMU handling" and the 
"workaround" together in one patch
is because it simplifies the synchronization of clearing and checking 
the interrupt enabling bits.
In the previous implementation, we could end up having multiple threads 
trying to access the status register
at the same time.

Suravee
>
>

Re: [PATCH] iommu/amd: Add workaround to propery clearing IOMMU status register

[Joerg Roedel]

On Tue, Apr 09, 2013 at 09:22:58AM -0500, Suthikulpanit, Suravee wrote:
> The reason I implemented the "per-thread IOMMU handling" and the
> "workaround" together in one patch
> is because it simplifies the synchronization of clearing and
> checking the interrupt enabling bits.
> In the previous implementation, we could end up having multiple
> threads trying to access the status register
> at the same time.

This is the reason for the iommu->lock spinlock in this code at the
moment.


	Joerg