Re: [Xen-devel] [Patch v11 3/3] vt-d: fix vt-d Device-TLB flush timeout issue

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.06.16 at 10:15, <quan.xu@xxxxxxxxx> wrote:
> On June 17, 2016 3:01 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> >>> On 17.06.16 at 08:08, <quan.xu@xxxxxxxxx> wrote:
>> 
>> > On June 16, 2016 5:05 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> >> >>> On 16.06.16 at 10:42, <quan.xu@xxxxxxxxx> wrote:
>> >> > On June 02, 2016 7:07 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> >> >> >>> On 01.06.16 at 11:05, <quan.xu@xxxxxxxxx> wrote:
>> >> >> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16
>> did,
>> >> >> > +                                         struct pci_ats_dev
>> >> >> > +*ats_dev) {
>> >> >> > +    struct domain *d = NULL;
>> >> >> > +    struct pci_dev *pdev;
>> >> >> > +
>> >> >> > +    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();
>> >> >> > +
>> >> >> > +    for_each_pdev(d, pdev)
>> >> >> > +    {
>> >> >> > +        if ( (pdev->seg == ats_dev->seg) &&
>> >> >> > +             (pdev->bus == ats_dev->bus) &&
>> >> >> > +             (pdev->devfn == ats_dev->devfn) )
>> >> >> > +        {
>> >> >> > +            ASSERT(pdev->domain);
>> >> >> > +            list_del(&pdev->domain_list);
>> >> >> > +            pdev->domain = NULL;
>> >> >> > +            pci_hide_existing_device(pdev);
>> >> >> > +            break;
>> >> >> > +        }
>> >> >> > +    }
>> >> >> > +
>> >> >> > +    pcidevs_unlock();
>> >> >>
>> >> >> ... this loop (and locking). (Of course such a change may better
>> >> >> be done in another preparatory patch.)
>> >> >>
>> >> >
>> >> > To eliminate the locking?  I am afraid the locking is still a must
>> >> > here even without the loop, also referring  to device_assigned()..
>> >>
>> >> If the entire loop gets eliminated, what would be left is
>> >>
>> >>     pcidevs_lock();
>> >>     pcidevs_unlock();
>> >>
>> >> which I don't think does any good.
>> >
>> > Why? I can't follow it..
>> 
>> I don't understand your question. Can you explain what use above code
>> sequence is, in your opinion? Or else - what does the "why"
>> refer to?
>> 
> 
> Ah, there may be a gap between us. without this loop,  these pdev operation 
> should be still there, such as,
> 
> 
> +    ASSERT(pdev->domain);
> +    list_del(&pdev->domain_list);
> +    pdev->domain = NULL;
> +    pci_hide_existing_device(pdev);
> 
> So, the left is not only:
>    pcidevs_lock();
>    pcidevs_unlock();

Oh, indeed. My bad.

Jan


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