[PATCH 0/2] VT-d flush issue

[PATCH 0/2] VT-d flush issue

[Quan Xu]

This patches are based on Kevin Tian's previous discussion 'Revisit VT-d asynchronous flush issue'.
Fix current timeout concern and also allow limited ATS support in a light way:

1. Reduce spin timeout to 1ms, which can be boot-time changed with 'iommu_qi_timeout_ms'.
   For example:
           multiboot /boot/xen.gz ats=1 iommu_qi_timeout_ms=100

2. Fix vt-d flush timeout issue.

    If IOTLB/Context/IETC flush is timeout, we should think all devices under this IOMMU cannot function correctly.
    So for each device under this IOMMU we'll mark it as unassignable and kill the domain owning the device.

    If Device-TLB flush is timeout, we'll mark the target ATS device as unassignable and kill the domain owning
    this device. When the invalidation request descriptor is timeout, hypervisor cannot find out which Device-TLB
    invalidate descriptor submitted before is not correct. So mark all of the domain's ATS devices as unassignable.

    If impacted domain is Dom0 or hardware domain, just throw out a warning. It's an open here whether we want to kill
    Dom0 or hardware domain (or directly panic hypervisor). Comments are welcomed.

    Device marked as unassignable will be disallowed to be further assigned to any domain.

*Kevin Tian did basic functional review.

Quan Xu (2):
  VT-d: Reduce spin timeout to 1ms, which can be boot-time changed.
  VT-d: Fix vt-d flush timeout issue.

 xen/drivers/passthrough/vtd/iommu.c  |   6 ++
 xen/drivers/passthrough/vtd/iommu.h  |   2 +
 xen/drivers/passthrough/vtd/qinval.c | 111 ++++++++++++++++++++++++++++++++++-
 xen/include/xen/pci.h                |   7 +++
 4 files changed, 123 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH 1/2] VT-d: Reduce spin timeout to 1ms, which can be boot-time changed.

[Quan Xu]

Signed-off-by: Quan Xu <quan.xu@intel.com>
---
 xen/drivers/passthrough/vtd/qinval.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c
index b81b0bd..990baf2 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 int __read_mostly iommu_qi_timeout_ms = 1;
+integer_param("iommu_qi_timeout_ms", iommu_qi_timeout_ms);
+
+#define IOMMU_QI_TIMEOUT (iommu_qi_timeout_ms * MILLISECS(1))
+
 static void print_qi_regs(struct iommu *iommu)
 {
     u64 val;
@@ -167,10 +172,12 @@ static int queue_invalidate_wait(struct iommu *iommu,
         start_time = NOW();
         while ( poll_slot != QINVAL_STAT_DONE )
         {
-            if ( NOW() > (start_time + DMAR_OPERATION_TIMEOUT) )
+            if ( NOW() > (start_time + IOMMU_QI_TIMEOUT) )
             {
                 print_qi_regs(iommu);
-                panic("queue invalidate wait descriptor was not executed");
+                dprintk(XENLOG_WARNING VTDPREFIX,
+                        "Queue invalidate wait descriptor was timeout.\n");
+                return -ETIMEDOUT;
             }
             cpu_relax();
         }
-- 
1.9.1

[PATCH 2/2] VT-d: Fix vt-d flush timeout issue.

[Quan Xu]

If IOTLB/Context/IETC flush is timeout, we should think
all devices under this IOMMU cannot function correctly.
So for each device under this IOMMU we'll mark it as
unassignable and kill the domain owning the device.

If Device-TLB flush is timeout, we'll mark the target
ATS device as unassignable and kill the domain owning
this device. When the invalidation request descriptor
is timeout, hypervisor cannot find out which Device-TLB
invalidate descriptor submitted before is not correct.
So mark all of the domain's ATS devices as unassignable.

If impacted domain is Dom0 or hardware domain, just throw
out a warning. It's an open here whether we want to kill
Dom0 or hardware domain (or directly panic hypervisor).
Comments are welcomed.

Device marked as unassignable will be disallowed to be
further assigned to any domain.

Signed-off-by: Quan Xu <quan.xu@intel.com>
---
 xen/drivers/passthrough/vtd/iommu.c  |   6 +++
 xen/drivers/passthrough/vtd/iommu.h  |   2 +
 xen/drivers/passthrough/vtd/qinval.c | 100 ++++++++++++++++++++++++++++++++++-
 xen/include/xen/pci.h                |   7 +++
 4 files changed, 114 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index dd13865..9317adb 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1890,6 +1890,9 @@ static int intel_iommu_add_device(u8 devfn, struct pci_dev *pdev)
     if ( !pdev->domain )
         return -EINVAL;
 
+    if ( IS_PDEV_UNASSIGNABLE(pdev) )
+        return -EACCES;
+
     ret = domain_context_mapping(pdev->domain, devfn, pdev);
     if ( ret )
     {
@@ -2301,6 +2304,9 @@ static int intel_iommu_assign_device(
     if ( list_empty(&acpi_drhd_units) )
         return -ENODEV;
 
+    if ( IS_PDEV_UNASSIGNABLE(pdev) )
+        return -EACCES;
+
     seg = pdev->seg;
     bus = pdev->bus;
     /*
diff --git a/xen/drivers/passthrough/vtd/iommu.h b/xen/drivers/passthrough/vtd/iommu.h
index ac71ed1..9437c42 100644
--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -452,6 +452,8 @@ struct qinval_entry {
 
 #define RESERVED_VAL        0
 
+#define INVALID_DID  ((u16)~0)
+
 #define TYPE_INVAL_CONTEXT      0x1
 #define TYPE_INVAL_IOTLB        0x2
 #define TYPE_INVAL_DEVICE_IOTLB 0x3
diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c
index 990baf2..786134f 100644
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -27,12 +27,66 @@
 #include "dmar.h"
 #include "vtd.h"
 #include "extern.h"
+#include "../ats.h"
 
 static int __read_mostly iommu_qi_timeout_ms = 1;
 integer_param("iommu_qi_timeout_ms", iommu_qi_timeout_ms);
 
 #define IOMMU_QI_TIMEOUT (iommu_qi_timeout_ms * MILLISECS(1))
 
+static void invalidate_timeout(struct iommu *iommu, int type, u16 did)
+{
+    struct domain *d;
+    unsigned long nr_dom, i;
+    struct pci_dev *pdev;
+
+    switch (type) {
+    case TYPE_INVAL_IOTLB:
+    case TYPE_INVAL_CONTEXT:
+    case TYPE_INVAL_IEC:
+        nr_dom = cap_ndoms(iommu->cap);
+        i = find_first_bit(iommu->domid_bitmap, nr_dom);
+        while ( i < nr_dom ) {
+            d = rcu_lock_domain_by_id(iommu->domid_map[i]);
+            ASSERT(d);
+
+            /* Mark the devices as unassignable. */
+            for_each_pdev(d, pdev)
+                mark_pdev_unassignable(pdev);
+            if ( d != hardware_domain && d->domain_id != 0 )
+                domain_kill(d);
+
+            rcu_unlock_domain(d);
+            i = find_next_bit(iommu->domid_bitmap, nr_dom, i + 1);
+        }
+        break;
+
+    case TYPE_INVAL_DEVICE_IOTLB:
+        d = rcu_lock_domain_by_id(iommu->domid_map[did]);
+        ASSERT(d);
+
+        /*
+         * When the invalidation request descriptor is timeout,
+         * hypervisor cannot find out which Device-TLB invalidate
+         * descriptor submitted before is not correct. So mark all
+         * of the domain's ATS devices as unassignable.
+         */
+        for_each_pdev(d, pdev)
+            if ( pci_ats_enabled(pdev->seg, pdev->bus, pdev->devfn) )
+                mark_pdev_unassignable(pdev);
+
+        if ( d != hardware_domain && d->domain_id != 0 )
+            domain_kill(d);
+        rcu_unlock_domain(d);
+        break;
+
+    default:
+        dprintk(XENLOG_WARNING VTDPREFIX, "Invalid VT-d flush type.\n");
+        break;
+
+    }
+}
+
 static void print_qi_regs(struct iommu *iommu)
 {
     u64 val;
@@ -262,6 +316,14 @@ static int __iommu_flush_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx)
 
     queue_invalidate_iec(iommu, granu, im, iidx);
     ret = invalidate_sync(iommu);
+
+    if ( ret == -ETIMEDOUT )
+    {
+        invalidate_timeout(iommu, TYPE_INVAL_IEC, INVALID_DID);
+        dprintk(XENLOG_WARNING VTDPREFIX,
+                "IEC flush timeout.\n");
+        return ret;
+    }
     /*
      * reading vt-d architecture register will ensure
      * draining happens in implementation independent way.
@@ -308,6 +370,14 @@ static int flush_context_qi(
         queue_invalidate_context(iommu, did, sid, fm,
                                  type >> DMA_CCMD_INVL_GRANU_OFFSET);
         ret = invalidate_sync(iommu);
+        if ( ret == -ETIMEDOUT )
+        {
+            invalidate_timeout(iommu, TYPE_INVAL_CONTEXT,
+                               INVALID_DID);
+            dprintk(XENLOG_WARNING  VTDPREFIX,
+                    "Context flush timeout.\n");
+            return ret;
+        }
     }
     return ret;
 }
@@ -349,9 +419,37 @@ static int flush_iotlb_qi(
         queue_invalidate_iotlb(iommu,
                                type >> DMA_TLB_FLUSH_GRANU_OFFSET, dr,
                                dw, did, size_order, 0, addr);
+
+        /*
+         * Synchronize with hardware for invalidation request descriptors
+         * submitted before Device-TLB invalidate descriptor.
+         */
+        rc = invalidate_sync(iommu);
+        if ( ret == -ETIMEDOUT )
+        {
+            invalidate_timeout(iommu, TYPE_INVAL_IOTLB, INVALID_DID);
+            dprintk(XENLOG_WARNING VTDPREFIX, "IOTLB flush timeout.\n");
+            return ret;
+        }
+
         if ( flush_dev_iotlb )
+        {
+            /*
+             * Synchronize with hardware for Device-TLB invalidate
+             * descriptor.
+             */
             ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
-        rc = invalidate_sync(iommu);
+            rc = invalidate_sync(iommu);
+
+            if ( ret == -ETIMEDOUT )
+            {
+                invalidate_timeout(iommu, TYPE_INVAL_DEVICE_IOTLB, did);
+                dprintk(XENLOG_WARNING VTDPREFIX,
+                        "Device-TLB flush timeout.\n");
+                return ret;
+            }
+        }
+
         if ( !ret )
             ret = rc;
     }
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index a5aef55..0bf6b1a 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -41,6 +41,7 @@
 struct pci_dev_info {
     bool_t is_extfn;
     bool_t is_virtfn;
+    bool_t is_unassignable;
     struct {
         u8 bus;
         u8 devfn;
@@ -88,6 +89,12 @@ struct pci_dev {
 #define for_each_pdev(domain, pdev) \
     list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
 
+#define PDEV_UNASSIGNABLE 1
+#define mark_pdev_unassignable(pdev) \
+    pdev->info.is_unassignable = PDEV_UNASSIGNABLE
+
+#define IS_PDEV_UNASSIGNABLE(pdev) pdev->info.is_unassignable
+
 /*
  * The pcidevs_lock protect alldevs_list, and the assignment for the 
  * devices, it also sync the access to the msi capability that is not
-- 
1.9.1

Re: [PATCH 0/2] VT-d flush issue

[Jan Beulich]

>>> On 03.12.15 at 09:09, <quan.xu@intel.com> wrote:
>     If impacted domain is Dom0 or hardware domain, just throw out a warning. 
> It's an open here whether we want to kill
>     Dom0 or hardware domain (or directly panic hypervisor). Comments are 
> welcomed.

I think that's a reasonable default, provided by that "Dom0 or
hardware domain" you really just mean the same domain known
with two different names (i.e. Dom0 should not be special when
it is not the hardware domain).

Jan

Re: [PATCH 0/2] VT-d flush issue

[Xu, Quan]

On 03.12.2015 at 9:16am, <JBeulich@suse.com> wrote:
> >>> On 03.12.15 at 09:09, <quan.xu@intel.com> wrote:
> >     If impacted domain is Dom0 or hardware domain, just throw out a
> warning.
> > It's an open here whether we want to kill
> >     Dom0 or hardware domain (or directly panic hypervisor). Comments
> > are welcomed.
> 
> I think that's a reasonable default, provided by that "Dom0 or hardware
> domain" you really just mean the same domain known with two different names
> (i.e. Dom0 should not be special when it is not the hardware domain).
> 
Jan,
   Long time no see.
   Hardware_domain is usually Dom0. Look forward to your further comments.

-Quan

Re: [PATCH 0/2] VT-d flush issue

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, December 03, 2015 9:16 PM
> 
> >>> On 03.12.15 at 09:09, <quan.xu@intel.com> wrote:
> >     If impacted domain is Dom0 or hardware domain, just throw out a warning.
> > It's an open here whether we want to kill
> >     Dom0 or hardware domain (or directly panic hypervisor). Comments are
> > welcomed.
> 
> I think that's a reasonable default, provided by that "Dom0 or
> hardware domain" you really just mean the same domain known
> with two different names (i.e. Dom0 should not be special when
> it is not the hardware domain).
> 

Are you suggest just checking hardware_domain should be enough in the code?

Thanks
Kevin

Re: [PATCH 2/2] VT-d: Fix vt-d flush timeout issue.

[Tian, Kevin]

> From: Xu, Quan
> Sent: Thursday, December 03, 2015 4:09 PM
> 
> If IOTLB/Context/IETC flush is timeout, we should think
> all devices under this IOMMU cannot function correctly.
> So for each device under this IOMMU we'll mark it as
> unassignable and kill the domain owning the device.
> 
> If Device-TLB flush is timeout, we'll mark the target
> ATS device as unassignable and kill the domain owning
> this device. When the invalidation request descriptor
> is timeout, hypervisor cannot find out which Device-TLB
> invalidate descriptor submitted before is not correct.
> So mark all of the domain's ATS devices as unassignable.

Another thought here. If we do timeout check within
dev_invalidate_iotlb for each ATS device, could we identify
bogus device accurately?

Re: [PATCH 2/2] VT-d: Fix vt-d flush timeout issue.

[Xu, Quan]

On December 04, 2015 at 9:52 AM, <Tian, Kevin> wrote:
> > From: Xu, Quan
> > Sent: Thursday, December 03, 2015 4:09 PM
> >
> > If IOTLB/Context/IETC flush is timeout, we should think all devices
> > under this IOMMU cannot function correctly.
> > So for each device under this IOMMU we'll mark it as unassignable and
> > kill the domain owning the device.
> >
> > If Device-TLB flush is timeout, we'll mark the target ATS device as
> > unassignable and kill the domain owning this device. When the
> > invalidation request descriptor is timeout, hypervisor cannot find out
> > which Device-TLB invalidate descriptor submitted before is not
> > correct.
> > So mark all of the domain's ATS devices as unassignable.
> 
> Another thought here. If we do timeout check within dev_invalidate_iotlb for
> each ATS device, could we identify bogus device accurately?

Yes, it can identify bogus device accurately.
But it may impact the performance. when Fence Flag(FN) bit is set, the descriptors Following the invalidation wait descriptor must be processed by hardware only after
The invalidation wait descriptor completes.

In current code, it can flush all of domain's device in batch(submit mutil Device-TLB invalidate descriptor with following one invalidation wait descriptor).
Otherwise it would flush all of domain's device one by one. And it may cost more than 100ms to flush one Device. 

Right?

-Quan

Re: [PATCH 0/2] VT-d flush issue

[Jan Beulich]

>>> "Tian, Kevin" <kevin.tian@intel.com> 12/04/15 2:49 AM >>>
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Thursday, December 03, 2015 9:16 PM
>> >>> On 03.12.15 at 09:09, <quan.xu@intel.com> wrote:
>> >     If impacted domain is Dom0 or hardware domain, just throw out a warning.
>> > It's an open here whether we want to kill
>> >     Dom0 or hardware domain (or directly panic hypervisor). Comments are
>> > welcomed.
>> 
>> I think that's a reasonable default, provided by that "Dom0 or
>> hardware domain" you really just mean the same domain known
>> with two different names (i.e. Dom0 should not be special when
>> it is not the hardware domain).
>
>Are you suggest just checking hardware_domain should be enough in the code?

Yes, absolutely.

Jan

Re: [PATCH 0/2] VT-d flush issue

[Xu, Quan]

On 04.12.2015 at 2:55pm, <jbeulich@suse.com> wrote:
> >>> "Tian, Kevin" <kevin.tian@intel.com> 12/04/15 2:49 AM >>>
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Thursday, December 03, 2015 9:16 PM
> >> >>> On 03.12.15 at 09:09, <quan.xu@intel.com> wrote:
> >> >     If impacted domain is Dom0 or hardware domain, just throw out a
> warning.
> >> > It's an open here whether we want to kill
> >> >     Dom0 or hardware domain (or directly panic hypervisor).
> >> > Comments are welcomed.
> >>
> >> I think that's a reasonable default, provided by that "Dom0 or
> >> hardware domain" you really just mean the same domain known with two
> >> different names (i.e. Dom0 should not be special when it is not the
> >> hardware domain).
> >
> >Are you suggest just checking hardware_domain should be enough in the
> code?
> 
> Yes, absolutely.


Agreed.

-Quan

Re: [PATCH 0/2] VT-d flush issue

[Xu, Quan]

>On 04.12.2015 at 2:55pm, <jbeulich@suse.com> wrote:
> >>> "Tian, Kevin" <kevin.tian@intel.com> 12/04/15 2:49 AM >>>
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Thursday, December 03, 2015 9:16 PM
> >> >>> On 03.12.15 at 09:09, <quan.xu@intel.com> wrote:
> >> >     If impacted domain is Dom0 or hardware domain, just throw out a
> warning.
> >> > It's an open here whether we want to kill
> >> >     Dom0 or hardware domain (or directly panic hypervisor).
> >> > Comments are welcomed.
> >>
> >> I think that's a reasonable default, provided by that "Dom0 or
> >> hardware domain" you really just mean the same domain known with two
> >> different names (i.e. Dom0 should not be special when it is not the
> >> hardware domain).
> >
> >Are you suggest just checking hardware_domain should be enough in the
> code?
> 
> Yes, absolutely.



Jan,
	Any more comment or suggestion?

Now there are 2 comments from you and Kevin Tian.
1. from you, just checking g hardware_domain should be enough.
2. from Kevin Tian, do timeout check within dev_invalidate_iotlb for each ATS device, to identify bogus device accurately.

I can fix these 2 comments and send out v2 patch set.

-Quan

Re: [PATCH 0/2] VT-d flush issue

[Jan Beulich]

>>> On 09.12.15 at 14:30, <quan.xu@intel.com> wrote:
> 	Any more comment or suggestion?
> 
> Now there are 2 comments from you and Kevin Tian.
> 1. from you, just checking g hardware_domain should be enough.
> 2. from Kevin Tian, do timeout check within dev_invalidate_iotlb for each 
> ATS device, to identify bogus device accurately.
> 
> I can fix these 2 comments and send out v2 patch set.

I'm sorry, I didn't get around to actually look at the patches, and
can't really predict when I would be able to. But since these are
VT-d changes I guess if Kevin and Feng had no other comments,
then you're fine to go ahead with v2.

Jan

Re: [PATCH 0/2] VT-d flush issue

[Xu, Quan]

> On 09.12.2015 at 9:38pm, <JBeulich@suse.com> wrote:
> >>> On 09.12.15 at 14:30, <quan.xu@intel.com> wrote:
> > 	Any more comment or suggestion?
> >
> > Now there are 2 comments from you and Kevin Tian.
> > 1. from you, just checking g hardware_domain should be enough.
> > 2. from Kevin Tian, do timeout check within dev_invalidate_iotlb for
> > each ATS device, to identify bogus device accurately.
> >
> > I can fix these 2 comments and send out v2 patch set.
> 
> I'm sorry, I didn't get around to actually look at the patches, and can't really
> predict when I would be able to. But since these are VT-d changes I guess if
> Kevin and Feng had no other comments, then you're fine to go ahead with v2.

Jan, but never mind. I am going ahead with v2.

-Quan