Re: [Xen-devel] [PATCH v12 6/6] vt-d: fix vt-d Device-TLB flush timeout issue

["Xu, Quan" <quan.xu@xxxxxxxxx>]

On June 27, 2016 4:24 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 24.06.16 at 07:51, <quan.xu@xxxxxxxxx> wrote:
> > @@ -199,24 +199,73 @@ static int __must_check
> queue_invalidate_wait(struct iommu *iommu,
> >      return -EOPNOTSUPP;
> >  }
> >
> > -static int __must_check invalidate_sync(struct iommu *iommu,
> > -                                        bool_t flush_dev_iotlb)
> > +static int __must_check invalidate_sync(struct iommu *iommu)
> >  {
> >      struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
> >
> >      ASSERT(qi_ctrl->qinval_maddr);
> >
> > -    return queue_invalidate_wait(iommu, 0, 1, 1, flush_dev_iotlb);
> > +    return queue_invalidate_wait(iommu, 0, 1, 1, 0); }
> > +
> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
> > +                                         struct pci_dev *pdev) {
> > +    struct domain *d = NULL;
> > +
> > +    if ( test_bit(did, iommu->domid_bitmap) )
> > +        d = rcu_lock_domain_by_id(iommu->domid_map[did]);
> > +
> > +    /*
> > +     * In case the domain has been freed or the IOMMU domid bitmap is
> > +     * not valid, the device no longer belongs to this domain.
> > +     */
> > +    if ( d == NULL )
> > +        return;
> > +
> > +    pcidevs_lock();
> > +    ASSERT(pdev->domain);
> > +    list_del(&pdev->domain_list);
> > +    pdev->domain = NULL;
> > +    pci_hide_existing_device(pdev);
> > +    pcidevs_unlock();
> > +
> > +    if ( !d->is_shutting_down && printk_ratelimit() )
> > +        printk(XENLOG_WARNING VTDPREFIX
> > +               " dom%d: ATS device %04x:%02x:%02x.%u flush failed\n",
> > +               d->domain_id, pdev->seg, pdev->bus,
> > +               PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> > +
> > +    if ( !is_hardware_domain(d) )
> > +        domain_crash(d);
> > +
> > +    rcu_unlock_domain(d);
> > +}
> 
> So in an earlier patch in this series you (supposedly) moved similar logic up 
> to
> the vendor independent layer. I think this then would better get moved up
> too, if at all possible.
> 

To be honest, I have not much reason for leaving domain crash here and I was 
aware of this problem, but crash_domain() here is not harmful (as the 
'd->is_shutting_down' is Set when to crash, and once the 'd->is_shutting_down' 
is Set then return  in domain_shutdown()  ).
In case crash domain directly, it may help us narrow down the 'window' (the 
domain is still running)..

To me, moving the logic up is acceptable.

In next version, could I only drop:

+    if ( !is_hardware_domain(d) )
+        domain_crash(d);

In this patch, and leave the rest as is ?

Quan



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