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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.06.16 at 09:06, <quan.xu@xxxxxxxxx> wrote:
> On June 27, 2016 11:21 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> >>> 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.
>> 
> 
> Yes, it could be out of vtd, and then benefit arm/amd  IOMMU to hide ATS 
> device.
> 
> But 'did'  and 'iommu->domid_bitmap' are really vtd specific. Both of them 
> are 
> to get domain* structure, not a big deal, and then I can use domain_id 
> instead.
> 
> IMO, the domain* structure is a must here,

I agree, I did overlook that domain lookup. The common code
function should be passed a struct domain *, as you suggest.

Jan


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