[PATCH v12 0/6] VT-d Device-TLB flush issue

[PATCH v12 0/6] VT-d Device-TLB flush issue

[Xu, Quan]

From: Quan Xu <quan.xu@intel.com>

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

1. add a timeout parameter for device IOTLB invalidation

    The parameter 'iommu_dev_iotlb_timeout' specifies the timeout
    of device IOTLB invalidation in milliseconds. By default, the
    timeout is 1000 milliseconds, which can be boot-time changed.

    We also confirmed with VT-d hardware team that 1 milliseconds
    is large enough for VT-d IOMMU internal invalidation.

    the existing panic() is eliminated and we bubble up the timeout
    of device IOTLB invalidation for further processing, as the
    PCI-e Address Translation Services (ATS) mandates a timeout of
    60 seconds for device IOTLB invalidation. Obviously we can't
    spin for 60 seconds or otherwise Xen hypervisor hangs.

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

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

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

3. move the domain crash logic up to the generic IOMMU layer

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

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

---
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 (after v12 review, I try to send out
       brain storming).
---
Quan Xu (6):
  IOMMU: add a timeout parameter for device IOTLB invalidation
  vt-d: synchronize for Device-TLB flush one by one
  vt-d: convert conditionals of qi_ctrl->qinval_maddr into ASSERT()s
  IOMMU/x86: using a struct pci_dev* instead of SBDF
  IOMMU: move the domain crash logic up to the generic IOMMU layer
  vt-d: fix vt-d Device-TLB flush timeout issue

 docs/misc/xen-command-line.markdown         |   9 ++
 xen/drivers/passthrough/amd/iommu_cmd.c     |  11 +-
 xen/drivers/passthrough/amd/pci_amd_iommu.c |   2 +-
 xen/drivers/passthrough/ats.h               |   6 +-
 xen/drivers/passthrough/iommu.c             |  33 +++++-
 xen/drivers/passthrough/pci.c               |   6 +-
 xen/drivers/passthrough/vtd/extern.h        |   6 +-
 xen/drivers/passthrough/vtd/iommu.c         |  19 +++-
 xen/drivers/passthrough/vtd/qinval.c        | 168 +++++++++++++++++++---------
 xen/drivers/passthrough/vtd/x86/ats.c       |  23 ++--
 xen/drivers/passthrough/x86/ats.c           |  52 ++++++---
 xen/include/xen/iommu.h                     |   2 +
 xen/include/xen/pci.h                       |   1 +
 13 files changed, 238 insertions(+), 100 deletions(-)

-- 
1.9.1


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

[PATCH v12 1/6] IOMMU: add a timeout parameter for device IOTLB invalidation

[Xu, Quan]

From: Quan Xu <quan.xu@intel.com>

The parameter 'iommu_dev_iotlb_timeout' specifies the timeout
of device IOTLB invalidation in milliseconds. By default, the
timeout is 1000 milliseconds, which can be boot-time changed.

We also confirmed with VT-d hardware team that 1 milliseconds
is large enough for VT-d IOMMU internal invalidation.

the existing panic() is eliminated and we bubble up the timeout
of device IOTLB invalidation for further processing, as the
PCI-e Address Translation Services (ATS) mandates a timeout of
60 seconds for device IOTLB invalidation. Obviously we can't
spin for 60 seconds or otherwise Xen hypervisor hangs.

Add a __must_check annotation. The followup patch titled
'VT-d IOTLB/Context/IEC flush issue' addresses the __mustcheck.
That is the other callers of this routine (two or three levels up)
ignore the return code. This patch does not address this but the
other does.

Signed-off-by: Quan Xu <quan.xu@intel.com>

CC: Julien Grall <julien.grall@arm.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Feng Wu <feng.wu@intel.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
v12:
   1. enhance commit message.
   2. change timeout from 1ms to 1000ms.
   3. change IOMMU_QI_TIMEOUT to VTD_QI_TIMEOUT, with VTD_QI_TIMEOUT
      having its MILLISECS() dropped.
   4. enhance the whole expression of 'timeout = ...'
   5. drop a blank line that doesn't belong here.
---
 docs/misc/xen-command-line.markdown  |  9 +++++++++
 xen/drivers/passthrough/iommu.c      |  3 +++
 xen/drivers/passthrough/vtd/qinval.c | 32 +++++++++++++++++++++-----------
 xen/include/xen/iommu.h              |  2 ++
 4 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 7a271c0..0046f0d 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1015,6 +1015,15 @@ debug hypervisor only).
 
 >> Enable IOMMU debugging code (implies `verbose`).
 
+### iommu\_dev\_iotlb\_timeout
+> `= <integer>`
+
+> Default: `1000`
+
+Specify the timeout of the device IOTLB invalidation in milliseconds.
+By default, the timeout is 1000 ms. When you see error 'Queue invalidate
+wait descriptor timed out', try increasing this value.
+
 ### iommu\_inclusive\_mapping (VT-d)
 > `= <boolean>`
 
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index ef80b3c..7656aeb 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -24,6 +24,9 @@
 static void parse_iommu_param(char *s);
 static void iommu_dump_p2m_table(unsigned char key);
 
+unsigned int __read_mostly iommu_dev_iotlb_timeout = 1000;
+integer_param("iommu_dev_iotlb_timeout", iommu_dev_iotlb_timeout);
+
 /*
  * The 'iommu' parameter enables the IOMMU.  Optional comma separated
  * value may contain:
diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c
index aa7841a..4788d5f 100644
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -28,6 +28,8 @@
 #include "vtd.h"
 #include "extern.h"
 
+#define VTD_QI_TIMEOUT	1
+
 static void print_qi_regs(struct iommu *iommu)
 {
     u64 val;
@@ -130,10 +132,10 @@ static void queue_invalidate_iotlb(struct iommu *iommu,
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 }
 
-static int queue_invalidate_wait(struct iommu *iommu,
-    u8 iflag, u8 sw, u8 fn)
+static int __must_check queue_invalidate_wait(struct iommu *iommu,
+                                              u8 iflag, u8 sw, u8 fn,
+                                              bool_t flush_dev_iotlb)
 {
-    s_time_t start_time;
     volatile u32 poll_slot = QINVAL_STAT_INIT;
     unsigned int index;
     unsigned long flags;
@@ -163,14 +165,20 @@ static int queue_invalidate_wait(struct iommu *iommu,
     /* Now we don't support interrupt method */
     if ( sw )
     {
+        s_time_t timeout;
+
         /* In case all wait descriptor writes to same addr with same data */
-        start_time = NOW();
+        timeout = NOW() + MILLISECS(flush_dev_iotlb ?
+                                    iommu_dev_iotlb_timeout : VTD_QI_TIMEOUT);
+
         while ( poll_slot != QINVAL_STAT_DONE )
         {
-            if ( NOW() > (start_time + DMAR_OPERATION_TIMEOUT) )
+            if ( NOW() > timeout )
             {
                 print_qi_regs(iommu);
-                panic("queue invalidate wait descriptor was not executed");
+                printk(XENLOG_WARNING VTDPREFIX
+                       " Queue invalidate wait descriptor timed out\n");
+                return -ETIMEDOUT;
             }
             cpu_relax();
         }
@@ -180,12 +188,14 @@ static int queue_invalidate_wait(struct iommu *iommu,
     return -EOPNOTSUPP;
 }
 
-static int invalidate_sync(struct iommu *iommu)
+static int __must_check invalidate_sync(struct iommu *iommu,
+                                        bool_t flush_dev_iotlb)
 {
     struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
 
     if ( qi_ctrl->qinval_maddr )
-        return queue_invalidate_wait(iommu, 0, 1, 1);
+        return queue_invalidate_wait(iommu, 0, 1, 1, flush_dev_iotlb);
+
     return 0;
 }
 
@@ -254,7 +264,7 @@ static int __iommu_flush_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx)
     int ret;
 
     queue_invalidate_iec(iommu, granu, im, iidx);
-    ret = invalidate_sync(iommu);
+    ret = invalidate_sync(iommu, 0);
     /*
      * reading vt-d architecture register will ensure
      * draining happens in implementation independent way.
@@ -300,7 +310,7 @@ static int __must_check flush_context_qi(void *_iommu, u16 did,
     {
         queue_invalidate_context(iommu, did, sid, fm,
                                  type >> DMA_CCMD_INVL_GRANU_OFFSET);
-        ret = invalidate_sync(iommu);
+        ret = invalidate_sync(iommu, 0);
     }
     return ret;
 }
@@ -344,7 +354,7 @@ static int __must_check flush_iotlb_qi(void *_iommu, u16 did, u64 addr,
                                dw, did, size_order, 0, addr);
         if ( flush_dev_iotlb )
             ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
-        rc = invalidate_sync(iommu);
+        rc = invalidate_sync(iommu, flush_dev_iotlb);
         if ( !ret )
             ret = rc;
     }
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 8b23cc9..a759f2b 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -35,6 +35,8 @@ extern bool_t iommu_hap_pt_share;
 extern bool_t iommu_debug;
 extern bool_t amd_iommu_perdev_intremap;
 
+extern unsigned int iommu_dev_iotlb_timeout;
+
 #define IOMMU_PAGE_SIZE(sz) (1UL << PAGE_SHIFT_##sz)
 #define IOMMU_PAGE_MASK(sz) (~(u64)0 << PAGE_SHIFT_##sz)
 #define IOMMU_PAGE_ALIGN(sz, addr)  (((addr) + ~PAGE_MASK_##sz) & PAGE_MASK_##sz)
-- 
1.9.1


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

Re: [PATCH v12 1/6] IOMMU: add a timeout parameter for device IOTLB invalidation

[Tian, Kevin]

> From: Xu, Quan
> Sent: Friday, June 24, 2016 1:52 PM
> 
> From: Quan Xu <quan.xu@intel.com>
> 
> The parameter 'iommu_dev_iotlb_timeout' specifies the timeout
> of device IOTLB invalidation in milliseconds. By default, the
> timeout is 1000 milliseconds, which can be boot-time changed.
> 
> We also confirmed with VT-d hardware team that 1 milliseconds
> is large enough for VT-d IOMMU internal invalidation.
> 
> the existing panic() is eliminated and we bubble up the timeout
> of device IOTLB invalidation for further processing, as the
> PCI-e Address Translation Services (ATS) mandates a timeout of
> 60 seconds for device IOTLB invalidation. Obviously we can't
> spin for 60 seconds or otherwise Xen hypervisor hangs.
> 
> Add a __must_check annotation. The followup patch titled
> 'VT-d IOTLB/Context/IEC flush issue' addresses the __mustcheck.
> That is the other callers of this routine (two or three levels up)
> ignore the return code. This patch does not address this but the
> other does.
> 
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> 

Acked-by: Kevin Tian <kevin.tian@intel.com>

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

Re: [PATCH v12 1/6] IOMMU: add a timeout parameter for device IOTLB invalidation

[Jan Beulich]

>>> On 24.06.16 at 07:51, <quan.xu@intel.com> wrote:
> From: Quan Xu <quan.xu@intel.com>
> 
> The parameter 'iommu_dev_iotlb_timeout' specifies the timeout
> of device IOTLB invalidation in milliseconds. By default, the
> timeout is 1000 milliseconds, which can be boot-time changed.
> 
> We also confirmed with VT-d hardware team that 1 milliseconds
> is large enough for VT-d IOMMU internal invalidation.
> 
> the existing panic() is eliminated and we bubble up the timeout
> of device IOTLB invalidation for further processing, as the
> PCI-e Address Translation Services (ATS) mandates a timeout of
> 60 seconds for device IOTLB invalidation. Obviously we can't
> spin for 60 seconds or otherwise Xen hypervisor hangs.
> 
> Add a __must_check annotation. The followup patch titled
> 'VT-d IOTLB/Context/IEC flush issue' addresses the __mustcheck.
> That is the other callers of this routine (two or three levels up)
> ignore the return code. This patch does not address this but the
> other does.

The patch itself looks okay, but I'm confused by this paragraph:
There's no patch with the named title later in this series. And
having gone through this patch I also don't see what remains to
be addressed wrt the __must_check-s getting added here.

Jan


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

Re: [PATCH v12 1/6] IOMMU: add a timeout parameter for device IOTLB invalidation

[Xu, Quan]

On June 27, 2016 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 24.06.16 at 07:51, <quan.xu@intel.com> wrote:
> > From: Quan Xu <quan.xu@intel.com>
> >
> > The parameter 'iommu_dev_iotlb_timeout' specifies the timeout of
> > device IOTLB invalidation in milliseconds. By default, the timeout is
> > 1000 milliseconds, which can be boot-time changed.
> >
> > We also confirmed with VT-d hardware team that 1 milliseconds is large
> > enough for VT-d IOMMU internal invalidation.
> >
> > the existing panic() is eliminated and we bubble up the timeout of
> > device IOTLB invalidation for further processing, as the PCI-e Address
> > Translation Services (ATS) mandates a timeout of
> > 60 seconds for device IOTLB invalidation. Obviously we can't spin for
> > 60 seconds or otherwise Xen hypervisor hangs.
> >
> > Add a __must_check annotation. The followup patch titled 'VT-d
> > IOTLB/Context/IEC flush issue' addresses the __mustcheck.
> > That is the other callers of this routine (two or three levels up)
> > ignore the return code. This patch does not address this but the other
> > does.
> 
> The patch itself looks okay,

Jan, thanks for your review.

> but I'm confused by this paragraph:
> There's no patch with the named title later in this series. And having gone
> through this patch I also don't see what remains to be addressed wrt the
> __must_check-s getting added here.
> 

This paragraph was added from a few rounds ago. I will drop it in next version.

Quan

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

Re: [PATCH v12 1/6] IOMMU: add a timeout parameter for device IOTLB invalidation

[Jan Beulich]

>>> On 27.06.16 at 10:19, <quan.xu@intel.com> wrote:
> On June 27, 2016 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 24.06.16 at 07:51, <quan.xu@intel.com> wrote:
>> > From: Quan Xu <quan.xu@intel.com>
>> >
>> > The parameter 'iommu_dev_iotlb_timeout' specifies the timeout of
>> > device IOTLB invalidation in milliseconds. By default, the timeout is
>> > 1000 milliseconds, which can be boot-time changed.
>> >
>> > We also confirmed with VT-d hardware team that 1 milliseconds is large
>> > enough for VT-d IOMMU internal invalidation.
>> >
>> > the existing panic() is eliminated and we bubble up the timeout of
>> > device IOTLB invalidation for further processing, as the PCI-e Address
>> > Translation Services (ATS) mandates a timeout of
>> > 60 seconds for device IOTLB invalidation. Obviously we can't spin for
>> > 60 seconds or otherwise Xen hypervisor hangs.
>> >
>> > Add a __must_check annotation. The followup patch titled 'VT-d
>> > IOTLB/Context/IEC flush issue' addresses the __mustcheck.
>> > That is the other callers of this routine (two or three levels up)
>> > ignore the return code. This patch does not address this but the other
>> > does.
>> 
>> The patch itself looks okay,
> 
> Jan, thanks for your review.
> 
>> but I'm confused by this paragraph:
>> There's no patch with the named title later in this series. And having gone
>> through this patch I also don't see what remains to be addressed wrt the
>> __must_check-s getting added here.
> 
> This paragraph was added from a few rounds ago. I will drop it in next 
> version.

Well, if dropping this paragraph is all that's needed, I can do this
while committing: Patches 1-3 appear to be ready to go in.

Jan


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

Re: [PATCH v12 1/6] IOMMU: add a timeout parameter for device IOTLB invalidation

[Xu, Quan]

On June 27, 2016 4:29 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 27.06.16 at 10:19, <quan.xu@intel.com> wrote:
> > On June 27, 2016 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 24.06.16 at 07:51, <quan.xu@intel.com> wrote:
> >> > From: Quan Xu <quan.xu@intel.com>
> >> >
> >> > The parameter 'iommu_dev_iotlb_timeout' specifies the timeout of
> >> > device IOTLB invalidation in milliseconds. By default, the timeout
> >> > is
> >> > 1000 milliseconds, which can be boot-time changed.
> >> >
> >> > We also confirmed with VT-d hardware team that 1 milliseconds is
> >> > large enough for VT-d IOMMU internal invalidation.
> >> >
> >> > the existing panic() is eliminated and we bubble up the timeout of
> >> > device IOTLB invalidation for further processing, as the PCI-e
> >> > Address Translation Services (ATS) mandates a timeout of
> >> > 60 seconds for device IOTLB invalidation. Obviously we can't spin
> >> > for
> >> > 60 seconds or otherwise Xen hypervisor hangs.
> >> >
> >> > Add a __must_check annotation. The followup patch titled 'VT-d
> >> > IOTLB/Context/IEC flush issue' addresses the __mustcheck.
> >> > That is the other callers of this routine (two or three levels up)
> >> > ignore the return code. This patch does not address this but the
> >> > other does.
> >>
> >> The patch itself looks okay,
> >
> > Jan, thanks for your review.
> >
> >> but I'm confused by this paragraph:
> >> There's no patch with the named title later in this series. And
> >> having gone through this patch I also don't see what remains to be
> >> addressed wrt the __must_check-s getting added here.
> >
> > This paragraph was added from a few rounds ago. I will drop it in next
> > version.
> 
> Well, if dropping this paragraph is all that's needed, 

Yes,

> I can do this while
> committing: Patches 1-3 appear to be ready to go in.
> 

Ah, that's great. Thanks.

Quan

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

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

[Xu, Quan]

From: Quan Xu <quan.xu@intel.com>

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

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

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

Signed-off-by: Quan Xu <quan.xu@intel.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

CC: Jan Beulich <jbeulich@suse.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Feng Wu <feng.wu@intel.com>
---
 xen/drivers/passthrough/vtd/extern.h  |  5 +--
 xen/drivers/passthrough/vtd/qinval.c  | 65 ++++++++++++++++++++---------------
 xen/drivers/passthrough/vtd/x86/ats.c |  8 ++---
 3 files changed, 45 insertions(+), 33 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h
index 6772839..45357f2 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -59,8 +59,9 @@ int ats_device(const struct pci_dev *, const struct acpi_drhd_unit *);
 int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
                          u64 addr, unsigned int size_order, u64 type);
 
-int qinval_device_iotlb(struct iommu *iommu,
-                        u32 max_invs_pend, u16 sid, u16 size, u64 addr);
+int __must_check qinval_device_iotlb_sync(struct iommu *iommu,
+                                          u32 max_invs_pend,
+                                          u16 sid, u16 size, u64 addr);
 
 unsigned int get_cache_line_size(void);
 void cacheline_flush(char *);
diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c
index 4788d5f..46c4c8f 100644
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -30,6 +30,9 @@
 
 #define VTD_QI_TIMEOUT	1
 
+static int __must_check invalidate_sync(struct iommu *iommu,
+                                        bool_t flush_dev_iotlb);
+
 static void print_qi_regs(struct iommu *iommu)
 {
     u64 val;
@@ -69,8 +72,10 @@ static void qinval_update_qtail(struct iommu *iommu, unsigned int index)
     dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << QINVAL_INDEX_SHIFT));
 }
 
-static void queue_invalidate_context(struct iommu *iommu,
-    u16 did, u16 source_id, u8 function_mask, u8 granu)
+static int __must_check queue_invalidate_context_sync(struct iommu *iommu,
+                                                      u16 did, u16 source_id,
+                                                      u8 function_mask,
+                                                      u8 granu)
 {
     unsigned long flags;
     unsigned int index;
@@ -97,10 +102,14 @@ static void queue_invalidate_context(struct iommu *iommu,
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 
     unmap_vtd_domain_page(qinval_entries);
+
+    return invalidate_sync(iommu, 0);
 }
 
-static void queue_invalidate_iotlb(struct iommu *iommu,
-    u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr)
+static int __must_check queue_invalidate_iotlb_sync(struct iommu *iommu,
+                                                    u8 granu, u8 dr, u8 dw,
+                                                    u16 did, u8 am, u8 ih,
+                                                    u64 addr)
 {
     unsigned long flags;
     unsigned int index;
@@ -130,6 +139,8 @@ static void queue_invalidate_iotlb(struct iommu *iommu,
     unmap_vtd_domain_page(qinval_entries);
     qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
+
+    return invalidate_sync(iommu, 0);
 }
 
 static int __must_check queue_invalidate_wait(struct iommu *iommu,
@@ -199,8 +210,9 @@ static int __must_check invalidate_sync(struct iommu *iommu,
     return 0;
 }
 
-int qinval_device_iotlb(struct iommu *iommu,
-    u32 max_invs_pend, u16 sid, u16 size, u64 addr)
+int qinval_device_iotlb_sync(struct iommu *iommu,
+                             u32 max_invs_pend,
+                             u16 sid, u16 size, u64 addr)
 {
     unsigned long flags;
     unsigned int index;
@@ -229,15 +241,17 @@ int qinval_device_iotlb(struct iommu *iommu,
     qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 
-    return 0;
+    return invalidate_sync(iommu, 1);
 }
 
-static void queue_invalidate_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx)
+static int __must_check queue_invalidate_iec_sync(struct iommu *iommu,
+                                                  u8 granu, u8 im, u16 iidx)
 {
     unsigned long flags;
     unsigned int index;
     u64 entry_base;
     struct qinval_entry *qinval_entry, *qinval_entries;
+    int ret;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);
@@ -257,14 +271,9 @@ static void queue_invalidate_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx)
     unmap_vtd_domain_page(qinval_entries);
     qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
-}
-
-static int __iommu_flush_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx)
-{
-    int ret;
 
-    queue_invalidate_iec(iommu, granu, im, iidx);
     ret = invalidate_sync(iommu, 0);
+
     /*
      * reading vt-d architecture register will ensure
      * draining happens in implementation independent way.
@@ -276,12 +285,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 __must_check flush_context_qi(void *_iommu, u16 did,
@@ -307,11 +316,9 @@ static int __must_check flush_context_qi(void *_iommu, u16 did,
     }
 
     if ( qi_ctrl->qinval_maddr != 0 )
-    {
-        queue_invalidate_context(iommu, did, sid, fm,
-                                 type >> DMA_CCMD_INVL_GRANU_OFFSET);
-        ret = invalidate_sync(iommu, 0);
-    }
+        ret = queue_invalidate_context_sync(iommu, did, sid, fm,
+                                            type >> DMA_CCMD_INVL_GRANU_OFFSET);
+
     return ret;
 }
 
@@ -349,14 +356,18 @@ static int __must_check flush_iotlb_qi(void *_iommu, u16 did, u64 addr,
         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);
-        if ( flush_dev_iotlb )
-            ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
-        rc = invalidate_sync(iommu, flush_dev_iotlb);
+        rc = queue_invalidate_iotlb_sync(iommu,
+                                         type >> DMA_TLB_FLUSH_GRANU_OFFSET,
+                                         dr, dw, did, size_order, 0, addr);
         if ( !ret )
             ret = rc;
+
+        if ( flush_dev_iotlb )
+        {
+            rc = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
+            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..dfa4d30 100644
--- a/xen/drivers/passthrough/vtd/x86/ats.c
+++ b/xen/drivers/passthrough/vtd/x86/ats.c
@@ -134,8 +134,8 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
             /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
             sbit = 1;
             addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;
-            rc = qinval_device_iotlb(iommu, pdev->ats_queue_depth,
-                                     sid, sbit, addr);
+            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
+                                          sid, sbit, addr);
             break;
         case DMA_TLB_PSI_FLUSH:
             if ( !device_in_domain(iommu, pdev, did) )
@@ -154,8 +154,8 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
                 addr |= (((u64)1 << (size_order - 1)) - 1) << PAGE_SHIFT_4K;
             }
 
-            rc = qinval_device_iotlb(iommu, pdev->ats_queue_depth,
-                                     sid, sbit, addr);
+            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
+                                          sid, sbit, addr);
             break;
         default:
             dprintk(XENLOG_WARNING VTDPREFIX, "invalid vt-d flush type\n");
-- 
1.9.1


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

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

[Tian, Kevin]

> From: Xu, Quan
> Sent: Friday, June 24, 2016 1:52 PM
> 
> From: Quan Xu <quan.xu@intel.com>
> 
> Today we do Device-TLB flush synchronization after issuing flush
> requests for all ATS devices belonging to a VM. Doing so however
> imposes a limitation, i.e. that we can not figure out which flush
> request is blocked in the flush queue list, based on VT-d spec.
> 
> To prepare correct Device-TLB flush timeout handling in next patch,
> we change the behavior to synchronize for every Device-TLB flush
> request. So the Device-TLB flush interface is changed a little bit,
> by checking timeout within the function instead of outside of function.
> 
> Accordingly we also do a similar change for flush interfaces of
> IOTLB/IEC/Context, i.e. moving synchronization into the function.
> Since there is no user of a non-synced interface, we just rename
> existing ones with _sync suffix.
> 
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 

Acked-by: Kevin Tian <kevin.tian@intel.com>

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

[PATCH v12 3/6] vt-d: convert conditionals of qi_ctrl->qinval_maddr into ASSERT()s

[Xu, Quan]

From: Quan Xu <quan.xu@intel.com>

QI ought to have got disabled if any of the IOMMU table setup
failed. A QI function (other than enable_qinval) is unreachable
when qi_ctrl->qinval_maddr is zero.

Signed-off-by: Quan Xu <quan.xu@intel.com>

CC: Jan Beulich <jbeulich@suse.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Feng Wu <feng.wu@intel.com>
---
 xen/drivers/passthrough/vtd/qinval.c | 52 ++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 29 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c
index 46c4c8f..4492b29 100644
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -204,10 +204,9 @@ static int __must_check 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, flush_dev_iotlb);
+    ASSERT(qi_ctrl->qinval_maddr);
 
-    return 0;
+    return queue_invalidate_wait(iommu, 0, 1, 1, flush_dev_iotlb);
 }
 
 int qinval_device_iotlb_sync(struct iommu *iommu,
@@ -297,10 +296,11 @@ static int __must_check flush_context_qi(void *_iommu, u16 did,
                                          u16 sid, u8 fm, u64 type,
                                          bool_t flush_non_present_entry)
 {
-    int ret = 0;
     struct iommu *iommu = (struct iommu *)_iommu;
     struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
 
+    ASSERT(qi_ctrl->qinval_maddr);
+
     /*
      * In the non-present entry flush case, if hardware doesn't cache
      * non-present entry we do nothing and if hardware cache non-present
@@ -315,11 +315,8 @@ static int __must_check flush_context_qi(void *_iommu, u16 did,
             did = 0;
     }
 
-    if ( qi_ctrl->qinval_maddr != 0 )
-        ret = queue_invalidate_context_sync(iommu, did, sid, fm,
-                                            type >> DMA_CCMD_INVL_GRANU_OFFSET);
-
-    return ret;
+    return queue_invalidate_context_sync(iommu, did, sid, fm,
+                                         type >> DMA_CCMD_INVL_GRANU_OFFSET);
 }
 
 static int __must_check flush_iotlb_qi(void *_iommu, u16 did, u64 addr,
@@ -328,10 +325,12 @@ static int __must_check flush_iotlb_qi(void *_iommu, u16 did, u64 addr,
                                        bool_t flush_dev_iotlb)
 {
     u8 dr = 0, dw = 0;
-    int ret = 0;
+    int ret = 0, rc;
     struct iommu *iommu = (struct iommu *)_iommu;
     struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
 
+    ASSERT(qi_ctrl->qinval_maddr);
+
     /*
      * In the non-present entry flush case, if hardware doesn't cache
      * non-present entry we do nothing and if hardware cache non-present
@@ -346,28 +345,23 @@ static int __must_check flush_iotlb_qi(void *_iommu, u16 did, u64 addr,
             did = 0;
     }
 
-    if ( qi_ctrl->qinval_maddr != 0 )
+    /* 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 */
+    rc = queue_invalidate_iotlb_sync(iommu,
+                                     type >> DMA_TLB_FLUSH_GRANU_OFFSET,
+                                     dr, dw, did, size_order, 0, addr);
+    if ( !ret )
+        ret = rc;
+
+    if ( flush_dev_iotlb )
     {
-        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 */
-        rc = queue_invalidate_iotlb_sync(iommu,
-                                         type >> DMA_TLB_FLUSH_GRANU_OFFSET,
-                                         dr, dw, did, size_order, 0, addr);
+        rc = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
         if ( !ret )
             ret = rc;
-
-        if ( flush_dev_iotlb )
-        {
-            rc = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
-            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 v12 3/6] vt-d: convert conditionals of qi_ctrl->qinval_maddr into ASSERT()s

[Tian, Kevin]

> From: Xu, Quan
> Sent: Friday, June 24, 2016 1:52 PM
> 
> From: Quan Xu <quan.xu@intel.com>
> 
> QI ought to have got disabled if any of the IOMMU table setup
> failed. A QI function (other than enable_qinval) is unreachable
> when qi_ctrl->qinval_maddr is zero.
> 
> Signed-off-by: Quan Xu <quan.xu@intel.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

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

[PATCH v12 4/6] IOMMU/x86: using a struct pci_dev* instead of SBDF

[Xu, Quan]

From: Quan Xu <quan.xu@intel.com>

a struct pci_dev* instead of SBDF is stored inside struct
pci_ats_dev and parameter to enable_ats_device().

Signed-off-by: Quan Xu <quan.xu@intel.com>

CC: Jan Beulich <jbeulich@suse.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Feng Wu <feng.wu@intel.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/drivers/passthrough/amd/iommu_cmd.c     | 11 +++---
 xen/drivers/passthrough/amd/pci_amd_iommu.c |  2 +-
 xen/drivers/passthrough/ats.h               |  6 ++--
 xen/drivers/passthrough/vtd/iommu.c         |  8 ++---
 xen/drivers/passthrough/vtd/x86/ats.c       | 20 ++++++++---
 xen/drivers/passthrough/x86/ats.c           | 52 +++++++++++++++++++----------
 6 files changed, 62 insertions(+), 37 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c b/xen/drivers/passthrough/amd/iommu_cmd.c
index 7c9d9be..b3094f3 100644
--- a/xen/drivers/passthrough/amd/iommu_cmd.c
+++ b/xen/drivers/passthrough/amd/iommu_cmd.c
@@ -298,24 +298,23 @@ void amd_iommu_flush_iotlb(u8 devfn, const struct pci_dev *pdev,
     if ( ats_pdev == NULL )
         return;
 
-    if ( !pci_ats_enabled(ats_pdev->seg, ats_pdev->bus, ats_pdev->devfn) )
+    if ( !pci_ats_enabled(pdev->seg, pdev->bus, pdev->devfn) )
         return;
 
-    iommu = find_iommu_for_device(ats_pdev->seg,
-                                  PCI_BDF2(ats_pdev->bus, ats_pdev->devfn));
+    iommu = find_iommu_for_device(pdev->seg, PCI_BDF2(pdev->bus, pdev->devfn));
 
     if ( !iommu )
     {
         AMD_IOMMU_DEBUG("%s: Can't find iommu for %04x:%02x:%02x.%u\n",
-                        __func__, ats_pdev->seg, ats_pdev->bus,
-                        PCI_SLOT(ats_pdev->devfn), PCI_FUNC(ats_pdev->devfn));
+                        __func__, pdev->seg, pdev->bus,
+                        PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
         return;
     }
 
     if ( !iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) )
         return;
 
-    req_id = get_dma_requestor_id(iommu->seg, PCI_BDF2(ats_pdev->bus, devfn));
+    req_id = get_dma_requestor_id(iommu->seg, PCI_BDF2(pdev->bus, devfn));
     queueid = req_id;
     maxpend = ats_pdev->ats_queue_depth & 0xff;
 
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 7761241..64ca78e 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -162,7 +162,7 @@ static void amd_iommu_setup_domain_device(
          !pci_ats_enabled(iommu->seg, bus, pdev->devfn) )
     {
         if ( devfn == pdev->devfn )
-            enable_ats_device(iommu->seg, bus, devfn, iommu);
+            enable_ats_device(iommu, pdev);
 
         amd_iommu_flush_iotlb(devfn, pdev, INV_IOMMU_ALL_PAGES_ADDRESS, 0);
     }
diff --git a/xen/drivers/passthrough/ats.h b/xen/drivers/passthrough/ats.h
index 5c91572..1359841 100644
--- a/xen/drivers/passthrough/ats.h
+++ b/xen/drivers/passthrough/ats.h
@@ -19,9 +19,7 @@
 
 struct pci_ats_dev {
     struct list_head list;
-    u16 seg;
-    u8 bus;
-    u8 devfn;
+    struct pci_dev *pci_dev;
     u16 ats_queue_depth;    /* ATS device invalidation queue depth */
     const void *iommu;      /* No common IOMMU struct so use void pointer */
 };
@@ -34,7 +32,7 @@ struct pci_ats_dev {
 extern struct list_head ats_devices;
 extern bool_t ats_enabled;
 
-int enable_ats_device(int seg, int bus, int devfn, const void *iommu);
+int enable_ats_device(const void *iommu, struct pci_dev *pci_dev);
 void disable_ats_device(int seg, int bus, int devfn);
 struct pci_ats_dev *get_ats_device(int seg, int bus, int devfn);
 
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index f010612..1b0a0f0 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1461,8 +1461,8 @@ int domain_context_mapping_one(
     return rc;
 }
 
-static int domain_context_mapping(
-    struct domain *domain, u8 devfn, const struct pci_dev *pdev)
+static int domain_context_mapping(struct domain *domain, u8 devfn,
+                                  struct pci_dev *pdev)
 {
     struct acpi_drhd_unit *drhd;
     int ret = 0;
@@ -1498,7 +1498,7 @@ static int domain_context_mapping(
         ret = domain_context_mapping_one(domain, drhd->iommu, bus, devfn,
                                          pdev);
         if ( !ret && devfn == pdev->devfn && ats_device(pdev, drhd) > 0 )
-            enable_ats_device(seg, bus, devfn, drhd->iommu);
+            enable_ats_device(drhd->iommu, pdev);
 
         break;
 
@@ -1994,7 +1994,7 @@ static int intel_iommu_enable_device(struct pci_dev *pdev)
     if ( ret <= 0 )
         return ret;
 
-    ret = enable_ats_device(pdev->seg, pdev->bus, pdev->devfn, drhd->iommu);
+    ret = enable_ats_device(drhd->iommu, pdev);
 
     return ret >= 0 ? 0 : ret;
 }
diff --git a/xen/drivers/passthrough/vtd/x86/ats.c b/xen/drivers/passthrough/vtd/x86/ats.c
index dfa4d30..a6c53ea 100644
--- a/xen/drivers/passthrough/vtd/x86/ats.c
+++ b/xen/drivers/passthrough/vtd/x86/ats.c
@@ -76,21 +76,25 @@ static int device_in_domain(struct iommu *iommu, struct pci_ats_dev *pdev, u16 d
     struct root_entry *root_entry = NULL;
     struct context_entry *ctxt_entry = NULL;
     int tt, found = 0;
+    struct pci_dev *pci_dev = pdev->pci_dev;
+
+    if ( !pci_dev )
+        return -ENODEV;
 
     root_entry = (struct root_entry *) map_vtd_domain_page(iommu->root_maddr);
-    if ( !root_entry || !root_present(root_entry[pdev->bus]) )
+    if ( !root_entry || !root_present(root_entry[pci_dev->bus]) )
         goto out;
 
     ctxt_entry = (struct context_entry *)
-                 map_vtd_domain_page(root_entry[pdev->bus].val);
+                 map_vtd_domain_page(root_entry[pci_dev->bus].val);
 
     if ( ctxt_entry == NULL )
         goto out;
 
-    if ( context_domain_id(ctxt_entry[pdev->devfn]) != did )
+    if ( context_domain_id(ctxt_entry[pci_dev->devfn]) != did )
         goto out;
 
-    tt = context_translation_type(ctxt_entry[pdev->devfn]);
+    tt = context_translation_type(ctxt_entry[pci_dev->devfn]);
     if ( tt != CONTEXT_TT_DEV_IOTLB )
         goto out;
 
@@ -116,10 +120,16 @@ 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);
+        struct pci_dev *pci_dev = pdev->pci_dev;
+        u16 sid;
         bool_t sbit;
         int rc = 0;
 
+        if ( !pci_dev )
+            continue;
+
+        sid = PCI_BDF2(pci_dev->bus, pci_dev->devfn);
+
         /* Only invalidate devices that belong to this IOMMU */
         if ( pdev->iommu != iommu )
             continue;
diff --git a/xen/drivers/passthrough/x86/ats.c b/xen/drivers/passthrough/x86/ats.c
index 40c9f40..8ce759f 100644
--- a/xen/drivers/passthrough/x86/ats.c
+++ b/xen/drivers/passthrough/x86/ats.c
@@ -22,26 +22,34 @@ LIST_HEAD(ats_devices);
 bool_t __read_mostly ats_enabled = 0;
 boolean_param("ats", ats_enabled);
 
-int enable_ats_device(int seg, int bus, int devfn, const void *iommu)
+int enable_ats_device(const void *iommu, struct pci_dev *pci_dev)
 {
     struct pci_ats_dev *pdev = NULL;
     u32 value;
     int pos;
 
-    pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
+    pos = pci_find_ext_capability(pci_dev->seg, pci_dev->bus, pci_dev->devfn,
+                                  PCI_EXT_CAP_ID_ATS);
     BUG_ON(!pos);
 
     if ( iommu_verbose )
         dprintk(XENLOG_INFO, "%04x:%02x:%02x.%u: ATS capability found\n",
-                seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
+                pci_dev->seg, pci_dev->bus, PCI_SLOT(pci_dev->devfn),
+                PCI_FUNC(pci_dev->devfn));
 
-    value = pci_conf_read16(seg, bus, PCI_SLOT(devfn),
-                            PCI_FUNC(devfn), pos + ATS_REG_CTL);
+    value = pci_conf_read16(pci_dev->seg, pci_dev->bus, PCI_SLOT(pci_dev->devfn),
+                            PCI_FUNC(pci_dev->devfn), pos + ATS_REG_CTL);
     if ( value & ATS_ENABLE )
     {
         list_for_each_entry ( pdev, &ats_devices, list )
         {
-            if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
+            struct pci_dev *pci_dev = pdev->pci_dev;
+
+            if ( !pci_dev )
+                continue;
+            if ( pci_dev->seg == pci_dev->seg &&
+                 pci_dev->bus == pci_dev->bus &&
+                 pci_dev->devfn == pci_dev->devfn )
             {
                 pos = 0;
                 break;
@@ -56,18 +64,16 @@ int enable_ats_device(int seg, int bus, int devfn, const void *iommu)
     if ( !(value & ATS_ENABLE) )
     {
         value |= ATS_ENABLE;
-        pci_conf_write16(seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
-                         pos + ATS_REG_CTL, value);
+        pci_conf_write16(pci_dev->seg, pci_dev->bus, PCI_SLOT(pci_dev->devfn),
+                         PCI_FUNC(pci_dev->devfn), pos + ATS_REG_CTL, value);
     }
 
     if ( pos )
     {
-        pdev->seg = seg;
-        pdev->bus = bus;
-        pdev->devfn = devfn;
+        pdev->pci_dev = pci_dev;
         pdev->iommu = iommu;
-        value = pci_conf_read16(seg, bus, PCI_SLOT(devfn),
-                                PCI_FUNC(devfn), pos + ATS_REG_CAP);
+        value = pci_conf_read16(pci_dev->seg, pci_dev->bus, PCI_SLOT(pci_dev->devfn),
+                                PCI_FUNC(pci_dev->devfn), pos + ATS_REG_CAP);
         pdev->ats_queue_depth = value & ATS_QUEUE_DEPTH_MASK ?:
                                 ATS_QUEUE_DEPTH_MASK + 1;
         list_add(&pdev->list, &ats_devices);
@@ -75,8 +81,8 @@ int enable_ats_device(int seg, int bus, int devfn, const void *iommu)
 
     if ( iommu_verbose )
         dprintk(XENLOG_INFO, "%04x:%02x:%02x.%u: ATS %s enabled\n",
-                seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
-                pos ? "is" : "was");
+                pci_dev->seg, pci_dev->bus, PCI_SLOT(pci_dev->devfn),
+                PCI_FUNC(pci_dev->devfn), pos ? "is" : "was");
 
     return pos;
 }
@@ -98,7 +104,13 @@ void disable_ats_device(int seg, int bus, int devfn)
 
     list_for_each_entry ( pdev, &ats_devices, list )
     {
-        if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
+        struct pci_dev *pci_dev = pdev->pci_dev;
+
+        if ( !pci_dev )
+            continue;
+        if ( pci_dev->seg == seg &&
+             pci_dev->bus == bus &&
+             pci_dev->devfn == devfn )
         {
             list_del(&pdev->list);
             xfree(pdev);
@@ -120,7 +132,13 @@ struct pci_ats_dev *get_ats_device(int seg, int bus, int devfn)
 
     list_for_each_entry ( pdev, &ats_devices, list )
     {
-        if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
+        struct pci_dev *pci_dev = pdev->pci_dev;
+
+        if ( !pci_dev )
+            continue;
+        if ( pci_dev->seg == seg &&
+             pci_dev->bus == bus &&
+             pci_dev->devfn == devfn )
             return pdev;
     }
 
-- 
1.9.1


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

Re: [PATCH v12 4/6] IOMMU/x86: using a struct pci_dev* instead of SBDF

[Tian, Kevin]

> From: Xu, Quan
> Sent: Friday, June 24, 2016 1:52 PM
> 
> From: Quan Xu <quan.xu@intel.com>
> 
> a struct pci_dev* instead of SBDF is stored inside struct
> pci_ats_dev and parameter to enable_ats_device().
> 
> Signed-off-by: Quan Xu <quan.xu@intel.com>

Can we unify the naming convention throughout the patch, e.g. 
always using ats_pdev for "struct pci_ats_dev" variable, while
pdev for "struct pci_dev". It's quite confusing when reading the
patch which has both named as pdev in various places.... I know 
the confusion is also in original code, but please take this chance 
to clean them up. :-)

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

Re: [PATCH v12 4/6] IOMMU/x86: using a struct pci_dev* instead of SBDF

[Xu, Quan]

On June 24, 2016 7:46 PM, Tian, Kevin <kevin.tian@intel.com> wrote:
> > From: Xu, Quan
> > Sent: Friday, June 24, 2016 1:52 PM
> >
> > From: Quan Xu <quan.xu@intel.com>
> >
> > a struct pci_dev* instead of SBDF is stored inside struct pci_ats_dev
> > and parameter to enable_ats_device().
> >
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> 
> Can we unify the naming convention throughout the patch, e.g.
> always using ats_pdev for "struct pci_ats_dev" variable, while pdev for "struct
> pci_dev". It's quite confusing when reading the patch which has both named
> as pdev in various places.... I know the confusion is also in original code, but
> please take this chance to clean them up. :-)

Make sense. I'll fix it in next patch soon.

Quan

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

Re: [PATCH v12 4/6] IOMMU/x86: using a struct pci_dev* instead of SBDF

[Xu, Quan]

On June 24, 2016 7:46 PM, Tian, Kevin <kevin.tian@intel.com> wrote:
> > From: Xu, Quan
> > Sent: Friday, June 24, 2016 1:52 PM
> >
> > From: Quan Xu <quan.xu@intel.com>
> >
> > a struct pci_dev* instead of SBDF is stored inside struct pci_ats_dev
> > and parameter to enable_ats_device().
> >
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> 
> Can we unify the naming convention throughout the patch, e.g.
> always using ats_pdev for "struct pci_ats_dev" variable,

Kevin, Is it 'ats_dev'? -Quan

> while pdev for "struct
> pci_dev". It's quite confusing when reading the patch which has both named
> as pdev in various places.... I know the confusion is also in original code, but
> please take this chance to clean them up. :-)

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

Re: [PATCH v12 4/6] IOMMU/x86: using a struct pci_dev* instead of SBDF

[Tian, Kevin]

> From: Xu, Quan
> Sent: Sunday, June 26, 2016 6:33 PM
> 
> On June 24, 2016 7:46 PM, Tian, Kevin <kevin.tian@intel.com> wrote:
> > > From: Xu, Quan
> > > Sent: Friday, June 24, 2016 1:52 PM
> > >
> > > From: Quan Xu <quan.xu@intel.com>
> > >
> > > a struct pci_dev* instead of SBDF is stored inside struct pci_ats_dev
> > > and parameter to enable_ats_device().
> > >
> > > Signed-off-by: Quan Xu <quan.xu@intel.com>
> >
> > Can we unify the naming convention throughout the patch, e.g.
> > always using ats_pdev for "struct pci_ats_dev" variable,
> 
> Kevin, Is it 'ats_dev'? -Quan
> 

yes.

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

Re: [PATCH v12 4/6] IOMMU/x86: using a struct pci_dev* instead of SBDF

[Jan Beulich]

>>> On 24.06.16 at 07:51, <quan.xu@intel.com> wrote:
> --- a/xen/drivers/passthrough/x86/ats.c
> +++ b/xen/drivers/passthrough/x86/ats.c
> @@ -22,26 +22,34 @@ LIST_HEAD(ats_devices);
>  bool_t __read_mostly ats_enabled = 0;
>  boolean_param("ats", ats_enabled);
>  
> -int enable_ats_device(int seg, int bus, int devfn, const void *iommu)
> +int enable_ats_device(const void *iommu, struct pci_dev *pci_dev)

Is there anything preventing the second parameter to become
a pointer to const too? Afaict that would in turn eliminate the
need for some of the changes further up.

>  {
>      struct pci_ats_dev *pdev = NULL;
>      u32 value;
>      int pos;
>  
> -    pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
> +    pos = pci_find_ext_capability(pci_dev->seg, pci_dev->bus, pci_dev->devfn,
> +                                  PCI_EXT_CAP_ID_ATS);

Please add local variables seg, bus, and devfn, which will greatly
reduce the number of changes you need to do to this function
(and which likely will also produce better code).

> @@ -98,7 +104,13 @@ void disable_ats_device(int seg, int bus, int devfn)

For symmetry reasons this function would also get converted to
taking const struct pci_dev *.

> @@ -120,7 +132,13 @@ struct pci_ats_dev *get_ats_device(int seg, int bus, int devfn)

And this one then probably too.

Jan


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

Re: [PATCH v12 4/6] IOMMU/x86: using a struct pci_dev* instead of SBDF

[Jan Beulich]

>>> On 27.06.16 at 10:17, <JBeulich@suse.com> wrote:
>>>> On 24.06.16 at 07:51, <quan.xu@intel.com> wrote:
>> --- a/xen/drivers/passthrough/x86/ats.c
>> +++ b/xen/drivers/passthrough/x86/ats.c
>> @@ -22,26 +22,34 @@ LIST_HEAD(ats_devices);
>>  bool_t __read_mostly ats_enabled = 0;
>>  boolean_param("ats", ats_enabled);
>>  
>> -int enable_ats_device(int seg, int bus, int devfn, const void *iommu)
>> +int enable_ats_device(const void *iommu, struct pci_dev *pci_dev)
> 
> Is there anything preventing the second parameter to become
> a pointer to const too? Afaict that would in turn eliminate the
> need for some of the changes further up.

I just figured that patch 6 depends on this being non-const.

Jan


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

Re: [PATCH v12 4/6] IOMMU/x86: using a struct pci_dev* instead of SBDF

[Xu, Quan]

On June 27, 2016 4:17 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 24.06.16 at 07:51, <quan.xu@intel.com> wrote:
> > --- a/xen/drivers/passthrough/x86/ats.c
> > +++ b/xen/drivers/passthrough/x86/ats.c
> > @@ -22,26 +22,34 @@ LIST_HEAD(ats_devices);  bool_t __read_mostly
> > ats_enabled = 0;  boolean_param("ats", ats_enabled);
> >
> > -int enable_ats_device(int seg, int bus, int devfn, const void *iommu)
> > +int enable_ats_device(const void *iommu, struct pci_dev *pci_dev)
> 
> Is there anything preventing the second parameter to become a pointer to
> const too? Afaict that would in turn eliminate the need for some of the
> changes further up.
> 

If I make the second parameter to const, then compilation fails:

"""
  ats.c: In function 'enable_ats_device':
ats.c:71:23: error: assignment discards 'const' qualifier from pointer target type [-Werror]
         ats_dev->pdev = pdev;
                       ^
"""

Also I will hide pci_dev as device IOTLB flush error, with changing of pci_dev.
A const 'struct pci_dev *'  in  'struct pci_ats_dev' is not working..  I have not verified this, correct me if  I am wrong.


> >  {
> >      struct pci_ats_dev *pdev = NULL;
> >      u32 value;
> >      int pos;
> >
> > -    pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
> > +    pos = pci_find_ext_capability(pci_dev->seg, pci_dev->bus, pci_dev-
> >devfn,
> > +                                  PCI_EXT_CAP_ID_ATS);
> 
> Please add local variables seg, bus, and devfn, which will greatly reduce the
> number of changes you need to do to this function (and which likely will also
> produce better code).

Agreed. Good idea.

> 
> > @@ -98,7 +104,13 @@ void disable_ats_device(int seg, int bus, int
> > devfn)
> 
> For symmetry reasons this function would also get converted to taking const
> struct pci_dev *.
> 

What about ' struct pci_dev *', without const?

> > @@ -120,7 +132,13 @@ struct pci_ats_dev *get_ats_device(int seg, int
> > bus, int devfn)
> 
> And this one then probably too.
> 

Ditto.

Quan


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

Re: [PATCH v12 4/6] IOMMU/x86: using a struct pci_dev* instead of SBDF

[Jan Beulich]

>>> On 27.06.16 at 13:11, <quan.xu@intel.com> wrote:
> On June 27, 2016 4:17 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 24.06.16 at 07:51, <quan.xu@intel.com> wrote:
>> > @@ -98,7 +104,13 @@ void disable_ats_device(int seg, int bus, int
>> > devfn)
>> 
>> For symmetry reasons this function would also get converted to taking const
>> struct pci_dev *.
>> 
> 
> What about ' struct pci_dev *', without const?

Sure - since the other one apparently can't have the const added,
this one doesn't need to either (but please nevertheless do add it
it that's actually possible without having to cast away constness
somewhere.

Jan


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

Re: [PATCH v12 4/6] IOMMU/x86: using a struct pci_dev* instead of SBDF

[Xu, Quan]

On June 27, 2016 11:19 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 27.06.16 at 13:11, <quan.xu@intel.com> wrote:
> > On June 27, 2016 4:17 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 24.06.16 at 07:51, <quan.xu@intel.com> wrote:
> >> > @@ -98,7 +104,13 @@ void disable_ats_device(int seg, int bus, int
> >> > devfn)
> >>
> >> For symmetry reasons this function would also get converted to taking
> >> const struct pci_dev *.
> >>
> >
> > What about ' struct pci_dev *', without const?
> 
> Sure - since the other one apparently can't have the const added, this one
> doesn't need to either (but please nevertheless do add it it that's actually
> possible without having to cast away constness somewhere.
> 

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

[PATCH v12 5/6] IOMMU: move the domain crash logic up to the generic IOMMU layer

[Xu, Quan]

From: Quan Xu <quan.xu@intel.com>

Signed-off-by: Quan Xu <quan.xu@intel.com>

CC: Julien Grall <julien.grall@arm.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Feng Wu <feng.wu@intel.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/drivers/passthrough/iommu.c     | 30 ++++++++++++++++++++++++++++--
 xen/drivers/passthrough/vtd/iommu.c | 11 +++++++++++
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 7656aeb..d793f5d 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -318,21 +318,47 @@ int iommu_iotlb_flush(struct domain *d, unsigned long gfn,
                       unsigned int page_count)
 {
     const struct domain_iommu *hd = dom_iommu(d);
+    int rc;
 
     if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush )
         return 0;
 
-    return hd->platform_ops->iotlb_flush(d, gfn, page_count);
+    rc = hd->platform_ops->iotlb_flush(d, gfn, page_count);
+    if ( unlikely(rc) )
+    {
+        if ( !d->is_shutting_down && printk_ratelimit() )
+            printk(XENLOG_ERR
+                   "d%d: IOMMU IOTLB flush failed: %d, gfn %#lx, page count %u\n",
+                   d->domain_id, rc, gfn, page_count);
+
+        if ( !is_hardware_domain(d) )
+            domain_crash(d);
+    }
+
+    return rc;
 }
 
 int iommu_iotlb_flush_all(struct domain *d)
 {
     const struct domain_iommu *hd = dom_iommu(d);
+    int rc;
 
     if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush_all )
         return 0;
 
-    return hd->platform_ops->iotlb_flush_all(d);
+    rc = hd->platform_ops->iotlb_flush_all(d);
+    if ( unlikely(rc) )
+    {
+        if ( !d->is_shutting_down && printk_ratelimit() )
+            printk(XENLOG_ERR
+                   "d%d: IOMMU IOTLB flush all failed: %d\n",
+                   d->domain_id, rc);
+
+        if ( !is_hardware_domain(d) )
+            domain_crash(d);
+    }
+
+    return rc;
 }
 
 int __init iommu_setup(void)
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 1b0a0f0..82332c8 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1847,6 +1847,17 @@ int iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte,
         }
     }
 
+    if ( unlikely(rc) )
+    {
+        if ( !d->is_shutting_down && printk_ratelimit() )
+            printk(XENLOG_ERR VTDPREFIX
+                   " d%d: IOMMU pages flush failed: %d\n",
+                   d->domain_id, rc);
+
+        if ( !is_hardware_domain(d) )
+            domain_crash(d);
+    }
+
     return rc;
 }
 
-- 
1.9.1


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

Re: [PATCH v12 5/6] IOMMU: move the domain crash logic up to the generic IOMMU layer

[Tian, Kevin]

> From: Xu, Quan
> Sent: Friday, June 24, 2016 1:52 PM
> 
> From: Quan Xu <quan.xu@intel.com>
> 
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> 
> CC: Julien Grall <julien.grall@arm.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Feng Wu <feng.wu@intel.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  xen/drivers/passthrough/iommu.c     | 30
> ++++++++++++++++++++++++++++--
>  xen/drivers/passthrough/vtd/iommu.c | 11 +++++++++++
>  2 files changed, 39 insertions(+), 2 deletions(-)

when you say "moving the logic up", I don't see any lines being
deleted. Looks you are just "adding the domain crash logic"?

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

Re: [PATCH v12 5/6] IOMMU: move the domain crash logic up to the generic IOMMU layer

[Xu, Quan]

On June 24, 2016 7:48 PM, Tian, Kevin <kevin.tian@intel.com> wrote:
> > From: Xu, Quan
> > Sent: Friday, June 24, 2016 1:52 PM
> >
> > From: Quan Xu <quan.xu@intel.com>
> >
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> >
> > CC: Julien Grall <julien.grall@arm.com>
> > CC: Kevin Tian <kevin.tian@intel.com>
> > CC: Feng Wu <feng.wu@intel.com>
> > CC: Jan Beulich <jbeulich@suse.com>
> > CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> > ---
> >  xen/drivers/passthrough/iommu.c     | 30
> > ++++++++++++++++++++++++++++--
> >  xen/drivers/passthrough/vtd/iommu.c | 11 +++++++++++
> >  2 files changed, 39 insertions(+), 2 deletions(-)
> 
> when you say "moving the logic up", I don't see any lines being deleted. Looks
> you are just "adding the domain crash logic"?

Yes, it is 'adding'..

Quan

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

Re: [PATCH v12 5/6] IOMMU: move the domain crash logic up to the generic IOMMU layer

[Jan Beulich]

>>> On 26.06.16 at 10:58, <quan.xu@intel.com> wrote:
> On June 24, 2016 7:48 PM, Tian, Kevin <kevin.tian@intel.com> wrote:
>> > From: Xu, Quan
>> > Sent: Friday, June 24, 2016 1:52 PM
>> >
>> > From: Quan Xu <quan.xu@intel.com>
>> >
>> > Signed-off-by: Quan Xu <quan.xu@intel.com>
>> >
>> > CC: Julien Grall <julien.grall@arm.com>
>> > CC: Kevin Tian <kevin.tian@intel.com>
>> > CC: Feng Wu <feng.wu@intel.com>
>> > CC: Jan Beulich <jbeulich@suse.com>
>> > CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> > ---
>> >  xen/drivers/passthrough/iommu.c     | 30
>> > ++++++++++++++++++++++++++++--
>> >  xen/drivers/passthrough/vtd/iommu.c | 11 +++++++++++
>> >  2 files changed, 39 insertions(+), 2 deletions(-)
>> 
>> when you say "moving the logic up", I don't see any lines being deleted. Looks
>> you are just "adding the domain crash logic"?
> 
> Yes, it is 'adding'..

But why don't you do here what the title says?

Jan


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

[PATCH v12 6/6] vt-d: fix vt-d Device-TLB flush timeout issue

[Xu, Quan]

From: Quan Xu <quan.xu@intel.com>

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

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

Signed-off-by: Quan Xu <quan.xu@intel.com>

CC: Jan Beulich <jbeulich@suse.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Feng Wu <feng.wu@intel.com>

---
v12:
   1. a forward declaration struct pci_ats_dev*, instead of
      including ats.h.
   2. eliminate the loop.
   3. use the same logic for logging and crashing as I did in
      other series (despite I have moved the domain crash logic
      up to the generic IOMMU layer, I think I am better still
      leave it as is).
   4. enhance dev_invalidate_sync() with ASSERT().
---
 xen/drivers/passthrough/pci.c         |  6 +--
 xen/drivers/passthrough/vtd/extern.h  |  5 ++-
 xen/drivers/passthrough/vtd/qinval.c  | 75 +++++++++++++++++++++++++++++------
 xen/drivers/passthrough/vtd/x86/ats.c |  9 +----
 xen/include/xen/pci.h                 |  1 +
 5 files changed, 71 insertions(+), 25 deletions(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 98936f55c..843dc88 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -419,7 +419,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;
@@ -436,7 +436,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();
@@ -466,7 +466,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 45357f2..efaff28 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -25,6 +25,7 @@
 
 #define VTDPREFIX "[VT-D]"
 
+struct pci_ats_dev;
 extern bool_t rwbf_quirk;
 
 void print_iommu_regs(struct acpi_drhd_unit *drhd);
@@ -60,8 +61,8 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
                          u64 addr, unsigned int size_order, u64 type);
 
 int __must_check qinval_device_iotlb_sync(struct iommu *iommu,
-                                          u32 max_invs_pend,
-                                          u16 sid, u16 size, u64 addr);
+                                          struct pci_ats_dev *ats_dev,
+                                          u16 did, 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 4492b29..e4e2771 100644
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -27,11 +27,11 @@
 #include "dmar.h"
 #include "vtd.h"
 #include "extern.h"
+#include "../ats.h"
 
 #define VTD_QI_TIMEOUT	1
 
-static int __must_check invalidate_sync(struct iommu *iommu,
-                                        bool_t flush_dev_iotlb);
+static int __must_check invalidate_sync(struct iommu *iommu);
 
 static void print_qi_regs(struct iommu *iommu)
 {
@@ -103,7 +103,7 @@ static int __must_check queue_invalidate_context_sync(struct iommu *iommu,
 
     unmap_vtd_domain_page(qinval_entries);
 
-    return invalidate_sync(iommu, 0);
+    return invalidate_sync(iommu);
 }
 
 static int __must_check queue_invalidate_iotlb_sync(struct iommu *iommu,
@@ -140,7 +140,7 @@ static int __must_check queue_invalidate_iotlb_sync(struct iommu *iommu,
     qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 
-    return invalidate_sync(iommu, 0);
+    return invalidate_sync(iommu);
 }
 
 static int __must_check queue_invalidate_wait(struct iommu *iommu,
@@ -199,24 +199,73 @@ static int __must_check queue_invalidate_wait(struct iommu *iommu,
     return -EOPNOTSUPP;
 }
 
-static int __must_check invalidate_sync(struct iommu *iommu,
-                                        bool_t flush_dev_iotlb)
+static int __must_check invalidate_sync(struct iommu *iommu)
 {
     struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
 
     ASSERT(qi_ctrl->qinval_maddr);
 
-    return queue_invalidate_wait(iommu, 0, 1, 1, flush_dev_iotlb);
+    return queue_invalidate_wait(iommu, 0, 1, 1, 0);
+}
+
+static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
+                                         struct pci_dev *pdev)
+{
+    struct domain *d = NULL;
+
+    if ( test_bit(did, iommu->domid_bitmap) )
+        d = rcu_lock_domain_by_id(iommu->domid_map[did]);
+
+    /*
+     * In case the domain has been freed or the IOMMU domid bitmap is
+     * not valid, the device no longer belongs to this domain.
+     */
+    if ( d == NULL )
+        return;
+
+    pcidevs_lock();
+    ASSERT(pdev->domain);
+    list_del(&pdev->domain_list);
+    pdev->domain = NULL;
+    pci_hide_existing_device(pdev);
+    pcidevs_unlock();
+
+    if ( !d->is_shutting_down && printk_ratelimit() )
+        printk(XENLOG_WARNING VTDPREFIX
+               " dom%d: ATS device %04x:%02x:%02x.%u flush failed\n",
+               d->domain_id, pdev->seg, pdev->bus,
+               PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+
+    if ( !is_hardware_domain(d) )
+        domain_crash(d);
+
+    rcu_unlock_domain(d);
+}
+
+static int __must_check dev_invalidate_sync(struct iommu *iommu, u16 did,
+                                            struct pci_dev *pdev)
+{
+    struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
+    int rc;
+
+    ASSERT(qi_ctrl->qinval_maddr);
+    rc = queue_invalidate_wait(iommu, 0, 1, 1, 1);
+
+    if ( rc == -ETIMEDOUT )
+        dev_invalidate_iotlb_timeout(iommu, did, pdev);
+
+    return rc;
 }
 
 int qinval_device_iotlb_sync(struct iommu *iommu,
-                             u32 max_invs_pend,
-                             u16 sid, u16 size, u64 addr)
+                             struct pci_ats_dev *ats_dev,
+                             u16 did, u16 size, u64 addr)
 {
     unsigned long flags;
     unsigned int index;
     u64 entry_base;
     struct qinval_entry *qinval_entry, *qinval_entries;
+    struct pci_dev *pdev = ats_dev->pci_dev;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);
@@ -227,9 +276,9 @@ int qinval_device_iotlb_sync(struct iommu *iommu,
 
     qinval_entry->q.dev_iotlb_inv_dsc.lo.type = TYPE_INVAL_DEVICE_IOTLB;
     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_1 = 0;
-    qinval_entry->q.dev_iotlb_inv_dsc.lo.max_invs_pend = max_invs_pend;
+    qinval_entry->q.dev_iotlb_inv_dsc.lo.max_invs_pend = ats_dev->ats_queue_depth;
     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_2 = 0;
-    qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = sid;
+    qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = PCI_BDF2(pdev->bus, pdev->devfn);
     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_3 = 0;
 
     qinval_entry->q.dev_iotlb_inv_dsc.hi.size = size;
@@ -240,7 +289,7 @@ int qinval_device_iotlb_sync(struct iommu *iommu,
     qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 
-    return invalidate_sync(iommu, 1);
+    return dev_invalidate_sync(iommu, did, pdev);
 }
 
 static int __must_check queue_invalidate_iec_sync(struct iommu *iommu,
@@ -271,7 +320,7 @@ static int __must_check queue_invalidate_iec_sync(struct iommu *iommu,
     qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 
-    ret = invalidate_sync(iommu, 0);
+    ret = invalidate_sync(iommu);
 
     /*
      * reading vt-d architecture register will ensure
diff --git a/xen/drivers/passthrough/vtd/x86/ats.c b/xen/drivers/passthrough/vtd/x86/ats.c
index a6c53ea..be2a155 100644
--- a/xen/drivers/passthrough/vtd/x86/ats.c
+++ b/xen/drivers/passthrough/vtd/x86/ats.c
@@ -121,15 +121,12 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
     list_for_each_entry( pdev, &ats_devices, list )
     {
         struct pci_dev *pci_dev = pdev->pci_dev;
-        u16 sid;
         bool_t sbit;
         int rc = 0;
 
         if ( !pci_dev )
             continue;
 
-        sid = PCI_BDF2(pci_dev->bus, pci_dev->devfn);
-
         /* Only invalidate devices that belong to this IOMMU */
         if ( pdev->iommu != iommu )
             continue;
@@ -144,8 +141,7 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
             /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
             sbit = 1;
             addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;
-            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
-                                          sid, sbit, addr);
+            rc = qinval_device_iotlb_sync(iommu, pdev, did, sbit, addr);
             break;
         case DMA_TLB_PSI_FLUSH:
             if ( !device_in_domain(iommu, pdev, did) )
@@ -164,8 +160,7 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
                 addr |= (((u64)1 << (size_order - 1)) - 1) << PAGE_SHIFT_4K;
             }
 
-            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
-                                          sid, sbit, addr);
+            rc = qinval_device_iotlb_sync(iommu, pdev, did, sbit, addr);
             break;
         default:
             dprintk(XENLOG_WARNING VTDPREFIX, "invalid vt-d flush type\n");
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 6ed29dd..e4940cd 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -118,6 +118,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
 int pci_remove_device(u16 seg, u8 bus, u8 devfn);
 int pci_ro_device(int seg, int bus, int devfn);
 int pci_hide_device(int bus, int devfn);
+void pci_hide_existing_device(struct pci_dev *pdev);
 struct pci_dev *pci_get_pdev(int seg, int bus, int devfn);
 struct pci_dev *pci_get_real_pdev(int seg, int bus, int devfn);
 struct pci_dev *pci_get_pdev_by_domain(
-- 
1.9.1


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

Re: [PATCH v12 6/6] vt-d: fix vt-d Device-TLB flush timeout issue

[Tian, Kevin]

> From: Xu, Quan
> Sent: Friday, June 24, 2016 1:52 PM
> 
> From: Quan Xu <quan.xu@intel.com>
> 
> If Device-TLB flush timed out, we hide the target ATS device
> immediately and crash the domain owning this ATS device. If
> impacted domain is hardware domain, just throw out a warning.
> 
> By hiding the device, we make sure it can't be assigned to any
> domain any longer (see device_assigned).
> 
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> 
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Feng Wu <feng.wu@intel.com>
> 
> ---
> v12:
>    1. a forward declaration struct pci_ats_dev*, instead of
>       including ats.h.
>    2. eliminate the loop.
>    3. use the same logic for logging and crashing as I did in
>       other series (despite I have moved the domain crash logic
>       up to the generic IOMMU layer, I think I am better still
>       leave it as is).
>    4. enhance dev_invalidate_sync() with ASSERT().
> ---
>  xen/drivers/passthrough/pci.c         |  6 +--
>  xen/drivers/passthrough/vtd/extern.h  |  5 ++-
>  xen/drivers/passthrough/vtd/qinval.c  | 75
> +++++++++++++++++++++++++++++------
>  xen/drivers/passthrough/vtd/x86/ats.c |  9 +----
>  xen/include/xen/pci.h                 |  1 +
>  5 files changed, 71 insertions(+), 25 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 98936f55c..843dc88 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -419,7 +419,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)

Don't understand what is the meaning of "hiding existing device".
Is there case where you may want to hide a non-existing device?

>  {
>      if ( pdev->domain )
>          return;
> @@ -436,7 +436,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();
> @@ -466,7 +466,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 45357f2..efaff28 100644
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -25,6 +25,7 @@
> 
>  #define VTDPREFIX "[VT-D]"
> 
> +struct pci_ats_dev;
>  extern bool_t rwbf_quirk;
> 
>  void print_iommu_regs(struct acpi_drhd_unit *drhd);
> @@ -60,8 +61,8 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
>                           u64 addr, unsigned int size_order, u64 type);
> 
>  int __must_check qinval_device_iotlb_sync(struct iommu *iommu,
> -                                          u32 max_invs_pend,
> -                                          u16 sid, u16 size, u64 addr);
> +                                          struct pci_ats_dev *ats_dev,
> +                                          u16 did, 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 4492b29..e4e2771 100644
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -27,11 +27,11 @@
>  #include "dmar.h"
>  #include "vtd.h"
>  #include "extern.h"
> +#include "../ats.h"

Earlier you said:
>    1. a forward declaration struct pci_ats_dev*, instead of
>       including ats.h.

But above you still have ats.h included.

> 
>  #define VTD_QI_TIMEOUT	1
> 
> -static int __must_check invalidate_sync(struct iommu *iommu,
> -                                        bool_t flush_dev_iotlb);
> +static int __must_check invalidate_sync(struct iommu *iommu);

I don't understand the rationale behind. In earlier patch you introduce
a new parameter which is however just removed later here....

Thanks
Kevin


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

Re: [PATCH v12 6/6] vt-d: fix vt-d Device-TLB flush timeout issue

[Jan Beulich]

>>> On 24.06.16 at 13:55, <kevin.tian@intel.com> wrote:
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -419,7 +419,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)
> 
> Don't understand what is the meaning of "hiding existing device".
> Is there case where you may want to hide a non-existing device?

The distinction is whether alloc_pdev() needs calling, as can be seen ...

>> @@ -436,7 +436,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;
>>      }

... as the beginning context of this hunk.

Jan


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

Re: [PATCH v12 6/6] vt-d: fix vt-d Device-TLB flush timeout issue

[Xu, Quan]

On June 24, 2016 7:55 PM, Tian, Kevin <kevin.tian@intel.com> wrote:
> > From: Xu, Quan
> > Sent: Friday, June 24, 2016 1:52 PM
> > diff --git a/xen/drivers/passthrough/vtd/extern.h
> > b/xen/drivers/passthrough/vtd/extern.h
> > index 45357f2..efaff28 100644
> > --- a/xen/drivers/passthrough/vtd/extern.h
> > +++ b/xen/drivers/passthrough/vtd/extern.h
> > @@ -25,6 +25,7 @@
> >
> >  #define VTDPREFIX "[VT-D]"
> >
> > +struct pci_ats_dev;
> >  extern bool_t rwbf_quirk;
> >
> >  void print_iommu_regs(struct acpi_drhd_unit *drhd); @@ -60,8 +61,8 @@
> > int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
> >                           u64 addr, unsigned int size_order, u64
> > type);
> >
> >  int __must_check qinval_device_iotlb_sync(struct iommu *iommu,
> > -                                          u32 max_invs_pend,
> > -                                          u16 sid, u16 size, u64 addr);
> > +                                          struct pci_ats_dev *ats_dev,
> > +                                          u16 did, 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 4492b29..e4e2771 100644
> > --- a/xen/drivers/passthrough/vtd/qinval.c
> > +++ b/xen/drivers/passthrough/vtd/qinval.c
> > @@ -27,11 +27,11 @@
> >  #include "dmar.h"
> >  #include "vtd.h"
> >  #include "extern.h"
> > +#include "../ats.h"
> 
> Earlier you said:
> >    1. a forward declaration struct pci_ats_dev*, instead of
> >       including ats.h.
>

This context is 'in extern.h', but..

> But above you still have ats.h included.
> 

.. I really need to include 'ats.h' here, as the 'struct pci_ats_dev*' is used in this file.

> >
> >  #define VTD_QI_TIMEOUT	1
> >
> > -static int __must_check invalidate_sync(struct iommu *iommu,
> > -                                        bool_t flush_dev_iotlb);
> > +static int __must_check invalidate_sync(struct iommu *iommu);
> 
> I don't understand the rationale behind. In earlier patch you introduce a new
> parameter which is however just removed later here....
> 
In earlier patch, refactor invalidate_sync() to indicate whether we need to flush device IOTLB or not.
change it back here, as I add a specific function - dev_invalidate_sync() for device IOTLB invalidation..

Quan





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

Re: [PATCH v12 6/6] vt-d: fix vt-d Device-TLB flush timeout issue

[Jan Beulich]

>>> On 26.06.16 at 11:18, <quan.xu@intel.com> wrote:
> On June 24, 2016 7:55 PM, Tian, Kevin <kevin.tian@intel.com> wrote:
>> > From: Xu, Quan
>> > Sent: Friday, June 24, 2016 1:52 PM
>> > diff --git a/xen/drivers/passthrough/vtd/extern.h
>> > b/xen/drivers/passthrough/vtd/extern.h
>> > index 45357f2..efaff28 100644
>> > --- a/xen/drivers/passthrough/vtd/extern.h
>> > +++ b/xen/drivers/passthrough/vtd/extern.h
>> > @@ -25,6 +25,7 @@
>> >
>> >  #define VTDPREFIX "[VT-D]"
>> >
>> > +struct pci_ats_dev;
>> >  extern bool_t rwbf_quirk;
>> >
>> >  void print_iommu_regs(struct acpi_drhd_unit *drhd); @@ -60,8 +61,8 @@
>> > int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
>> >                           u64 addr, unsigned int size_order, u64
>> > type);
>> >
>> >  int __must_check qinval_device_iotlb_sync(struct iommu *iommu,
>> > -                                          u32 max_invs_pend,
>> > -                                          u16 sid, u16 size, u64 addr);
>> > +                                          struct pci_ats_dev *ats_dev,
>> > +                                          u16 did, 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 4492b29..e4e2771 100644
>> > --- a/xen/drivers/passthrough/vtd/qinval.c
>> > +++ b/xen/drivers/passthrough/vtd/qinval.c
>> > @@ -27,11 +27,11 @@
>> >  #include "dmar.h"
>> >  #include "vtd.h"
>> >  #include "extern.h"
>> > +#include "../ats.h"
>> 
>> Earlier you said:
>> >    1. a forward declaration struct pci_ats_dev*, instead of
>> >       including ats.h.
>>
> 
> This context is 'in extern.h', but..
> 
>> But above you still have ats.h included.
>> 
> 
> .. I really need to include 'ats.h' here, as the 'struct pci_ats_dev*' is 
> used in this file.

Used as in "a variable of this type, or a pointer to this type
de-referenced", or only as in "a variable/parameter of pointer
to this type declared"? In the latter case you don't need the
include.

Jan


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

Re: [PATCH v12 6/6] vt-d: fix vt-d Device-TLB flush timeout issue

[Jan Beulich]

>>> On 24.06.16 at 07:51, <quan.xu@intel.com> wrote:
> @@ -199,24 +199,73 @@ static int __must_check queue_invalidate_wait(struct iommu *iommu,
>      return -EOPNOTSUPP;
>  }
>  
> -static int __must_check invalidate_sync(struct iommu *iommu,
> -                                        bool_t flush_dev_iotlb)
> +static int __must_check invalidate_sync(struct iommu *iommu)
>  {
>      struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
>  
>      ASSERT(qi_ctrl->qinval_maddr);
>  
> -    return queue_invalidate_wait(iommu, 0, 1, 1, flush_dev_iotlb);
> +    return queue_invalidate_wait(iommu, 0, 1, 1, 0);
> +}
> +
> +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
> +                                         struct pci_dev *pdev)
> +{
> +    struct domain *d = NULL;
> +
> +    if ( test_bit(did, iommu->domid_bitmap) )
> +        d = rcu_lock_domain_by_id(iommu->domid_map[did]);
> +
> +    /*
> +     * In case the domain has been freed or the IOMMU domid bitmap is
> +     * not valid, the device no longer belongs to this domain.
> +     */
> +    if ( d == NULL )
> +        return;
> +
> +    pcidevs_lock();
> +    ASSERT(pdev->domain);
> +    list_del(&pdev->domain_list);
> +    pdev->domain = NULL;
> +    pci_hide_existing_device(pdev);
> +    pcidevs_unlock();
> +
> +    if ( !d->is_shutting_down && printk_ratelimit() )
> +        printk(XENLOG_WARNING VTDPREFIX
> +               " dom%d: ATS device %04x:%02x:%02x.%u flush failed\n",
> +               d->domain_id, pdev->seg, pdev->bus,
> +               PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> +
> +    if ( !is_hardware_domain(d) )
> +        domain_crash(d);
> +
> +    rcu_unlock_domain(d);
> +}

So in an earlier patch in this series you (supposedly) moved similar
logic up to the vendor independent layer. I think this then would
better get moved up too, if at all possible.

Jan


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

Re: [PATCH v12 6/6] vt-d: fix vt-d Device-TLB flush timeout issue

[Xu, Quan]

On June 27, 2016 4:24 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 24.06.16 at 07:51, <quan.xu@intel.com> wrote:
> > @@ -199,24 +199,73 @@ static int __must_check
> queue_invalidate_wait(struct iommu *iommu,
> >      return -EOPNOTSUPP;
> >  }
> >
> > -static int __must_check invalidate_sync(struct iommu *iommu,
> > -                                        bool_t flush_dev_iotlb)
> > +static int __must_check invalidate_sync(struct iommu *iommu)
> >  {
> >      struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
> >
> >      ASSERT(qi_ctrl->qinval_maddr);
> >
> > -    return queue_invalidate_wait(iommu, 0, 1, 1, flush_dev_iotlb);
> > +    return queue_invalidate_wait(iommu, 0, 1, 1, 0); }
> > +
> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
> > +                                         struct pci_dev *pdev) {
> > +    struct domain *d = NULL;
> > +
> > +    if ( test_bit(did, iommu->domid_bitmap) )
> > +        d = rcu_lock_domain_by_id(iommu->domid_map[did]);
> > +
> > +    /*
> > +     * In case the domain has been freed or the IOMMU domid bitmap is
> > +     * not valid, the device no longer belongs to this domain.
> > +     */
> > +    if ( d == NULL )
> > +        return;
> > +
> > +    pcidevs_lock();
> > +    ASSERT(pdev->domain);
> > +    list_del(&pdev->domain_list);
> > +    pdev->domain = NULL;
> > +    pci_hide_existing_device(pdev);
> > +    pcidevs_unlock();
> > +
> > +    if ( !d->is_shutting_down && printk_ratelimit() )
> > +        printk(XENLOG_WARNING VTDPREFIX
> > +               " dom%d: ATS device %04x:%02x:%02x.%u flush failed\n",
> > +               d->domain_id, pdev->seg, pdev->bus,
> > +               PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> > +
> > +    if ( !is_hardware_domain(d) )
> > +        domain_crash(d);
> > +
> > +    rcu_unlock_domain(d);
> > +}
> 
> So in an earlier patch in this series you (supposedly) moved similar logic up to
> the vendor independent layer. I think this then would better get moved up
> too, if at all possible.
> 

To be honest, I have not much reason for leaving domain crash here and I was aware of this problem, but crash_domain() here is not harmful (as the 'd->is_shutting_down' is Set when to crash, and once the 'd->is_shutting_down' is Set then return  in domain_shutdown()  ).
In case crash domain directly, it may help us narrow down the 'window' (the domain is still running)..

To me, moving the logic up is acceptable.

In next version, could I only drop:

+    if ( !is_hardware_domain(d) )
+        domain_crash(d);

In this patch, and leave the rest as is ?

Quan



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

Re: [PATCH v12 6/6] vt-d: fix vt-d Device-TLB flush timeout issue

[Jan Beulich]

>>> On 27.06.16 at 14:56, <quan.xu@intel.com> wrote:
> On June 27, 2016 4:24 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 24.06.16 at 07:51, <quan.xu@intel.com> wrote:
>> > @@ -199,24 +199,73 @@ static int __must_check
>> queue_invalidate_wait(struct iommu *iommu,
>> >      return -EOPNOTSUPP;
>> >  }
>> >
>> > -static int __must_check invalidate_sync(struct iommu *iommu,
>> > -                                        bool_t flush_dev_iotlb)
>> > +static int __must_check invalidate_sync(struct iommu *iommu)
>> >  {
>> >      struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
>> >
>> >      ASSERT(qi_ctrl->qinval_maddr);
>> >
>> > -    return queue_invalidate_wait(iommu, 0, 1, 1, flush_dev_iotlb);
>> > +    return queue_invalidate_wait(iommu, 0, 1, 1, 0); }
>> > +
>> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
>> > +                                         struct pci_dev *pdev) {
>> > +    struct domain *d = NULL;
>> > +
>> > +    if ( test_bit(did, iommu->domid_bitmap) )
>> > +        d = rcu_lock_domain_by_id(iommu->domid_map[did]);
>> > +
>> > +    /*
>> > +     * In case the domain has been freed or the IOMMU domid bitmap is
>> > +     * not valid, the device no longer belongs to this domain.
>> > +     */
>> > +    if ( d == NULL )
>> > +        return;
>> > +
>> > +    pcidevs_lock();
>> > +    ASSERT(pdev->domain);
>> > +    list_del(&pdev->domain_list);
>> > +    pdev->domain = NULL;
>> > +    pci_hide_existing_device(pdev);
>> > +    pcidevs_unlock();
>> > +
>> > +    if ( !d->is_shutting_down && printk_ratelimit() )
>> > +        printk(XENLOG_WARNING VTDPREFIX
>> > +               " dom%d: ATS device %04x:%02x:%02x.%u flush failed\n",
>> > +               d->domain_id, pdev->seg, pdev->bus,
>> > +               PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>> > +
>> > +    if ( !is_hardware_domain(d) )
>> > +        domain_crash(d);
>> > +
>> > +    rcu_unlock_domain(d);
>> > +}
>> 
>> So in an earlier patch in this series you (supposedly) moved similar logic up to
>> the vendor independent layer. I think this then would better get moved up
>> too, if at all possible.
>> 
> 
> To be honest, I have not much reason for leaving domain crash here and I was 
> aware of this problem, but crash_domain() here is not harmful (as the 
> 'd->is_shutting_down' is Set when to crash, and once the 'd->is_shutting_down' 
> is Set then return  in domain_shutdown()  ).
> In case crash domain directly, it may help us narrow down the 'window' (the 
> domain is still running)..
> 
> To me, moving the logic up is acceptable.
> 
> In next version, could I only drop:
> 
> +    if ( !is_hardware_domain(d) )
> +        domain_crash(d);
> 
> In this patch, and leave the rest as is ?

Not really - the entire function looks like it could move out of vtd/,
as I can't see anything VT-d specific in it.

Jan


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

Re: [PATCH v12 6/6] vt-d: fix vt-d Device-TLB flush timeout issue

[Xu, Quan]

On June 27, 2016 11:21 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 27.06.16 at 14:56, <quan.xu@intel.com> wrote:
> > On June 27, 2016 4:24 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 24.06.16 at 07:51, <quan.xu@intel.com> wrote:
> >> > @@ -199,24 +199,73 @@ static int __must_check
> >> queue_invalidate_wait(struct iommu *iommu,
> >> >      return -EOPNOTSUPP;
> >> >  }
> >> >
> >> > -static int __must_check invalidate_sync(struct iommu *iommu,
> >> > -                                        bool_t flush_dev_iotlb)
> >> > +static int __must_check invalidate_sync(struct iommu *iommu)
> >> >  {
> >> >      struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
> >> >
> >> >      ASSERT(qi_ctrl->qinval_maddr);
> >> >
> >> > -    return queue_invalidate_wait(iommu, 0, 1, 1, flush_dev_iotlb);
> >> > +    return queue_invalidate_wait(iommu, 0, 1, 1, 0); }
> >> > +
> >> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
> >> > +                                         struct pci_dev *pdev) {
> >> > +    struct domain *d = NULL;
> >> > +
> >> > +    if ( test_bit(did, iommu->domid_bitmap) )
> >> > +        d = rcu_lock_domain_by_id(iommu->domid_map[did]);
> >> > +
> >> > +    /*
> >> > +     * In case the domain has been freed or the IOMMU domid bitmap is
> >> > +     * not valid, the device no longer belongs to this domain.
> >> > +     */
> >> > +    if ( d == NULL )
> >> > +        return;
> >> > +
> >> > +    pcidevs_lock();
> >> > +    ASSERT(pdev->domain);
> >> > +    list_del(&pdev->domain_list);
> >> > +    pdev->domain = NULL;
> >> > +    pci_hide_existing_device(pdev);
> >> > +    pcidevs_unlock();
> >> > +
> >> > +    if ( !d->is_shutting_down && printk_ratelimit() )
> >> > +        printk(XENLOG_WARNING VTDPREFIX
> >> > +               " dom%d: ATS device %04x:%02x:%02x.%u flush failed\n",
> >> > +               d->domain_id, pdev->seg, pdev->bus,
> >> > +               PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> >> > +
> >> > +    if ( !is_hardware_domain(d) )
> >> > +        domain_crash(d);
> >> > +
> >> > +    rcu_unlock_domain(d);
> >> > +}
> >>
> >> So in an earlier patch in this series you (supposedly) moved similar
> >> logic up to the vendor independent layer. I think this then would
> >> better get moved up too, if at all possible.
> >>
> >
> > To be honest, I have not much reason for leaving domain crash here and
> > I was aware of this problem, but crash_domain() here is not harmful
> > (as the 'd->is_shutting_down' is Set when to crash, and once the 'd-
> >is_shutting_down'
> > is Set then return  in domain_shutdown()  ).
> > In case crash domain directly, it may help us narrow down the 'window'
> > (the domain is still running)..
> >
> > To me, moving the logic up is acceptable.
> >
> > In next version, could I only drop:
> >
> > +    if ( !is_hardware_domain(d) )
> > +        domain_crash(d);
> >
> > In this patch, and leave the rest as is ?
> 
> Not really - the entire function looks like it could move out of vtd/, as I can't
> see anything VT-d specific in it.
> 

Yes, it could be out of vtd, and then benefit arm/amd  IOMMU to hide ATS device.

But 'did'  and 'iommu->domid_bitmap' are really vtd specific. Both of them are to get domain* structure, not a big deal, and then I can use domain_id instead.

IMO, the domain* structure is a must here,
As mentioned, not all of call trees of device iotlb flush are under pcidevs_lock, (.i.e  ...--iommu_iotlb_flush()-- xenmem_add_to_physmap()... )
In extreme cases, the domain may has been freed or the device may has been detached or even attached to another domain ( I also need to add 'if (pdev->domain == d )' before to hide device).
the domain* structure can help us check above cases.

Quan

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

Re: [PATCH v12 6/6] vt-d: fix vt-d Device-TLB flush timeout issue

[Jan Beulich]

>>> On 28.06.16 at 09:06, <quan.xu@intel.com> wrote:
> On June 27, 2016 11:21 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 27.06.16 at 14:56, <quan.xu@intel.com> wrote:
>> > On June 27, 2016 4:24 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >> >>> On 24.06.16 at 07:51, <quan.xu@intel.com> wrote:
>> >> > @@ -199,24 +199,73 @@ static int __must_check
>> >> queue_invalidate_wait(struct iommu *iommu,
>> >> >      return -EOPNOTSUPP;
>> >> >  }
>> >> >
>> >> > -static int __must_check invalidate_sync(struct iommu *iommu,
>> >> > -                                        bool_t flush_dev_iotlb)
>> >> > +static int __must_check invalidate_sync(struct iommu *iommu)
>> >> >  {
>> >> >      struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
>> >> >
>> >> >      ASSERT(qi_ctrl->qinval_maddr);
>> >> >
>> >> > -    return queue_invalidate_wait(iommu, 0, 1, 1, flush_dev_iotlb);
>> >> > +    return queue_invalidate_wait(iommu, 0, 1, 1, 0); }
>> >> > +
>> >> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
>> >> > +                                         struct pci_dev *pdev) {
>> >> > +    struct domain *d = NULL;
>> >> > +
>> >> > +    if ( test_bit(did, iommu->domid_bitmap) )
>> >> > +        d = rcu_lock_domain_by_id(iommu->domid_map[did]);
>> >> > +
>> >> > +    /*
>> >> > +     * In case the domain has been freed or the IOMMU domid bitmap is
>> >> > +     * not valid, the device no longer belongs to this domain.
>> >> > +     */
>> >> > +    if ( d == NULL )
>> >> > +        return;
>> >> > +
>> >> > +    pcidevs_lock();
>> >> > +    ASSERT(pdev->domain);
>> >> > +    list_del(&pdev->domain_list);
>> >> > +    pdev->domain = NULL;
>> >> > +    pci_hide_existing_device(pdev);
>> >> > +    pcidevs_unlock();
>> >> > +
>> >> > +    if ( !d->is_shutting_down && printk_ratelimit() )
>> >> > +        printk(XENLOG_WARNING VTDPREFIX
>> >> > +               " dom%d: ATS device %04x:%02x:%02x.%u flush failed\n",
>> >> > +               d->domain_id, pdev->seg, pdev->bus,
>> >> > +               PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>> >> > +
>> >> > +    if ( !is_hardware_domain(d) )
>> >> > +        domain_crash(d);
>> >> > +
>> >> > +    rcu_unlock_domain(d);
>> >> > +}
>> >>
>> >> So in an earlier patch in this series you (supposedly) moved similar
>> >> logic up to the vendor independent layer. I think this then would
>> >> better get moved up too, if at all possible.
>> >>
>> >
>> > To be honest, I have not much reason for leaving domain crash here and
>> > I was aware of this problem, but crash_domain() here is not harmful
>> > (as the 'd->is_shutting_down' is Set when to crash, and once the 'd-
>> >is_shutting_down'
>> > is Set then return  in domain_shutdown()  ).
>> > In case crash domain directly, it may help us narrow down the 'window'
>> > (the domain is still running)..
>> >
>> > To me, moving the logic up is acceptable.
>> >
>> > In next version, could I only drop:
>> >
>> > +    if ( !is_hardware_domain(d) )
>> > +        domain_crash(d);
>> >
>> > In this patch, and leave the rest as is ?
>> 
>> Not really - the entire function looks like it could move out of vtd/, as I can't
>> see anything VT-d specific in it.
>> 
> 
> Yes, it could be out of vtd, and then benefit arm/amd  IOMMU to hide ATS 
> device.
> 
> But 'did'  and 'iommu->domid_bitmap' are really vtd specific. Both of them are 
> to get domain* structure, not a big deal, and then I can use domain_id 
> instead.
> 
> IMO, the domain* structure is a must here,

I agree, I did overlook that domain lookup. The common code
function should be passed a struct domain *, as you suggest.

Jan


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