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.c...@intel.com>
---
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@lists.xen.org
http://lists.xen.org/xen-devel
