Re: [Xen-devel] [Patch v11 1/3] IOMMU: add a timeout parameter for device IOTLB invalidation

[Xu, Quan]

On June 02, 2016 6:25 PM, Jan Beulich  wrote:
> >>> On 01.06.16 at 11:05,  wrote:
> > From: Quan Xu 
> > 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.
> 

Got it. It should be:

---
v11:
   - ...
   - ...
---


> > --- 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
> > +> `= `
> > +
> > +> 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.
> 

I really misunderstood the 'current value', which should be about 
'DMAR_OPERATION_TIMEOUT MILLISECS(1000) ', instead of ' IOMMU_QI_TIMEOUT 
MILLISECS(1)' in my patch.
So the default is 1000.
 
> > --- 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)?
> 

Agreed. VTD_QI_TIMEOUT is a better one.

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


I prefer this later one.

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

Re: [Xen-devel] [Patch v11 1/3] IOMMU: add a timeout parameter for device IOTLB invalidation

[Jan Beulich]

>>> On 01.06.16 at 11:05,  wrote:
> From: Quan Xu 
> 
> 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
> +> `= `
> +
> +> 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

[Xen-devel] [Patch v11 1/3] IOMMU: add a timeout parameter for device IOTLB invalidation

[Xu, Quan]

From: Quan Xu 

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.

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.

Signed-off-by: Quan Xu 
---
 docs/misc/xen-command-line.markdown  |  9 +
 xen/drivers/passthrough/iommu.c  |  3 +++
 xen/drivers/passthrough/vtd/qinval.c | 34 +++---
 xen/include/xen/iommu.h  |  2 ++
 4 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown 
b/docs/misc/xen-command-line.markdown
index b4bae11..34a0f9c 100644
--- 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
+> `= `
+
+> Default: `1`
+
+Specify the timeout of the device IOTLB invalidation in milliseconds.
+By default, the timeout is 1 ms. When you see error 'Queue invalidate
+wait descriptor timed out', try increasing this value.
+
 ### iommu\_inclusive\_mapping (VT-d)
 > `= `
 
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 098b601..e12de69 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -24,6 +24,9 @@
 static void parse_iommu_param(char *s);
 static void iommu_dump_p2m_table(unsigned char key);
 
+unsigned int __read_mostly iommu_dev_iotlb_timeout = 1;
+integer_param("iommu_dev_iotlb_timeout", iommu_dev_iotlb_timeout);
+
 /*
  * The 'iommu' parameter enables the IOMMU.  Optional comma separated
  * value may contain:
diff --git a/xen/drivers/passthrough/vtd/qinval.c 
b/xen/drivers/passthrough/vtd/qinval.c
index aa7841a..1a37565 100644
--- 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)
+
 static void print_qi_regs(struct iommu *iommu)
 {
 u64 val;
@@ -130,10 +132,10 @@ static void queue_invalidate_iotlb(struct iommu *iommu,
 spin_unlock_irqrestore(>register_lock, flags);
 }
 
-static int queue_invalidate_wait(struct iommu *iommu,
-u8 iflag, u8 sw, u8 fn)
+static int __must_check queue_invalidate_wait(struct iommu *iommu,
+  u8 iflag, u8 sw, u8 fn,
+  bool_t flush_dev_iotlb)
 {
-s_time_t start_time;
 volatile u32 poll_slot = QINVAL_STAT_INIT;
 unsigned int index;
 unsigned long flags;
@@ -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)) :
+  (NOW() + IOMMU_QI_TIMEOUT);
+
 while ( poll_slot != QINVAL_STAT_DONE )
 {
-if ( NOW() > (start_time + DMAR_OPERATION_TIMEOUT) )
+if ( NOW() > timeout )
 {
 print_qi_regs(iommu);
-panic("queue invalidate wait descriptor was not executed");
+printk(XENLOG_WARNING VTDPREFIX
+   " Queue invalidate wait descriptor timed out\n");
+return -ETIMEDOUT;
 }
 cpu_relax();
 }
@@ -180,12 +189,14 @@ static int queue_invalidate_wait(struct iommu *iommu,
 return -EOPNOTSUPP;
 }
 
-static int invalidate_sync(struct iommu *iommu)
+static int __must_check invalidate_sync(struct iommu *iommu,
+bool_t flush_dev_iotlb)
 {
 struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
 
 if ( qi_ctrl->qinval_maddr )
-return queue_invalidate_wait(iommu, 0, 1, 1);
+return queue_invalidate_wait(iommu, 0, 1, 1, flush_dev_iotlb);
+
 return 0;
 }
 
@@ -254,7 +265,7 @@ static int __iommu_flush_iec(struct iommu *iommu, u8 granu, 
u8 im, u16 iidx)
 int ret;
 
 queue_invalidate_iec(iommu, granu, im, iidx);
-ret = invalidate_sync(iommu);
+ret = invalidate_sync(iommu, 0);
 /*
  * reading vt-d architecture register will ensure
  * draining happens in implementation independent way.
@@ -300,7 +311,7 @@ static int __must_check flush_context_qi(void *_iommu, u16 
did,
 {