[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 2/3] xen/vt-d: mask interrupt message generation



Yang,

Thanks for your review.

On 2015/5/4 12:07, Zhang, Yang Z wrote:
Chen, Tiejun wrote on 2015-05-04:
While initializing VT-D we should mask interrupt message generation
to avoid receiving any interrupt as pending before enable DMA
translation, and also mask that before disable DMA engine.

Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
---
  xen/drivers/passthrough/vtd/iommu.c | 31 +++++++++++++++++++++++--------
  1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/iommu.c
b/xen/drivers/passthrough/vtd/iommu.c index c7bda73..72cd854 100644 ---
a/xen/drivers/passthrough/vtd/iommu.c +++
b/xen/drivers/passthrough/vtd/iommu.c @@ -1000,15 +1000,21 @@ static
void dma_msi_unmask(struct irq_desc *desc)
      iommu->msi.msi_attrib.masked = 0;
  }
-static void dma_msi_mask(struct irq_desc *desc)
+static void mask_dma_interrupt(struct iommu *iommu)
  {
      unsigned long flags;
-    struct iommu *iommu = desc->action->dev_id;

-    /* mask it */
      spin_lock_irqsave(&iommu->register_lock, flags);
      dmar_writel(iommu->reg, DMAR_FECTL_REG, DMA_FECTL_IM);
      spin_unlock_irqrestore(&iommu->register_lock, flags);
+}
+
+static void dma_msi_mask(struct irq_desc *desc)
+{
+    struct iommu *iommu = desc->action->dev_id;
+
+    /* mask it */
+    mask_dma_interrupt(iommu);
      iommu->msi.msi_attrib.masked = 1;
  }
@@ -1997,7 +2003,6 @@ static int init_vtd_hw(void)
      struct iommu *iommu;
      struct iommu_flush *flush = NULL;
      int ret;
-    unsigned long flags;

      /*
       * Basic VT-d HW init: set VT-d interrupt, clear VT-d faults.
@@ -2008,11 +2013,16 @@ static int init_vtd_hw(void)

          iommu = drhd->iommu;
-        clear_fault_bits(iommu);
+        /*
+         * We shouldn't receive any VT-d interrupt while initializing
+         * VT-d so just mask interrupt message generation.
+         */
+        mask_dma_interrupt(iommu);

-        spin_lock_irqsave(&iommu->register_lock, flags);
-        dmar_writel(iommu->reg, DMAR_FECTL_REG, 0);
-        spin_unlock_irqrestore(&iommu->register_lock, flags);
+        /*
+         * And then clear all previous faults.
+         */
+        clear_fault_bits(iommu);
      }

      /*
@@ -2350,6 +2360,11 @@ static void vtd_suspend(void)
          if ( force_iommu )
              continue;
+        /*
+         * Mask interrupt message generation.
+         */
+        mask_dma_interrupt(iommu);
+
          iommu_disable_translation(iommu);

          /* If interrupt remapping is enabled, queued invalidation

Just curious that do you observe a real issue or just catch it through reading 
code?


Yes, we observed this problem on BDW. And actually one HP customer also had this same issue.

Once we enable IOMMU translation, an IOMMU fault, 'Present bit in root entry is clear', is triggered like this,

(XEN) [VT-D]iommu.c:731: iommu_enable_translation: iommu->reg = ffff82c000201000
(XEN) [VT-D]iommu.c:875: iommu_fault_status: Fault Overflow
(XEN) [VT-D]iommu.c:877: iommu_fault_status: Primary Pending Fault
(XEN) [VT-D]DMAR:[DMA Write] Request device [0000:00:02.0] fault addr 0, iommu reg = ffff82c000201000
(XEN) [VT-D]d32767v0 DMAR: reason 01 - Present bit in root entry is clear
(XEN) print_vtd_entries: iommu ffff83012aa88600 dev 0000:00:02.0 gmfn 0
(XEN)     root_entry = ffff83012aa85000
(XEN)     root_entry[0] = 80f5001
(XEN)     context = ffff8300080f5000
(XEN)     context[10] = 2_8b75001
(XEN)     l4 = ffff830008b75000
(XEN)     l4_index = 0
(XEN)     l4[0] = 8b74003
(XEN)     l3 = ffff830008b74000
(XEN)     l3_index = 0
(XEN)     l3[0] = 8b73003
(XEN)     l2 = ffff830008b73000
(XEN)     l2_index = 0
(XEN)     l2[0] = 8b72003
(XEN)     l1 = ffff830008b72000
(XEN)     l1_index = 0
(XEN)     l1[0] = 3

At first I doubted this is issued by some improper cache behaviors.
Because as you see, "root_entry[0] = 80f5001" indicates we already set that present bit. But Caching Mode bit is zero in BDW so this means remapping hardware doesn't own contest cache. And I also check xen always calls clflush to write back memory on CPU side. (Even I also tried to use wbinvd to replace clflush). And plus, you can see this guest fault address is a weird zero, so I turned to concern if this is just an unexpected fault which is pending because of some BIOS cleanup, or undefined hardware behaviors. Here I mean clear_fault_bits() is already not enough to clear this kind of pending interrupt before we complete all VT-D initializations.

And anyway, I also think we should disable interrupt generation during VT-D setup until VT-D device enable interrupt explicitly.

Thanks
Tiejun

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.