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

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

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