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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.06.16 at 14:56, <quan.xu@xxxxxxxxx> wrote:
> 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 ?

Not really - the entire function looks like it could move out of vtd/,
as I can't see anything VT-d specific in it.

Jan


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