Re: [PATCH 2/2 V2] iommu/amd: Workaround for erratum 787

[Suravee Suthikulanit <suravee.suthikulpanit@amd.com>]

Hi All,

We should also check if the EventLogInt and PPRInt bits are set before actually
going into the log processing code.  Also, I agree with Jan that we should not need
to disable the Event log and the PPR log in the IOMMU control register.
This could be handled simply through the status register.

Also, I think we can further simplify the logic for the workaround by having only
one loop instead of two.  Here is the newly proposed changes for the patch.  However,
I am still not sure if we should reschedule the tasklet instead of just using the
while loop here.

Thank you,

Suravee


diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index b5a39a9..bd9913f 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -615,19 +615,8 @@ static void iommu_check_event_log(struct amd_iommu *iommu)
      
      /*check event overflow */
      entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
-
-    /* RW1C interrupt status bit */
-    writel(IOMMU_STATUS_EVENT_LOG_INT_MASK,
-           iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
-
      if ( iommu_get_bit(entry, IOMMU_STATUS_EVENT_OVERFLOW_SHIFT) )
          iommu_reset_log(iommu, &iommu->event_log, set_iommu_event_log_control);
-    else
-    {
-        entry = readl(iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
-        iommu_set_bit(&entry, IOMMU_CONTROL_EVENT_LOG_INT_SHIFT);
-        writel(entry, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
-    }
  
      spin_unlock_irqrestore(&iommu->lock, flags);
  }
@@ -689,26 +678,20 @@ static void iommu_check_ppr_log(struct amd_iommu *iommu)
  
      /*check event overflow */
      entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
-
-    /* RW1C interrupt status bit */
-    writel(IOMMU_STATUS_PPR_LOG_INT_MASK,
-           iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
-
      if ( iommu_get_bit(entry, IOMMU_STATUS_PPR_LOG_OVERFLOW_SHIFT) )
          iommu_reset_log(iommu, &iommu->ppr_log, set_iommu_ppr_log_control);
-    else
-    {
-        entry = readl(iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
-        iommu_set_bit(&entry, IOMMU_CONTROL_PPR_LOG_INT_SHIFT);
-        writel(entry, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
-    }
  
      spin_unlock_irqrestore(&iommu->lock, flags);
  }
  
+#define IOMMU_INT_PENDING(x)	( (x & IOMMU_STATUS_EVENT_LOG_INT_MASK) || \
+				  (x & IOMMU_STATUS_PPR_LOG_INT_MASK) )
+
  static void do_amd_iommu_irq(unsigned long data)
  {
      struct amd_iommu *iommu;
+    u32 status, entry;
+    unsigned long flags;
  
      if ( !iommu_found() )
      {
@@ -722,33 +705,47 @@ static void do_amd_iommu_irq(unsigned long data)
       * tasklet (instead of one per each IOMMUs).
       */
      for_each_amd_iommu ( iommu ) {
-        iommu_check_event_log(iommu);
+        /* Get the IOMMU status register */
+        spin_lock_irqsave(&iommu->lock, flags);
+        status = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
+        spin_unlock_irqrestore(&iommu->lock, flags);
+
+        while ( IOMMU_INT_PENDING(status) )
+        {
+            entry = 0;
+
+            if ( status & IOMMU_STATUS_EVENT_LOG_INT_MASK )
+            {
+                iommu_check_event_log(iommu);
+                iommu_set_bit(&entry, IOMMU_STATUS_EVENT_LOG_INT_SHIFT);
+	    }
  
-        if ( iommu->ppr_log.buffer != NULL )
-            iommu_check_ppr_log(iommu);
+            if ( (iommu->ppr_log.buffer != NULL)
+            && (status & IOMMU_STATUS_PPR_LOG_INT_MASK) )
+            {
+                iommu_check_ppr_log(iommu);
+                iommu_set_bit(&entry, IOMMU_STATUS_PPR_LOG_INT_SHIFT);
+            }
+
+            spin_lock_irqsave(&iommu->lock, flags);
+
+            /* RW1C interrupt status bit */
+            writel(entry, iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
+
+            /*
+             * Workaround for erratum787:
+             * Re-check to make sure the bit has been cleared.
+             */
+            status = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
+
+            spin_unlock_irqrestore(&iommu->lock, flags);
+        }
      }
  }
  
  static void iommu_interrupt_handler(int irq, void *dev_id,
                                      struct cpu_user_regs *regs)
  {
-    u32 entry;
-    unsigned long flags;
-    struct amd_iommu *iommu = dev_id;
-
-    spin_lock_irqsave(&iommu->lock, flags);
-
-    /*
-     * Silence interrupts from both event and PPR by clearing the
-     * enable logging bits in the control register
-     */
-    entry = readl(iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
-    iommu_clear_bit(&entry, IOMMU_CONTROL_EVENT_LOG_INT_SHIFT);
-    iommu_clear_bit(&entry, IOMMU_CONTROL_PPR_LOG_INT_SHIFT);
-    writel(entry, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
-
-    spin_unlock_irqrestore(&iommu->lock, flags);
-
      /* It is the tasklet that will clear the logs and re-enable interrupts */
      tasklet_schedule(&amd_iommu_irq_tasklet);
  }