[RFC,v3,13/14] AMD/IOMMU: correct IRTE updating

Commit Message

Jan Beulich July 16, 2019, 4:40 p.m. UTC

Flushing didn't get done along the lines of what the specification says.
Mark entries to be updated as not remapped (which will result in
interrupt requests to get target aborted, but the interrupts should be
masked anyway at that point in time), issue the flush, and only then
write the new entry.

In update_intremap_entry_from_msi_msg() also fold the duplicate initial
lock determination and acquire into just a single instance.

Signed-off-by: Jan Beulich <[e-mail]>
---
RFC: Putting the flush invocations in loops isn't overly nice, but I
      don't think this can really be abused, since callers up the stack
      hold further locks. Nevertheless I'd like to ask for better
      suggestions.
---
v3: Remove stale parts of description. Re-base.
v2: Parts morphed into earlier patch.

Comments

Andrew Cooper July 19, 2019, 5:44 p.m. UTC | #1

On 16/07/2019 17:40, Jan Beulich wrote:
> Flushing didn't get done along the lines of what the specification says.
> Mark entries to be updated as not remapped (which will result in
> interrupt requests to get target aborted, but the interrupts should be
> masked anyway at that point in time), issue the flush, and only then
> write the new entry.
>
> In update_intremap_entry_from_msi_msg() also fold the duplicate initial
> lock determination and acquire into just a single instance.
>
> Signed-off-by: Jan Beulich <[e-mail]>
> ---
> RFC: Putting the flush invocations in loops isn't overly nice, but I
>       don't think this can really be abused, since callers up the stack
>       hold further locks. Nevertheless I'd like to ask for better
>       suggestions.

Looking again, and at v2, I think this is a consequence of our insane
!x2apic interrupt set up, where we wrap an already-established system
with interrupt remapping.

Longer term, when we undo that, we should have far more clear code
structure.  Therefore, I think it is fine for now.

Acked-by: Andrew Cooper <[e-mail]>

Patch

--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -207,9 +207,7 @@  static void update_intremap_entry(const
              .vector = vector,
          };
  
-        ACCESS_ONCE(entry.ptr128->raw[0]) = 0;
-        /* Low half, in particular RemapEn, needs to be cleared first. */
-        barrier();
+        ASSERT(!entry.ptr128->full.remap_en);
          entry.ptr128->raw[1] =
              container_of(&full, union irte128, full)->raw[1];
          /* High half needs to be set before low one (containing RemapEn). */
@@ -288,6 +286,20 @@  static int update_intremap_entry_from_io
      }
  
      entry = get_intremap_entry(iommu, req_id, offset);
+
+    /* The RemapEn fields match for all formats. */
+    while ( iommu->enabled && entry.ptr32->basic.remap_en )
+    {
+        entry.ptr32->basic.remap_en = false;
+        spin_unlock(lock);
+
+        spin_lock(&iommu->lock);
+        amd_iommu_flush_intremap(iommu, req_id);
+        spin_unlock(&iommu->lock);
+
+        spin_lock(lock);
+    }
+
      if ( fresh )
          /* nothing */;
      else if ( !lo_update )
@@ -317,13 +329,6 @@  static int update_intremap_entry_from_io
  
      spin_unlock_irqrestore(lock, flags);
  
-    if ( iommu->enabled && !fresh )
-    {
-        spin_lock_irqsave(&iommu->lock, flags);
-        amd_iommu_flush_intremap(iommu, req_id);
-        spin_unlock_irqrestore(&iommu->lock, flags);
-    }
-
      set_rte_index(rte, offset);
  
      return 0;
@@ -579,19 +584,27 @@  static int update_intremap_entry_from_ms
      req_id = get_dma_requestor_id(iommu->seg, bdf);
      alias_id = get_intremap_requestor_id(iommu->seg, bdf);
  
+    lock = get_intremap_lock(iommu->seg, req_id);
+    spin_lock_irqsave(lock, flags);
+
      if ( msg == NULL )
      {
-        lock = get_intremap_lock(iommu->seg, req_id);
-        spin_lock_irqsave(lock, flags);
          for ( i = 0; i < nr; ++i )
              free_intremap_entry(iommu, req_id, *remap_index + i);
          spin_unlock_irqrestore(lock, flags);
-        goto done;
-    }
  
-    lock = get_intremap_lock(iommu->seg, req_id);
+        if ( iommu->enabled )
+        {
+            spin_lock_irqsave(&iommu->lock, flags);
+            amd_iommu_flush_intremap(iommu, req_id);
+            if ( alias_id != req_id )
+                amd_iommu_flush_intremap(iommu, alias_id);
+            spin_unlock_irqrestore(&iommu->lock, flags);
+        }
+
+        return 0;
+    }
  
-    spin_lock_irqsave(lock, flags);
      dest_mode = (msg->address_lo >> MSI_ADDR_DESTMODE_SHIFT) & 0x1;
      delivery_mode = (msg->data >> MSI_DATA_DELIVERY_MODE_SHIFT) & 0x1;
      vector = (msg->data >> MSI_DATA_VECTOR_SHIFT) & MSI_DATA_VECTOR_MASK;
@@ -615,6 +628,22 @@  static int update_intremap_entry_from_ms
      }
  
      entry = get_intremap_entry(iommu, req_id, offset);
+
+    /* The RemapEn fields match for all formats. */
+    while ( iommu->enabled && entry.ptr32->basic.remap_en )
+    {
+        entry.ptr32->basic.remap_en = false;
+        spin_unlock(lock);
+
+        spin_lock(&iommu->lock);
+        amd_iommu_flush_intremap(iommu, req_id);
+        if ( alias_id != req_id )
+            amd_iommu_flush_intremap(iommu, alias_id);
+        spin_unlock(&iommu->lock);
+
+        spin_lock(lock);
+    }
+
      update_intremap_entry(iommu, entry, vector, delivery_mode, dest_mode, dest);
      spin_unlock_irqrestore(lock, flags);
  
@@ -634,16 +663,6 @@  static int update_intremap_entry_from_ms
                 get_ivrs_mappings(iommu->seg)[alias_id].intremap_table);
      }
  
-done:
-    if ( iommu->enabled )
-    {
-        spin_lock_irqsave(&iommu->lock, flags);
-        amd_iommu_flush_intremap(iommu, req_id);
-        if ( alias_id != req_id )
-            amd_iommu_flush_intremap(iommu, alias_id);
-        spin_unlock_irqrestore(&iommu->lock, flags);
-    }
-
      return 0;
  }