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

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Chen, Tiejun wrote on 2015-05-04:
> 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.

So this patch helps nothing in this case?

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

The question is why there will be a fault interrupt generated during VT-D setup?

> 
> Thanks
> Tiejun


Best regards,
Yang



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