[PATCH v2 0/2] VT-d flush issue

[PATCH v2 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.

    If impacted domain is hardware domain, just throw out a warning. It's an open here whether we want to kill
    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.

--Changes in v2:
    1. Checking hardware_domain should be enough.
    2. Do timeout check within dev_invalidate_iotlb for each ATS device, to identify bogus device accurately.

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/extern.h  |  4 ++
 xen/drivers/passthrough/vtd/iommu.c   |  6 +++
 xen/drivers/passthrough/vtd/iommu.h   |  5 ++
 xen/drivers/passthrough/vtd/qinval.c  | 97 +++++++++++++++++++++++++++++++++--
 xen/drivers/passthrough/vtd/x86/ats.c | 16 ++++++
 xen/include/xen/pci.h                 |  7 +++
 6 files changed, 131 insertions(+), 4 deletions(-)

-- 
1.9.1

[PATCH v2 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 v2 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.

If impacted domain is hardware domain, just throw out
a warning. It's an open here whether we want to kill
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/extern.h  |  4 ++
 xen/drivers/passthrough/vtd/iommu.c   |  6 +++
 xen/drivers/passthrough/vtd/iommu.h   |  5 ++
 xen/drivers/passthrough/vtd/qinval.c  | 86 ++++++++++++++++++++++++++++++++++-
 xen/drivers/passthrough/vtd/x86/ats.c | 16 +++++++
 xen/include/xen/pci.h                 |  7 +++
 6 files changed, 122 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h
index 8acf889..0a7d795 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -62,6 +62,10 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
 int qinval_device_iotlb(struct iommu *iommu,
                         u32 max_invs_pend, u16 sid, u16 size, u64 addr);
 
+void invalidate_timeout(struct iommu *iommu, int type, u16 did,
+                        u16 seg, u8 bus, u8 devfn);
+int invalidate_sync(struct iommu *iommu);
+
 unsigned int get_cache_line_size(void);
 void cacheline_flush(char *);
 void flush_all_cache(void);
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..c3beaa6 100644
--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -452,6 +452,11 @@ struct qinval_entry {
 
 #define RESERVED_VAL        0
 
+#define INVALID_DID    ((u16)~0)
+#define INVALID_SEG    ((u16)~0)
+#define INVALID_BUS    ((u8)~0)
+#define INVALID_DEVFN  ((u8)~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..bf7f5b0 100644
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -27,12 +27,62 @@
 #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))
 
+void invalidate_timeout(struct iommu *iommu, int type, u16 did,
+                        u16 seg, u8 bus, u8 devfn)
+{
+    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 )
+                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);
+        for_each_pdev(d, pdev)
+            if ( (pdev->seg == seg) &&
+                 (pdev->bus == bus) &&
+                 (pdev->devfn == devfn) )
+                mark_pdev_unassignable(pdev);
+
+        if ( d != hardware_domain )
+            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;
@@ -187,7 +237,7 @@ static int queue_invalidate_wait(struct iommu *iommu,
     return -EOPNOTSUPP;
 }
 
-static int invalidate_sync(struct iommu *iommu)
+int invalidate_sync(struct iommu *iommu)
 {
     struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
 
@@ -262,6 +312,15 @@ 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,
+                           INVALID_SEG, INVALID_BUS, INVALID_DEVFN);
+        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 +367,15 @@ 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,
+                               INVALID_SEG, INVALID_BUS, INVALID_DEVFN);
+            dprintk(XENLOG_WARNING  VTDPREFIX,
+                    "Context flush timeout.\n");
+            return ret;
+        }
     }
     return ret;
 }
@@ -349,9 +417,23 @@ 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 ( rc == -ETIMEDOUT )
+        {
+            invalidate_timeout(iommu, TYPE_INVAL_IOTLB, INVALID_DID,
+                               INVALID_SEG, INVALID_BUS, INVALID_DEVFN);
+            dprintk(XENLOG_WARNING VTDPREFIX, "IOTLB flush timeout.\n");
+            return rc;
+        }
+
         if ( flush_dev_iotlb )
             ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
-        rc = invalidate_sync(iommu);
+
         if ( !ret )
             ret = rc;
     }
diff --git a/xen/drivers/passthrough/vtd/x86/ats.c b/xen/drivers/passthrough/vtd/x86/ats.c
index 7c797f6..8745ef4 100644
--- a/xen/drivers/passthrough/vtd/x86/ats.c
+++ b/xen/drivers/passthrough/vtd/x86/ats.c
@@ -156,6 +156,22 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
 
             rc = qinval_device_iotlb(iommu, pdev->ats_queue_depth,
                                      sid, sbit, addr);
+
+            /*
+             * Synchronize with hardware for Device-TLB invalidate
+             * descriptor.
+             */
+            rc = invalidate_sync(iommu);
+
+            if ( rc == -ETIMEDOUT )
+            {
+                invalidate_timeout(iommu, TYPE_INVAL_DEVICE_IOTLB, did,
+                                   pdev->seg, pdev->bus, pdev->devfn);
+                dprintk(XENLOG_WARNING VTDPREFIX,
+                        "Device-TLB flush timeout.\n");
+                return rc;
+            }
+
             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 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 v2 1/2] VT-d: Reduce spin timeout to 1ms, which can be boot-time changed.

[Andrew Cooper]

On 10/12/15 09:33, Quan Xu wrote:
> 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();
>          }

This patch misses a second use of DMAR_OPERATION_TIMEOUT, in
IOMMU_WAIT_OP() which in turn is used in a large number of locations. 
All of these locations equally need to be chopped down to a low number
of milliseconds.

~Andrew

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

[Andrew Cooper]

On 10/12/15 09:33, Quan Xu wrote:
> 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

Static inlines please.

These macros lack any hygene whatsoever, but don't need to be macros in
the first place.

~Andrew

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

[Xu, Quan]

On 11.12.2015 at 3:03pm, <andrew.cooper3@citrix.com> wrote:
> On 10/12/15 09:33, Quan Xu wrote:
> > 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();
> >          }
> 
> This patch misses a second use of DMAR_OPERATION_TIMEOUT, in
> IOMMU_WAIT_OP() which in turn is used in a large number of locations.
> All of these locations equally need to be chopped down to a low number of
> milliseconds.

Andrew, thanks for your comments.

I know that DMAR_OPERATION_TIMEOUT should be also chopped down to a low number of milliseconds.
As Kevin Tian mentioned in 'Revisit VT-d asynchronous flush issue', We also confirmed with hardware team
that 1ms is large enough for IOMMU internal flush.
So I can change DMAR_OPERATION_TIMEOUT from 1000 ms to 1 ms.

IOMMU_WAIT_OP() is only for VT-d registers read/write, and there is also a panic. We need a further discussion 
whether or how to remove this panic. I can send another patch set to fix it. in this patch set, I want to focus on VT-d
QI flush.

-Quan

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

[Xu, Quan]

On 11.12.2015 at 3:05pm, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 10/12/15 09:33, Quan Xu wrote:
> > 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
> 
> Static inlines please.
> 
> These macros lack any hygene whatsoever, but don't need to be macros in the
> first place.
> 

Andrew,

Could I modify it as below:

+static inline void mark_pdev_unassignable(struct pci_dev *pdev)
+{
+    pdev->info.is_unassignable = 1;
+}
+
+static inline bool_t is_pdev_unassignable(struct pci_dev *pdev)
+{
+    return pdev->info.is_unassignable;
+}

Correct me If I still don't get the point. Thanks.




-Quan

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

[Tian, Kevin]

> From: Xu, Quan
> Sent: Friday, December 11, 2015 10:09 AM
> 
> On 11.12.2015 at 3:03pm, <andrew.cooper3@citrix.com> wrote:
> > On 10/12/15 09:33, Quan Xu wrote:
> > > 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();
> > >          }
> >
> > This patch misses a second use of DMAR_OPERATION_TIMEOUT, in
> > IOMMU_WAIT_OP() which in turn is used in a large number of locations.
> > All of these locations equally need to be chopped down to a low number of
> > milliseconds.
> 
> Andrew, thanks for your comments.
> 
> I know that DMAR_OPERATION_TIMEOUT should be also chopped down to a low number of
> milliseconds.
> As Kevin Tian mentioned in 'Revisit VT-d asynchronous flush issue', We also confirmed with
> hardware team
> that 1ms is large enough for IOMMU internal flush.
> So I can change DMAR_OPERATION_TIMEOUT from 1000 ms to 1 ms.
> 
> IOMMU_WAIT_OP() is only for VT-d registers read/write, and there is also a panic. We need
> a further discussion
> whether or how to remove this panic. I can send another patch set to fix it. in this patch
> set, I want to focus on VT-d
> QI flush.
> 

Please describe this plan in your summary in next version. :-)

Thanks
Kevin

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

[Tian, Kevin]

> From: Xu, Quan
> Sent: Thursday, December 10, 2015 5:33 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.
> 
> If impacted domain is hardware domain, just throw out
> a warning. It's an open here whether we want to kill
> 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/extern.h  |  4 ++
>  xen/drivers/passthrough/vtd/iommu.c   |  6 +++
>  xen/drivers/passthrough/vtd/iommu.h   |  5 ++
>  xen/drivers/passthrough/vtd/qinval.c  | 86
> ++++++++++++++++++++++++++++++++++-
>  xen/drivers/passthrough/vtd/x86/ats.c | 16 +++++++
>  xen/include/xen/pci.h                 |  7 +++
>  6 files changed, 122 insertions(+), 2 deletions(-)
> 
[...]
> diff --git a/xen/drivers/passthrough/vtd/iommu.h
> b/xen/drivers/passthrough/vtd/iommu.h
> index ac71ed1..c3beaa6 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -452,6 +452,11 @@ struct qinval_entry {
> 
>  #define RESERVED_VAL        0
> 
> +#define INVALID_DID    ((u16)~0)
> +#define INVALID_SEG    ((u16)~0)
> +#define INVALID_BUS    ((u8)~0)
> +#define INVALID_DEVFN  ((u8)~0)
> +

Are those invalid values defined by specification? Or if they
are software defined, does related mgmt. code guarantee
that they won't be allocated?

>  #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..bf7f5b0 100644
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -27,12 +27,62 @@
>  #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))
> 
> +void invalidate_timeout(struct iommu *iommu, int type, u16 did,
> +                        u16 seg, u8 bus, u8 devfn)
> +{
> +    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 )
> +                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);
> +        for_each_pdev(d, pdev)
> +            if ( (pdev->seg == seg) &&
> +                 (pdev->bus == bus) &&
> +                 (pdev->devfn == devfn) )
> +                mark_pdev_unassignable(pdev);

Once found you can break the for loop immediately since BDF is unique.

Thanks
Kevin

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

[Xu, Quan]

On 11.12.2015 at 3:01pm, <Tian, Kevin> wrote:
> > From: Xu, Quan
> > Sent: Friday, December 11, 2015 10:09 AM
> >
> > On 11.12.2015 at 3:03pm, <andrew.cooper3@citrix.com> wrote:
> > > On 10/12/15 09:33, Quan Xu wrote:
> > > > 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();
> > > >          }
> > >
> > > This patch misses a second use of DMAR_OPERATION_TIMEOUT, in
> > > IOMMU_WAIT_OP() which in turn is used in a large number of locations.
> > > All of these locations equally need to be chopped down to a low
> > > number of milliseconds.
> >
> > Andrew, thanks for your comments.
> >
> > I know that DMAR_OPERATION_TIMEOUT should be also chopped down to a
> > low number of milliseconds.
> > As Kevin Tian mentioned in 'Revisit VT-d asynchronous flush issue', We
> > also confirmed with hardware team that 1ms is large enough for IOMMU
> > internal flush.
> > So I can change DMAR_OPERATION_TIMEOUT from 1000 ms to 1 ms.
> >
> > IOMMU_WAIT_OP() is only for VT-d registers read/write, and there is
> > also a panic. We need a further discussion whether or how to remove
> > this panic. I can send another patch set to fix it. in this patch set,
> > I want to focus on VT-d QI flush.
> >
> 
> Please describe this plan in your summary in next version. :-)


Agreed. :):)

Quan

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

[Xu, Quan]

On 11.12.2015 at 3:28pm, <Tian, Kevin> wrote:
> > From: Xu, Quan
> > Sent: Thursday, December 10, 2015 5:33 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.
> >
> > If impacted domain is hardware domain, just throw out a warning. It's
> > an open here whether we want to kill 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>
> > ---
> [...]
> > diff --git a/xen/drivers/passthrough/vtd/iommu.h
> > b/xen/drivers/passthrough/vtd/iommu.h
> > index ac71ed1..c3beaa6 100644
> > --- a/xen/drivers/passthrough/vtd/iommu.h
> > +++ b/xen/drivers/passthrough/vtd/iommu.h
> > @@ -452,6 +452,11 @@ struct qinval_entry {
> >
> >  #define RESERVED_VAL        0
> >
> > +#define INVALID_DID    ((u16)~0)
> > +#define INVALID_SEG    ((u16)~0)
> > +#define INVALID_BUS    ((u8)~0)
> > +#define INVALID_DEVFN  ((u8)~0)
> > +
> 
> Are those invalid values defined by specification? 
 This is not defined by specification.

>Or if they are software
> defined, does related mgmt. code guarantee that they won't be allocated?
> 

As similar as the other Xen code, it defined invalid value with "~0". Such as:
          $#define INVALID_MFN (~0UL)
          $#define INVALID_GFN (~0UL)
          .etc

Code can't not guarantee that won't be allocated, but it can guarantee it will not be used when it is INVALID_*.
Any idea, how to indicate that the value is invalid?


> >  #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..bf7f5b0 100644
> > --- a/xen/drivers/passthrough/vtd/qinval.c
> > +++ b/xen/drivers/passthrough/vtd/qinval.c
> > @@ -27,12 +27,62 @@
> >  #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))
> >
> > +void invalidate_timeout(struct iommu *iommu, int type, u16 did,
> > +                        u16 seg, u8 bus, u8 devfn) {
[...]
> > +    case TYPE_INVAL_DEVICE_IOTLB:
> > +        d = rcu_lock_domain_by_id(iommu->domid_map[did]);
> > +        ASSERT(d);
> > +        for_each_pdev(d, pdev)
> > +            if ( (pdev->seg == seg) &&
> > +                 (pdev->bus == bus) &&
> > +                 (pdev->devfn == devfn) )
> > +                mark_pdev_unassignable(pdev);
> 
> Once found you can break the for loop immediately since BDF is unique.
> 


Good advice. Agreed.

Quan

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

[Andrew Cooper]

On 11/12/2015 02:09, Xu, Quan wrote:
> On 11.12.2015 at 3:03pm, <andrew.cooper3@citrix.com> wrote:
>> On 10/12/15 09:33, Quan Xu wrote:
>>> 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();
>>>          }
>> This patch misses a second use of DMAR_OPERATION_TIMEOUT, in
>> IOMMU_WAIT_OP() which in turn is used in a large number of locations.
>> All of these locations equally need to be chopped down to a low number of
>> milliseconds.
> Andrew, thanks for your comments.
>
> I know that DMAR_OPERATION_TIMEOUT should be also chopped down to a low number of milliseconds.
> As Kevin Tian mentioned in 'Revisit VT-d asynchronous flush issue', We also confirmed with hardware team
> that 1ms is large enough for IOMMU internal flush.
> So I can change DMAR_OPERATION_TIMEOUT from 1000 ms to 1 ms.

Ok - sounds good.

>
> IOMMU_WAIT_OP() is only for VT-d registers read/write, and there is also a panic. We need a further discussion 
> whether or how to remove this panic.

We certainly should see about removing it.

> I can send another patch set to fix it. in this patch set, I want to focus on VT-d
> QI flush.

Agreed.

~Andrew

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

[Andrew Cooper]

On 11/12/2015 05:37, Xu, Quan wrote:
> On 11.12.2015 at 3:05pm, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 10/12/15 09:33, Quan Xu wrote:
>>> 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
>> Static inlines please.
>>
>> These macros lack any hygene whatsoever, but don't need to be macros in the
>> first place.
>>
> Andrew,
>
> Could I modify it as below:
>
> +static inline void mark_pdev_unassignable(struct pci_dev *pdev)
> +{
> +    pdev->info.is_unassignable = 1;
> +}
> +
> +static inline bool_t is_pdev_unassignable(struct pci_dev *pdev)

const struct pci_dev *pdev

> +{
> +    return pdev->info.is_unassignable;
> +}
>
> Correct me If I still don't get the point. Thanks.

but otherwise, yes.

~Andrew

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

[Xu, Quan]

On 11.12.2015 at 4:38pm, <Andrew Cooper> wrote:
> On 11/12/2015 05:37, Xu, Quan wrote:
> > On 11.12.2015 at 3:05pm, Andrew Cooper <andrew.cooper3@citrix.com>
> wrote:
> >> On 10/12/15 09:33, Quan Xu wrote:
> >>> 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
> >> Static inlines please.
> >>
> >> These macros lack any hygene whatsoever, but don't need to be macros
> >> in the first place.
> >>
> > Andrew,
> >
> > Could I modify it as below:
> >
> > +static inline void mark_pdev_unassignable(struct pci_dev *pdev) {
> > +    pdev->info.is_unassignable = 1;
> > +}
> > +
> > +static inline bool_t is_pdev_unassignable(struct pci_dev *pdev)
> 
> const struct pci_dev *pdev
> 
> > +{
> > +    return pdev->info.is_unassignable; }
> >
> > Correct me If I still don't get the point. Thanks.
> 
> but otherwise, yes.


Thanks Andrew.
I will modify it in v3.


Quan

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

[Xu, Quan]

On 11.12.2015 at 4:37pm, <Andrew Cooper> wrote:
> On 11/12/2015 02:09, Xu, Quan wrote:
> > On 11.12.2015 at 3:03pm, <andrew.cooper3@citrix.com> wrote:
> >> On 10/12/15 09:33, Quan Xu wrote:
> >>> Signed-off-by: Quan Xu <quan.xu@intel.com>


> >> This patch misses a second use of DMAR_OPERATION_TIMEOUT, in
> >> IOMMU_WAIT_OP() which in turn is used in a large number of locations.
> >> All of these locations equally need to be chopped down to a low
> >> number of milliseconds.
> > Andrew, thanks for your comments.
> >
> > I know that DMAR_OPERATION_TIMEOUT should be also chopped down to a
> low number of milliseconds.
> > As Kevin Tian mentioned in 'Revisit VT-d asynchronous flush issue', We
> > also confirmed with hardware team that 1ms is large enough for IOMMU
> internal flush.
> > So I can change DMAR_OPERATION_TIMEOUT from 1000 ms to 1 ms.
> 
> Ok - sounds good.
> 
> >
> > IOMMU_WAIT_OP() is only for VT-d registers read/write, and there is
> > also a panic. We need a further discussion whether or how to remove this
> panic.
> 
> We certainly should see about removing it.
> 

Agreed.

-Quan


> > I can send another patch set to fix it. in this patch set, I want to
> > focus on VT-d QI flush.
> 
> Agreed.
> 
> ~Andrew

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

[Jan Beulich]

>>> On 10.12.15 at 10:33, <quan.xu@intel.com> wrote:
> @@ -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;
>              }

I don't see such a change be valid without making sure callers actually
honor errors. For example, no caller of iommu_flush_iec_{global,index}()
cares to check. And not even your second patch addresses this (i.e.
it's also not just bad patch ordering).

If you require errors to be dealt with (and you do here), make use of
__must_check all the way up the call trees.

Jan

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

[Jan Beulich]

>>> On 11.12.15 at 09:01, <quan.xu@intel.com> wrote:
> On 11.12.2015 at 3:28pm, <Tian, Kevin> wrote:
>> > From: Xu, Quan
>> > Sent: Thursday, December 10, 2015 5:33 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.
>> >
>> > If impacted domain is hardware domain, just throw out a warning. It's
>> > an open here whether we want to kill 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>
>> > ---
>> [...]
>> > diff --git a/xen/drivers/passthrough/vtd/iommu.h
>> > b/xen/drivers/passthrough/vtd/iommu.h
>> > index ac71ed1..c3beaa6 100644
>> > --- a/xen/drivers/passthrough/vtd/iommu.h
>> > +++ b/xen/drivers/passthrough/vtd/iommu.h
>> > @@ -452,6 +452,11 @@ struct qinval_entry {
>> >
>> >  #define RESERVED_VAL        0
>> >
>> > +#define INVALID_DID    ((u16)~0)
>> > +#define INVALID_SEG    ((u16)~0)
>> > +#define INVALID_BUS    ((u8)~0)
>> > +#define INVALID_DEVFN  ((u8)~0)
>> > +
>> 
>> Are those invalid values defined by specification? 
>  This is not defined by specification.
> 
>>Or if they are software
>> defined, does related mgmt. code guarantee that they won't be allocated?
>> 
> 
> As similar as the other Xen code, it defined invalid value with "~0". Such 
> as:
>           $#define INVALID_MFN (~0UL)
>           $#define INVALID_GFN (~0UL)
>           .etc
> 
> Code can't not guarantee that won't be allocated, but it can guarantee it 
> will not be used when it is INVALID_*.
> Any idea, how to indicate that the value is invalid?

Some other means is needed (be creative). Comparing with
INVALID_{MFN,GFN} is bogus, since frame numbers truly can't
reach this big a value (there being just 52 bits in physical
addresses, i.e. 40 bits in a frame number).

Jan

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

[Xu, Quan]

On 11.12.2015 at 6:05pm, <JBeulich@suse.com> wrote:
> >>> On 11.12.15 at 09:01, <quan.xu@intel.com> wrote:
> > On 11.12.2015 at 3:28pm, <Tian, Kevin> wrote:
> >> > From: Xu, Quan
> >> > Sent: Thursday, December 10, 2015 5:33 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.
> >> >
> >> > If impacted domain is hardware domain, just throw out a warning.
> >> > It's an open here whether we want to kill 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>
> >> > ---
> >> [...]
> >> > diff --git a/xen/drivers/passthrough/vtd/iommu.h
> >> > b/xen/drivers/passthrough/vtd/iommu.h
> >> > index ac71ed1..c3beaa6 100644
> >> > --- a/xen/drivers/passthrough/vtd/iommu.h
> >> > +++ b/xen/drivers/passthrough/vtd/iommu.h
> >> > @@ -452,6 +452,11 @@ struct qinval_entry {
> >> >
> >> >  #define RESERVED_VAL        0
> >> >
> >> > +#define INVALID_DID    ((u16)~0)
> >> > +#define INVALID_SEG    ((u16)~0)
> >> > +#define INVALID_BUS    ((u8)~0)
> >> > +#define INVALID_DEVFN  ((u8)~0)
> >> > +
> >>
> >> Are those invalid values defined by specification?
> >  This is not defined by specification.
> >
> >>Or if they are software
> >> defined, does related mgmt. code guarantee that they won't be allocated?
> >>
> >
> > As similar as the other Xen code, it defined invalid value with "~0".
> > Such
> > as:
> >           $#define INVALID_MFN (~0UL)
> >           $#define INVALID_GFN (~0UL)
> >           .etc
> >
> > Code can't not guarantee that won't be allocated, but it can guarantee
> > it will not be used when it is INVALID_*.
> > Any idea, how to indicate that the value is invalid?
> 
> Some other means is needed (be creative). Comparing with INVALID_{MFN,GFN}
> is bogus, since frame numbers truly can't reach this big a value (there being just
> 52 bits in physical addresses, i.e. 40 bits in a frame number).

Jan, thanks for your comments.
I think I can't use INVALID_* in my patch any more. If I can separate invalidate_timeout() into 2
Functions. then I can ignore these INVALID_* parameters.

i.e. 
separate INVALID_* parameters. ignore these INVALID_* parameters.
void invalidate_timeout(struct iommu *iommu, int type, u16 did, u16 seg, u8 bus, u8 devfn)

into

invalidate_timeout(struct iommu *iommu) 
and
device_tlb_invalidate_timeout(struct iommu *iommu, u16 did, u16 seg, u8 bus, u8 devfn)



invalidate_timeout() is for iotlb/iec/context flush error.
device_tlb_invalidate_timeout is for Device-TLB flush error. 

Then ignore these INVALID_* parameters.

Right?

Quan

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

[Xu, Quan]

> On 11.12.2015 at 6:01pm, <JBeulich@suse.com> wrote:
> >>> On 10.12.15 at 10:33, <quan.xu@intel.com> wrote:
> > @@ -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;
> >              }
> 
> I don't see such a change be valid without making sure callers actually honor
> errors. For example, no caller of iommu_flush_iec_{global,index}() cares to
> check. And not even your second patch addresses this (i.e.
> it's also not just bad patch ordering).
> 
> If you require errors to be dealt with (and you do here), make use of
> __must_check all the way up the call trees.
> 

ok, I will make use of __must_check all the way up the call trees in v3.

Quan

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

[Xu, Quan]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org
> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Xu, Quan
> Sent: Friday, December 11, 2015 10:00 PM
> To: Jan Beulich
> Cc: Tian, Kevin; Wu, Feng; george.dunlap@eu.citrix.com;
> andrew.cooper3@citrix.com; tim@xen.org; xen-devel@lists.xen.org; Nakajima,
> Jun; keir@xen.org
> Subject: Re: [Xen-devel] [PATCH v2 2/2] VT-d: Fix vt-d flush timeout issue.
> 
> On 11.12.2015 at 6:05pm, <JBeulich@suse.com> wrote:
> > >>> On 11.12.15 at 09:01, <quan.xu@intel.com> wrote:
> > > On 11.12.2015 at 3:28pm, <Tian, Kevin> wrote:
> > >> > From: Xu, Quan
> > >> > Sent: Thursday, December 10, 2015 5:33 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.
> > >> >
> > >> > If impacted domain is hardware domain, just throw out a warning.
> > >> > It's an open here whether we want to kill 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>
> > >> > ---
> > >> [...]
> > >> > diff --git a/xen/drivers/passthrough/vtd/iommu.h
> > >> > b/xen/drivers/passthrough/vtd/iommu.h
> > >> > index ac71ed1..c3beaa6 100644
> > >> > --- a/xen/drivers/passthrough/vtd/iommu.h
> > >> > +++ b/xen/drivers/passthrough/vtd/iommu.h
> > >> > @@ -452,6 +452,11 @@ struct qinval_entry {
> > >> >
> > >> >  #define RESERVED_VAL        0
> > >> >
> > >> > +#define INVALID_DID    ((u16)~0)
> > >> > +#define INVALID_SEG    ((u16)~0)
> > >> > +#define INVALID_BUS    ((u8)~0)
> > >> > +#define INVALID_DEVFN  ((u8)~0)
> > >> > +
> > >>
> > >> Are those invalid values defined by specification?
> > >  This is not defined by specification.
> > >
> > >>Or if they are software
> > >> defined, does related mgmt. code guarantee that they won't be allocated?
> > >>
> > >
> > > As similar as the other Xen code, it defined invalid value with "~0".
> > > Such
> > > as:
> > >           $#define INVALID_MFN (~0UL)
> > >           $#define INVALID_GFN (~0UL)
> > >           .etc
> > >
> > > Code can't not guarantee that won't be allocated, but it can
> > > guarantee it will not be used when it is INVALID_*.
> > > Any idea, how to indicate that the value is invalid?
> >
> > Some other means is needed (be creative). Comparing with
> > INVALID_{MFN,GFN} is bogus, since frame numbers truly can't reach this
> > big a value (there being just
> > 52 bits in physical addresses, i.e. 40 bits in a frame number).
> 
> Jan, thanks for your comments.
> I think I can't use INVALID_* in my patch any more. If I can separate
> invalidate_timeout() into 2 Functions. then I can ignore these INVALID_*
> parameters.
> 
> i.e.
> separate INVALID_* parameters. ignore these INVALID_* parameters.
> void invalidate_timeout(struct iommu *iommu, int type, u16 did, u16 seg, u8
> bus, u8 devfn)
> 
> into
> 
> invalidate_timeout(struct iommu *iommu) and
> device_tlb_invalidate_timeout(struct iommu *iommu, u16 did, u16 seg, u8 bus,
> u8 devfn)
> 
> 
> 
> invalidate_timeout() is for iotlb/iec/context flush error.
> device_tlb_invalidate_timeout is for Device-TLB flush error.
> 
> Then ignore these INVALID_* parameters.
> 
> Right?
> 
> Quan
> 
> 


Correct it.

 i.e.
 separate
void invalidate_timeout(struct iommu *iommu, int type, u16 did, u16 seg, u8
bus, u8 devfn)
 
 into
 
 invalidate_timeout(struct iommu *iommu) 

and

 device_tlb_invalidate_timeout(struct iommu *iommu, u16 did, u16 seg, u8 bus,
 u8 devfn)
  
 invalidate_timeout() is for iotlb/iec/context flush error.
 device_tlb_invalidate_timeout() is for Device-TLB flush error.


-Quan

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

[Xu, Quan]

>On 11.12.2015 at 6:01pm, <JBeulich@suse.com> wrpte:
> >>> On 10.12.15 at 10:33, <quan.xu@intel.com> wrote:
> > @@ -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;
> >              }
> 
> I don't see such a change be valid without making sure callers actually honor
> errors. For example, no caller of iommu_flush_iec_{global,index}() cares to
> check. And not even your second patch addresses this (i.e.
> it's also not just bad patch ordering).
> 

I check it again. 
For iommu_flush_iec_{global,index}()  are both call __iommu_flush_iec().
In my patch, I have check it in __iommu_flush_iec().
I think it does not need to check it in iommu_flush_iec_{global,index}() again. Right?

-Quan


> If you require errors to be dealt with (and you do here), make use of
> __must_check all the way up the call trees.
> 
> Jan

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

[Jan Beulich]

>>> On 12.12.15 at 10:03, <quan.xu@intel.com> wrote:
>> On 11.12.2015 at 6:01pm, <JBeulich@suse.com> wrpte:
>> >>> On 10.12.15 at 10:33, <quan.xu@intel.com> wrote:
>> > @@ -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;
>> >              }
>> 
>> I don't see such a change be valid without making sure callers actually honor
>> errors. For example, no caller of iommu_flush_iec_{global,index}() cares to
>> check. And not even your second patch addresses this (i.e.
>> it's also not just bad patch ordering).
>> 
> 
> I check it again. 
> For iommu_flush_iec_{global,index}()  are both call __iommu_flush_iec().
> In my patch, I have check it in __iommu_flush_iec().
> I think it does not need to check it in iommu_flush_iec_{global,index}() 
> again. Right?

No, not in the v2 version of it. While you call invalidate_timeout() in
the -ETIMEDOUT case, you still pass on the error code, and hence
the callers need to also either pass it on or deal with it.

Jan

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

[Xu, Quan]

> On 14.12.2015 at 4:19pm, <JBeulich@suse.com> wrote:
> >>> On 12.12.15 at 10:03, <quan.xu@intel.com> wrote:
> >> On 11.12.2015 at 6:01pm, <JBeulich@suse.com> wrpte:
> >> >>> On 10.12.15 at 10:33, <quan.xu@intel.com> wrote:
> >> > @@ -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;
> >> >              }
> >>
> >> I don't see such a change be valid without making sure callers
> >> actually honor errors. For example, no caller of
> >> iommu_flush_iec_{global,index}() cares to check. And not even your second
> patch addresses this (i.e.
> >> it's also not just bad patch ordering).
> >>
> >
> > I check it again.
> > For iommu_flush_iec_{global,index}()  are both call __iommu_flush_iec().
> > In my patch, I have check it in __iommu_flush_iec().
> > I think it does not need to check it in
> > iommu_flush_iec_{global,index}() again. Right?
> 
> No, not in the v2 version of it. While you call invalidate_timeout() in the
> -ETIMEDOUT case, you still pass on the error code, and hence the callers need
> to also either pass it on or deal with it.

Jan, sorry for that. I still don't get the point.
Check it again :(:(

Should I also check it where call iommu_flush_iec_{global,index}(), if it is -ETIMEDOUT, it should return or deal with it??

.i.e. 

static void free_remap_entry(struct iommu *iommu, int index)
{
...
    iommu_flush_iec_index(iommu, 0, index);
    *<------------------------------------------------------------------------------  (should I also check it where call iommu_flush_iec_{global,index}(), if it is -ETIMEDOUT, it should return or deal with it) ??
...                     
}



Quan

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

[Jan Beulich]

>>> On 14.12.15 at 09:31, <quan.xu@intel.com> wrote:
>>  On 14.12.2015 at 4:19pm, <JBeulich@suse.com> wrote:
>> >>> On 12.12.15 at 10:03, <quan.xu@intel.com> wrote:
>> >> On 11.12.2015 at 6:01pm, <JBeulich@suse.com> wrpte:
>> >> >>> On 10.12.15 at 10:33, <quan.xu@intel.com> wrote:
>> >> > @@ -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;
>> >> >              }
>> >>
>> >> I don't see such a change be valid without making sure callers
>> >> actually honor errors. For example, no caller of
>> >> iommu_flush_iec_{global,index}() cares to check. And not even your second
>> patch addresses this (i.e.
>> >> it's also not just bad patch ordering).
>> >>
>> >
>> > I check it again.
>> > For iommu_flush_iec_{global,index}()  are both call __iommu_flush_iec().
>> > In my patch, I have check it in __iommu_flush_iec().
>> > I think it does not need to check it in
>> > iommu_flush_iec_{global,index}() again. Right?
>> 
>> No, not in the v2 version of it. While you call invalidate_timeout() in the
>> -ETIMEDOUT case, you still pass on the error code, and hence the callers need
>> to also either pass it on or deal with it.
> 
> Jan, sorry for that. I still don't get the point.
> Check it again :(:(
> 
> Should I also check it where call iommu_flush_iec_{global,index}(), if it is 
> -ETIMEDOUT, it should return or deal with it??

You should check for _any_ kind of error here. I.e. the problem
doesn't get introduced by your patches, it only gets made worse
(by virtue of the fact that the only error queue_invalidate_wait()
may return before your patch is for when calling code sets a flag
it isn't currently supposed to set, i.e. not handling errors right
now is pretty much benign), and hence properly dealing with
errors here would probably best go into a prereq patch.

Jan