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

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

(* I will CC arm/amd maintainer after this vt-d specific discussion, and then 
send out my proposal...)

On June 02, 2016 7:07 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 01.06.16 at 11:05, <quan.xu@xxxxxxxxx> wrote:
> > --- a/xen/drivers/passthrough/vtd/extern.h
> > +++ b/xen/drivers/passthrough/vtd/extern.h
> > @@ -21,6 +21,7 @@
> >  #define _VTD_EXTERN_H_
> >
> >  #include "dmar.h"
> > +#include "../ats.h"
> 
> Why? You don't de-reference struct pci_ats_dev * in this file, so all you'd 
> need
> is a forward declaration. But then this is not in line with your v11 change
> description above, so I wonder whether you actually sent a stale patch.

Sorry, this patch is really strange to me.

> After all I thought the v10 discussion (see
> http://lists.xenproject.org/archives/html/xen-devel/2016-
> 05/msg02208.html
> ) had made clear that this passing down,

Sure, what you said is very clear. But I read these code again, I found a  
pci_get_pdev_by_domain()
Can also get *pdev without below loop.. (also hardware domain calls 
pci_get_pdev_by_domain() to get pdev.)

To be honest,  now I don't like to add a struct pci_dev * inside struct 
pci_ats_dev, as I need to change
' struct pci_ats_dev *pdev' to ' struct pci_ats_dev * pci_ats_dev ' in some 
functions as well.


> besides reducing the number of
> arguments of some function, would also be meant to eliminate ...
> 
> > +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()..


> 
> > +static int __must_check dev_invalidate_sync(struct iommu *iommu, u16
> did,
> > +                                            struct pci_ats_dev
> > +*ats_dev) {
> > +    struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
> > +    int rc = 0;
> > +
> > +    if ( qi_ctrl->qinval_maddr )
> > +    {
> > +        rc = queue_invalidate_wait(iommu, 0, 1, 1, 1);
> > +
> > +        if ( rc == -ETIMEDOUT )
> > +            dev_invalidate_iotlb_timeout(iommu, did, ats_dev);
> > +    }
> > +
> > +    return rc;
> > +}
> 
> I've never really understood why invalidate_sync() returns success when it
> didn't do anything. Now that you copy this same behavior here, I really need
> to ask you to explain that.
> 

It is acceptable to me, returning success when it didn't do anything -- this is 
worth reflection and criticism:(..
It is better:
+    if ( qi_ctrl->qinval_maddr )
+        ...
+    else
+        rc = -ENOENT;

A question:
I find the page related to qi_ctrl->qinval_maddr is not freed at all. IMO,
In disable_qinval (), we need to do:
     - free the page related to qi_ctrl->qinval_maddr.
     - qi_ctrl->qinval_maddr = 0;

Right?

Quan

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