xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: "Jan Beulich" <JBeulich@suse.com>
To: Quan Xu <quan.xu@intel.com>
Cc: "dario.faggioli@citrix.com" <dario.faggioli@citrix.com>,
	Feng Wu <feng.wu@intel.com>, Kevin Tian <kevin.tian@intel.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Subject: Re: [Patch v11 3/3] vt-d: fix vt-d Device-TLB flush timeout issue
Date: Thu, 16 Jun 2016 03:04:32 -0600	[thread overview]
Message-ID: <576287C002000078000F5946@prv-mh.provo.novell.com> (raw)
In-Reply-To: <945CA011AD5F084CBEA3E851C0AB28894B8E6711@SHSMSX103.ccr.corp.intel.com>

>>> On 16.06.16 at 10:42, <quan.xu@intel.com> wrote:
> On June 02, 2016 7:07 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 01.06.16 at 11:05, <quan.xu@intel.com> 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@lists.xen.org
http://lists.xen.org/xen-devel

  reply	other threads:[~2016-06-16  9:04 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-01  9:05 [Patch v11 0/3] VT-d Device-TLB flush issue Xu, Quan
2016-06-01  9:05 ` [Patch v11 1/3] IOMMU: add a timeout parameter for device IOTLB invalidation Xu, Quan
2016-06-02 10:24   ` Jan Beulich
2016-06-15  2:55     ` Xu, Quan
2016-06-01  9:05 ` [Patch v11 2/3] vt-d: synchronize for Device-TLB flush one by one Xu, Quan
2016-06-02 10:49   ` Jan Beulich
2016-06-01  9:05 ` [Patch v11 3/3] vt-d: fix vt-d Device-TLB flush timeout issue Xu, Quan
2016-06-02 11:07   ` Jan Beulich
2016-06-16  8:42     ` Xu, Quan
2016-06-16  9:04       ` Jan Beulich [this message]
2016-06-17  6:08         ` Xu, Quan
2016-06-17  7:00           ` Jan Beulich
2016-06-17  8:15             ` Xu, Quan
2016-06-17  8:40               ` Jan Beulich
2016-06-22 15:54             ` Xu, Quan
2016-06-22 16:18               ` Jan Beulich
2016-06-23  2:08                 ` Xu, Quan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=576287C002000078000F5946@prv-mh.provo.novell.com \
    --to=jbeulich@suse.com \
    --cc=dario.faggioli@citrix.com \
    --cc=feng.wu@intel.com \
    --cc=kevin.tian@intel.com \
    --cc=quan.xu@intel.com \
    --cc=xen-devel@lists.xen.org \
    --subject='Re: [Patch v11 3/3] vt-d: fix vt-d Device-TLB flush timeout issue' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).