[PATCH v10 0/3] VT-d Device-TLB flush issue

[PATCH v10 0/3] VT-d Device-TLB flush issue

[Quan Xu]

This patches fix current timeout concern and also allow limited ATS support:

1. Add a timeout parameter for Queued Invalidation

 The parameter 'vtd_qi_timeout' specifies the timeout of the
 VT-d Queued 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.

2. Synchronize for Device-TLB flush one by one

 Today we do Device-TLB flush synchronization after issuing flush
 requests for all ATS devices belonging to a VM. Doing so however
 imposes a limitation, i.e. that we can not figure out which flush
 request is blocked in the flush queue list, based on VT-d spec.

 To prepare correct Device-TLB flush timeout handling in next patch,
 we change the behavior to synchronize for every Device-TLB flush
 request. So the Device-TLB flush interface is changed a little bit,
 by checking timeout within the function instead of outside of function.

 Accordingly we also do a similar change for flush interfaces of
 IOTLB/IEC/Context, i.e. moving synchronization into the function.
 Since there is no user of a non-synced interface, we just rename
 existing ones with _sync suffix.

3. Fix vt-d Device-TLB flush timeout issue

 If Device-TLB flush timed out, we hide the target ATS device
 immediately and crash the domain owning this ATS device. If
 impacted domain is hardware domain, just throw out a warning.

 By hiding the device, we make sure it can't be assigned to any
 domain any longer (see device_assigned).

**NOTE**
   This patch set should base on one prereq patch sets:
    a). Check VT-d Device-TLB flush error.

----
Not covered in this series:

    a) Eliminate the panic() in IOMMU_WAIT_OP, used only in VT-d register read/write.
       Further discussion is required on whether and how to improve it.
    b) Handle IOTLB/Context/IEC flush timeout.

--Changes in v10:
#patch1
   * Enhance commit message.
   * Enhance description in docs/misc/xen-command-line.markdown


#patch2
   * Enhance commit message.
   * Fix indentation issue.
   * Add __must_check anotation.
   * Remove redundant comment.
   * leave the existing logic as - best effort invalidation even when an error has occurred.

#patch3
   * Remove stray blanks.
   * Remove pointless local variable.
   * Add comment.
   * Add printk().

Quan Xu (3):
  vt-d: add a timeout parameter for Queued Invalidation
  vt-d: synchronize for Device-TLB flush one by one
  vt-d: fix vt-d Device-TLB flush timeout issue

 docs/misc/xen-command-line.markdown   |  10 +++
 xen/drivers/passthrough/pci.c         |   6 +-
 xen/drivers/passthrough/vtd/extern.h  |   6 +-
 xen/drivers/passthrough/vtd/qinval.c  | 141 ++++++++++++++++++++++++++--------
 xen/drivers/passthrough/vtd/x86/ats.c |  11 +--
 xen/include/xen/pci.h                 |   1 +
 6 files changed, 134 insertions(+), 41 deletions(-)

-- 
1.9.1


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

[PATCH v10 1/3] vt-d: add a timeout parameter for Queued Invalidation

[Quan Xu]

The parameter 'vtd_qi_timeout' specifies the timeout of the
VT-d Queued 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.

Signed-off-by: Quan Xu <quan.xu@intel.com>
---
 docs/misc/xen-command-line.markdown  | 10 ++++++++++
 xen/drivers/passthrough/vtd/qinval.c | 17 ++++++++++++-----
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index ca77e3b..0b83068 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1532,6 +1532,16 @@ Note that if **watchdog** option is also specified vpmu will be turned off.
 As the virtualisation is not 100% safe, don't use the vpmu flag on
 production systems (see http://xenbits.xen.org/xsa/advisory-163.html)!
 
+### vtd\_qi\_timeout (VT-d)
+> `= <integer>`
+
+> Default: `1`
+
+Specify the timeout of the VT-d Queued Invalidation in milliseconds.
+
+By default, the timeout is 1ms. When you see error 'Queue invalidate wait
+descriptor timed out', try increasing this value.
+
 ### watchdog
 > `= force | <boolean>`
 
diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c
index b81b0bd..52ba2c2 100644
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -28,6 +28,11 @@
 #include "vtd.h"
 #include "extern.h"
 
+static unsigned int __read_mostly vtd_qi_timeout = 1;
+integer_param("vtd_qi_timeout", vtd_qi_timeout);
+
+#define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1))
+
 static void print_qi_regs(struct iommu *iommu)
 {
     u64 val;
@@ -130,10 +135,10 @@ static void queue_invalidate_iotlb(struct iommu *iommu,
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 }
 
-static int queue_invalidate_wait(struct iommu *iommu,
+static int __must_check queue_invalidate_wait(struct iommu *iommu,
     u8 iflag, u8 sw, u8 fn)
 {
-    s_time_t start_time;
+    s_time_t timeout;
     volatile u32 poll_slot = QINVAL_STAT_INIT;
     unsigned int index;
     unsigned long flags;
@@ -164,13 +169,15 @@ static int queue_invalidate_wait(struct iommu *iommu,
     if ( sw )
     {
         /* In case all wait descriptor writes to same addr with same data */
-        start_time = NOW();
+        timeout = 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();
         }
-- 
1.9.1


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

Re: [PATCH v10 1/3] vt-d: add a timeout parameter for Queued Invalidation

[Jan Beulich]

>>> On 22.04.16 at 12:54, <quan.xu@intel.com> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1532,6 +1532,16 @@ Note that if **watchdog** option is also specified vpmu will be turned off.
>  As the virtualisation is not 100% safe, don't use the vpmu flag on
>  production systems (see http://xenbits.xen.org/xsa/advisory-163.html)! 
>  
> +### vtd\_qi\_timeout (VT-d)
> +> `= <integer>`
> +
> +> Default: `1`
> +
> +Specify the timeout of the VT-d Queued Invalidation in milliseconds.
> +
> +By default, the timeout is 1ms. When you see error 'Queue invalidate wait
> +descriptor timed out', try increasing this value.

So when someone enables ATS, will the 1ms timeout apply to the
dev iotlb invalidations too? If so, that's surely too short, and would
ideally be adjusted automatically, but the need for a higher timeout
in that case should in any event be mentioned here.

Apart from that aspect this patch seems to be ready, but will clearly
need a VT-d maintainer's ack.

Jan


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

Re: [PATCH v10 1/3] vt-d: add a timeout parameter for Queued Invalidation

[Xu, Quan]

On May 13, 2016 11:28 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 22.04.16 at 12:54, <quan.xu@intel.com> wrote:
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -1532,6 +1532,16 @@ Note that if **watchdog** option is also
> specified vpmu will be turned off.
> >  As the virtualisation is not 100% safe, don't use the vpmu flag on
> > production systems (see http://xenbits.xen.org/xsa/advisory-163.html)!
> >
> > +### vtd\_qi\_timeout (VT-d)
> > +> `= <integer>`
> > +
> > +> Default: `1`
> > +
> > +Specify the timeout of the VT-d Queued Invalidation in milliseconds.
> > +
> > +By default, the timeout is 1ms. When you see error 'Queue invalidate
> > +wait descriptor timed out', try increasing this value.
> 
> So when someone enables ATS, will the 1ms timeout apply to the dev iotlb
> invalidations too?

Yes,
The timeout is the same for IOTLB, Context, IEC and Device-TLB invalidation.

> If so, that's surely too short, and would ideally be adjusted
> automatically, but the need for a higher timeout in that case should in any
> event be mentioned here.

I can try to use 1ms for IOTLB, Context and  IEC invalidation. As mentioned, 1 ms is enough for IOTLB, Context and  IEC invalidation.
What about 10 ms for Device-TLB (10 ms is just a higher timeout,  no specific meaning)?

> 
> Apart from that aspect this patch seems to be ready, but will clearly need a VT-
> d maintainer's ack.
> 

Thanks for your review. I will also test this patch against the last commit ( I am still out of office and I will do it around this Wednesday).

Quan


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

Re: [PATCH v10 1/3] vt-d: add a timeout parameter for Queued Invalidation

[Tian, Kevin]

> From: Xu, Quan
> Sent: Monday, May 16, 2016 11:26 PM
> 
> On May 13, 2016 11:28 PM, Jan Beulich <JBeulich@suse.com> wrote:
> > >>> On 22.04.16 at 12:54, <quan.xu@intel.com> wrote:
> > > --- a/docs/misc/xen-command-line.markdown
> > > +++ b/docs/misc/xen-command-line.markdown
> > > @@ -1532,6 +1532,16 @@ Note that if **watchdog** option is also
> > specified vpmu will be turned off.
> > >  As the virtualisation is not 100% safe, don't use the vpmu flag on
> > > production systems (see http://xenbits.xen.org/xsa/advisory-163.html)!
> > >
> > > +### vtd\_qi\_timeout (VT-d)
> > > +> `= <integer>`
> > > +
> > > +> Default: `1`
> > > +
> > > +Specify the timeout of the VT-d Queued Invalidation in milliseconds.
> > > +
> > > +By default, the timeout is 1ms. When you see error 'Queue invalidate
> > > +wait descriptor timed out', try increasing this value.
> >
> > So when someone enables ATS, will the 1ms timeout apply to the dev iotlb
> > invalidations too?
> 
> Yes,
> The timeout is the same for IOTLB, Context, IEC and Device-TLB invalidation.
> 
> > If so, that's surely too short, and would ideally be adjusted
> > automatically, but the need for a higher timeout in that case should in any
> > event be mentioned here.
> 
> I can try to use 1ms for IOTLB, Context and  IEC invalidation. As mentioned, 1 ms is
> enough for IOTLB, Context and  IEC invalidation.
> What about 10 ms for Device-TLB (10 ms is just a higher timeout,  no specific meaning)?

I remember in earlier discussion we agreed to use 1ms as the default for both
IOMMU-side and device-side flushes. For device-side flushes, we checked internal
HW team that 1ms is a reasonable threshold for integrated devices. It's likely
insufficient for discrete devices. We may check any automatic adjustment method
later when it becomes a real problem. For now, please elaborate above information
in the text.

> 
> >
> > Apart from that aspect this patch seems to be ready, but will clearly need a VT-
> > d maintainer's ack.
> >
> 
> Thanks for your review. I will also test this patch against the last commit ( I am still out
> of office and I will do it around this Wednesday).
> 

Jan, do you want me to ack this series now, or wait until previous series 
handling error is checked in? (I thought you wanted the later in one of
your replies earlier). Either way is OK to me. :-)

Thanks
Kevin

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

Re: [PATCH v10 1/3] vt-d: add a timeout parameter for Queued Invalidation

[Jan Beulich]

>>> On 17.05.16 at 05:19, <kevin.tian@intel.com> wrote:
>>  From: Xu, Quan
>> Sent: Monday, May 16, 2016 11:26 PM
>> 
>> On May 13, 2016 11:28 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> > >>> On 22.04.16 at 12:54, <quan.xu@intel.com> wrote:
>> > > --- a/docs/misc/xen-command-line.markdown
>> > > +++ b/docs/misc/xen-command-line.markdown
>> > > @@ -1532,6 +1532,16 @@ Note that if **watchdog** option is also
>> > specified vpmu will be turned off.
>> > >  As the virtualisation is not 100% safe, don't use the vpmu flag on
>> > > production systems (see http://xenbits.xen.org/xsa/advisory-163.html)! 
>> > >
>> > > +### vtd\_qi\_timeout (VT-d)
>> > > +> `= <integer>`
>> > > +
>> > > +> Default: `1`
>> > > +
>> > > +Specify the timeout of the VT-d Queued Invalidation in milliseconds.
>> > > +
>> > > +By default, the timeout is 1ms. When you see error 'Queue invalidate
>> > > +wait descriptor timed out', try increasing this value.
>> >
>> > So when someone enables ATS, will the 1ms timeout apply to the dev iotlb
>> > invalidations too?
>> 
>> Yes,
>> The timeout is the same for IOTLB, Context, IEC and Device-TLB invalidation.
>> 
>> > If so, that's surely too short, and would ideally be adjusted
>> > automatically, but the need for a higher timeout in that case should in any
>> > event be mentioned here.
>> 
>> I can try to use 1ms for IOTLB, Context and  IEC invalidation. As mentioned, 1 ms is
>> enough for IOTLB, Context and  IEC invalidation.
>> What about 10 ms for Device-TLB (10 ms is just a higher timeout,  no specific meaning)?
> 
> I remember in earlier discussion we agreed to use 1ms as the default for both
> IOMMU-side and device-side flushes. For device-side flushes, we checked internal
> HW team that 1ms is a reasonable threshold for integrated devices. It's likely
> insufficient for discrete devices. We may check any automatic adjustment method
> later when it becomes a real problem. For now, please elaborate above information
> in the text.

Well, taking care of automation later is fine with me, but tying
everything to a single timeout, when device iotlb invalidation may
require a much larger value, isn't.

>> > Apart from that aspect this patch seems to be ready, but will clearly need a VT-
>> > d maintainer's ack.
>> >
>> 
>> Thanks for your review. I will also test this patch against the last commit ( I am still out
>> of office and I will do it around this Wednesday).
>> 
> 
> Jan, do you want me to ack this series now, or wait until previous series 
> handling error is checked in? (I thought you wanted the later in one of
> your replies earlier). Either way is OK to me. :-)

Well, I'm not to tell you when to ack a certain series. You should ack
it once it meets the criteria for you to ack it. The dependency on the
other series is what should be spelled out explicitly in this series, or
even better this series should be merged with the other one (to the
dependency make most obvious). But I'm repeating myself...

Jan


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

Re: [PATCH v10 1/3] vt-d: add a timeout parameter for Queued Invalidation

[Xu, Quan]

On May 17, 2016 3:48 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 17.05.16 at 05:19, <kevin.tian@intel.com> wrote:
> >>  From: Xu, Quan
> >> Sent: Monday, May 16, 2016 11:26 PM
> >>
> >> On May 13, 2016 11:28 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> > >>> On 22.04.16 at 12:54, <quan.xu@intel.com> wrote:
> >> > > --- a/docs/misc/xen-command-line.markdown
> >> > > +++ b/docs/misc/xen-command-line.markdown
> >> > > @@ -1532,6 +1532,16 @@ Note that if **watchdog** option is also
> >> > specified vpmu will be turned off.
> >> > >  As the virtualisation is not 100% safe, don't use the vpmu flag
> >> > > on production systems (see http://xenbits.xen.org/xsa/advisory-
> 163.html)!
> >> > >
> >> > > +### vtd\_qi\_timeout (VT-d)
> >> > > +> `= <integer>`
> >> > > +
> >> > > +> Default: `1`
> >> > > +
> >> > > +Specify the timeout of the VT-d Queued Invalidation in milliseconds.
> >> > > +
> >> > > +By default, the timeout is 1ms. When you see error 'Queue
> >> > > +invalidate wait descriptor timed out', try increasing this value.
> >> >
> >> > So when someone enables ATS, will the 1ms timeout apply to the dev
> >> > iotlb invalidations too?
> >>
> >> Yes,
> >> The timeout is the same for IOTLB, Context, IEC and Device-TLB invalidation.
> >>
> >> > If so, that's surely too short, and would ideally be adjusted
> >> > automatically, but the need for a higher timeout in that case
> >> > should in any event be mentioned here.
> >>
> >> I can try to use 1ms for IOTLB, Context and  IEC invalidation. As
> >> mentioned, 1 ms is enough for IOTLB, Context and  IEC invalidation.
> >> What about 10 ms for Device-TLB (10 ms is just a higher timeout,  no
> specific meaning)?
> >
> > I remember in earlier discussion we agreed to use 1ms as the default
> > for both IOMMU-side and device-side flushes. For device-side flushes,
> > we checked internal HW team that 1ms is a reasonable threshold for
> > integrated devices. It's likely insufficient for discrete devices. We
> > may check any automatic adjustment method later when it becomes a real
> > problem. For now, please elaborate above information in the text.
> 
> Well, taking care of automation later is fine with me, 
> but tying everything to a
> single timeout, when device iotlb invalidation may require a much larger value,
> isn't.
>

A little bit confused. Check it -- could I leave patch 1/3 as is? 

btw, I have tested it against the last commit, no conflict.


Quan

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

Re: [PATCH v10 1/3] vt-d: add a timeout parameter for Queued Invalidation

[Jan Beulich]

>>> On 18.05.16 at 14:53, <quan.xu@intel.com> wrote:
> On May 17, 2016 3:48 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 17.05.16 at 05:19, <kevin.tian@intel.com> wrote:
>> >>  From: Xu, Quan
>> >> Sent: Monday, May 16, 2016 11:26 PM
>> >>
>> >> On May 13, 2016 11:28 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >> > >>> On 22.04.16 at 12:54, <quan.xu@intel.com> wrote:
>> >> > > --- a/docs/misc/xen-command-line.markdown
>> >> > > +++ b/docs/misc/xen-command-line.markdown
>> >> > > @@ -1532,6 +1532,16 @@ Note that if **watchdog** option is also
>> >> > specified vpmu will be turned off.
>> >> > >  As the virtualisation is not 100% safe, don't use the vpmu flag
>> >> > > on production systems (see http://xenbits.xen.org/xsa/advisory- 
>> 163.html)!
>> >> > >
>> >> > > +### vtd\_qi\_timeout (VT-d)
>> >> > > +> `= <integer>`
>> >> > > +
>> >> > > +> Default: `1`
>> >> > > +
>> >> > > +Specify the timeout of the VT-d Queued Invalidation in milliseconds.
>> >> > > +
>> >> > > +By default, the timeout is 1ms. When you see error 'Queue
>> >> > > +invalidate wait descriptor timed out', try increasing this value.
>> >> >
>> >> > So when someone enables ATS, will the 1ms timeout apply to the dev
>> >> > iotlb invalidations too?
>> >>
>> >> Yes,
>> >> The timeout is the same for IOTLB, Context, IEC and Device-TLB invalidation.
>> >>
>> >> > If so, that's surely too short, and would ideally be adjusted
>> >> > automatically, but the need for a higher timeout in that case
>> >> > should in any event be mentioned here.
>> >>
>> >> I can try to use 1ms for IOTLB, Context and  IEC invalidation. As
>> >> mentioned, 1 ms is enough for IOTLB, Context and  IEC invalidation.
>> >> What about 10 ms for Device-TLB (10 ms is just a higher timeout,  no
>> specific meaning)?
>> >
>> > I remember in earlier discussion we agreed to use 1ms as the default
>> > for both IOMMU-side and device-side flushes. For device-side flushes,
>> > we checked internal HW team that 1ms is a reasonable threshold for
>> > integrated devices. It's likely insufficient for discrete devices. We
>> > may check any automatic adjustment method later when it becomes a real
>> > problem. For now, please elaborate above information in the text.
>> 
>> Well, taking care of automation later is fine with me, 
>> but tying everything to a
>> single timeout, when device iotlb invalidation may require a much larger value,
>> isn't.
>>
> 
> A little bit confused. Check it -- could I leave patch 1/3 as is? 

The patch can imo remain as is only if the new default timeout
is large enough for all possible cases (including those users
who are adventurous enough to turn on ATS).

> btw, I have tested it against the last commit, no conflict.

No idea what you mean to say with this.

Jan


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

Re: [PATCH v10 1/3] vt-d: add a timeout parameter for Queued Invalidation

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, May 18, 2016 11:05 PM
> 
> >>> On 18.05.16 at 14:53, <quan.xu@intel.com> wrote:
> > On May 17, 2016 3:48 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 17.05.16 at 05:19, <kevin.tian@intel.com> wrote:
> >> >>  From: Xu, Quan
> >> >> Sent: Monday, May 16, 2016 11:26 PM
> >> >>
> >> >> On May 13, 2016 11:28 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >> > >>> On 22.04.16 at 12:54, <quan.xu@intel.com> wrote:
> >> >> > > --- a/docs/misc/xen-command-line.markdown
> >> >> > > +++ b/docs/misc/xen-command-line.markdown
> >> >> > > @@ -1532,6 +1532,16 @@ Note that if **watchdog** option is also
> >> >> > specified vpmu will be turned off.
> >> >> > >  As the virtualisation is not 100% safe, don't use the vpmu flag
> >> >> > > on production systems (see http://xenbits.xen.org/xsa/advisory-
> >> 163.html)!
> >> >> > >
> >> >> > > +### vtd\_qi\_timeout (VT-d)
> >> >> > > +> `= <integer>`
> >> >> > > +
> >> >> > > +> Default: `1`
> >> >> > > +
> >> >> > > +Specify the timeout of the VT-d Queued Invalidation in milliseconds.
> >> >> > > +
> >> >> > > +By default, the timeout is 1ms. When you see error 'Queue
> >> >> > > +invalidate wait descriptor timed out', try increasing this value.
> >> >> >
> >> >> > So when someone enables ATS, will the 1ms timeout apply to the dev
> >> >> > iotlb invalidations too?
> >> >>
> >> >> Yes,
> >> >> The timeout is the same for IOTLB, Context, IEC and Device-TLB invalidation.
> >> >>
> >> >> > If so, that's surely too short, and would ideally be adjusted
> >> >> > automatically, but the need for a higher timeout in that case
> >> >> > should in any event be mentioned here.
> >> >>
> >> >> I can try to use 1ms for IOTLB, Context and  IEC invalidation. As
> >> >> mentioned, 1 ms is enough for IOTLB, Context and  IEC invalidation.
> >> >> What about 10 ms for Device-TLB (10 ms is just a higher timeout,  no
> >> specific meaning)?
> >> >
> >> > I remember in earlier discussion we agreed to use 1ms as the default
> >> > for both IOMMU-side and device-side flushes. For device-side flushes,
> >> > we checked internal HW team that 1ms is a reasonable threshold for
> >> > integrated devices. It's likely insufficient for discrete devices. We
> >> > may check any automatic adjustment method later when it becomes a real
> >> > problem. For now, please elaborate above information in the text.
> >>
> >> Well, taking care of automation later is fine with me,
> >> but tying everything to a
> >> single timeout, when device iotlb invalidation may require a much larger value,
> >> isn't.
> >>
> >
> > A little bit confused. Check it -- could I leave patch 1/3 as is?
> 
> The patch can imo remain as is only if the new default timeout
> is large enough for all possible cases (including those users
> who are adventurous enough to turn on ATS).
> 
> > btw, I have tested it against the last commit, no conflict.
> 
> No idea what you mean to say with this.
> 

A single default value for both IOMMU-side and device-side is
anyway not optimal. What about introducing a new knob
e.g. vtd_qi_device_timeout specifically for device-side flush
while using vtd_qi_timeout for other places? If device-side
timeout is not specified, it is then default to vtd_qi_timeout.

Thanks
Kevin

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

Re: [PATCH v10 1/3] vt-d: add a timeout parameter for Queued Invalidation

[Xu, Quan]

On May 19, 2016 8:33 AM, Tian, Kevin <kevin.tian@intel.com> wrote:
> > From: Jan Beulich [mailto:JBeulich@suse.com]
> > Sent: Wednesday, May 18, 2016 11:05 PM
> >
> > >>> On 18.05.16 at 14:53, <quan.xu@intel.com> wrote:
> > > On May 17, 2016 3:48 PM, Jan Beulich <JBeulich@suse.com> wrote:
> > >> >>> On 17.05.16 at 05:19, <kevin.tian@intel.com> wrote:
> > >> >>  From: Xu, Quan
> > >> >> Sent: Monday, May 16, 2016 11:26 PM
> > >> >>
> > >> >> On May 13, 2016 11:28 PM, Jan Beulich <JBeulich@suse.com> wrote:
> > >> >> > >>> On 22.04.16 at 12:54, <quan.xu@intel.com> wrote:
> > >> >> > > --- a/docs/misc/xen-command-line.markdown
> > >> >> > > +++ b/docs/misc/xen-command-line.markdown
> > >> >> > > @@ -1532,6 +1532,16 @@ Note that if **watchdog** option is
> > >> >> > > also
> > >> >> > specified vpmu will be turned off.
> > >> >> > >  As the virtualisation is not 100% safe, don't use the vpmu
> > >> >> > > flag on production systems (see
> > >> >> > > http://xenbits.xen.org/xsa/advisory-
> > >> 163.html)!
> > >> >> > >
> > >> >> > > +### vtd\_qi\_timeout (VT-d)
> > >> >> > > +> `= <integer>`
> > >> >> > > +
> > >> >> > > +> Default: `1`
> > >> >> > > +
> > >> >> > > +Specify the timeout of the VT-d Queued Invalidation in
> milliseconds.
> > >> >> > > +
> > >> >> > > +By default, the timeout is 1ms. When you see error 'Queue
> > >> >> > > +invalidate wait descriptor timed out', try increasing this value.
> > >> >> >
> > >> >> > So when someone enables ATS, will the 1ms timeout apply to the
> > >> >> > dev iotlb invalidations too?
> > >> >>
> > >> >> Yes,
> > >> >> The timeout is the same for IOTLB, Context, IEC and Device-TLB
> invalidation.
> > >> >>
> > >> >> > If so, that's surely too short, and would ideally be adjusted
> > >> >> > automatically, but the need for a higher timeout in that case
> > >> >> > should in any event be mentioned here.
> > >> >>
> > >> >> I can try to use 1ms for IOTLB, Context and  IEC invalidation.
> > >> >> As mentioned, 1 ms is enough for IOTLB, Context and  IEC invalidation.
> > >> >> What about 10 ms for Device-TLB (10 ms is just a higher timeout,
> > >> >> no
> > >> specific meaning)?
> > >> >
> > >> > I remember in earlier discussion we agreed to use 1ms as the
> > >> > default for both IOMMU-side and device-side flushes. For
> > >> > device-side flushes, we checked internal HW team that 1ms is a
> > >> > reasonable threshold for integrated devices. It's likely
> > >> > insufficient for discrete devices. We may check any automatic
> > >> > adjustment method later when it becomes a real problem. For now,
> please elaborate above information in the text.
> > >>
> > >> Well, taking care of automation later is fine with me, but tying
> > >> everything to a single timeout, when device iotlb invalidation may
> > >> require a much larger value, isn't.
> > >>
> > >
> > > A little bit confused. Check it -- could I leave patch 1/3 as is?
> >
> > The patch can imo remain as is only if the new default timeout is
> > large enough for all possible cases (including those users who are
> > adventurous enough to turn on ATS).
> >

Jan,

I only have an ATS device (MYRICOM Inc. Myri-10G Dual-Protocol NIC). 1 ms is large enough for invalidation so far.
Any suggestion for this new default timeout?


> A single default value for both IOMMU-side and device-side is anyway not
> optimal. What about introducing a new knob e.g. vtd_qi_device_timeout
> specifically for device-side flush while using vtd_qi_timeout for other places? If
> device-side timeout is not specified, it is then default to vtd_qi_timeout.
>

IMO, we are better to introduce only one  variable  for VT-d invalidation.
The users may be not interested in such a detailed VT-d knowledge. Also this is taking into consideration of  consistency.  :)

Quan

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

Re: [PATCH v10 1/3] vt-d: add a timeout parameter for Queued Invalidation

[Jan Beulich]

>>> "Xu, Quan" <quan.xu@intel.com> 05/19/16 3:35 AM >>>
>On May 19, 2016 8:33 AM, Tian, Kevin <kevin.tian@intel.com> wrote:
>> > From: Jan Beulich [mailto:JBeulich@suse.com]
>> > Sent: Wednesday, May 18, 2016 11:05 PM
>> > >>> On 18.05.16 at 14:53, <quan.xu@intel.com> wrote:
>> > The patch can imo remain as is only if the new default timeout is
>> > large enough for all possible cases (including those users who are
>> > adventurous enough to turn on ATS).
>
>I only have an ATS device (MYRICOM Inc. Myri-10G Dual-Protocol NIC). 1 ms is large enough for invalidation so far.
>Any suggestion for this new default timeout?

Unless you have theoretical proof that a lower than the current value would
suffice (as you do for the IOMMU side flushes), I think the default needs to
remain the same as it is right now (which iirc is already _much_ lower than
the real theoretical one).

>> A single default value for both IOMMU-side and device-side is anyway not
>> optimal. What about introducing a new knob e.g. vtd_qi_device_timeout
>> specifically for device-side flush while using vtd_qi_timeout for other places? If
>> device-side timeout is not specified, it is then default to vtd_qi_timeout.

There should imo be a single command line option, allowing for two values to be
passed (e.g. comma-separated).

Jan


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

Re: [PATCH v10 1/3] vt-d: add a timeout parameter for Queued Invalidation

[Xu, Quan]

On May 19, 2016 2:13 PM, Jan Beulich <jbeulich@suse.com> wrote:
> >>> "Xu, Quan" <quan.xu@intel.com> 05/19/16 3:35 AM >>>
> >On May 19, 2016 8:33 AM, Tian, Kevin <kevin.tian@intel.com> wrote:
> >> A single default value for both IOMMU-side and device-side is anyway
> >> not optimal. What about introducing a new knob e.g.
> >> vtd_qi_device_timeout specifically for device-side flush while using
> >> vtd_qi_timeout for other places? If device-side timeout is not specified, it is
> then default to vtd_qi_timeout.
> 
> There should imo be a single command line option, allowing for two values to
> be passed (e.g. comma-separated).
> 

As mentioned, 1 ms is enough for VT-d IOTLB/Context/IEC invalidation, so we are no need to increasing the value of timeout or introduce a boot-time changed parameter.
What about a constant (e.g. a macro), 1 ms, for VT-d IOTLB/Context/IEC invalidation timeout.

For Device-TLB invalidation, we can introduce 'vtd_qi_device_timeout', which is boot-time changed, and 1 ms by default.

Quan

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

Re: [PATCH v10 1/3] vt-d: add a timeout parameter for Queued Invalidation

[Jan Beulich]

>>> On 19.05.16 at 13:26, <quan.xu@intel.com> wrote:
> On May 19, 2016 2:13 PM, Jan Beulich <jbeulich@suse.com> wrote:
>> >>> "Xu, Quan" <quan.xu@intel.com> 05/19/16 3:35 AM >>>
>> >On May 19, 2016 8:33 AM, Tian, Kevin <kevin.tian@intel.com> wrote:
>> >> A single default value for both IOMMU-side and device-side is anyway
>> >> not optimal. What about introducing a new knob e.g.
>> >> vtd_qi_device_timeout specifically for device-side flush while using
>> >> vtd_qi_timeout for other places? If device-side timeout is not specified, it is
>> then default to vtd_qi_timeout.
>> 
>> There should imo be a single command line option, allowing for two values to
>> be passed (e.g. comma-separated).
> 
> As mentioned, 1 ms is enough for VT-d IOTLB/Context/IEC invalidation, so we 
> are no need to increasing the value of timeout or introduce a boot-time 
> changed parameter.
> What about a constant (e.g. a macro), 1 ms, for VT-d IOTLB/Context/IEC 
> invalidation timeout.

If you're absolutely certain no-one will ever find a need to increase
that value - sure.

> For Device-TLB invalidation, we can introduce 'vtd_qi_device_timeout', which 
> is boot-time changed, and 1 ms by default.

One question is whether making this a VT-d specific command line
option is a good idea: Other IOMMU implementations ought to be
in need of doing device IOTLB invalidation too, at least sooner or
later.

The other question is whether any timeout lower than the current
one can be considered safe (and hence is usable as a default).

Jan


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

Re: [PATCH v10 1/3] vt-d: add a timeout parameter for Queued Invalidation

[Xu, Quan]

On May 19, 2016 7:36 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 19.05.16 at 13:26, <quan.xu@intel.com> wrote:
> > On May 19, 2016 2:13 PM, Jan Beulich <jbeulich@suse.com> wrote:
> >> >>> "Xu, Quan" <quan.xu@intel.com> 05/19/16 3:35 AM >>>
> >> >On May 19, 2016 8:33 AM, Tian, Kevin <kevin.tian@intel.com> wrote:
> >> >> A single default value for both IOMMU-side and device-side is
> >> >> anyway not optimal. What about introducing a new knob e.g.
> >> >> vtd_qi_device_timeout specifically for device-side flush while
> >> >> using vtd_qi_timeout for other places? If device-side timeout is
> >> >> not specified, it is
> >> then default to vtd_qi_timeout.
> >>
> >> There should imo be a single command line option, allowing for two
> >> values to be passed (e.g. comma-separated).
> >
> > As mentioned, 1 ms is enough for VT-d IOTLB/Context/IEC invalidation,
> > so we are no need to increasing the value of timeout or introduce a
> > boot-time changed parameter.
> > What about a constant (e.g. a macro), 1 ms, for VT-d IOTLB/Context/IEC
> > invalidation timeout.
> 
> If you're absolutely certain no-one will ever find a need to increase that value -
> sure.
> 

Sure.
Also this was mentioned in Kevin's ' Revisit VT-d asynchronous flush issue ' , http://lists.xenproject.org/archives/html/xen-devel/2015-11/msg00041.html 

"""-For context/iotlb/iec flush, our measurements show worst cases <10us. We also confirmed with hardware team, that 1ms is large  enough for IOMMU internal flush. """

> > For Device-TLB invalidation, we can introduce 'vtd_qi_device_timeout',
> > which is boot-time changed, and 1 ms by default.
> 
> One question is whether making this a VT-d specific command line option is a
> good idea: Other IOMMU implementations ought to be in need of doing
> device IOTLB invalidation too, at least sooner or later.
> 

This is indeed farsighted. Agreed.

> The other question is whether any timeout lower than the current one can be
> considered safe (and hence is usable as a default).
> 

Actually the criticality, 1 ms, is from hardware team.
IOW, Our hardware team confirmed that 1ms should be enough for integrated PCI devices with ATS.
for discrete PCI devices with ATS, it's uncertain whether 1ms  or 10ms is too restrictive to them, but there are only a few devices now in the market.

Quan

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

[PATCH v10 2/3] vt-d: synchronize for Device-TLB flush one by one

[Quan Xu]

Today we do Device-TLB flush synchronization after issuing flush
requests for all ATS devices belonging to a VM. Doing so however
imposes a limitation, i.e. that we can not figure out which flush
request is blocked in the flush queue list, based on VT-d spec.

To prepare correct Device-TLB flush timeout handling in next patch,
we change the behavior to synchronize for every Device-TLB flush
request. So the Device-TLB flush interface is changed a little bit,
by checking timeout within the function instead of outside of function.

Accordingly we also do a similar change for flush interfaces of
IOTLB/IEC/Context, i.e. moving synchronization into the function.
Since there is no user of a non-synced interface, we just rename
existing ones with _sync suffix.

Signed-off-by: Quan Xu <quan.xu@intel.com>
---
 xen/drivers/passthrough/vtd/extern.h  |  5 +--
 xen/drivers/passthrough/vtd/qinval.c  | 61 +++++++++++++++++++++--------------
 xen/drivers/passthrough/vtd/x86/ats.c |  8 ++---
 3 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h
index d4d37c3..ab7ecad 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -59,8 +59,9 @@ int ats_device(const struct pci_dev *, const struct acpi_drhd_unit *);
 int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
                          u64 addr, unsigned int size_order, u64 type);
 
-int qinval_device_iotlb(struct iommu *iommu,
-                        u32 max_invs_pend, u16 sid, u16 size, u64 addr);
+int __must_check qinval_device_iotlb_sync(struct iommu *iommu,
+                                          u32 max_invs_pend,
+                                          u16 sid, u16 size, u64 addr);
 
 unsigned int get_cache_line_size(void);
 void cacheline_flush(char *);
diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c
index 52ba2c2..69cc6bf 100644
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -33,6 +33,8 @@ integer_param("vtd_qi_timeout", vtd_qi_timeout);
 
 #define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1))
 
+static int invalidate_sync(struct iommu *iommu);
+
 static void print_qi_regs(struct iommu *iommu)
 {
     u64 val;
@@ -72,8 +74,10 @@ static void qinval_update_qtail(struct iommu *iommu, unsigned int index)
     dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << QINVAL_INDEX_SHIFT));
 }
 
-static void queue_invalidate_context(struct iommu *iommu,
-    u16 did, u16 source_id, u8 function_mask, u8 granu)
+static int __must_check queue_invalidate_context_sync(struct iommu *iommu,
+                                                      u16 did, u16 source_id,
+                                                      u8 function_mask,
+                                                      u8 granu)
 {
     unsigned long flags;
     unsigned int index;
@@ -100,10 +104,14 @@ static void queue_invalidate_context(struct iommu *iommu,
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 
     unmap_vtd_domain_page(qinval_entries);
+
+    return invalidate_sync(iommu);
 }
 
-static void queue_invalidate_iotlb(struct iommu *iommu,
-    u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr)
+static int __must_check queue_invalidate_iotlb_sync(struct iommu *iommu,
+                                                    u8 granu, u8 dr, u8 dw,
+                                                    u16 did, u8 am, u8 ih,
+                                                    u64 addr)
 {
     unsigned long flags;
     unsigned int index;
@@ -133,10 +141,12 @@ static void queue_invalidate_iotlb(struct iommu *iommu,
     unmap_vtd_domain_page(qinval_entries);
     qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
+
+    return invalidate_sync(iommu);
 }
 
 static int __must_check queue_invalidate_wait(struct iommu *iommu,
-    u8 iflag, u8 sw, u8 fn)
+                                              u8 iflag, u8 sw, u8 fn)
 {
     s_time_t timeout;
     volatile u32 poll_slot = QINVAL_STAT_INIT;
@@ -196,8 +206,10 @@ static int invalidate_sync(struct iommu *iommu)
     return 0;
 }
 
-int qinval_device_iotlb(struct iommu *iommu,
-    u32 max_invs_pend, u16 sid, u16 size, u64 addr)
+int __must_check qinval_device_iotlb_sync(struct iommu *iommu,
+                                          u32 max_invs_pend,
+                                          u16 sid, u16 size,
+                                          u64 addr)
 {
     unsigned long flags;
     unsigned int index;
@@ -226,15 +238,17 @@ int qinval_device_iotlb(struct iommu *iommu,
     qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 
-    return 0;
+    return invalidate_sync(iommu);
 }
 
-static void queue_invalidate_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx)
+static int __must_check queue_invalidate_iec_sync(struct iommu *iommu,
+                                                  u8 granu, u8 im, u16 iidx)
 {
     unsigned long flags;
     unsigned int index;
     u64 entry_base;
     struct qinval_entry *qinval_entry, *qinval_entries;
+    int ret;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);
@@ -254,14 +268,9 @@ static void queue_invalidate_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx)
     unmap_vtd_domain_page(qinval_entries);
     qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
-}
 
-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);
+
     /*
      * reading vt-d architecture register will ensure
      * draining happens in implementation independent way.
@@ -273,12 +282,12 @@ static int __iommu_flush_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx)
 
 int iommu_flush_iec_global(struct iommu *iommu)
 {
-    return __iommu_flush_iec(iommu, IEC_GLOBAL_INVL, 0, 0);
+    return queue_invalidate_iec_sync(iommu, IEC_GLOBAL_INVL, 0, 0);
 }
 
 int iommu_flush_iec_index(struct iommu *iommu, u8 im, u16 iidx)
 {
-   return __iommu_flush_iec(iommu, IEC_INDEX_INVL, im, iidx);
+   return queue_invalidate_iec_sync(iommu, IEC_INDEX_INVL, im, iidx);
 }
 
 static int flush_context_qi(
@@ -304,11 +313,9 @@ static int flush_context_qi(
     }
 
     if ( qi_ctrl->qinval_maddr != 0 )
-    {
-        queue_invalidate_context(iommu, did, sid, fm,
-                                 type >> DMA_CCMD_INVL_GRANU_OFFSET);
-        ret = invalidate_sync(iommu);
-    }
+        ret = queue_invalidate_context_sync(iommu, did, sid, fm,
+                                            type >> DMA_CCMD_INVL_GRANU_OFFSET);
+
     return ret;
 }
 
@@ -346,9 +353,13 @@ static int flush_iotlb_qi(
         if (cap_read_drain(iommu->cap))
             dr = 1;
         /* Need to conside the ih bit later */
-        queue_invalidate_iotlb(iommu,
-                               type >> DMA_TLB_FLUSH_GRANU_OFFSET, dr,
-                               dw, did, size_order, 0, addr);
+        ret = queue_invalidate_iotlb_sync(iommu,
+                                          type >> DMA_TLB_FLUSH_GRANU_OFFSET,
+                                          dr, dw, did, size_order, 0, addr);
+
+        if ( ret )
+            return ret;
+
         if ( flush_dev_iotlb )
             ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
         rc = invalidate_sync(iommu);
diff --git a/xen/drivers/passthrough/vtd/x86/ats.c b/xen/drivers/passthrough/vtd/x86/ats.c
index 334b9c1..dfa4d30 100644
--- a/xen/drivers/passthrough/vtd/x86/ats.c
+++ b/xen/drivers/passthrough/vtd/x86/ats.c
@@ -134,8 +134,8 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
             /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
             sbit = 1;
             addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;
-            rc = qinval_device_iotlb(iommu, pdev->ats_queue_depth,
-                                     sid, sbit, addr);
+            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
+                                          sid, sbit, addr);
             break;
         case DMA_TLB_PSI_FLUSH:
             if ( !device_in_domain(iommu, pdev, did) )
@@ -154,8 +154,8 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
                 addr |= (((u64)1 << (size_order - 1)) - 1) << PAGE_SHIFT_4K;
             }
 
-            rc = qinval_device_iotlb(iommu, pdev->ats_queue_depth,
-                                     sid, sbit, addr);
+            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
+                                          sid, sbit, addr);
             break;
         default:
             dprintk(XENLOG_WARNING VTDPREFIX, "invalid vt-d flush type\n");
-- 
1.9.1


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

Re: [PATCH v10 2/3] vt-d: synchronize for Device-TLB flush one by one

[Jan Beulich]

>>> On 22.04.16 at 12:54, <quan.xu@intel.com> wrote:
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -33,6 +33,8 @@ integer_param("vtd_qi_timeout", vtd_qi_timeout);
>  
>  #define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1))
>  
> +static int invalidate_sync(struct iommu *iommu);

__must_check?

> -static void queue_invalidate_iotlb(struct iommu *iommu,
> -    u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr)
> +static int __must_check queue_invalidate_iotlb_sync(struct iommu *iommu,
> +                                                    u8 granu, u8 dr, u8 dw,
> +                                                    u16 did, u8 am, u8 ih,
> +                                                    u64 addr)
>  {
>      unsigned long flags;
>      unsigned int index;
> @@ -133,10 +141,12 @@ static void queue_invalidate_iotlb(struct iommu *iommu,
>      unmap_vtd_domain_page(qinval_entries);
>      qinval_update_qtail(iommu, index);
>      spin_unlock_irqrestore(&iommu->register_lock, flags);
> +
> +    return invalidate_sync(iommu);
>  }

With this, ...

> @@ -346,9 +353,13 @@ static int flush_iotlb_qi(
>          if (cap_read_drain(iommu->cap))
>              dr = 1;
>          /* Need to conside the ih bit later */
> -        queue_invalidate_iotlb(iommu,
> -                               type >> DMA_TLB_FLUSH_GRANU_OFFSET, dr,
> -                               dw, did, size_order, 0, addr);
> +        ret = queue_invalidate_iotlb_sync(iommu,
> +                                          type >> DMA_TLB_FLUSH_GRANU_OFFSET,
> +                                          dr, dw, did, size_order, 0, addr);
> +
> +        if ( ret )
> +            return ret;
> +
>          if ( flush_dev_iotlb )
>              ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
>          rc = invalidate_sync(iommu);

... why does this invalidate_sync() not go away?

Jan


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

Re: [PATCH v10 2/3] vt-d: synchronize for Device-TLB flush one by one

[Xu, Quan]

On May 17, 2016 8:37 PM, Jan Beulich <JBeulich@suse.com>wrote:
> >>> On 22.04.16 at 12:54, <quan.xu@intel.com> wrote:
> > --- a/xen/drivers/passthrough/vtd/qinval.c
> > +++ b/xen/drivers/passthrough/vtd/qinval.c
> > @@ -33,6 +33,8 @@ integer_param("vtd_qi_timeout", vtd_qi_timeout);
> >
> >  #define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1))
> >
> > +static int invalidate_sync(struct iommu *iommu);
> 
> __must_check?
> 

Yes, I will add it.


> > -static void queue_invalidate_iotlb(struct iommu *iommu,
> > -    u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr)
> > +static int __must_check queue_invalidate_iotlb_sync(struct iommu
> *iommu,
> > +                                                    u8 granu, u8 dr, u8 dw,
> > +                                                    u16 did, u8 am, u8 ih,
> > +                                                    u64 addr)
> >  {
> >      unsigned long flags;
> >      unsigned int index;
> > @@ -133,10 +141,12 @@ static void queue_invalidate_iotlb(struct iommu
> *iommu,
> >      unmap_vtd_domain_page(qinval_entries);
> >      qinval_update_qtail(iommu, index);
> >      spin_unlock_irqrestore(&iommu->register_lock, flags);
> > +
> > +    return invalidate_sync(iommu);
> >  }
> 
> With this, ...
> 
> > @@ -346,9 +353,13 @@ static int flush_iotlb_qi(
> >          if (cap_read_drain(iommu->cap))
> >              dr = 1;
> >          /* Need to conside the ih bit later */
> > -        queue_invalidate_iotlb(iommu,
> > -                               type >> DMA_TLB_FLUSH_GRANU_OFFSET, dr,
> > -                               dw, did, size_order, 0, addr);
> > +        ret = queue_invalidate_iotlb_sync(iommu,
> > +                                          type >> DMA_TLB_FLUSH_GRANU_OFFSET,
> > +                                          dr, dw, did, size_order, 0, addr);
> > +
> > +        if ( ret )
> > +            return ret;
> > +
> >          if ( flush_dev_iotlb )
> >              ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
> >          rc = invalidate_sync(iommu);
> 
> ... why does this invalidate_sync() not go away?
> 

Oh, it is your suggestion -- leaving the existing logic as is would be better - best effort
invalidation even when an error has occurred.

http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg00523.html

Quan




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

Re: [PATCH v10 2/3] vt-d: synchronize for Device-TLB flush one by one

[Jan Beulich]

>>> On 18.05.16 at 10:53, <quan.xu@intel.com> wrote:
> On May 17, 2016 8:37 PM, Jan Beulich <JBeulich@suse.com>wrote:
>> >>> On 22.04.16 at 12:54, <quan.xu@intel.com> wrote:
>> > -static void queue_invalidate_iotlb(struct iommu *iommu,
>> > -    u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr)
>> > +static int __must_check queue_invalidate_iotlb_sync(struct iommu
>> *iommu,
>> > +                                                    u8 granu, u8 dr, u8 dw,
>> > +                                                    u16 did, u8 am, u8 ih,
>> > +                                                    u64 addr)
>> >  {
>> >      unsigned long flags;
>> >      unsigned int index;
>> > @@ -133,10 +141,12 @@ static void queue_invalidate_iotlb(struct iommu
>> *iommu,
>> >      unmap_vtd_domain_page(qinval_entries);
>> >      qinval_update_qtail(iommu, index);
>> >      spin_unlock_irqrestore(&iommu->register_lock, flags);
>> > +
>> > +    return invalidate_sync(iommu);
>> >  }
>> 
>> With this, ...
>> 
>> > @@ -346,9 +353,13 @@ static int flush_iotlb_qi(
>> >          if (cap_read_drain(iommu->cap))
>> >              dr = 1;
>> >          /* Need to conside the ih bit later */
>> > -        queue_invalidate_iotlb(iommu,
>> > -                               type >> DMA_TLB_FLUSH_GRANU_OFFSET, dr,
>> > -                               dw, did, size_order, 0, addr);
>> > +        ret = queue_invalidate_iotlb_sync(iommu,
>> > +                                          type >> DMA_TLB_FLUSH_GRANU_OFFSET,
>> > +                                          dr, dw, did, size_order, 0, addr);
>> > +
>> > +        if ( ret )
>> > +            return ret;
>> > +
>> >          if ( flush_dev_iotlb )
>> >              ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
>> >          rc = invalidate_sync(iommu);
>> 
>> ... why does this invalidate_sync() not go away?
>> 
> 
> Oh, it is your suggestion -- leaving the existing logic as is would be better - 
> best effort invalidation even when an error has occurred.
> 
> http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg00523.html 

Look like this was a bad comment of mine (resulting from
dev_invalidate_iotlb(), other than the other respective functions,
not getting a _sync tag added), and I would have appreciated if
you had simply pointed out the redundancy. Please remember
that the review process is bi-directional, and hence doesn't mean
you need to blindly do everything a reviewer asks for: Things you
agree with should be changed in code. For things you don't agree
with you should reply verbally, explaining why a requested change
shouldn't be done.

Jan


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

Re: [PATCH v10 2/3] vt-d: synchronize for Device-TLB flush one by one

[Xu, Quan]

On May 18, 2016 5:29 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 18.05.16 at 10:53, <quan.xu@intel.com> wrote:
> > On May 17, 2016 8:37 PM, Jan Beulich <JBeulich@suse.com>wrote:
> >> >>> On 22.04.16 at 12:54, <quan.xu@intel.com> wrote:
> >> > -static void queue_invalidate_iotlb(struct iommu *iommu,
> >> > -    u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr)
> >> > +static int __must_check queue_invalidate_iotlb_sync(struct iommu
> >> *iommu,
> >> > +                                                    u8 granu, u8 dr, u8 dw,
> >> > +                                                    u16 did, u8 am, u8 ih,
> >> > +                                                    u64 addr)
> >> >  {
> >> >      unsigned long flags;
> >> >      unsigned int index;
> >> > @@ -133,10 +141,12 @@ static void queue_invalidate_iotlb(struct
> >> > iommu
> >> *iommu,
> >> >      unmap_vtd_domain_page(qinval_entries);
> >> >      qinval_update_qtail(iommu, index);
> >> >      spin_unlock_irqrestore(&iommu->register_lock, flags);
> >> > +
> >> > +    return invalidate_sync(iommu);
> >> >  }
> >>
> >> With this, ...
> >>
> >> > @@ -346,9 +353,13 @@ static int flush_iotlb_qi(
> >> >          if (cap_read_drain(iommu->cap))
> >> >              dr = 1;
> >> >          /* Need to conside the ih bit later */
> >> > -        queue_invalidate_iotlb(iommu,
> >> > -                               type >> DMA_TLB_FLUSH_GRANU_OFFSET, dr,
> >> > -                               dw, did, size_order, 0, addr);
> >> > +        ret = queue_invalidate_iotlb_sync(iommu,
> >> > +                                          type >> DMA_TLB_FLUSH_GRANU_OFFSET,
> >> > +                                          dr, dw, did, size_order,
> >> > + 0, addr);
> >> > +
> >> > +        if ( ret )
> >> > +            return ret;
> >> > +
> >> >          if ( flush_dev_iotlb )
> >> >              ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
> >> >          rc = invalidate_sync(iommu);
> >>
> >> ... why does this invalidate_sync() not go away?
> >>
> >
> > Oh, it is your suggestion -- leaving the existing logic as is would be
> > better - best effort invalidation even when an error has occurred.
> >
> > http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg00523.h
> > tml
> 
> Look like this was a bad comment of mine (resulting from
> dev_invalidate_iotlb(), other than the other respective functions, not getting a
> _sync tag added), and I would have appreciated if you had simply pointed out
> the redundancy.

I just issued an open for this point in v9 discussion. I felt a strange, but really didn't have obvious reasons at that time.
--
I'll  drop this invalidate_sync() in v11.

> Please remember that the review process is bi-directional,
> and hence doesn't mean you need to blindly do everything a reviewer asks for:
> Things you agree with should be changed in code. For things you don't agree
> with you should reply verbally, explaining why a requested change shouldn't
> be done.
> 
Thanks. I will try to follow it.


Quan

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

[PATCH v10 3/3] vt-d: fix vt-d Device-TLB flush timeout issue

[Quan Xu]

If Device-TLB flush timed out, we hide the target ATS device
immediately and crash the domain owning this ATS device. If
impacted domain is hardware domain, just throw out a warning.

By hiding the device, we make sure it can't be assigned to any
domain any longer (see device_assigned).

Signed-off-by: Quan Xu <quan.xu@intel.com>
---
 xen/drivers/passthrough/pci.c         |  6 +--
 xen/drivers/passthrough/vtd/extern.h  |  5 ++-
 xen/drivers/passthrough/vtd/qinval.c  | 71 ++++++++++++++++++++++++++++++++---
 xen/drivers/passthrough/vtd/x86/ats.c | 11 +++---
 xen/include/xen/pci.h                 |  1 +
 5 files changed, 79 insertions(+), 15 deletions(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 9f1716a..9a214c6 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -420,7 +420,7 @@ static void free_pdev(struct pci_seg *pseg, struct pci_dev *pdev)
     xfree(pdev);
 }
 
-static void _pci_hide_device(struct pci_dev *pdev)
+void pci_hide_existing_device(struct pci_dev *pdev)
 {
     if ( pdev->domain )
         return;
@@ -437,7 +437,7 @@ int __init pci_hide_device(int bus, int devfn)
     pdev = alloc_pdev(get_pseg(0), bus, devfn);
     if ( pdev )
     {
-        _pci_hide_device(pdev);
+        pci_hide_existing_device(pdev);
         rc = 0;
     }
     pcidevs_unlock();
@@ -467,7 +467,7 @@ int __init pci_ro_device(int seg, int bus, int devfn)
     }
 
     __set_bit(PCI_BDF2(bus, devfn), pseg->ro_map);
-    _pci_hide_device(pdev);
+    pci_hide_existing_device(pdev);
 
     return 0;
 }
diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h
index ab7ecad..b54a15c 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -60,8 +60,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
                          u64 addr, unsigned int size_order, u64 type);
 
 int __must_check qinval_device_iotlb_sync(struct iommu *iommu,
-                                          u32 max_invs_pend,
-                                          u16 sid, u16 size, u64 addr);
+                                          u32 max_invs_pend, u16 did,
+                                          u16 seg, u8 bus, u8 devfn,
+                                          u16 size, u64 addr);
 
 unsigned int get_cache_line_size(void);
 void cacheline_flush(char *);
diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c
index 69cc6bf..c795e6b 100644
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -206,10 +206,71 @@ static int invalidate_sync(struct iommu *iommu)
     return 0;
 }
 
+static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
+                                         u16 seg, u8 bus, u8 devfn)
+{
+    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 == seg) &&
+             (pdev->bus == bus) &&
+             (pdev->devfn == devfn) )
+        {
+            ASSERT(pdev->domain);
+            list_del(&pdev->domain_list);
+            pdev->domain = NULL;
+            pci_hide_existing_device(pdev);
+            break;
+        }
+    }
+
+    pcidevs_unlock();
+
+    if ( !is_hardware_domain(d) )
+        domain_crash(d);
+    else
+        printk(XENLOG_WARNING VTDPREFIX
+               " dom%d: ATS device %04x:%02x:%02x.%u flush failed.\n",
+               d->domain_id,
+               seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
+
+    rcu_unlock_domain(d);
+}
+
+int dev_invalidate_sync(struct iommu *iommu, u16 did,
+                        u16 seg, u8 bus, u8 devfn)
+{
+    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);
+        if ( rc == -ETIMEDOUT )
+            dev_invalidate_iotlb_timeout(iommu, did, seg, bus, devfn);
+    }
+
+    return rc;
+}
+
 int __must_check qinval_device_iotlb_sync(struct iommu *iommu,
-                                          u32 max_invs_pend,
-                                          u16 sid, u16 size,
-                                          u64 addr)
+                                          u32 max_invs_pend, u16 did,
+                                          u16 seg, u8 bus, u8 devfn,
+                                          u16 size, u64 addr)
 {
     unsigned long flags;
     unsigned int index;
@@ -227,7 +288,7 @@ int __must_check qinval_device_iotlb_sync(struct iommu *iommu,
     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_1 = 0;
     qinval_entry->q.dev_iotlb_inv_dsc.lo.max_invs_pend = max_invs_pend;
     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_2 = 0;
-    qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = sid;
+    qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = PCI_BDF2(bus, devfn);
     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_3 = 0;
 
     qinval_entry->q.dev_iotlb_inv_dsc.hi.size = size;
@@ -238,7 +299,7 @@ int __must_check qinval_device_iotlb_sync(struct iommu *iommu,
     qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 
-    return invalidate_sync(iommu);
+    return dev_invalidate_sync(iommu, did, seg, bus, devfn);
 }
 
 static int __must_check queue_invalidate_iec_sync(struct iommu *iommu,
diff --git a/xen/drivers/passthrough/vtd/x86/ats.c b/xen/drivers/passthrough/vtd/x86/ats.c
index dfa4d30..50190f2 100644
--- a/xen/drivers/passthrough/vtd/x86/ats.c
+++ b/xen/drivers/passthrough/vtd/x86/ats.c
@@ -116,7 +116,6 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
 
     list_for_each_entry( pdev, &ats_devices, list )
     {
-        u16 sid = PCI_BDF2(pdev->bus, pdev->devfn);
         bool_t sbit;
         int rc = 0;
 
@@ -134,8 +133,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
             /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
             sbit = 1;
             addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;
-            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
-                                          sid, sbit, addr);
+            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, did,
+                                          pdev->seg, pdev->bus, pdev->devfn,
+                                          sbit, addr);
             break;
         case DMA_TLB_PSI_FLUSH:
             if ( !device_in_domain(iommu, pdev, did) )
@@ -154,8 +154,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
                 addr |= (((u64)1 << (size_order - 1)) - 1) << PAGE_SHIFT_4K;
             }
 
-            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
-                                          sid, sbit, addr);
+            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, did,
+                                          pdev->seg, pdev->bus, pdev->devfn,
+                                          sbit, addr);
             break;
         default:
             dprintk(XENLOG_WARNING VTDPREFIX, "invalid vt-d flush type\n");
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 6ed29dd..e4940cd 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -118,6 +118,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
 int pci_remove_device(u16 seg, u8 bus, u8 devfn);
 int pci_ro_device(int seg, int bus, int devfn);
 int pci_hide_device(int bus, int devfn);
+void pci_hide_existing_device(struct pci_dev *pdev);
 struct pci_dev *pci_get_pdev(int seg, int bus, int devfn);
 struct pci_dev *pci_get_real_pdev(int seg, int bus, int devfn);
 struct pci_dev *pci_get_pdev_by_domain(
-- 
1.9.1


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

Re: [PATCH v10 3/3] vt-d: fix vt-d Device-TLB flush timeout issue

[Jan Beulich]

>>> On 22.04.16 at 12:54, <quan.xu@intel.com> wrote:
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -206,10 +206,71 @@ static int invalidate_sync(struct iommu *iommu)
>      return 0;
>  }
>  
> +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
> +                                         u16 seg, u8 bus, u8 devfn)
> +{
> +    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 == seg) &&
> +             (pdev->bus == bus) &&
> +             (pdev->devfn == devfn) )
> +        {
> +            ASSERT(pdev->domain);
> +            list_del(&pdev->domain_list);
> +            pdev->domain = NULL;
> +            pci_hide_existing_device(pdev);
> +            break;
> +        }
> +    }

A loop like this is of course not ideal (especially for Dom0, which may
have many devices). And I wonder why you, ...

> @@ -134,8 +133,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
>              /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
>              sbit = 1;
>              addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;
> -            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> -                                          sid, sbit, addr);
> +            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, did,
> +                                          pdev->seg, pdev->bus, pdev->devfn,
> +                                          sbit, addr);
>              break;
>          case DMA_TLB_PSI_FLUSH:
>              if ( !device_in_domain(iommu, pdev, did) )
> @@ -154,8 +154,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
>                  addr |= (((u64)1 << (size_order - 1)) - 1) << PAGE_SHIFT_4K;
>              }
>  
> -            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> -                                          sid, sbit, addr);
> +            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, did,
> +                                          pdev->seg, pdev->bus, pdev->devfn,
> +                                          sbit, addr);
>              break;

... holding pdev in your hands here, don't just pass it down (which
at once would make the function signature less convoluted: you
could even eliminate the currently 2nd parameter that way).

Jan


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

Re: [PATCH v10 3/3] vt-d: fix vt-d Device-TLB flush timeout issue

[Xu, Quan]

On May 17, 2016 10:00 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 22.04.16 at 12:54, <quan.xu@intel.com> wrote:
> > --- a/xen/drivers/passthrough/vtd/qinval.c
> > +++ b/xen/drivers/passthrough/vtd/qinval.c
> > @@ -206,10 +206,71 @@ static int invalidate_sync(struct iommu *iommu)
> >      return 0;
> >  }
> >
> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
> > +                                         u16 seg, u8 bus, u8 devfn) {
> > +    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 == seg) &&
> > +             (pdev->bus == bus) &&
> > +             (pdev->devfn == devfn) )
> > +        {
> > +            ASSERT(pdev->domain);
> > +            list_del(&pdev->domain_list);
> > +            pdev->domain = NULL;
> > +            pci_hide_existing_device(pdev);
> > +            break;
> > +        }
> > +    }
> 
> A loop like this is of course not ideal (especially for Dom0, which may have
> many devices). And I wonder why you, ...
> 
> > @@ -134,8 +133,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16
> did,
> >              /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
> >              sbit = 1;
> >              addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;
> > -            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> > -                                          sid, sbit, addr);
> > +            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, did,
> > +                                          pdev->seg, pdev->bus, pdev->devfn,
> > +                                          sbit, addr);
> >              break;
> >          case DMA_TLB_PSI_FLUSH:
> >              if ( !device_in_domain(iommu, pdev, did) ) @@ -154,8
> > +154,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
> >                  addr |= (((u64)1 << (size_order - 1)) - 1) << PAGE_SHIFT_4K;
> >              }
> >
> > -            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> > -                                          sid, sbit, addr);
> > +            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, did,
> > +                                          pdev->seg, pdev->bus, pdev->devfn,
> > +                                          sbit, addr);
> >              break;
> 
> ... holding pdev in your hands here, don't just pass it down (which at once
> would make the function signature less convoluted: you could even eliminate
> the currently 2nd parameter that way).
> 

Good idea!!

Quan

















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

Re: [PATCH v10 3/3] vt-d: fix vt-d Device-TLB flush timeout issue

[Xu, Quan]

On May 17, 2016 10:00 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 22.04.16 at 12:54, <quan.xu@intel.com> wrote:
> > --- a/xen/drivers/passthrough/vtd/qinval.c
> > +++ b/xen/drivers/passthrough/vtd/qinval.c
> > @@ -206,10 +206,71 @@ static int invalidate_sync(struct iommu *iommu)
> >      return 0;
> >  }
> >
> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
> > +                                         u16 seg, u8 bus, u8 devfn) {
> > +    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 == seg) &&
> > +             (pdev->bus == bus) &&
> > +             (pdev->devfn == devfn) )
> > +        {
> > +            ASSERT(pdev->domain);
> > +            list_del(&pdev->domain_list);
> > +            pdev->domain = NULL;
> > +            pci_hide_existing_device(pdev);
> > +            break;
> > +        }
> > +    }
> 
> A loop like this is of course not ideal (especially for Dom0, which may have
> many devices). And I wonder why you, ...
> 
> > @@ -134,8 +133,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16
> did,
> >              /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
> >              sbit = 1;
> >              addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;
> > -            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> > -                                          sid, sbit, addr);
> > +            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, did,
> > +                                          pdev->seg, pdev->bus, pdev->devfn,
> > +                                          sbit, addr);
> >              break;
> >          case DMA_TLB_PSI_FLUSH:
> >              if ( !device_in_domain(iommu, pdev, did) ) @@ -154,8
> > +154,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
> >                  addr |= (((u64)1 << (size_order - 1)) - 1) << PAGE_SHIFT_4K;
> >              }
> >
> > -            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> > -                                          sid, sbit, addr);
> > +            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, did,
> > +                                          pdev->seg, pdev->bus, pdev->devfn,
> > +                                          sbit, addr);
> >              break;
> 
> ... holding pdev in your hands here, don't just pass it down (which at once
> would make the function signature less convoluted: you could even eliminate
> the currently 2nd parameter that way).
> 

Jan, 
    I am afraid we need to leave it as is.. this pdev , in dev_invalidate_iotlb(), is 'struct pci_ats_dev',
but we need a 'struct pci_dev' to hide device in dev_invalidate_iotlb_timeout().

'struct pci_ats_dev' and 'struct pci_dev' are quite different, however, SBDF is connection between them..

Quan

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

Re: [PATCH v10 3/3] vt-d: fix vt-d Device-TLB flush timeout issue

[Jan Beulich]

>>> On 20.05.16 at 09:15, <quan.xu@intel.com> wrote:
> On May 17, 2016 10:00 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 22.04.16 at 12:54, <quan.xu@intel.com> wrote:
>> > --- a/xen/drivers/passthrough/vtd/qinval.c
>> > +++ b/xen/drivers/passthrough/vtd/qinval.c
>> > @@ -206,10 +206,71 @@ static int invalidate_sync(struct iommu *iommu)
>> >      return 0;
>> >  }
>> >
>> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
>> > +                                         u16 seg, u8 bus, u8 devfn) {
>> > +    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 == seg) &&
>> > +             (pdev->bus == bus) &&
>> > +             (pdev->devfn == devfn) )
>> > +        {
>> > +            ASSERT(pdev->domain);
>> > +            list_del(&pdev->domain_list);
>> > +            pdev->domain = NULL;
>> > +            pci_hide_existing_device(pdev);
>> > +            break;
>> > +        }
>> > +    }
>> 
>> A loop like this is of course not ideal (especially for Dom0, which may have
>> many devices). And I wonder why you, ...
>> 
>> > @@ -134,8 +133,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16
>> did,
>> >              /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 
> */
>> >              sbit = 1;
>> >              addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;
>> > -            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
>> > -                                          sid, sbit, addr);
>> > +            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, did,
>> > +                                          pdev->seg, pdev->bus, pdev->devfn,
>> > +                                          sbit, addr);
>> >              break;
>> >          case DMA_TLB_PSI_FLUSH:
>> >              if ( !device_in_domain(iommu, pdev, did) ) @@ -154,8
>> > +154,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
>> >                  addr |= (((u64)1 << (size_order - 1)) - 1) << PAGE_SHIFT_4K;
>> >              }
>> >
>> > -            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
>> > -                                          sid, sbit, addr);
>> > +            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, did,
>> > +                                          pdev->seg, pdev->bus, pdev->devfn,
>> > +                                          sbit, addr);
>> >              break;
>> 
>> ... holding pdev in your hands here, don't just pass it down (which at once
>> would make the function signature less convoluted: you could even eliminate
>> the currently 2nd parameter that way).
> 
>     I am afraid we need to leave it as is.. this pdev , in 
> dev_invalidate_iotlb(), is 'struct pci_ats_dev',
> but we need a 'struct pci_dev' to hide device in 
> dev_invalidate_iotlb_timeout().
> 
> 'struct pci_ats_dev' and 'struct pci_dev' are quite different, however, SBDF 
> is connection between them..

Oh, indeed. Yet - can't enable_ats_device() be passed a
struct pci_dev *, and that be stored instead of SBDF inside
struct pci_ats_dev?

Jan


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

Re: [PATCH v10 3/3] vt-d: fix vt-d Device-TLB flush timeout issue

[Xu, Quan]

On May 20, 2016 5:59 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 20.05.16 at 09:15, <quan.xu@intel.com> wrote:
> > On May 17, 2016 10:00 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 22.04.16 at 12:54, <quan.xu@intel.com> wrote:
> >> > --- a/xen/drivers/passthrough/vtd/qinval.c
> >> > +++ b/xen/drivers/passthrough/vtd/qinval.c
> >> > @@ -206,10 +206,71 @@ static int invalidate_sync(struct iommu
> *iommu)
> >> >      return 0;
> >> >  }
> >> >
> >> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
> >> > +                                         u16 seg, u8 bus, u8 devfn) {
> >> > +    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 == seg) &&
> >> > +             (pdev->bus == bus) &&
> >> > +             (pdev->devfn == devfn) )
> >> > +        {
> >> > +            ASSERT(pdev->domain);
> >> > +            list_del(&pdev->domain_list);
> >> > +            pdev->domain = NULL;
> >> > +            pci_hide_existing_device(pdev);
> >> > +            break;
> >> > +        }
> >> > +    }
> >>
> >> A loop like this is of course not ideal (especially for Dom0, which
> >> may have many devices). And I wonder why you, ...
> >>
> >> > @@ -134,8 +133,9 @@ int dev_invalidate_iotlb(struct iommu *iommu,
> >> > u16
> >> did,
> >> >              /* invalidate all translations:
> >> > sbit=1,bit_63=0,bit[62:12]=1
> > */
> >> >              sbit = 1;
> >> >              addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;
> >> > -            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> >> > -                                          sid, sbit, addr);
> >> > +            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> did,
> >> > +                                          pdev->seg, pdev->bus, pdev->devfn,
> >> > +                                          sbit, addr);
> >> >              break;
> >> >          case DMA_TLB_PSI_FLUSH:
> >> >              if ( !device_in_domain(iommu, pdev, did) ) @@ -154,8
> >> > +154,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
> >> >                  addr |= (((u64)1 << (size_order - 1)) - 1) << PAGE_SHIFT_4K;
> >> >              }
> >> >
> >> > -            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> >> > -                                          sid, sbit, addr);
> >> > +            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> did,
> >> > +                                          pdev->seg, pdev->bus, pdev->devfn,
> >> > +                                          sbit, addr);
> >> >              break;
> >>
> >> ... holding pdev in your hands here, don't just pass it down (which
> >> at once would make the function signature less convoluted: you could
> >> even eliminate the currently 2nd parameter that way).
> >
> >     I am afraid we need to leave it as is.. this pdev , in
> > dev_invalidate_iotlb(), is 'struct pci_ats_dev', but we need a 'struct
> > pci_dev' to hide device in dev_invalidate_iotlb_timeout().
> >
> > 'struct pci_ats_dev' and 'struct pci_dev' are quite different,
> > however, SBDF is connection between them..
> 
> Oh, indeed. Yet - can't enable_ats_device() be passed a struct pci_dev *, and
> that be stored instead of SBDF inside struct pci_ats_dev?
> 

Make sense.  I appreciate your specific advice.

Quan

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