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

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

[Quan Xu]

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

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

2. Wrap a _sync version for all VT-d flush interfaces.

    For consistency, we wrap a _sync version for all VT-d flush interfaces.
    It simplifies caller logic and makes code more readable as well.

3. Fix vt-d Device-TLB flush timeout issue.
   If Device-TLB flush timed out, we would hide the target ATS
   device and crash the domain owning this ATS device. If impacted
   domain is hardware domain, just throw out a warning.

   The hidden device should be disallowed to be further assigned
   to any domain.

**NOTE**
   This patch set should base on 2 prereq patch sets:
    a). Make the pcidevs_lock a recursive one.
    b). 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 v8:
  *Rebase against a6f2cdb633bf519244a16674031b8034b581ba7f.
#patch 1
  *Add __must_check annotation.   
#patch 2
  *It is a new patch that wraps a _sync version for all VT-d flush interfaces.
#patch 3
  *Add a blank line between:
        +    pcidevs_lock();
        +    for_each_pdev(d, pdev)

Quan Xu (3):
  VT-d: Reduce spin timeout to 1ms, which can be boot-time changed
  VT-d: Wrap a _sync version for all VT-d flush interfaces
  VT-d: Fix vt-d Device-TLB flush timeout issue

 docs/misc/xen-command-line.markdown   |   7 ++
 xen/drivers/passthrough/pci.c         |   6 +-
 xen/drivers/passthrough/vtd/extern.h  |   3 +
 xen/drivers/passthrough/vtd/qinval.c  | 202 ++++++++++++++++++++++++----------
 xen/drivers/passthrough/vtd/x86/ats.c |  15 +--
 xen/include/xen/pci.h                 |   1 +
 6 files changed, 165 insertions(+), 69 deletions(-)

-- 
1.9.1

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

[PATCH v8 2/3] VT-d: Wrap a _sync version for all VT-d flush interfaces

[Quan Xu]

For consistency, we wrap a _sync version for all VT-d flush interfaces.
It simplifies caller logic and makes code more readable as well.

Signed-off-by: Quan Xu <quan.xu@intel.com>
---
 xen/drivers/passthrough/vtd/extern.h  |   2 +
 xen/drivers/passthrough/vtd/qinval.c  | 173 ++++++++++++++++++++--------------
 xen/drivers/passthrough/vtd/x86/ats.c |  12 +--
 3 files changed, 106 insertions(+), 81 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h
index d4d37c3..6d3187d 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -61,6 +61,8 @@ 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);
+int 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..ad9e265 100644
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -72,6 +72,70 @@ static void qinval_update_qtail(struct iommu *iommu, unsigned int index)
     dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << QINVAL_INDEX_SHIFT));
 }
 
+static int __must_check queue_invalidate_wait(struct iommu *iommu,
+    u8 iflag, u8 sw, u8 fn)
+{
+    s_time_t timeout;
+    volatile u32 poll_slot = QINVAL_STAT_INIT;
+    unsigned int index;
+    unsigned long flags;
+    u64 entry_base;
+    struct qinval_entry *qinval_entry, *qinval_entries;
+
+    spin_lock_irqsave(&iommu->register_lock, flags);
+    index = qinval_next_index(iommu);
+    entry_base = iommu_qi_ctrl(iommu)->qinval_maddr +
+                 ((index >> QINVAL_ENTRY_ORDER) << PAGE_SHIFT);
+    qinval_entries = map_vtd_domain_page(entry_base);
+    qinval_entry = &qinval_entries[index % (1 << QINVAL_ENTRY_ORDER)];
+
+    qinval_entry->q.inv_wait_dsc.lo.type = TYPE_INVAL_WAIT;
+    qinval_entry->q.inv_wait_dsc.lo.iflag = iflag;
+    qinval_entry->q.inv_wait_dsc.lo.sw = sw;
+    qinval_entry->q.inv_wait_dsc.lo.fn = fn;
+    qinval_entry->q.inv_wait_dsc.lo.res_1 = 0;
+    qinval_entry->q.inv_wait_dsc.lo.sdata = QINVAL_STAT_DONE;
+    qinval_entry->q.inv_wait_dsc.hi.res_1 = 0;
+    qinval_entry->q.inv_wait_dsc.hi.saddr = virt_to_maddr(&poll_slot) >> 2;
+
+    unmap_vtd_domain_page(qinval_entries);
+    qinval_update_qtail(iommu, index);
+    spin_unlock_irqrestore(&iommu->register_lock, flags);
+
+    /* Now we don't support interrupt method */
+    if ( sw )
+    {
+        /* In case all wait descriptor writes to same addr with same data */
+        timeout = NOW() + IOMMU_QI_TIMEOUT;
+        while ( poll_slot != QINVAL_STAT_DONE )
+        {
+            if ( NOW() > timeout )
+            {
+                print_qi_regs(iommu);
+                printk(XENLOG_WARNING VTDPREFIX
+                       "Queue invalidate wait descriptor timed out.\n");
+                return -ETIMEDOUT;
+            }
+
+            cpu_relax();
+        }
+
+        return 0;
+    }
+
+    return -EOPNOTSUPP;
+}
+
+static int invalidate_sync(struct iommu *iommu)
+{
+    struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
+
+    if ( qi_ctrl->qinval_maddr )
+        return queue_invalidate_wait(iommu, 0, 1, 1);
+
+    return 0;
+}
+
 static void queue_invalidate_context(struct iommu *iommu,
     u16 did, u16 source_id, u8 function_mask, u8 granu)
 {
@@ -102,6 +166,15 @@ static void queue_invalidate_context(struct iommu *iommu,
     unmap_vtd_domain_page(qinval_entries);
 }
 
+static int queue_invalidate_context_sync(struct iommu *iommu,
+    u16 did, u16 source_id, u8 function_mask, u8 granu)
+{
+    queue_invalidate_context(iommu, did, source_id,
+                             function_mask, granu);
+
+    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)
 {
@@ -135,65 +208,12 @@ static void queue_invalidate_iotlb(struct iommu *iommu,
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 }
 
-static int __must_check queue_invalidate_wait(struct iommu *iommu,
-    u8 iflag, u8 sw, u8 fn)
-{
-    s_time_t timeout;
-    volatile u32 poll_slot = QINVAL_STAT_INIT;
-    unsigned int index;
-    unsigned long flags;
-    u64 entry_base;
-    struct qinval_entry *qinval_entry, *qinval_entries;
-
-    spin_lock_irqsave(&iommu->register_lock, flags);
-    index = qinval_next_index(iommu);
-    entry_base = iommu_qi_ctrl(iommu)->qinval_maddr +
-                 ((index >> QINVAL_ENTRY_ORDER) << PAGE_SHIFT);
-    qinval_entries = map_vtd_domain_page(entry_base);
-    qinval_entry = &qinval_entries[index % (1 << QINVAL_ENTRY_ORDER)];
-
-    qinval_entry->q.inv_wait_dsc.lo.type = TYPE_INVAL_WAIT;
-    qinval_entry->q.inv_wait_dsc.lo.iflag = iflag;
-    qinval_entry->q.inv_wait_dsc.lo.sw = sw;
-    qinval_entry->q.inv_wait_dsc.lo.fn = fn;
-    qinval_entry->q.inv_wait_dsc.lo.res_1 = 0;
-    qinval_entry->q.inv_wait_dsc.lo.sdata = QINVAL_STAT_DONE;
-    qinval_entry->q.inv_wait_dsc.hi.res_1 = 0;
-    qinval_entry->q.inv_wait_dsc.hi.saddr = virt_to_maddr(&poll_slot) >> 2;
-
-    unmap_vtd_domain_page(qinval_entries);
-    qinval_update_qtail(iommu, index);
-    spin_unlock_irqrestore(&iommu->register_lock, flags);
-
-    /* Now we don't support interrupt method */
-    if ( sw )
-    {
-        /* In case all wait descriptor writes to same addr with same data */
-        timeout = NOW() + IOMMU_QI_TIMEOUT;
-        while ( poll_slot != QINVAL_STAT_DONE )
-        {
-            if ( NOW() > timeout )
-            {
-                print_qi_regs(iommu);
-                printk(XENLOG_WARNING VTDPREFIX
-                       "Queue invalidate wait descriptor timed out.\n");
-                return -ETIMEDOUT;
-            }
-            cpu_relax();
-        }
-        return 0;
-    }
-
-    return -EOPNOTSUPP;
-}
-
-static int invalidate_sync(struct iommu *iommu)
+static int queue_invalidate_iotlb_sync(struct iommu *iommu,
+    u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr)
 {
-    struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
+    queue_invalidate_iotlb(iommu, granu, dr, dw, did, am, ih, addr);
 
-    if ( qi_ctrl->qinval_maddr )
-        return queue_invalidate_wait(iommu, 0, 1, 1);
-    return 0;
+    return invalidate_sync(iommu);
 }
 
 int qinval_device_iotlb(struct iommu *iommu,
@@ -229,6 +249,14 @@ int qinval_device_iotlb(struct iommu *iommu,
     return 0;
 }
 
+int qinval_device_iotlb_sync(struct iommu *iommu,
+    u32 max_invs_pend, u16 sid, u16 size, u64 addr)
+{
+    qinval_device_iotlb(iommu, max_invs_pend, sid, size, addr);
+
+    return invalidate_sync(iommu);
+}
+
 static void queue_invalidate_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx)
 {
     unsigned long flags;
@@ -256,7 +284,7 @@ static void queue_invalidate_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx)
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 }
 
-static int __iommu_flush_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx)
+static int queue_invalidate_iec_sync(struct iommu *iommu, u8 granu, u8 im, u16 iidx)
 {
     int ret;
 
@@ -273,12 +301,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 +332,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;
 }
 
@@ -338,23 +364,24 @@ static int flush_iotlb_qi(
 
     if ( qi_ctrl->qinval_maddr != 0 )
     {
-        int rc;
-
         /* use queued invalidation */
         if (cap_write_drain(iommu->cap))
             dw = 1;
         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);
+
+        /* TODO: Timeout error handling to be added later */
+        if ( ret )
+            return ret;
+
         if ( flush_dev_iotlb )
             ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
-        rc = invalidate_sync(iommu);
-        if ( !ret )
-            ret = rc;
     }
+
     return ret;
 }
 
diff --git a/xen/drivers/passthrough/vtd/x86/ats.c b/xen/drivers/passthrough/vtd/x86/ats.c
index 334b9c1..7b1c07b 100644
--- a/xen/drivers/passthrough/vtd/x86/ats.c
+++ b/xen/drivers/passthrough/vtd/x86/ats.c
@@ -118,7 +118,6 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
     {
         u16 sid = PCI_BDF2(pdev->bus, pdev->devfn);
         bool_t sbit;
-        int rc = 0;
 
         /* Only invalidate devices that belong to this IOMMU */
         if ( pdev->iommu != iommu )
@@ -134,8 +133,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);
+            ret = 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,16 +153,13 @@ 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);
+            ret = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
+                                           sid, sbit, addr);
             break;
         default:
             dprintk(XENLOG_WARNING VTDPREFIX, "invalid vt-d flush type\n");
             return -EOPNOTSUPP;
         }
-
-        if ( !ret )
-            ret = rc;
     }
 
     return ret;
-- 
1.9.1


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

Re: [PATCH v8 2/3] VT-d: Wrap a _sync version for all VT-d flush interfaces

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 5325 bytes --]

On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote:
> For consistency, we wrap a _sync version for all VT-d flush
> interfaces.
>
I'm sorry, maybe it's me, but "for consistency" with what?

I see from where this comes, if I look at v7. But when this patch will
be committed, what it is doing and why we decided to do it should be
evident by just reading the changelog, without having to google for the
review history.

So, please, try to describe the situation a little bit better (e.g., do
we have inconsistencies, right now? Is this needed for avoiding
introducing inconsistencies by means of this series? Etc.).

> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -72,6 +72,70 @@ static void qinval_update_qtail(struct iommu
> *iommu, unsigned int index)
>      dmar_writeq(iommu->reg, DMAR_IQT_REG, (val <<
> QINVAL_INDEX_SHIFT));
>  }
>  
> +static int __must_check queue_invalidate_wait(struct iommu *iommu,
> +    u8 iflag, u8 sw, u8 fn)
> +{
>
It looks like you are "just" moving this function, without making any
change to the code, is this the case?

Assuming it is, mixing pure code movement and functional changes in the
same patch makes reviewing the patch itself harder. And pure code motio
is also bad for archaeologists (`git blame' would point at this commit
for all of this function!).

So, I'd say either isolate the code movement in a pre-patch, or try
using forward declarations. Given how moving messes up history, my
personal preference would be for the latter.

> +static int invalidate_sync(struct iommu *iommu)
> +{
> +    struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
> +
> +    if ( qi_ctrl->qinval_maddr )
> +        return queue_invalidate_wait(iommu, 0, 1, 1);
> +
> +    return 0;
> +}

Same for this, even worse, in terms of how hard this makes to review
this patch, as can be seen...

> @@ -135,65 +208,12 @@ static void queue_invalidate_iotlb(struct iommu
> *iommu,
>
> -static int invalidate_sync(struct iommu *iommu)
> +static int queue_invalidate_iotlb_sync(struct iommu *iommu,
> +    u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr)
>  {
> -    struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
> +    queue_invalidate_iotlb(iommu, granu, dr, dw, did, am, ih, addr);
>  
> -    if ( qi_ctrl->qinval_maddr )
> -        return queue_invalidate_wait(iommu, 0, 1, 1);
> -    return 0;
> +    return invalidate_sync(iommu);
>  }
>  
...here!

The rest of this patch looks fine to me, with only one more doubt.
Here:

> --- a/xen/drivers/passthrough/vtd/x86/ats.c
> +++ b/xen/drivers/passthrough/vtd/x86/ats.c
> @@ -118,7 +118,6 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16
> did,
>      {
>          u16 sid = PCI_BDF2(pdev->bus, pdev->devfn);
>          bool_t sbit;
> -        int rc = 0;
>  
>          /* Only invalidate devices that belong to this IOMMU */
>          if ( pdev->iommu != iommu )
> @@ -134,8 +133,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);
> +            ret = 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,16 +153,13 @@ 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);
> +            ret = qinval_device_iotlb_sync(iommu, pdev-
> >ats_queue_depth,
> +                                           sid, sbit, addr);
>              break;
>          default:
>              dprintk(XENLOG_WARNING VTDPREFIX, "invalid vt-d flush
> type\n");
>              return -EOPNOTSUPP;
>          }
> -
> -        if ( !ret )
> -            ret = rc;
>      }
>  
>      return ret;
>
Am I misreading something or we are introducing synchronous handling,
which was not there before?

If yes, is it ok to do this in this patch?

And if yes again, I think that it at least should be noted in the
changelog, as it would mean that the patch is not only introducing some
wrappers.

Regads,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v8 2/3] VT-d: Wrap a _sync version for all VT-d flush interfaces

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 3012 bytes --]

On Thu, 2016-03-24 at 14:56 +0100, Dario Faggioli wrote:
> On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote:
> > 
> > @@ -134,8 +133,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);
> > +            ret = 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,16 +153,13 @@ 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);
> > +            ret = qinval_device_iotlb_sync(iommu, pdev-
> > > 
> > > ats_queue_depth,
> > +                                           sid, sbit, addr);
> >              break;
> >          default:
> >              dprintk(XENLOG_WARNING VTDPREFIX, "invalid vt-d flush
> > type\n");
> >              return -EOPNOTSUPP;
> >          }
> > -
> > -        if ( !ret )
> > -            ret = rc;
> >      }
> >  
> >      return ret;
> > 
> Am I misreading something or we are introducing synchronous handling,
> which was not there before?
> 
> If yes, is it ok to do this in this patch?
> 
> And if yes again, I think that it at least should be noted in the
> changelog, as it would mean that the patch is not only introducing
> some
> wrappers.
> 
Ok, I think I see what's happening here. Before this patch,
invalidate_sync() was being called inside qinval_device_iotlb(), so we
were synchronous already, and we need to continue to be like that, by
calling the _sync() variants.

Yes, if this is what happens, this also looks ok to me.

Sorry for the noise.

Regards,
Dario

> Regads,
> Dario
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v8 2/3] VT-d: Wrap a _sync version for all VT-d flush interfaces

[Xu, Quan]

On March 24, 2016 11:06pm, <dario.faggioli@citrix.com> wrote:
> On Thu, 2016-03-24 at 14:56 +0100, Dario Faggioli wrote:
> > On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote:
> > >
> > > @@ -134,8 +133,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);
> > > +            ret = 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,16
> > > +153,13 @@ 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);
> > > +            ret = qinval_device_iotlb_sync(iommu, pdev-
> > > >
> > > > ats_queue_depth,
> > > +                                           sid, sbit,
> addr);
> > >              break;
> > >          default:
> > >              dprintk(XENLOG_WARNING VTDPREFIX, "invalid vt-d
> flush
> > > type\n");
> > >              return -EOPNOTSUPP;
> > >          }
> > > -
> > > -        if ( !ret )
> > > -            ret = rc;
> > >      }
> > >
> > >      return ret;
> > >
> > Am I misreading something or we are introducing synchronous handling,
> > which was not there before?
> >
> > If yes, is it ok to do this in this patch?
> >
> > And if yes again, I think that it at least should be noted in the
> > changelog, as it would mean that the patch is not only introducing
> > some wrappers.
> >
> Ok, I think I see what's happening here. Before this patch,
> invalidate_sync() was being called inside qinval_device_iotlb(), so we were
> synchronous already, and we need to continue to be like that, by calling the
> _sync() variants.
> 
yes, it not as well understood, but to me, it is difficult to describe it in changelog.
Let me elaborate briefly on the evolution:
1. In original code, without my patch, it is:
        ...
        if ( flush_dev_iotlb )
            ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
        rc = invalidate_sync(iommu);
        ...


In dev_invalidate_iotlb(), it scans ats_devices list the calls qinval_device_iotlb() to flush the Device-TLB via 'Device-TLB Invalidate Descriptor'.
In invalidate_sync(), it synchronize with hardware for the invalidation request descriptors submitted before the wait descriptor via 'Invalidation Wait Descriptor'.

If we assign multiple ATS devices to a domain and invalidate_sync() times out, we couldn't find which one times out. Then,

2. in my previous patch, I put the invalidate_sync() variant (-as we need to pass down the device's SBDF to hide the ATS device) within dev_invalidate_iotlb() to flush 
the ATS device one by one. if it timed out, I could find the bogus device and hide it.

3. as Kevin mentioned, I put invalidation sync within dev_invalidate_iotlb, while for all other IOMMU invalidations the sync is put after. That is the consistency issue.
  That's also why I provide _sync version in v8. (could you help me enhance the description? :) )

> Yes, if this is what happens, this also looks ok to me.
> Sorry for the noise.
> 
Dario, feel free to express yourself. As you know, I'd appreciate your (and the other's) comments.:)

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

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

[Quan Xu]

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

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index ca77e3b..384dde7 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1532,6 +1532,13 @@ 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.
+
 ### 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 v8 1/3] VT-d: Reduce spin timeout to 1ms, which can be boot-time changed

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1919 bytes --]

On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote:
> 
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1532,6 +1532,13 @@ 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.
> +
Perhaps we should put down a quick paragraph to explain what this
parameter is about, and what happens if the timeout expires?

Giving hints on under what conditions one should change this value
would be even more welcome (e.g. "if you see error XXX, try increasing
the timeout to nnn").

> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> 
> @@ -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)
>  {
>
I'm not sure it is ok to add the __must_check annotation here in this
patch, considering that the subject says "VT-d: Reduce spin timeout to
1ms, which can be boot-time changed", and there is no changelog.

Personally, I think it could be ok, but at least add a few words of
changelog, mentioning that this is also being done.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Xu, Quan]

On March 24, 2016 7:04pm, <dario.faggioli@citrix.com> wrote:
> On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote:
> >
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -1532,6 +1532,13 @@ 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.
> > +
> Perhaps we should put down a quick paragraph to explain what this parameter
> is about, and what happens if the timeout expires?
> 
> Giving hints on under what conditions one should change this value would be
> even more welcome (e.g. "if you see error XXX, try increasing the timeout to
> nnn").
> 
Make sense. I would send out this paragraph before v9. I hope you could help me correct it. :)


> > --- a/xen/drivers/passthrough/vtd/qinval.c
> > +++ b/xen/drivers/passthrough/vtd/qinval.c
> >
> > @@ -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)
> >  {
> >
> I'm not sure it is ok to add the __must_check annotation here in this patch,
> considering that the subject says "VT-d: Reduce spin timeout to 1ms, which can
> be boot-time changed", and there is no changelog.
> 
> Personally, I think it could be ok, but at least add a few words of changelog,
> mentioning that this is also being done.
> 
Thanks for your reminding. It's better to add a few words of changelog.

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

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

[Konrad Rzeszutek Wilk]

On Thu, Mar 24, 2016 at 01:57:58PM +0800, Quan Xu wrote:

Hey!

Thanks for the patch! 

I see that you have __must_check..

But if you check the callchain:

iommu_flush_iec_[index|global] ->
    __iommu_flush_iec->invalidate_sync-> queue_invalidate_wait

you will see that the callers of iommu_flush_iec_[index|global]
ignore the return value.

So ... perhaps you could explain in this commit description on how to address
that? Is there an followup patch (if so just put in the name in here) to
address that?

Or should the top callers: enable_intremap, ioapic_rte_to_remap_entry,
free_remap_entry, msi_msg_to_remap_entry, and pi_update_irte do something?

I guess the 'free_remap_entry' reall can't. As you are suppose to always
be able to free something.


msi_msg_to_remap_entry, _msg_to_remap_entry, and ioapic_rte_to_remap_entry
could return an value... Or considering this is v8 - was there some epic
conversation that went over this quite a lot? In which case I would recommend
you say why it was not attempted this way so that folks six months from
now when reading this patch won't ask again.


> Signed-off-by: Quan Xu <quan.xu@intel.com>
> ---
>  docs/misc/xen-command-line.markdown  |  7 +++++++
>  xen/drivers/passthrough/vtd/qinval.c | 17 ++++++++++++-----
>  2 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index ca77e3b..384dde7 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1532,6 +1532,13 @@ 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.
> +
>  ### 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

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

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

[Xu, Quan]

On March 26, 2016 4:07am, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Thu, Mar 24, 2016 at 01:57:58PM +0800, Quan Xu wrote:
> 
> Hey!
> 
> Thanks for the patch!
> 
> I see that you have __must_check..
> 
> But if you check the callchain:
> 
> iommu_flush_iec_[index|global] ->
>     __iommu_flush_iec->invalidate_sync-> queue_invalidate_wait
> 
> you will see that the callers of iommu_flush_iec_[index|global] ignore the
> return value.
> 
> So ... perhaps you could explain in this commit description on how to address
> that?

I mentioned it in 0000-cover-letter.patch -- "Not covered in this series:".
IMO, it is not a good idea to explain in this commit description, as I don't touch it.  
Right?

> Is there an followup patch (if so just put in the name in here) to address
> that?
> 
Yes,

> Or should the top callers: enable_intremap, ioapic_rte_to_remap_entry,
> free_remap_entry, msi_msg_to_remap_entry, and pi_update_irte do
> something?
> 

I'd prefer to discuss about it in another thread. btw, I have spent a lot of time on 
Interrupt research, including the logical topology of interrupt hardware/software, .e.g, lapci/vlapic/ioapic/vioapic/msi/PI.
In last months, I wondered whether I could disable the interrupt remapping dynamically( by 'iommu_intremap = 0') or not.
Now I think it would be insecure.
IMO, it is not a good time to discuss this new topic, otherwise it makes me exhausted.

Quan

> I guess the 'free_remap_entry' reall can't. As you are suppose to always be able
> to free something.
> 
> 
> msi_msg_to_remap_entry, _msg_to_remap_entry, and
> ioapic_rte_to_remap_entry could return an value... Or considering this is v8 -
> was there some epic conversation that went over this quite a lot? In which case
> I would recommend you say why it was not attempted this way so that folks six
> months from now when reading this patch won't ask again.
> 
> 
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> > ---
> >  docs/misc/xen-command-line.markdown  |  7 +++++++
> > xen/drivers/passthrough/vtd/qinval.c | 17 ++++++++++++-----
> >  2 files changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/docs/misc/xen-command-line.markdown
> > b/docs/misc/xen-command-line.markdown
> > index ca77e3b..384dde7 100644
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -1532,6 +1532,13 @@ 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.
> > +
> >  ### 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
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Konrad Rzeszutek Wilk]

On Mon, Mar 28, 2016 at 06:27:21AM +0000, Xu, Quan wrote:
> On March 26, 2016 4:07am, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > On Thu, Mar 24, 2016 at 01:57:58PM +0800, Quan Xu wrote:
> > 
> > Hey!
> > 
> > Thanks for the patch!
> > 
> > I see that you have __must_check..
> > 
> > But if you check the callchain:
> > 
> > iommu_flush_iec_[index|global] ->
> >     __iommu_flush_iec->invalidate_sync-> queue_invalidate_wait
> > 
> > you will see that the callers of iommu_flush_iec_[index|global] ignore the
> > return value.
> > 
> > So ... perhaps you could explain in this commit description on how to address
> > that?
> 
> I mentioned it in 0000-cover-letter.patch -- "Not covered in this series:".

But that goodness never gets documented in the code..

> IMO, it is not a good idea to explain in this commit description, as I don't touch it.  
> Right?

Why?

Put yourself in the shoes of a an engineer who wants to fix or add some code.

He or she will look at the code and say: Ok, they added __mustcheck so they
MUST have thought it is important to check the return code. But then tracking
through the different users and then it is ignored two or three levels up.

Huh? Why didn't the account for that?

If you include that information in the description it will save them further
work from having to guess or email you.

> 
> > Is there an followup patch (if so just put in the name in here) to address
> > that?
> > 
> Yes,

In which case I would just say that:

"The followup patch titled XYZ 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."

And that 
> 
> > Or should the top callers: enable_intremap, ioapic_rte_to_remap_entry,
> > free_remap_entry, msi_msg_to_remap_entry, and pi_update_irte do
> > something?
> > 
> 
> I'd prefer to discuss about it in another thread. btw, I have spent a lot of time on 
> Interrupt research, including the logical topology of interrupt hardware/software, .e.g, lapci/vlapic/ioapic/vioapic/msi/PI.
> In last months, I wondered whether I could disable the interrupt remapping dynamically( by 'iommu_intremap = 0') or not.
> Now I think it would be insecure.
> IMO, it is not a good time to discuss this new topic, otherwise it makes me exhausted.

<grins>

I would recommend you send out an email to xen-devel with [BRAINSTORM] or such to explain
the problem or ways to make it work. That way we can all benefit from your hard digging
in the code and untangling it.

Or if you can - be at the Xen Hackathon in April to brainstorm this in person?

> 
> Quan
> 
> > I guess the 'free_remap_entry' reall can't. As you are suppose to always be able
> > to free something.
> > 
> > 
> > msi_msg_to_remap_entry, _msg_to_remap_entry, and
> > ioapic_rte_to_remap_entry could return an value... Or considering this is v8 -
> > was there some epic conversation that went over this quite a lot? In which case
> > I would recommend you say why it was not attempted this way so that folks six
> > months from now when reading this patch won't ask again.
> > 
> > 
> > > Signed-off-by: Quan Xu <quan.xu@intel.com>
> > > ---
> > >  docs/misc/xen-command-line.markdown  |  7 +++++++
> > > xen/drivers/passthrough/vtd/qinval.c | 17 ++++++++++++-----
> > >  2 files changed, 19 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/docs/misc/xen-command-line.markdown
> > > b/docs/misc/xen-command-line.markdown
> > > index ca77e3b..384dde7 100644
> > > --- a/docs/misc/xen-command-line.markdown
> > > +++ b/docs/misc/xen-command-line.markdown
> > > @@ -1532,6 +1532,13 @@ 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.
> > > +
> > >  ### 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
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel

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

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

[Xu, Quan]

On March 28, 2016 9:32pm, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Mon, Mar 28, 2016 at 06:27:21AM +0000, Xu, Quan wrote:
> > On March 26, 2016 4:07am, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> wrote:
> > > On Thu, Mar 24, 2016 at 01:57:58PM +0800, Quan Xu wrote:


> <grins>
> 
 <grins> DX

> I would recommend you send out an email to xen-devel with [BRAINSTORM] or


Yes, I will do it.

> such to explain the problem or ways to make it work. 
> That way we can all benefit from your hard digging in the code and untangling it.
> 
> Or if you can - be at the Xen Hackathon in April to brainstorm this in person?
> 

I would be very happy, if I can have a f2f discussion with you, Jan, Dario, Wei, Daniel, Stefano  ...
But sorry, I can't. I have the problems with my passport. I am in Shanghai China now.

Quan

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

[PATCH v8 3/3] VT-d: Fix vt-d Device-TLB flush timeout issue

[Quan Xu]

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

The hidden device should be disallowed to be further assigned
to any domain.

Signed-off-by: Quan Xu <quan.xu@intel.com>
---
 xen/drivers/passthrough/pci.c         |  6 ++--
 xen/drivers/passthrough/vtd/extern.h  |  3 +-
 xen/drivers/passthrough/vtd/qinval.c  | 58 +++++++++++++++++++++++++++++++++--
 xen/drivers/passthrough/vtd/x86/ats.c | 11 ++++---
 xen/include/xen/pci.h                 |  1 +
 5 files changed, 68 insertions(+), 11 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 6d3187d..94e2c11 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -62,7 +62,8 @@ 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);
 int qinval_device_iotlb_sync(struct iommu *iommu, u32 max_invs_pend,
-                             u16 sid, u16 size, u64 addr);
+                             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 ad9e265..10c5684 100644
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -216,6 +216,58 @@ static int queue_invalidate_iotlb_sync(struct iommu *iommu,
     return invalidate_sync(iommu);
 }
 
+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]);
+
+    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);
+
+    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 qinval_device_iotlb(struct iommu *iommu,
     u32 max_invs_pend, u16 sid, u16 size, u64 addr)
 {
@@ -250,11 +302,13 @@ int qinval_device_iotlb(struct iommu *iommu,
 }
 
 int 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)
 {
+    u16 sid = PCI_BDF2(bus, devfn);
+
     qinval_device_iotlb(iommu, max_invs_pend, sid, size, addr);
 
-    return invalidate_sync(iommu);
+    return dev_invalidate_sync(iommu, did, seg, bus, devfn);
 }
 
 static void queue_invalidate_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx)
diff --git a/xen/drivers/passthrough/vtd/x86/ats.c b/xen/drivers/passthrough/vtd/x86/ats.c
index 7b1c07b..5d1ebea 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;
 
         /* Only invalidate devices that belong to this IOMMU */
@@ -133,8 +132,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;
-            ret = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
-                                           sid, sbit, addr);
+            ret = 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) )
@@ -153,8 +153,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
                 addr |= (((u64)1 << (size_order - 1)) - 1) << PAGE_SHIFT_4K;
             }
 
-            ret = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
-                                           sid, sbit, addr);
+            ret = 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..bb9f791 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -116,6 +116,7 @@ const unsigned long *pci_get_ro_map(u16 seg);
 int pci_add_device(u16 seg, u8 bus, u8 devfn,
                    const struct pci_dev_info *, nodeid_t node);
 int pci_remove_device(u16 seg, u8 bus, u8 devfn);
+void pci_hide_existing_device(struct pci_dev *pdev);
 int pci_ro_device(int seg, int bus, int devfn);
 int pci_hide_device(int bus, int devfn);
 struct pci_dev *pci_get_pdev(int seg, int bus, int devfn);
-- 
1.9.1


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

Re: [PATCH v8 3/3] VT-d: Fix vt-d Device-TLB flush timeout issue

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1119 bytes --]

On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote:
> If Device-TLB flush timed out, we would hide the target ATS
> device and crash the domain owning this ATS device. If impacted
> domain is hardware domain, just throw out a warning.
> 
> The hidden device should be disallowed to be further assigned
> to any domain.
> 
What is "should be disallowed" supposed to mean here? Isn't the
situation that, by hiding the device, which this patch is doing, we
actually disallow any further assignment?

If yes, this should rather be (something like):

"By hiding the device, we make sure it can't be assigned to any domain
any longer."

Other than this, the patch looks good to me, but I'll re-review it when
the new version comes out (with the other patches from the preliminary
series folded in), before saying Reviewed-by.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v8 3/3] VT-d: Fix vt-d Device-TLB flush timeout issue

[Xu, Quan]

On March 24, 2016 11:38pm, <dario.faggioli@citrix.com> wrote:
> On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote:
> > If Device-TLB flush timed out, we would hide the target ATS device and
> > crash the domain owning this ATS device. If impacted domain is
> > hardware domain, just throw out a warning.
> >
> > The hidden device should be disallowed to be further assigned to any
> > domain.
> >
> What is "should be disallowed" supposed to mean here? Isn't the situation that,
> by hiding the device, which this patch is doing, we actually disallow any further
> assignment?
> 
> If yes, this should rather be (something like):
> 
> "By hiding the device, we make sure it can't be assigned to any domain any
> longer."
> 

 Let me introduce what happen first.

If the ATS device flush timed out, it would be assigned to dom_xen (dummy domain).
Then, .e.g, the ATS device is 0000:81:00.0 in my platform. If the ATS device 0000:81:00.0 is hidden and we assign the ATS device 0000:81:00.0 to another domain with xl tool,
It would fail and print out:
   """ libxl: error: libxl_pci.c:1216:libxl__device_pci_add: PCI device 0000:81:00.0 already assigned to a different guest? """

To be honest, I'd prefer your description. do you think?

Quan

> Other than this, the patch looks good to me, but I'll re-review it when the new
> version comes out (with the other patches from the preliminary series folded in),
> before saying Reviewed-by.

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

Re: [PATCH v8 3/3] VT-d: Fix vt-d Device-TLB flush timeout issue

[Konrad Rzeszutek Wilk]

On Thu, Mar 24, 2016 at 04:38:05PM +0100, Dario Faggioli wrote:
> On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote:
> > If Device-TLB flush timed out, we would hide the target ATS
> > device and crash the domain owning this ATS device. If impacted
> > domain is hardware domain, just throw out a warning.
> > 
> > The hidden device should be disallowed to be further assigned
> > to any domain.
> > 
> What is "should be disallowed" supposed to mean here? Isn't the
> situation that, by hiding the device, which this patch is doing, we
> actually disallow any further assignment?

Yes.

Take a look at device_assigned. This patch reassigns the device to dom_xen
so device_assigned will return -EBUSY.

Actually that information could be part of the commit to get an
idea of the effects of this patch.
> 
> If yes, this should rather be (something like):
> 
> "By hiding the device, we make sure it can't be assigned to any domain
> any longer."
> 
> Other than this, the patch looks good to me, but I'll re-review it when
> the new version comes out (with the other patches from the preliminary
> series folded in), before saying Reviewed-by.
> 
> Regards,
> Dario
> -- 
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
> 



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


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

Re: [PATCH v8 3/3] VT-d: Fix vt-d Device-TLB flush timeout issue

[Xu, Quan]

On March 26, 2016 4:40am, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Thu, Mar 24, 2016 at 04:38:05PM +0100, Dario Faggioli wrote:
> > On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote:
> > > If Device-TLB flush timed out, we would hide the target ATS device
> > > and crash the domain owning this ATS device. If impacted domain is
> > > hardware domain, just throw out a warning.
> > >
> > > The hidden device should be disallowed to be further assigned to any
> > > domain.
> > >
> > What is "should be disallowed" supposed to mean here? Isn't the
> > situation that, by hiding the device, which this patch is doing, we
> > actually disallow any further assignment?
> 
> Yes.
> 
> Take a look at device_assigned. This patch reassigns the device to dom_xen so
> device_assigned will return -EBUSY.
> 
> Actually that information could be part of the commit to get an idea of the
> effects of this patch.
> >

Konrad, Welcome.
Yes, I would try to add it in changelog.

Quan

> > If yes, this should rather be (something like):
> >
> > "By hiding the device, we make sure it can't be assigned to any domain
> > any longer."
> >
> > Other than this, the patch looks good to me, but I'll re-review it
> > when the new version comes out (with the other patches from the
> > preliminary series folded in), before saying Reviewed-by.
> >
> > Regards,
> > Dario
> > --
> > <<This happens because I choose it to happen!>> (Raistlin Majere)
> > -----------------------------------------------------------------
> > Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software
> > Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
> >
> 
> 
> 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v8 3/3] VT-d: Fix vt-d Device-TLB flush timeout issue

[Xu, Quan]

+cc Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, who is also reviewing this patch.

On March 24, 2016 11:38pm, Dario Faggioli <dario.faggioli@citrix.com> wrote:
> On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote:
> > If Device-TLB flush timed out, we would hide the target ATS device and
> > crash the domain owning this ATS device. If impacted domain is
> > hardware domain, just throw out a warning.
> >
> > The hidden device should be disallowed to be further assigned to any
> > domain.
> >
> What is "should be disallowed" supposed to mean here? Isn't the situation that,
> by hiding the device, which this patch is doing, we actually disallow any further
> assignment?
> 
> If yes, this should rather be (something like):
> 
> "By hiding the device, we make sure it can't be assigned to any domain any
> longer."
> 
> Other than this, the patch looks good to me, but I'll re-review it when the new
> version comes out (with the other patches from the preliminary series folded in),
> before saying Reviewed-by.

Dario,
What about this one:

"""
VT-d: Fix vt-d Device-TLB flush timeout issue

If Device-TLB flush timed out, we would hide the target ATS
device and crash the domain owning this ATS device. If impacted
domain is hardware domain, the error handling is just ignored.

By hiding the device, we make sure it can't be assigned to any
domain any longer.
"""

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

Re: [PATCH v8 3/3] VT-d: Fix vt-d Device-TLB flush timeout issue

[Konrad Rzeszutek Wilk]

On Thu, Mar 24, 2016 at 01:57:59PM +0800, Quan Xu wrote:
> If Device-TLB flush timed out, we would hide the target ATS
> device and crash the domain owning this ATS device. If impacted
> domain is hardware domain, just throw out a warning.
> 
> The hidden device should be disallowed to be further assigned
> to any domain.
> 
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> ---
>  xen/drivers/passthrough/pci.c         |  6 ++--
>  xen/drivers/passthrough/vtd/extern.h  |  3 +-
>  xen/drivers/passthrough/vtd/qinval.c  | 58 +++++++++++++++++++++++++++++++++--
>  xen/drivers/passthrough/vtd/x86/ats.c | 11 ++++---
>  xen/include/xen/pci.h                 |  1 +
>  5 files changed, 68 insertions(+), 11 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 6d3187d..94e2c11 100644
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -62,7 +62,8 @@ 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);
>  int qinval_device_iotlb_sync(struct iommu *iommu, u32 max_invs_pend,
> -                             u16 sid, u16 size, u64 addr);
> +                             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 ad9e265..10c5684 100644
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -216,6 +216,58 @@ static int queue_invalidate_iotlb_sync(struct iommu *iommu,
>      return invalidate_sync(iommu);
>  }
>  
> +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]);
> +
> +    if ( d == NULL )
> +        return;
> +
> +    pcidevs_lock();
> +
> +    for_each_pdev(d, pdev)
> +    {
> +        if ( ( pdev->seg == seg ) &&
> +             ( pdev->bus == bus ) &&
> +             ( pdev->devfn == devfn ) )
> +        {
> +            ASSERT ( pdev->domain );

Oddly enough (and I don't see this in the StyleGuide), the ASSERTS
do not require the spaces. So it can be:

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);

The description said something about 'just throw out a warning' (if
the domain owning it is a hardware domain). That seems to be missing?


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

Re: [PATCH v8 3/3] VT-d: Fix vt-d Device-TLB flush timeout issue

[Xu, Quan]

On March 26, 2016 4:32am, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Thu, Mar 24, 2016 at 01:57:59PM +0800, Quan Xu wrote:
> > If Device-TLB flush timed out, we would hide the target ATS device and
> > crash the domain owning this ATS device. If impacted domain is
> > hardware domain, just throw out a warning.
> >
> > The hidden device should be disallowed to be further assigned to any
> > domain.
> >
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> > ---
> >  xen/drivers/passthrough/pci.c         |  6 ++--
> >  xen/drivers/passthrough/vtd/extern.h  |  3 +-
> > xen/drivers/passthrough/vtd/qinval.c  | 58
> > +++++++++++++++++++++++++++++++++--
> >  xen/drivers/passthrough/vtd/x86/ats.c | 11 ++++---
> >  xen/include/xen/pci.h                 |  1 +
> >  5 files changed, 68 insertions(+), 11 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 6d3187d..94e2c11 100644
> > --- a/xen/drivers/passthrough/vtd/extern.h
> > +++ b/xen/drivers/passthrough/vtd/extern.h
> > @@ -62,7 +62,8 @@ 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);  int qinval_device_iotlb_sync(struct iommu *iommu, u32
> max_invs_pend,
> > -                             u16 sid, u16 size, u64 addr);
> > +                             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 ad9e265..10c5684 100644
> > --- a/xen/drivers/passthrough/vtd/qinval.c
> > +++ b/xen/drivers/passthrough/vtd/qinval.c
> > @@ -216,6 +216,58 @@ static int queue_invalidate_iotlb_sync(struct iommu
> *iommu,
> >      return invalidate_sync(iommu);
> >  }
> >
> > +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]);
> > +
> > +    if ( d == NULL )
> > +        return;
> > +
> > +    pcidevs_lock();
> > +
> > +    for_each_pdev(d, pdev)
> > +    {
> > +        if ( ( pdev->seg == seg ) &&
> > +             ( pdev->bus == bus ) &&
> > +             ( pdev->devfn == devfn ) )
> > +        {
> > +            ASSERT ( pdev->domain );
> 
> Oddly enough (and I don't see this in the StyleGuide), the ASSERTS do not
> require the spaces. So it can be:
> 
> ASSERT(pdev->domain);

Got it, I will fix it in next v9.


> > +            list_del(&pdev->domain_list);
> > +            pdev->domain = NULL;
> > +            pci_hide_existing_device(pdev);
> > +            break;
> > +        }
> > +    }
> > +
> > +    pcidevs_unlock();
> > +
> > +    if ( !is_hardware_domain(d) )
> > +        domain_crash(d);
> 
> The description said something about 'just throw out a warning' (if the domain
> owning it is a hardware domain). That seems to be missing?
> 
> 

The warning is in the call path, in queue_invalidate_wait():
  """Queue invalidate wait descriptor timed out."""

Then, does it make sense?

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

Re: [PATCH v8 3/3] VT-d: Fix vt-d Device-TLB flush timeout issue

[Konrad Rzeszutek Wilk]

> 
> > > +            list_del(&pdev->domain_list);
> > > +            pdev->domain = NULL;
> > > +            pci_hide_existing_device(pdev);
> > > +            break;
> > > +        }
> > > +    }
> > > +
> > > +    pcidevs_unlock();
> > > +
> > > +    if ( !is_hardware_domain(d) )
> > > +        domain_crash(d);
> > 
> > The description said something about 'just throw out a warning' (if the domain
> > owning it is a hardware domain). That seems to be missing?
> > 
> > 
> 
> The warning is in the call path, in queue_invalidate_wait():
>   """Queue invalidate wait descriptor timed out."""

Aah, right.
> 
> Then, does it make sense?

Yes. I would recommend you modify the commit description so that clueless
folks like me can see it. You could modify the commit description to say:

"just throw out a warning (done in queue_invalidate_wait)."


> 
> Quan

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

Re: [PATCH v8 3/3] VT-d: Fix vt-d Device-TLB flush timeout issue

[Xu, Quan]

On March 28, 2016 10:11pm, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> >
> > > > +            list_del(&pdev->domain_list);
> > > > +            pdev->domain = NULL;
> > > > +            pci_hide_existing_device(pdev);
> > > > +            break;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    pcidevs_unlock();
> > > > +
> > > > +    if ( !is_hardware_domain(d) )
> > > > +        domain_crash(d);
> > >
> > > The description said something about 'just throw out a warning' (if
> > > the domain owning it is a hardware domain). That seems to be missing?
> > >
> > >
> >
> > The warning is in the call path, in queue_invalidate_wait():
> >   """Queue invalidate wait descriptor timed out."""
> 
> Aah, right.
> >
> > Then, does it make sense?
> 
> Yes. I would recommend you modify the commit description so that clueless
> folks like me can see it. You could modify the commit description to say:
> 
> "just throw out a warning (done in queue_invalidate_wait)."
> 
> 

Then, based on Dario/your suggestion, the changelog could be:
"""
VT-d: Fix vt-d Device-TLB flush timeout issue

If Device-TLB flush timed out, we would hide the target ATS device and crash the domain owning this ATS device.
If impacted domain is hardware domain, just throw out a warning (done in queue_invalidate_wait).

By hiding the device, we make sure it can't be assigned to any domain any longer.
"""

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

Re: [PATCH v8 3/3] VT-d: Fix vt-d Device-TLB flush timeout issue

[Konrad Rzeszutek Wilk]

On Tue, Mar 29, 2016 at 01:32:02AM +0000, Xu, Quan wrote:
> On March 28, 2016 10:11pm, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > >
> > > > > +            list_del(&pdev->domain_list);
> > > > > +            pdev->domain = NULL;
> > > > > +            pci_hide_existing_device(pdev);
> > > > > +            break;
> > > > > +        }
> > > > > +    }
> > > > > +
> > > > > +    pcidevs_unlock();
> > > > > +
> > > > > +    if ( !is_hardware_domain(d) )
> > > > > +        domain_crash(d);
> > > >
> > > > The description said something about 'just throw out a warning' (if
> > > > the domain owning it is a hardware domain). That seems to be missing?
> > > >
> > > >
> > >
> > > The warning is in the call path, in queue_invalidate_wait():
> > >   """Queue invalidate wait descriptor timed out."""
> > 
> > Aah, right.
> > >
> > > Then, does it make sense?
> > 
> > Yes. I would recommend you modify the commit description so that clueless
> > folks like me can see it. You could modify the commit description to say:
> > 
> > "just throw out a warning (done in queue_invalidate_wait)."
> > 
> > 
> 
> Then, based on Dario/your suggestion, the changelog could be:
> """
> VT-d: Fix vt-d Device-TLB flush timeout issue
> 
> If Device-TLB flush timed out, we would hide the target ATS device and crash the domain owning this ATS device.
> If impacted domain is hardware domain, just throw out a warning (done in queue_invalidate_wait).
> 
> By hiding the device, we make sure it can't be assigned to any domain any longer.
> """

s/to any domain any longer./to any domain any longer (see device_assigned)./


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

Re: [PATCH v8 3/3] VT-d: Fix vt-d Device-TLB flush timeout issue

[Xu, Quan]

On March 29, 2016 10:21pm, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Tue, Mar 29, 2016 at 01:32:02AM +0000, Xu, Quan wrote:
> > On March 28, 2016 10:11pm, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> > > >
> > > > > > +            list_del(&pdev->domain_list);
> > > > > > +            pdev->domain = NULL;
> > > > > > +            pci_hide_existing_device(pdev);
> > > > > > +            break;
> > > > > > +        }
> > > > > > +    }
> > > > > > +
> > > > > > +    pcidevs_unlock();
> > > > > > +
> > > > > > +    if ( !is_hardware_domain(d) )
> > > > > > +        domain_crash(d);
> > > > >
> > > > > The description said something about 'just throw out a warning'
> > > > > (if the domain owning it is a hardware domain). That seems to be
> missing?
> > > > >
> > > > >
> > > >
> > > > The warning is in the call path, in queue_invalidate_wait():
> > > >   """Queue invalidate wait descriptor timed out."""
> > >
> > > Aah, right.
> > > >
> > > > Then, does it make sense?
> > >
> > > Yes. I would recommend you modify the commit description so that
> > > clueless folks like me can see it. You could modify the commit description to
> say:
> > >
> > > "just throw out a warning (done in queue_invalidate_wait)."
> > >
> > >
> >
> > Then, based on Dario/your suggestion, the changelog could be:
> > """
> > VT-d: Fix vt-d Device-TLB flush timeout issue
> >
> > If Device-TLB flush timed out, we would hide the target ATS device and crash
> the domain owning this ATS device.
> > If impacted domain is hardware domain, just throw out a warning (done in
> queue_invalidate_wait).
> >
> > By hiding the device, we make sure it can't be assigned to any domain any
> longer.
> > """
> 
> s/to any domain any longer./to any domain any longer (see device_assigned)./
> 
Got it, thanks.

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

Re: [PATCH v8 0/3] VT-d Device-TLB flush issue

[Jan Beulich]

>>> On 24.03.16 at 06:57, <quan.xu@intel.com> wrote:
> **NOTE**
>    This patch set should base on 2 prereq patch sets:
>     a). Make the pcidevs_lock a recursive one.

This one already went in, so is pointless to mention here.

>     b). Check VT-d Device-TLB flush error.

And this one is WIP, so it's continuing to be cumbersome to review
this series without the other one even known to be almost ready
to go in. I think loose dependencies are okay, but in the case here
everything would better be presented together in one series.

Jan


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

Re: [PATCH v8 0/3] VT-d Device-TLB flush issue

[Xu, Quan]

On March 24, 2016 6:33pm, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 24.03.16 at 06:57, <quan.xu@intel.com> wrote:
> > **NOTE**
> >    This patch set should base on 2 prereq patch sets:
> >     a). Make the pcidevs_lock a recursive one.
> 
> This one already went in, so is pointless to mention here.


Agreed,

> 
> >     b). Check VT-d Device-TLB flush error.
> 
> And this one is WIP,
Yes,

> so it's continuing to be cumbersome to review this series

Sorry, I don't think so.

> without the other one even known to be almost ready to go in. I think loose
> dependencies are okay, but in the case here everything would better be
> presented together in one series.
> 
ok. I would send them together in next series.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v8 0/3] VT-d Device-TLB flush issue

[Xu, Quan]

On March 24, 2016 7:11pm, <quan.xu@intel.com> wrote:
> On March 24, 2016 6:33pm, Jan Beulich <JBeulich@suse.com> wrote:
> > >>> On 24.03.16 at 06:57, <quan.xu@intel.com> wrote:


> > without the other one even known to be almost ready to go in. I think
> > loose dependencies are okay, but in the case here everything would
> > better be presented together in one series.
> >
> ok. I would send them together in next series.

Think twice, I'd better fix them and send out asap. When the interval is too long, I am afraid I may overlook some
Of the details, even I can read all of reply.

Quan

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