iommu/amd: Fix logic for clearing the IOMMU interrupt bits

The IOMMU interrupt bits in the IOMMU status registers are
"read-only, and write-1-to-clear (RW1C).  Therefore, the existing
logic which reads the register, set the bit, and then writing back
the values could accidentally clear certain bits if it has been set.

The correct logic would just be writing only the value which only
set the interrupt bits, and leave the rest to zeros.

This patch also, clean up #define masks as Jan has suggested.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

With iommu_interrupt_handler() properly having got switched its readl()
from status to control register, the subsequent writel() needed to be
switched too (and the RW1C comment there was bogus).

Some of the cleanup went too far - undone.

Further, with iommu_interrupt_handler() now actually disabling the
interrupt sources, they also need to get re-enabled by the tasklet once
it finished processing the respective log. This also implies re-running
the tasklet so that log entries added between reading the log and re-
enabling the interrupt will get handled in a timely manner.

Finally, guest write emulation to the status register needs to be done
with the RW1C (and RO for all other bits) semantics in mind too.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Tim Deegan <tim@xen.org>
Acked-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

3 files changed, 67 insertions, 40 deletions

diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c b/xen/drivers/passthrough/amd/iommu_cmd.c
index 4c6014950b..d27bd3c90b 100644
--- a/xen/drivers/passthrough/amd/iommu_cmd.c
+++ b/xen/drivers/passthrough/amd/iommu_cmd.c
@@ -75,11 +75,9 @@ static void flush_command_buffer(struct amd_iommu *iommu)
     u32 cmd[4], status;
     int loop_count, comp_wait;
 
-    /* clear 'ComWaitInt' in status register (WIC) */
-    set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, 0,
-                         IOMMU_STATUS_COMP_WAIT_INT_MASK,
-                         IOMMU_STATUS_COMP_WAIT_INT_SHIFT, &status);
-    writel(status, iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
+    /* RW1C 'ComWaitInt' in status register */
+    writel(IOMMU_STATUS_COMP_WAIT_INT_MASK,
+           iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
 
     /* send an empty COMPLETION_WAIT command to flush command buffer */
     cmd[3] = cmd[2] = 0;
@@ -103,9 +101,9 @@ static void flush_command_buffer(struct amd_iommu *iommu)
 
     if ( comp_wait )
     {
-        /* clear 'ComWaitInt' in status register (WIC) */
-        status &= IOMMU_STATUS_COMP_WAIT_INT_MASK;
-        writel(status, iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
+        /* RW1C 'ComWaitInt' in status register */
+        writel(IOMMU_STATUS_COMP_WAIT_INT_MASK,
+               iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
         return;
     }
     AMD_IOMMU_DEBUG("Warning: ComWaitInt bit did not assert!\n");
diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
index e2daf0e52e..85f23610a4 100644
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -754,7 +754,14 @@ static void guest_iommu_mmio_write64(struct guest_iommu *iommu,
         u64_to_reg(&iommu->ppr_log.reg_tail, val);
         break;
     case IOMMU_STATUS_MMIO_OFFSET:
-        u64_to_reg(&iommu->reg_status, val);
+        val &= IOMMU_STATUS_EVENT_OVERFLOW_MASK |
+               IOMMU_STATUS_EVENT_LOG_INT_MASK |
+               IOMMU_STATUS_COMP_WAIT_INT_MASK |
+               IOMMU_STATUS_PPR_LOG_OVERFLOW_MASK |
+               IOMMU_STATUS_PPR_LOG_INT_MASK |
+               IOMMU_STATUS_GAPIC_LOG_OVERFLOW_MASK |
+               IOMMU_STATUS_GAPIC_LOG_INT_MASK;
+        u64_to_reg(&iommu->reg_status, reg_to_u64(iommu->reg_status) & ~val);
         break;
 
     default:
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index b2b92baba1..b01e630ef3 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -344,13 +344,13 @@ static void set_iommu_ppr_log_control(struct amd_iommu *iommu,
         writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_TAIL_OFFSET);
 
         iommu_set_bit(&entry, IOMMU_CONTROL_PPR_ENABLE_SHIFT);
-        iommu_set_bit(&entry, IOMMU_CONTROL_PPR_INT_SHIFT);
+        iommu_set_bit(&entry, IOMMU_CONTROL_PPR_LOG_INT_SHIFT);
         iommu_set_bit(&entry, IOMMU_CONTROL_PPR_LOG_ENABLE_SHIFT);
     }
     else
     {
         iommu_clear_bit(&entry, IOMMU_CONTROL_PPR_ENABLE_SHIFT);
-        iommu_clear_bit(&entry, IOMMU_CONTROL_PPR_INT_SHIFT);
+        iommu_clear_bit(&entry, IOMMU_CONTROL_PPR_LOG_INT_SHIFT);
         iommu_clear_bit(&entry, IOMMU_CONTROL_PPR_LOG_ENABLE_SHIFT);
     }
 
@@ -410,7 +410,7 @@ static void iommu_reset_log(struct amd_iommu *iommu,
                             void (*ctrl_func)(struct amd_iommu *iommu, int))
 {
     u32 entry;
-    int log_run, run_bit, of_bit;
+    int log_run, run_bit;
     int loop_count = 1000;
 
     BUG_ON(!iommu || ((log != &iommu->event_log) && (log != &iommu->ppr_log)));
@@ -419,10 +419,6 @@ static void iommu_reset_log(struct amd_iommu *iommu,
         IOMMU_STATUS_EVENT_LOG_RUN_SHIFT :
         IOMMU_STATUS_PPR_LOG_RUN_SHIFT;
 
-    of_bit = ( log == &iommu->event_log ) ?
-        IOMMU_STATUS_EVENT_OVERFLOW_SHIFT :
-        IOMMU_STATUS_PPR_LOG_OVERFLOW_SHIFT;
-
     /* wait until EventLogRun bit = 0 */
     do {
         entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
@@ -439,9 +435,10 @@ static void iommu_reset_log(struct amd_iommu *iommu,
 
     ctrl_func(iommu, IOMMU_CONTROL_DISABLED);
 
-    /*clear overflow bit */
-    iommu_clear_bit(&entry, of_bit);
-    writel(entry, iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
+    /* RW1C overflow bit */
+    writel(log == &iommu->event_log ? IOMMU_STATUS_EVENT_OVERFLOW_MASK
+                                    : IOMMU_STATUS_PPR_LOG_OVERFLOW_MASK,
+           iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
 
     /*reset event log base address */
     log->head = 0;
@@ -611,22 +608,33 @@ static void iommu_check_event_log(struct amd_iommu *iommu)
     u32 entry;
     unsigned long flags;
 
+    /* RW1C interrupt status bit */
+    writel(IOMMU_STATUS_EVENT_LOG_INT_MASK,
+           iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
+
     iommu_read_log(iommu, &iommu->event_log,
                    sizeof(event_entry_t), parse_event_log_entry);
 
     spin_lock_irqsave(&iommu->lock, flags);
     
-    /*check event overflow */
+    /* Check event overflow. */
     entry = readl(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);
-
-    /* reset interrupt status bit */
-    entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
-    iommu_set_bit(&entry, IOMMU_STATUS_EVENT_LOG_INT_SHIFT);
-
-    writel(entry, iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
+    else
+    {
+        entry = readl(iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
+        if ( !(entry & IOMMU_CONTROL_EVENT_LOG_INT_MASK) )
+        {
+            entry |= IOMMU_CONTROL_EVENT_LOG_INT_MASK;
+            writel(entry, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
+            /*
+             * Re-schedule the tasklet to handle eventual log entries added
+             * between reading the log above and re-enabling the interrupt.
+             */
+            tasklet_schedule(&amd_iommu_irq_tasklet);
+        }
+    }
 
     spin_unlock_irqrestore(&iommu->lock, flags);
 }
@@ -681,22 +689,33 @@ static void iommu_check_ppr_log(struct amd_iommu *iommu)
     u32 entry;
     unsigned long flags;
 
+    /* RW1C interrupt status bit */
+    writel(IOMMU_STATUS_PPR_LOG_INT_MASK,
+           iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
+
     iommu_read_log(iommu, &iommu->ppr_log,
                    sizeof(ppr_entry_t), parse_ppr_log_entry);
     
     spin_lock_irqsave(&iommu->lock, flags);
 
-    /*check event overflow */
+    /* Check event overflow. */
     entry = readl(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);
-
-    /* reset interrupt status bit */
-    entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
-    iommu_set_bit(&entry, IOMMU_STATUS_PPR_LOG_INT_SHIFT);
-
-    writel(entry, iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
+    else
+    {
+        entry = readl(iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
+        if ( !(entry & IOMMU_CONTROL_PPR_LOG_INT_MASK) )
+        {
+            entry |= IOMMU_CONTROL_PPR_LOG_INT_MASK;
+            writel(entry, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
+            /*
+             * Re-schedule the tasklet to handle eventual log entries added
+             * between reading the log above and re-enabling the interrupt.
+             */
+            tasklet_schedule(&amd_iommu_irq_tasklet);
+        }
+    }
 
     spin_unlock_irqrestore(&iommu->lock, flags);
 }
@@ -733,11 +752,14 @@ static void iommu_interrupt_handler(int irq, void *dev_id,
 
     spin_lock_irqsave(&iommu->lock, flags);
 
-    /* Silence interrupts from both event and PPR logging */
-    entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
-    iommu_clear_bit(&entry, IOMMU_STATUS_EVENT_LOG_INT_SHIFT);
-    iommu_clear_bit(&entry, IOMMU_STATUS_PPR_LOG_INT_SHIFT);
-    writel(entry, iommu->mmio_base+IOMMU_STATUS_MMIO_OFFSET);
+    /*
+     * 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);