[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



>>> 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:
>> > --- 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.

Well, it's yours, so you ought to be able to explain it.

>> 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.)

Which would not eliminate the loop - pci_get_pdev_by_domain()
does have a loop itself.

> 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.

I don't see why variables would need renaming. If you need a
variable of type struct pci_dev * in a function which already has
a variable named pdev, simply name the new variable differently
(e.g. pci_dev).

>> 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()..

If the entire loop gets eliminated, what would be left is

    pcidevs_lock();
    pcidevs_unlock();

which I don't think does any good.

>> > +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;

Right. And perhaps a separate patch to make invalidate_sync()
do the same. Question is whether this really ought to be a
conditional, or whether instead this code is unreachable when
qinval is not in use, in which case these conditionals would imo
better be converted to ASSERT()s.

> 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;

Well, that's a correct observation, but not a big problem imo: If this
was a per-domain resource, it surely would need fixing. But if freeing
a couple of these pages (one per IOMMU) causes synchronization
headaches (e.g. because there may still be dangling references to
it), then I think freeing them is not a must. But if freeing them is safe
(like you seem to imply), then I'm certainly fine with you fixing this
(not that my opinion would matter much here, as I'm not the
maintainer of this code).

Jan

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.