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, feng.wu@intel.com,
	kevin.tian@intel.com, xen-devel@lists.xen.org
Subject: Re: [Patch v11 1/3] IOMMU: add a timeout parameter for device IOTLB invalidation
Date: Thu, 02 Jun 2016 04:24:49 -0600	[thread overview]
Message-ID: <5750259102000078000F0D75@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1464771922-7794-2-git-send-email-quan.xu@intel.com>

>>> On 01.06.16 at 11:05, <quan.xu@intel.com> wrote:
> From: Quan Xu <quan.xu@intel.com>
> 
> The parameter 'iommu_dev_iotlb_timeout' specifies the timeout of
> the device IOTLB invalidation in milliseconds. By default, the
> timeout is 1ms, which can be boot-time changed.
> 
> Add a __must_check annotation. The followup patch titled
> 'VT-d IOTLB/Context/IEC flush issue' addresses the __mustcheck.
> That is the other callers of this routine (two or three levels up)
> ignore the return code. This patch does not address this but the
> other does.

The description really needs to mention that the dropping of
the panic() is intentional, and why.

> v11: Change the timeout parameter from 'vtd_qi_timeout' to
>     'iommu_dev_iotlb_timeout', which is not only for VT-d device
>     IOTLB invalidation, but also for other IOMMU implementations.

This goes after the first --- separator.

> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -996,6 +996,15 @@ debug hypervisor only).
>  
>  >> Enable IOMMU debugging code (implies `verbose`).
>  
> +### iommu\_dev\_iotlb\_timeout
> +> `= <integer>`
> +
> +> Default: `1`

So on v10 I had made clear that any timeout reduction from its
current value is, for the ATS case, not acceptable, unless you have
proof that this lower value will fit all past, present, and future
devices. Otherwise we're risking a regression here.

> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -28,6 +28,8 @@
>  #include "vtd.h"
>  #include "extern.h"
>  
> +#define IOMMU_QI_TIMEOUT MILLISECS(1)

May I suggest VTD_QI_TIMEOUT (but see also below)?

> @@ -163,14 +165,21 @@ static int queue_invalidate_wait(struct iommu *iommu,
>      /* Now we don't support interrupt method */
>      if ( sw )
>      {
> +        s_time_t timeout;
> +
>          /* In case all wait descriptor writes to same addr with same data */
> -        start_time = NOW();
> +        timeout = flush_dev_iotlb ?
> +                  (NOW() + iommu_dev_iotlb_timeout * MILLISECS(1)) :

MILLISECS(iommu_dev_iotlb_timeout)

> +                  (NOW() + IOMMU_QI_TIMEOUT);

Or really the whole expression should probably simply become

        timeout = NOW() + MILLISECS(flush_dev_iotlb ? iommu_dev_iotlb_timeout : VTD_QI_TIMEOUT);

(of course with VTD_QI_TIMEOUT having its MILLISECS() dropped,
and suitably line wrapped).

> @@ -344,10 +355,11 @@ static int __must_check flush_iotlb_qi(void *_iommu, u16 did, u64 addr,
>                                 dw, did, size_order, 0, addr);
>          if ( flush_dev_iotlb )
>              ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
> -        rc = invalidate_sync(iommu);
> +        rc = invalidate_sync(iommu, flush_dev_iotlb);
>          if ( !ret )
>              ret = rc;
>      }
> +
>      return ret;

Once again an addition of a blank line that doesn't belong here.

Jan


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

  reply	other threads:[~2016-06-02 10:24 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 [this message]
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
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=5750259102000078000F0D75@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 1/3] IOMMU: add a timeout parameter for device IOTLB invalidation' \
    /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).