* [PATCH v7 0/2] VT-d Device-TLB flush issue
@ 2016-03-17 7:12 Quan Xu
2016-03-17 7:12 ` [PATCH v7 1/2] VT-d: Reduce spin timeout to 1ms, which can be boot-time changed Quan Xu
2016-03-17 7:12 ` [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue Quan Xu
0 siblings, 2 replies; 22+ messages in thread
From: Quan Xu @ 2016-03-17 7:12 UTC (permalink / raw)
To: xen-devel; +Cc: Quan Xu, kevin.tian, feng.wu, dario.faggioli, jbeulich
1. Reduce spin timeout to 1ms, which can be boot-time changed with 'vtd_qi_timeout'.
For example:
multiboot /boot/xen.gz ... vtd_qi_timeout=100 ...
2. Fix vt-d Device-TLB flush timeout issue.
If Device-TLB flush timed out, we would hide the target ATS
device and crash the domain owning this ATS device. If impacted
domain is hardware domain, just throw out a warning.
The hidden device should be disallowed to be further assigned
to any domain.
**NOTE**
This patch set should base on 2 prereq patch sets:
a). Make the pcidevs_lock a recursive one.
b). Check VT-d Device-TLB flush error.
----
Not covered in this series:
a) Eliminate the panic() in IOMMU_WAIT_OP, used only in VT-d register read/write.
Further discussion is required on whether and how to improve it.
b) Handle IOTLB/Context/IEC flush timeout.
--Changes in v6:
#patch 1
* Add an entry in docs/misc/xen-command-line.markdown _alphabetically_.
* Add a __must_check annotation on the function queue_invalidate_wait().
#patch 2
* Add Stray blanks inside the parentheses.
* Don't iterate over pdev-s without holding that lock, and hold pcidevs_lock for pdev-s list.
* Print SBDF in canonical ssss:bb:dd.f form.
* Handle 'ret'/'rc' variables in the same function, and remove the pointless rc.
--Changes in v7:
#patch 1
* Enhance printout infomation.
* Enhance time unit conversion.
#patch 2
*Enhance changelog.
Quan Xu (2):
VT-d: Reduce spin timeout to 1ms, which can be boot-time changed
VT-d: Fix vt-d Device-TLB flush timeout issue
docs/misc/xen-command-line.markdown | 7 +++
xen/drivers/passthrough/pci.c | 6 +--
xen/drivers/passthrough/vtd/extern.h | 2 +
xen/drivers/passthrough/vtd/qinval.c | 84 +++++++++++++++++++++++++++++++----
xen/drivers/passthrough/vtd/x86/ats.c | 12 +++++
xen/include/xen/pci.h | 1 +
6 files changed, 100 insertions(+), 12 deletions(-)
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH v7 1/2] VT-d: Reduce spin timeout to 1ms, which can be boot-time changed 2016-03-17 7:12 [PATCH v7 0/2] VT-d Device-TLB flush issue Quan Xu @ 2016-03-17 7:12 ` Quan Xu 2016-03-17 7:45 ` Tian, Kevin 2016-03-17 7:12 ` [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue Quan Xu 1 sibling, 1 reply; 22+ messages in thread From: Quan Xu @ 2016-03-17 7:12 UTC (permalink / raw) To: xen-devel; +Cc: Quan Xu, kevin.tian, feng.wu, dario.faggioli, jbeulich Signed-off-by: Quan Xu <quan.xu@intel.com> --- docs/misc/xen-command-line.markdown | 7 +++++++ xen/drivers/passthrough/vtd/qinval.c | 19 +++++++++++++++---- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown index ca77e3b..384dde7 100644 --- a/docs/misc/xen-command-line.markdown +++ b/docs/misc/xen-command-line.markdown @@ -1532,6 +1532,13 @@ Note that if **watchdog** option is also specified vpmu will be turned off. As the virtualisation is not 100% safe, don't use the vpmu flag on production systems (see http://xenbits.xen.org/xsa/advisory-163.html)! +### vtd\_qi\_timeout (VT-d) +> `= <integer>` + +> Default: `1` + +Specify the timeout of the VT-d Queued Invalidation in milliseconds. + ### watchdog > `= force | <boolean>` diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c index b81b0bd..37a15fb 100644 --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -28,6 +28,11 @@ #include "vtd.h" #include "extern.h" +static unsigned int __read_mostly vtd_qi_timeout = 1; +integer_param("vtd_qi_timeout", vtd_qi_timeout); + +#define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1)) + static void print_qi_regs(struct iommu *iommu) { u64 val; @@ -130,10 +135,14 @@ static void queue_invalidate_iotlb(struct iommu *iommu, spin_unlock_irqrestore(&iommu->register_lock, flags); } +/* + * NB. We must check all kinds of error and all the way up the + * call trees. + */ static int queue_invalidate_wait(struct iommu *iommu, u8 iflag, u8 sw, u8 fn) { - s_time_t start_time; + s_time_t timeout; volatile u32 poll_slot = QINVAL_STAT_INIT; unsigned int index; unsigned long flags; @@ -164,13 +173,15 @@ static int queue_invalidate_wait(struct iommu *iommu, if ( sw ) { /* In case all wait descriptor writes to same addr with same data */ - start_time = NOW(); + timeout = NOW() + IOMMU_QI_TIMEOUT; while ( poll_slot != QINVAL_STAT_DONE ) { - if ( NOW() > (start_time + DMAR_OPERATION_TIMEOUT) ) + if ( NOW() > timeout ) { print_qi_regs(iommu); - panic("queue invalidate wait descriptor was not executed"); + printk(XENLOG_WARNING VTDPREFIX + "Queue invalidate wait descriptor timed out.\n"); + return -ETIMEDOUT; } cpu_relax(); } -- 1.9.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v7 1/2] VT-d: Reduce spin timeout to 1ms, which can be boot-time changed 2016-03-17 7:12 ` [PATCH v7 1/2] VT-d: Reduce spin timeout to 1ms, which can be boot-time changed Quan Xu @ 2016-03-17 7:45 ` Tian, Kevin 2016-03-17 8:11 ` Xu, Quan 0 siblings, 1 reply; 22+ messages in thread From: Tian, Kevin @ 2016-03-17 7:45 UTC (permalink / raw) To: Xu, Quan, xen-devel; +Cc: dario.faggioli, Wu, Feng, jbeulich > From: Xu, Quan > Sent: Thursday, March 17, 2016 3:13 PM > diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c > index b81b0bd..37a15fb 100644 > --- a/xen/drivers/passthrough/vtd/qinval.c > +++ b/xen/drivers/passthrough/vtd/qinval.c > @@ -28,6 +28,11 @@ > #include "vtd.h" > #include "extern.h" > > +static unsigned int __read_mostly vtd_qi_timeout = 1; > +integer_param("vtd_qi_timeout", vtd_qi_timeout); > + > +#define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1)) > + > static void print_qi_regs(struct iommu *iommu) > { > u64 val; > @@ -130,10 +135,14 @@ static void queue_invalidate_iotlb(struct iommu *iommu, > spin_unlock_irqrestore(&iommu->register_lock, flags); > } > > +/* > + * NB. We must check all kinds of error and all the way up the > + * call trees. > + */ This comment is meaningless put in the code. It just reflects what you need do to push the whole patch series, but it's obvious a straightforward requirement. > static int queue_invalidate_wait(struct iommu *iommu, > u8 iflag, u8 sw, u8 fn) > { Thanks Kevin _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 1/2] VT-d: Reduce spin timeout to 1ms, which can be boot-time changed 2016-03-17 7:45 ` Tian, Kevin @ 2016-03-17 8:11 ` Xu, Quan 2016-03-17 8:13 ` Jan Beulich 0 siblings, 1 reply; 22+ messages in thread From: Xu, Quan @ 2016-03-17 8:11 UTC (permalink / raw) To: Tian, Kevin, xen-devel, jbeulich; +Cc: dario.faggioli, Wu, Feng On March 17, 2016 3:45pm, Tian, Kevin <kevin.tian@intel.com> wrote: > > From: Xu, Quan > > Sent: Thursday, March 17, 2016 3:13 PM diff --git > > a/xen/drivers/passthrough/vtd/qinval.c > > b/xen/drivers/passthrough/vtd/qinval.c > > index b81b0bd..37a15fb 100644 > > --- a/xen/drivers/passthrough/vtd/qinval.c > > +++ b/xen/drivers/passthrough/vtd/qinval.c > > @@ -28,6 +28,11 @@ > > #include "vtd.h" > > #include "extern.h" > > > > +static unsigned int __read_mostly vtd_qi_timeout = 1; > > +integer_param("vtd_qi_timeout", vtd_qi_timeout); > > + > > +#define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1)) > > + > > static void print_qi_regs(struct iommu *iommu) { > > u64 val; > > @@ -130,10 +135,14 @@ static void queue_invalidate_iotlb(struct iommu > *iommu, > > spin_unlock_irqrestore(&iommu->register_lock, flags); } > > > > +/* > > + * NB. We must check all kinds of error and all the way up the > > + * call trees. > > + */ > > This comment is meaningless put in the code. It just reflects what you need do > to push the whole patch series, but it's obvious a straightforward requirement. > Kevin, This is for Jan requirement: http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg02388.html Jan said: """ Without a __must_check annotation on the function I cannot see how I should reasonably convince myself that all call sites now handle such an error in one way or another.""" Now I think I misunderstood what Jan said. It may be: + static int __must_check queue_invalidate_wait(struct iommu *iommu, u8 iflag, u8 sw, u8 fn) I found that there is a '__must_check' in xen code. Quan > > static int queue_invalidate_wait(struct iommu *iommu, > > u8 iflag, u8 sw, u8 fn) > > { _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 1/2] VT-d: Reduce spin timeout to 1ms, which can be boot-time changed 2016-03-17 8:11 ` Xu, Quan @ 2016-03-17 8:13 ` Jan Beulich 2016-03-17 8:17 ` Xu, Quan 0 siblings, 1 reply; 22+ messages in thread From: Jan Beulich @ 2016-03-17 8:13 UTC (permalink / raw) To: Quan Xu; +Cc: dario.faggioli, Feng Wu, Kevin Tian, xen-devel >>> On 17.03.16 at 09:11, <quan.xu@intel.com> wrote: > On March 17, 2016 3:45pm, Tian, Kevin <kevin.tian@intel.com> wrote: >> > From: Xu, Quan >> > Sent: Thursday, March 17, 2016 3:13 PM diff --git >> > a/xen/drivers/passthrough/vtd/qinval.c >> > b/xen/drivers/passthrough/vtd/qinval.c >> > index b81b0bd..37a15fb 100644 >> > --- a/xen/drivers/passthrough/vtd/qinval.c >> > +++ b/xen/drivers/passthrough/vtd/qinval.c >> > @@ -28,6 +28,11 @@ >> > #include "vtd.h" >> > #include "extern.h" >> > >> > +static unsigned int __read_mostly vtd_qi_timeout = 1; >> > +integer_param("vtd_qi_timeout", vtd_qi_timeout); >> > + >> > +#define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1)) >> > + >> > static void print_qi_regs(struct iommu *iommu) { >> > u64 val; >> > @@ -130,10 +135,14 @@ static void queue_invalidate_iotlb(struct iommu >> *iommu, >> > spin_unlock_irqrestore(&iommu->register_lock, flags); } >> > >> > +/* >> > + * NB. We must check all kinds of error and all the way up the >> > + * call trees. >> > + */ >> >> This comment is meaningless put in the code. It just reflects what you need > do >> to push the whole patch series, but it's obvious a straightforward > requirement. >> > Kevin, > This is for Jan requirement: > > http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg02388.html > Jan said: > """ Without a __must_check annotation on the function I cannot see > how I should reasonably convince myself that all call sites now > handle such an error in one way or another.""" > > Now I think I misunderstood what Jan said. It may be: > > + static int __must_check queue_invalidate_wait(struct iommu *iommu, u8 > iflag, u8 sw, u8 fn) > > I found that there is a '__must_check' in xen code. Indeed that's what I meant. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 1/2] VT-d: Reduce spin timeout to 1ms, which can be boot-time changed 2016-03-17 8:13 ` Jan Beulich @ 2016-03-17 8:17 ` Xu, Quan 0 siblings, 0 replies; 22+ messages in thread From: Xu, Quan @ 2016-03-17 8:17 UTC (permalink / raw) To: Jan Beulich; +Cc: dario.faggioli, Wu, Feng, Tian, Kevin, xen-devel On March 17, 2016 4:14pm, <JBeulich@suse.com> wrote: > >>> On 17.03.16 at 09:11, <quan.xu@intel.com> wrote: > > On March 17, 2016 3:45pm, Tian, Kevin <kevin.tian@intel.com> wrote: > >> > From: Xu, Quan > >> > Sent: Thursday, March 17, 2016 3:13 PM diff --git > >> > a/xen/drivers/passthrough/vtd/qinval.c > >> > b/xen/drivers/passthrough/vtd/qinval.c > >> > index b81b0bd..37a15fb 100644 > >> > --- a/xen/drivers/passthrough/vtd/qinval.c > >> > +++ b/xen/drivers/passthrough/vtd/qinval.c > >> > @@ -28,6 +28,11 @@ > >> > #include "vtd.h" > >> > #include "extern.h" > >> > > >> > +static unsigned int __read_mostly vtd_qi_timeout = 1; > >> > +integer_param("vtd_qi_timeout", vtd_qi_timeout); > >> > + > >> > +#define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1)) > >> > + > >> > static void print_qi_regs(struct iommu *iommu) { > >> > u64 val; > >> > @@ -130,10 +135,14 @@ static void queue_invalidate_iotlb(struct > >> > iommu > >> *iommu, > >> > spin_unlock_irqrestore(&iommu->register_lock, flags); } > >> > > >> > +/* > >> > + * NB. We must check all kinds of error and all the way up the > >> > + * call trees. > >> > + */ > >> > >> This comment is meaningless put in the code. It just reflects what > >> you need > > do > >> to push the whole patch series, but it's obvious a straightforward > > requirement. > >> > > Kevin, > > This is for Jan requirement: > > > > http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg02388.h > > tml > > Jan said: > > """ Without a __must_check annotation on the function I cannot see how > > I should reasonably convince myself that all call sites now handle > > such an error in one way or another.""" > > > > Now I think I misunderstood what Jan said. It may be: > > > > + static int __must_check queue_invalidate_wait(struct iommu *iommu, > > + u8 > > iflag, u8 sw, u8 fn) > > > > I found that there is a '__must_check' in xen code. > > Indeed that's what I meant. > Sorry:(.. I will fix it next v8. Quan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue 2016-03-17 7:12 [PATCH v7 0/2] VT-d Device-TLB flush issue Quan Xu 2016-03-17 7:12 ` [PATCH v7 1/2] VT-d: Reduce spin timeout to 1ms, which can be boot-time changed Quan Xu @ 2016-03-17 7:12 ` Quan Xu 2016-03-17 8:17 ` Tian, Kevin 2016-03-18 11:18 ` Jan Beulich 1 sibling, 2 replies; 22+ messages in thread From: Quan Xu @ 2016-03-17 7:12 UTC (permalink / raw) To: xen-devel; +Cc: Quan Xu, kevin.tian, feng.wu, dario.faggioli, jbeulich If Device-TLB flush timed out, we would hide the target ATS device and crash the domain owning this ATS device. If impacted domain is hardware domain, just throw out a warning. The hidden device should be disallowed to be further assigned to any domain. Signed-off-by: Quan Xu <quan.xu@intel.com> --- xen/drivers/passthrough/pci.c | 6 ++-- xen/drivers/passthrough/vtd/extern.h | 2 ++ xen/drivers/passthrough/vtd/qinval.c | 65 ++++++++++++++++++++++++++++++++--- xen/drivers/passthrough/vtd/x86/ats.c | 12 +++++++ xen/include/xen/pci.h | 1 + 5 files changed, 78 insertions(+), 8 deletions(-) diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 9f1716a..9a214c6 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -420,7 +420,7 @@ static void free_pdev(struct pci_seg *pseg, struct pci_dev *pdev) xfree(pdev); } -static void _pci_hide_device(struct pci_dev *pdev) +void pci_hide_existing_device(struct pci_dev *pdev) { if ( pdev->domain ) return; @@ -437,7 +437,7 @@ int __init pci_hide_device(int bus, int devfn) pdev = alloc_pdev(get_pseg(0), bus, devfn); if ( pdev ) { - _pci_hide_device(pdev); + pci_hide_existing_device(pdev); rc = 0; } pcidevs_unlock(); @@ -467,7 +467,7 @@ int __init pci_ro_device(int seg, int bus, int devfn) } __set_bit(PCI_BDF2(bus, devfn), pseg->ro_map); - _pci_hide_device(pdev); + pci_hide_existing_device(pdev); return 0; } diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h index d4d37c3..5b8673e 100644 --- a/xen/drivers/passthrough/vtd/extern.h +++ b/xen/drivers/passthrough/vtd/extern.h @@ -58,6 +58,8 @@ 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 dev_invalidate_iotlb_sync(struct iommu *iommu, u16 did, + u16 seg, u8 bus, u8 devfn); int qinval_device_iotlb(struct iommu *iommu, u32 max_invs_pend, u16 sid, u16 size, u64 addr); diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c index 37a15fb..2a5c638 100644 --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -233,6 +233,57 @@ int qinval_device_iotlb(struct iommu *iommu, return 0; } +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did, + u16 seg, u8 bus, u8 devfn) +{ + struct domain *d = NULL; + struct pci_dev *pdev; + + if ( test_bit(did, iommu->domid_bitmap) ) + d = rcu_lock_domain_by_id(iommu->domid_map[did]); + + if ( d == NULL ) + return; + + pcidevs_lock(); + for_each_pdev(d, pdev) + { + if ( ( pdev->seg == seg ) && + ( pdev->bus == bus ) && + ( pdev->devfn == devfn ) ) + { + ASSERT ( pdev->domain ); + list_del(&pdev->domain_list); + pdev->domain = NULL; + pci_hide_existing_device(pdev); + break; + } + } + + pcidevs_unlock(); + + if ( !is_hardware_domain(d) ) + domain_crash(d); + + rcu_unlock_domain(d); +} + +int dev_invalidate_iotlb_sync(struct iommu *iommu, u16 did, + u16 seg, u8 bus, u8 devfn) +{ + struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu); + int rc = 0; + + if ( qi_ctrl->qinval_maddr ) + { + rc = queue_invalidate_wait(iommu, 0, 1, 1); + if ( rc == -ETIMEDOUT ) + dev_invalidate_iotlb_timeout(iommu, did, seg, bus, devfn); + } + + return rc; +} + static void queue_invalidate_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx) { unsigned long flags; @@ -342,8 +393,6 @@ static int flush_iotlb_qi( if ( qi_ctrl->qinval_maddr != 0 ) { - int rc; - /* use queued invalidation */ if (cap_write_drain(iommu->cap)) dw = 1; @@ -353,11 +402,17 @@ static int flush_iotlb_qi( queue_invalidate_iotlb(iommu, type >> DMA_TLB_FLUSH_GRANU_OFFSET, dr, dw, did, size_order, 0, addr); + + /* + * Before Device-TLB invalidation we need to synchronize + * invalidation completions with hardware. + */ + ret = invalidate_sync(iommu); + if ( ret ) + return ret; + if ( flush_dev_iotlb ) ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type); - rc = invalidate_sync(iommu); - if ( !ret ) - ret = rc; } return ret; } diff --git a/xen/drivers/passthrough/vtd/x86/ats.c b/xen/drivers/passthrough/vtd/x86/ats.c index 334b9c1..c87ffe3 100644 --- a/xen/drivers/passthrough/vtd/x86/ats.c +++ b/xen/drivers/passthrough/vtd/x86/ats.c @@ -162,6 +162,18 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did, return -EOPNOTSUPP; } + /* + * Synchronize with hardware for Device-TLB invalidate + * descriptor. + */ + rc = dev_invalidate_iotlb_sync(iommu, did, pdev->seg, + pdev->bus, pdev->devfn); + if ( rc ) + printk(XENLOG_ERR + "Flush error %d on device %04x:%02x:%02x.%u.\n", + ret, pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), + PCI_FUNC(pdev->devfn)); + if ( !ret ) ret = rc; } diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index 6ed29dd..bb9f791 100644 --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -116,6 +116,7 @@ const unsigned long *pci_get_ro_map(u16 seg); int pci_add_device(u16 seg, u8 bus, u8 devfn, const struct pci_dev_info *, nodeid_t node); int pci_remove_device(u16 seg, u8 bus, u8 devfn); +void pci_hide_existing_device(struct pci_dev *pdev); int pci_ro_device(int seg, int bus, int devfn); int pci_hide_device(int bus, int devfn); struct pci_dev *pci_get_pdev(int seg, int bus, int devfn); -- 1.9.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue 2016-03-17 7:12 ` [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue Quan Xu @ 2016-03-17 8:17 ` Tian, Kevin 2016-03-17 9:43 ` Jan Beulich 2016-03-18 12:21 ` Xu, Quan 2016-03-18 11:18 ` Jan Beulich 1 sibling, 2 replies; 22+ messages in thread From: Tian, Kevin @ 2016-03-17 8:17 UTC (permalink / raw) To: Xu, Quan, xen-devel; +Cc: dario.faggioli, Wu, Feng, jbeulich > From: Xu, Quan > Sent: Thursday, March 17, 2016 3:13 PM > diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c > index 37a15fb..2a5c638 100644 > --- a/xen/drivers/passthrough/vtd/qinval.c > +++ b/xen/drivers/passthrough/vtd/qinval.c > @@ -233,6 +233,57 @@ int qinval_device_iotlb(struct iommu *iommu, > return 0; > } > > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did, > + u16 seg, u8 bus, u8 devfn) > +{ > + struct domain *d = NULL; > + struct pci_dev *pdev; > + > + if ( test_bit(did, iommu->domid_bitmap) ) > + d = rcu_lock_domain_by_id(iommu->domid_map[did]); > + > + if ( d == NULL ) > + return; > + > + pcidevs_lock(); > + for_each_pdev(d, pdev) we need a 'safe' version here since you're deleting nodes when walking list. for_each_pdev today is based on list_for_each_entry. Or if it's sure that only one pdev can match, we can break out of the loop to do removal. > + { > + if ( ( pdev->seg == seg ) && > + ( pdev->bus == bus ) && > + ( pdev->devfn == devfn ) ) > + { > + ASSERT ( pdev->domain ); > + list_del(&pdev->domain_list); > + pdev->domain = NULL; > + pci_hide_existing_device(pdev); > + break; > + } > + } > + > + pcidevs_unlock(); > + > + if ( !is_hardware_domain(d) ) > + domain_crash(d); > + > + rcu_unlock_domain(d); > +} > + > +int dev_invalidate_iotlb_sync(struct iommu *iommu, u16 did, > + u16 seg, u8 bus, u8 devfn) > +{ > + struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu); > + int rc = 0; > + > + if ( qi_ctrl->qinval_maddr ) > + { > + rc = queue_invalidate_wait(iommu, 0, 1, 1); > + if ( rc == -ETIMEDOUT ) > + dev_invalidate_iotlb_timeout(iommu, did, seg, bus, devfn); > + } > + > + return rc; > +} > + Is this function a temporary one which will be removed later once we can handle timeout for all types of flushes (at that time suppose this logic will be reflected in invalidate_sync directly)? > static void queue_invalidate_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx) > { > unsigned long flags; > @@ -342,8 +393,6 @@ static int flush_iotlb_qi( > > if ( qi_ctrl->qinval_maddr != 0 ) > { > - int rc; > - > /* use queued invalidation */ > if (cap_write_drain(iommu->cap)) > dw = 1; > @@ -353,11 +402,17 @@ static int flush_iotlb_qi( > queue_invalidate_iotlb(iommu, > type >> DMA_TLB_FLUSH_GRANU_OFFSET, dr, > dw, did, size_order, 0, addr); > + > + /* > + * Before Device-TLB invalidation we need to synchronize > + * invalidation completions with hardware. > + */ > + ret = invalidate_sync(iommu); > + if ( ret ) > + return ret; > + > if ( flush_dev_iotlb ) > ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type); > - rc = invalidate_sync(iommu); > - if ( !ret ) > - ret = rc; Current change looks not consistent. For IOMMU iotlb flush, we have invalidate_sync out of invalidate operation, however below... > } > return ret; > } > diff --git a/xen/drivers/passthrough/vtd/x86/ats.c > b/xen/drivers/passthrough/vtd/x86/ats.c > index 334b9c1..c87ffe3 100644 > --- a/xen/drivers/passthrough/vtd/x86/ats.c > +++ b/xen/drivers/passthrough/vtd/x86/ats.c > @@ -162,6 +162,18 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did, > return -EOPNOTSUPP; > } > > + /* > + * Synchronize with hardware for Device-TLB invalidate > + * descriptor. > + */ > + rc = dev_invalidate_iotlb_sync(iommu, did, pdev->seg, > + pdev->bus, pdev->devfn); > + if ( rc ) > + printk(XENLOG_ERR > + "Flush error %d on device %04x:%02x:%02x.%u.\n", > + ret, pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), > + PCI_FUNC(pdev->devfn)); > + > if ( !ret ) for device iotlb flush, you moved the invalidate_sync inside the invalidate operation. If this is only temporary as I guessed earlier, is it clearer to change like below: > @@ -353,11 +402,17 @@ static int flush_iotlb_qi( queue_invalidate_iotlb(iommu, type >> DMA_TLB_FLUSH_GRANU_OFFSET, dr, dw, did, size_order, 0, addr); /* * Before Device-TLB invalidation we need to synchronize * invalidation completions with hardware. * TODO: timeout error handling to be added later */ ret = invalidate_sync(iommu); if ( ret ) return ret; if ( flush_dev_iotlb ) ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type); rc = invalidate_sync(iommu); if ( rc == -ETIMEDOUT ) dev_invalidate_iotlb_timeout(iommu, did, seg, bus, devfn); if ( !ret ) ret = rc; This way later when we have invalidate_sync handling timeout error for all types of flushes, above two lines of timeout handling can be removed. Thanks Kevin _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue 2016-03-17 8:17 ` Tian, Kevin @ 2016-03-17 9:43 ` Jan Beulich 2016-03-17 11:13 ` Tian, Kevin 2016-03-18 12:21 ` Xu, Quan 1 sibling, 1 reply; 22+ messages in thread From: Jan Beulich @ 2016-03-17 9:43 UTC (permalink / raw) To: Kevin Tian; +Cc: dario.faggioli, Feng Wu, Quan Xu, xen-devel >>> On 17.03.16 at 09:17, <kevin.tian@intel.com> wrote: >> From: Xu, Quan >> Sent: Thursday, March 17, 2016 3:13 PM >> --- a/xen/drivers/passthrough/vtd/qinval.c >> +++ b/xen/drivers/passthrough/vtd/qinval.c >> @@ -233,6 +233,57 @@ int qinval_device_iotlb(struct iommu *iommu, >> return 0; >> } >> >> +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did, >> + u16 seg, u8 bus, u8 devfn) >> +{ >> + struct domain *d = NULL; >> + struct pci_dev *pdev; >> + >> + if ( test_bit(did, iommu->domid_bitmap) ) >> + d = rcu_lock_domain_by_id(iommu->domid_map[did]); >> + >> + if ( d == NULL ) >> + return; >> + >> + pcidevs_lock(); >> + for_each_pdev(d, pdev) > > we need a 'safe' version here since you're deleting nodes > when walking list. for_each_pdev today is based on > list_for_each_entry. Or if it's sure that only one pdev > can match, we can break out of the loop to do removal. But breaking out of the loop is what is already being done ... >> + { >> + if ( ( pdev->seg == seg ) && >> + ( pdev->bus == bus ) && >> + ( pdev->devfn == devfn ) ) >> + { >> + ASSERT ( pdev->domain ); >> + list_del(&pdev->domain_list); >> + pdev->domain = NULL; >> + pci_hide_existing_device(pdev); >> + break; ... here. >> + } >> + } >> + >> + pcidevs_unlock(); No need for using "safe" list traversal afaict. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue 2016-03-17 9:43 ` Jan Beulich @ 2016-03-17 11:13 ` Tian, Kevin 2016-03-17 11:30 ` Xu, Quan ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Tian, Kevin @ 2016-03-17 11:13 UTC (permalink / raw) To: Jan Beulich; +Cc: dario.faggioli, Wu, Feng, Xu, Quan, xen-devel > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Thursday, March 17, 2016 5:43 PM > > >>> On 17.03.16 at 09:17, <kevin.tian@intel.com> wrote: > >> From: Xu, Quan > >> Sent: Thursday, March 17, 2016 3:13 PM > >> --- a/xen/drivers/passthrough/vtd/qinval.c > >> +++ b/xen/drivers/passthrough/vtd/qinval.c > >> @@ -233,6 +233,57 @@ int qinval_device_iotlb(struct iommu *iommu, > >> return 0; > >> } > >> > >> +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did, > >> + u16 seg, u8 bus, u8 devfn) > >> +{ > >> + struct domain *d = NULL; > >> + struct pci_dev *pdev; > >> + > >> + if ( test_bit(did, iommu->domid_bitmap) ) > >> + d = rcu_lock_domain_by_id(iommu->domid_map[did]); > >> + > >> + if ( d == NULL ) > >> + return; > >> + > >> + pcidevs_lock(); > >> + for_each_pdev(d, pdev) > > > > we need a 'safe' version here since you're deleting nodes > > when walking list. for_each_pdev today is based on > > list_for_each_entry. Or if it's sure that only one pdev > > can match, we can break out of the loop to do removal. > > But breaking out of the loop is what is already being done ... > > >> + { > >> + if ( ( pdev->seg == seg ) && > >> + ( pdev->bus == bus ) && > >> + ( pdev->devfn == devfn ) ) > >> + { > >> + ASSERT ( pdev->domain ); > >> + list_del(&pdev->domain_list); > >> + pdev->domain = NULL; > >> + pci_hide_existing_device(pdev); > >> + break; > > ... here. > however you see list_del happens before breaking out, right? Thanks Kevin _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue 2016-03-17 11:13 ` Tian, Kevin @ 2016-03-17 11:30 ` Xu, Quan 2016-03-17 11:32 ` Tian, Kevin 2016-03-17 11:33 ` Jan Beulich 2 siblings, 0 replies; 22+ messages in thread From: Xu, Quan @ 2016-03-17 11:30 UTC (permalink / raw) To: Tian, Kevin, Jan Beulich; +Cc: dario.faggioli, Wu, Feng, xen-devel On March 17, 2016 7:14pm, Tian, Kevin <kevin.tian@intel.com> wrote: > > From: Jan Beulich [mailto:JBeulich@suse.com] > > Sent: Thursday, March 17, 2016 5:43 PM > > > > >>> On 17.03.16 at 09:17, <kevin.tian@intel.com> wrote: > > >> From: Xu, Quan > > >> Sent: Thursday, March 17, 2016 3:13 PM > > >> --- a/xen/drivers/passthrough/vtd/qinval.c > > >> +++ b/xen/drivers/passthrough/vtd/qinval.c > > >> @@ -233,6 +233,57 @@ int qinval_device_iotlb(struct iommu *iommu, > > >> return 0; > > >> } > > >> > > >> +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did, > > >> + u16 seg, u8 bus, u8 > > >> +devfn) { > > >> + struct domain *d = NULL; > > >> + struct pci_dev *pdev; > > >> + > > >> + if ( test_bit(did, iommu->domid_bitmap) ) > > >> + d = rcu_lock_domain_by_id(iommu->domid_map[did]); > > >> + > > >> + if ( d == NULL ) > > >> + return; > > >> + > > >> + pcidevs_lock(); > > >> + for_each_pdev(d, pdev) > > > > > > we need a 'safe' version here since you're deleting nodes when > > > walking list. for_each_pdev today is based on list_for_each_entry. > > > Or if it's sure that only one pdev can match, we can break out of > > > the loop to do removal. > > > > But breaking out of the loop is what is already being done ... > > > > >> + { > > >> + if ( ( pdev->seg == seg ) && > > >> + ( pdev->bus == bus ) && > > >> + ( pdev->devfn == devfn ) ) > > >> + { > > >> + ASSERT ( pdev->domain ); > > >> + list_del(&pdev->domain_list); > > >> + pdev->domain = NULL; > > >> + pci_hide_existing_device(pdev); > > >> + break; > > > > ... here. > > > > however you see list_del happens before breaking out, right? > For these version of macros: a). list_for_each_entry - iterate over list of given type b). list_for_each_entry_safe - iterate over list of given type safe against removal of list entry __iiuc__, I think the key point is: -in general, we'd better remove entry under list_for_each_entry_safe. Yes, it is correct to use list_for_each_entry_safe for this point too. - If we delete the iterate from list_for_each_entry, it may point to undefined state, leading to xen panic. but the pdev->domain_list is not a point ( it is a "struct list_head domain_list" variable), so it is safe to delete it under list_for_each_entry in this case. Jan, is it similar to yours? Quan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue 2016-03-17 11:13 ` Tian, Kevin 2016-03-17 11:30 ` Xu, Quan @ 2016-03-17 11:32 ` Tian, Kevin 2016-03-17 11:33 ` Jan Beulich 2 siblings, 0 replies; 22+ messages in thread From: Tian, Kevin @ 2016-03-17 11:32 UTC (permalink / raw) To: Tian, Kevin, Jan Beulich; +Cc: dario.faggioli, Wu, Feng, Xu, Quan, xen-devel > From: Tian, Kevin > Sent: Thursday, March 17, 2016 7:14 PM > > > From: Jan Beulich [mailto:JBeulich@suse.com] > > Sent: Thursday, March 17, 2016 5:43 PM > > > > >>> On 17.03.16 at 09:17, <kevin.tian@intel.com> wrote: > > >> From: Xu, Quan > > >> Sent: Thursday, March 17, 2016 3:13 PM > > >> --- a/xen/drivers/passthrough/vtd/qinval.c > > >> +++ b/xen/drivers/passthrough/vtd/qinval.c > > >> @@ -233,6 +233,57 @@ int qinval_device_iotlb(struct iommu *iommu, > > >> return 0; > > >> } > > >> > > >> +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did, > > >> + u16 seg, u8 bus, u8 devfn) > > >> +{ > > >> + struct domain *d = NULL; > > >> + struct pci_dev *pdev; > > >> + > > >> + if ( test_bit(did, iommu->domid_bitmap) ) > > >> + d = rcu_lock_domain_by_id(iommu->domid_map[did]); > > >> + > > >> + if ( d == NULL ) > > >> + return; > > >> + > > >> + pcidevs_lock(); > > >> + for_each_pdev(d, pdev) > > > > > > we need a 'safe' version here since you're deleting nodes > > > when walking list. for_each_pdev today is based on > > > list_for_each_entry. Or if it's sure that only one pdev > > > can match, we can break out of the loop to do removal. > > > > But breaking out of the loop is what is already being done ... > > > > >> + { > > >> + if ( ( pdev->seg == seg ) && > > >> + ( pdev->bus == bus ) && > > >> + ( pdev->devfn == devfn ) ) > > >> + { > > >> + ASSERT ( pdev->domain ); > > >> + list_del(&pdev->domain_list); > > >> + pdev->domain = NULL; > > >> + pci_hide_existing_device(pdev); > > >> + break; > > > > ... here. > > > > however you see list_del happens before breaking out, right? > Sorry you're right and please forget my earlier reply. :-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue 2016-03-17 11:13 ` Tian, Kevin 2016-03-17 11:30 ` Xu, Quan 2016-03-17 11:32 ` Tian, Kevin @ 2016-03-17 11:33 ` Jan Beulich 2 siblings, 0 replies; 22+ messages in thread From: Jan Beulich @ 2016-03-17 11:33 UTC (permalink / raw) To: Kevin Tian; +Cc: dario.faggioli, Feng Wu, Quan Xu, xen-devel >>> On 17.03.16 at 12:13, <kevin.tian@intel.com> wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Thursday, March 17, 2016 5:43 PM >> >> >>> On 17.03.16 at 09:17, <kevin.tian@intel.com> wrote: >> >> From: Xu, Quan >> >> Sent: Thursday, March 17, 2016 3:13 PM >> >> --- a/xen/drivers/passthrough/vtd/qinval.c >> >> +++ b/xen/drivers/passthrough/vtd/qinval.c >> >> @@ -233,6 +233,57 @@ int qinval_device_iotlb(struct iommu *iommu, >> >> return 0; >> >> } >> >> >> >> +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did, >> >> + u16 seg, u8 bus, u8 devfn) >> >> +{ >> >> + struct domain *d = NULL; >> >> + struct pci_dev *pdev; >> >> + >> >> + if ( test_bit(did, iommu->domid_bitmap) ) >> >> + d = rcu_lock_domain_by_id(iommu->domid_map[did]); >> >> + >> >> + if ( d == NULL ) >> >> + return; >> >> + >> >> + pcidevs_lock(); >> >> + for_each_pdev(d, pdev) >> > >> > we need a 'safe' version here since you're deleting nodes >> > when walking list. for_each_pdev today is based on >> > list_for_each_entry. Or if it's sure that only one pdev >> > can match, we can break out of the loop to do removal. >> >> But breaking out of the loop is what is already being done ... >> >> >> + { >> >> + if ( ( pdev->seg == seg ) && >> >> + ( pdev->bus == bus ) && >> >> + ( pdev->devfn == devfn ) ) >> >> + { >> >> + ASSERT ( pdev->domain ); >> >> + list_del(&pdev->domain_list); >> >> + pdev->domain = NULL; >> >> + pci_hide_existing_device(pdev); >> >> + break; >> >> ... here. >> > > however you see list_del happens before breaking out, right? I'm sorry Kevin, but ??? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue 2016-03-17 8:17 ` Tian, Kevin 2016-03-17 9:43 ` Jan Beulich @ 2016-03-18 12:21 ` Xu, Quan 2016-03-21 3:27 ` Tian, Kevin 1 sibling, 1 reply; 22+ messages in thread From: Xu, Quan @ 2016-03-18 12:21 UTC (permalink / raw) To: Tian, Kevin, xen-devel; +Cc: dario.faggioli, Wu, Feng, jbeulich On March 17, 2016 4:17pm, Tian, Kevin <kevin.tian@intel.com> wrote: > > From: Xu, Quan > > Sent: Thursday, March 17, 2016 3:13 PM diff --git > > a/xen/drivers/passthrough/vtd/qinval.c > > b/xen/drivers/passthrough/vtd/qinval.c > > index 37a15fb..2a5c638 100644 > > --- a/xen/drivers/passthrough/vtd/qinval.c > > +++ b/xen/drivers/passthrough/vtd/qinval.c > > + { > > + if ( ( pdev->seg == seg ) && > > + ( pdev->bus == bus ) && > > + ( pdev->devfn == devfn ) ) > > + { > > + ASSERT ( pdev->domain ); > > + list_del(&pdev->domain_list); > > + pdev->domain = NULL; > > + pci_hide_existing_device(pdev); > > + break; > > + } > > + } > > + > > + pcidevs_unlock(); > > + > > + if ( !is_hardware_domain(d) ) > > + domain_crash(d); > > + > > + rcu_unlock_domain(d); > > +} > > + > > +int dev_invalidate_iotlb_sync(struct iommu *iommu, u16 did, > > + u16 seg, u8 bus, u8 devfn) { > > + struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu); > > + int rc = 0; > > + > > + if ( qi_ctrl->qinval_maddr ) > > + { > > + rc = queue_invalidate_wait(iommu, 0, 1, 1); > > + if ( rc == -ETIMEDOUT ) > > + dev_invalidate_iotlb_timeout(iommu, did, seg, bus, devfn); > > + } > > + > > + return rc; > > +} > > + > > Is this function a temporary one which will be removed later once we can > handle timeout for all types of flushes (at that time suppose this logic will be > reflected in invalidate_sync directly)? > No, it's not a temporary one. dev_invalidate_iotlb_sync -- for Device-TLB invalidation sync, as we need SBDF to indicate which device flush timed out. invalidate_sync -- for VT-d iotlb/iec/context invalidation sync. > > static void queue_invalidate_iec(struct iommu *iommu, u8 granu, u8 > > im, u16 iidx) { > > unsigned long flags; > > @@ -342,8 +393,6 @@ static int flush_iotlb_qi( > > > > if ( qi_ctrl->qinval_maddr != 0 ) > > { > > - int rc; > > - > > /* use queued invalidation */ > > if (cap_write_drain(iommu->cap)) > > dw = 1; > > @@ -353,11 +402,17 @@ static int flush_iotlb_qi( > > queue_invalidate_iotlb(iommu, > > type >> > DMA_TLB_FLUSH_GRANU_OFFSET, dr, > > dw, did, size_order, 0, addr); > > + > > + /* > > + * Before Device-TLB invalidation we need to synchronize > > + * invalidation completions with hardware. > > + */ > > + ret = invalidate_sync(iommu); > > + if ( ret ) > > + return ret; > > + > > if ( flush_dev_iotlb ) > > ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type); > > - rc = invalidate_sync(iommu); > > - if ( !ret ) > > - ret = rc; > > Current change looks not consistent. For IOMMU iotlb flush, we have > invalidate_sync out of invalidate operation, however below... > Now, does it still look not consistent? > > } > > return ret; > > } > > diff --git a/xen/drivers/passthrough/vtd/x86/ats.c > > b/xen/drivers/passthrough/vtd/x86/ats.c > > index 334b9c1..c87ffe3 100644 > > --- a/xen/drivers/passthrough/vtd/x86/ats.c > > +++ b/xen/drivers/passthrough/vtd/x86/ats.c > > @@ -162,6 +162,18 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 > did, > > return -EOPNOTSUPP; > > } > > > > + /* > > + * Synchronize with hardware for Device-TLB invalidate > > + * descriptor. > > + */ > > + rc = dev_invalidate_iotlb_sync(iommu, did, pdev->seg, > > + pdev->bus, pdev->devfn); > > + if ( rc ) > > + printk(XENLOG_ERR > > + "Flush error %d on device %04x:%02x:%02x.%u.\n", > > + ret, pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), > > + PCI_FUNC(pdev->devfn)); > > + > > if ( !ret ) > > for device iotlb flush, you moved the invalidate_sync inside the invalidate > operation. > > If this is only temporary as I guessed earlier, is it clearer to change like below: > > > @@ -353,11 +402,17 @@ static int flush_iotlb_qi( > queue_invalidate_iotlb(iommu, > type >> > DMA_TLB_FLUSH_GRANU_OFFSET, dr, > dw, did, size_order, 0, addr); > > /* > * Before Device-TLB invalidation we need to synchronize > * invalidation completions with hardware. > * TODO: timeout error handling to be added later > */ I'd like this TODO, and I would add it in v8. Quan > ret = invalidate_sync(iommu); > if ( ret ) > return ret; > > if ( flush_dev_iotlb ) > ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type); > > rc = invalidate_sync(iommu); > if ( rc == -ETIMEDOUT ) > dev_invalidate_iotlb_timeout(iommu, did, seg, bus, devfn); > if ( !ret ) > ret = rc; > > This way later when we have invalidate_sync handling timeout error for all > types of flushes, above two lines of timeout handling can be removed. > > Thanks > Kevin _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue 2016-03-18 12:21 ` Xu, Quan @ 2016-03-21 3:27 ` Tian, Kevin 2016-03-23 2:12 ` Xu, Quan 2016-03-23 3:29 ` Xu, Quan 0 siblings, 2 replies; 22+ messages in thread From: Tian, Kevin @ 2016-03-21 3:27 UTC (permalink / raw) To: Xu, Quan, xen-devel; +Cc: dario.faggioli, Wu, Feng, jbeulich > From: Xu, Quan > Sent: Friday, March 18, 2016 8:22 PM > > > > +int dev_invalidate_iotlb_sync(struct iommu *iommu, u16 did, > > > + u16 seg, u8 bus, u8 devfn) { > > > + struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu); > > > + int rc = 0; > > > + > > > + if ( qi_ctrl->qinval_maddr ) > > > + { > > > + rc = queue_invalidate_wait(iommu, 0, 1, 1); > > > + if ( rc == -ETIMEDOUT ) > > > + dev_invalidate_iotlb_timeout(iommu, did, seg, bus, devfn); > > > + } > > > + > > > + return rc; > > > +} > > > + > > > > Is this function a temporary one which will be removed later once we can > > handle timeout for all types of flushes (at that time suppose this logic will be > > reflected in invalidate_sync directly)? > > > No, it's not a temporary one. > dev_invalidate_iotlb_sync -- for Device-TLB invalidation sync, as we need SBDF to indicate > which device flush timed out. > invalidate_sync -- for VT-d iotlb/iec/context invalidation sync. Thanks. I recalled it. Once you defined some INVALID seg/bus/devfn to reuse same interface, and then the suggestion is to go with different interfaces.:-) > > > > > static void queue_invalidate_iec(struct iommu *iommu, u8 granu, u8 > > > im, u16 iidx) { > > > unsigned long flags; > > > @@ -342,8 +393,6 @@ static int flush_iotlb_qi( > > > > > > if ( qi_ctrl->qinval_maddr != 0 ) > > > { > > > - int rc; > > > - > > > /* use queued invalidation */ > > > if (cap_write_drain(iommu->cap)) > > > dw = 1; > > > @@ -353,11 +402,17 @@ static int flush_iotlb_qi( > > > queue_invalidate_iotlb(iommu, > > > type >> > > DMA_TLB_FLUSH_GRANU_OFFSET, dr, > > > dw, did, size_order, 0, addr); > > > + > > > + /* > > > + * Before Device-TLB invalidation we need to synchronize > > > + * invalidation completions with hardware. > > > + */ > > > + ret = invalidate_sync(iommu); > > > + if ( ret ) > > > + return ret; > > > + > > > if ( flush_dev_iotlb ) > > > ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type); > > > - rc = invalidate_sync(iommu); > > > - if ( !ret ) > > > - ret = rc; > > > > Current change looks not consistent. For IOMMU iotlb flush, we have > > invalidate_sync out of invalidate operation, however below... > > > > Now, does it still look not consistent? > Yes, still inconsistent. As I said, you put invalidation sync within dev_invalidate_iotlb, while for all other IOMMU invalidations the sync is put after. Below would be consistent then: if ( flush_dev_iotlb ) ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type); rc = dev_invalidate_iotlb_sync(...); if ( !ret ) ret = rc; Thanks Kevin _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue 2016-03-21 3:27 ` Tian, Kevin @ 2016-03-23 2:12 ` Xu, Quan 2016-03-23 3:29 ` Xu, Quan 1 sibling, 0 replies; 22+ messages in thread From: Xu, Quan @ 2016-03-23 2:12 UTC (permalink / raw) To: Tian, Kevin, xen-devel; +Cc: dario.faggioli, Wu, Feng, jbeulich (( __ sorry, I was out of office on Mon./Tues. __)) On March 21, 2016 11:27am, Tian, Kevin <kevin.tian@intel.com> wrote: > > From: Xu, Quan > > Sent: Friday, March 18, 2016 8:22 PM > > > > static void queue_invalidate_iec(struct iommu *iommu, u8 granu, > > > > u8 im, u16 iidx) { > > > > unsigned long flags; > > > > @@ -342,8 +393,6 @@ static int flush_iotlb_qi( > > > > > > > > if ( qi_ctrl->qinval_maddr != 0 ) > > > > { > > > > - int rc; > > > > - > > > > /* use queued invalidation */ > > > > if (cap_write_drain(iommu->cap)) > > > > dw = 1; > > > > @@ -353,11 +402,17 @@ static int flush_iotlb_qi( > > > > queue_invalidate_iotlb(iommu, > > > > type >> > > > DMA_TLB_FLUSH_GRANU_OFFSET, dr, > > > > dw, did, size_order, 0, addr); > > > > + > > > > + /* > > > > + * Before Device-TLB invalidation we need to synchronize > > > > + * invalidation completions with hardware. > > > > + */ > > > > + ret = invalidate_sync(iommu); > > > > + if ( ret ) > > > > + return ret; > > > > + > > > > if ( flush_dev_iotlb ) > > > > ret = dev_invalidate_iotlb(iommu, did, addr, size_order, > type); > > > > - rc = invalidate_sync(iommu); > > > > - if ( !ret ) > > > > - ret = rc; > > > > > > Current change looks not consistent. For IOMMU iotlb flush, we have > > > invalidate_sync out of invalidate operation, however below... > > > > > > > Now, does it still look not consistent? > > > > Yes, still inconsistent. As I said, you put invalidation sync within > dev_invalidate_iotlb, while for all other IOMMU invalidations the sync is put > after. Below would be consistent then: > > if ( flush_dev_iotlb ) > ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type); > rc = dev_invalidate_iotlb_sync(...); > if ( !ret ) > ret = rc; > Make sense, I will fix it in next v8. Now this patch set looks clear, I'd better summarize and send out v8. Of course, I would continue to explain and fix the prereq patch set. Quan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue 2016-03-21 3:27 ` Tian, Kevin 2016-03-23 2:12 ` Xu, Quan @ 2016-03-23 3:29 ` Xu, Quan 2016-03-23 5:36 ` Tian, Kevin 1 sibling, 1 reply; 22+ messages in thread From: Xu, Quan @ 2016-03-23 3:29 UTC (permalink / raw) To: Tian, Kevin, xen-devel; +Cc: dario.faggioli, Wu, Feng, jbeulich On March 21, 2016 11:27am, Tian, Kevin <kevin.tian@intel.com> wrote: > > From: Xu, Quan > > Sent: Friday, March 18, 2016 8:22 PM > > > > static void queue_invalidate_iec(struct iommu *iommu, u8 granu, > > > > u8 im, u16 iidx) { > > > > unsigned long flags; > > > > @@ -342,8 +393,6 @@ static int flush_iotlb_qi( > > > > > > > > if ( qi_ctrl->qinval_maddr != 0 ) > > > > { > > > > - int rc; > > > > - > > > > /* use queued invalidation */ > > > > if (cap_write_drain(iommu->cap)) > > > > dw = 1; > > > > @@ -353,11 +402,17 @@ static int flush_iotlb_qi( > > > > queue_invalidate_iotlb(iommu, > > > > type >> > > > DMA_TLB_FLUSH_GRANU_OFFSET, dr, > > > > dw, did, size_order, 0, addr); > > > > + > > > > + /* > > > > + * Before Device-TLB invalidation we need to synchronize > > > > + * invalidation completions with hardware. > > > > + */ > > > > + ret = invalidate_sync(iommu); > > > > + if ( ret ) > > > > + return ret; > > > > + > > > > if ( flush_dev_iotlb ) > > > > ret = dev_invalidate_iotlb(iommu, did, addr, size_order, > type); > > > > - rc = invalidate_sync(iommu); > > > > - if ( !ret ) > > > > - ret = rc; > > > > > > Current change looks not consistent. For IOMMU iotlb flush, we have > > > invalidate_sync out of invalidate operation, however below... > > > > > > > Now, does it still look not consistent? > > > > Yes, still inconsistent. As I said, you put invalidation sync within > dev_invalidate_iotlb, while for all other IOMMU invalidations the sync is put > after. Below would be consistent then: > > if ( flush_dev_iotlb ) > ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type); > rc = dev_invalidate_iotlb_sync(...); > if ( !ret ) > ret = rc; > Kevin, now I doubt that I should put invalidation sync within dev_invalidate_iotlb, which was also your suggestion. As the dev_invalidate_iotlb() is invalidation for all of domain's ATS devices. If in this consistent way, we couldn't Find which ATS device flush timed out, then we need to hide all of domain's ATS devices. Do you recall it? Also I think it is reluctant to put invalidate_sync within queue_invalidate_iotlb() for consistent issue. Quan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue 2016-03-23 3:29 ` Xu, Quan @ 2016-03-23 5:36 ` Tian, Kevin 2016-03-23 5:39 ` Xu, Quan 0 siblings, 1 reply; 22+ messages in thread From: Tian, Kevin @ 2016-03-23 5:36 UTC (permalink / raw) To: Xu, Quan, xen-devel; +Cc: dario.faggioli, Wu, Feng, jbeulich > From: Xu, Quan > Sent: Wednesday, March 23, 2016 11:30 AM > > > > > Yes, still inconsistent. As I said, you put invalidation sync within > > dev_invalidate_iotlb, while for all other IOMMU invalidations the sync is put > > after. Below would be consistent then: > > > > if ( flush_dev_iotlb ) > > ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type); > > rc = dev_invalidate_iotlb_sync(...); > > if ( !ret ) > > ret = rc; > > > Kevin, > now I doubt that I should put invalidation sync within dev_invalidate_iotlb, which was also > your suggestion. > As the dev_invalidate_iotlb() is invalidation for all of domain's ATS devices. If in this > consistent way, we couldn't > Find which ATS device flush timed out, then we need to hide all of domain's ATS devices. > Do you recall it? > Also I think it is reluctant to put invalidate_sync within queue_invalidate_iotlb() for > consistent issue. > Quan Yes I recall this story. What about doing this? Let's wrap a _sync version for all flush interfaces, like below: static int queue_invalidate_context_sync(...) { queue_invalidate_context(...); return invalidate_sync(...); } Then invoke _sync version at all callers, e.g.: static int flush_context_qi(...) { ... if ( qi_ctrl->qinval_maddr != 0 ) ret = queue_invalidate_context_sync(...); } similarly we'll have dev_invalidate_iotlb_sync for device IOTLB flush. It simplifies caller logic and make code more readable. :-) Thanks Kevin _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue 2016-03-23 5:36 ` Tian, Kevin @ 2016-03-23 5:39 ` Xu, Quan 0 siblings, 0 replies; 22+ messages in thread From: Xu, Quan @ 2016-03-23 5:39 UTC (permalink / raw) To: Tian, Kevin, xen-devel; +Cc: dario.faggioli, Wu, Feng, jbeulich On March 23, 2016 1:37pm, <kevin.tian@intel.com> wrote: > > From: Xu, Quan > > Sent: Wednesday, March 23, 2016 11:30 AM > > > > > > > > Yes, still inconsistent. As I said, you put invalidation sync within > > > dev_invalidate_iotlb, while for all other IOMMU invalidations the > > > sync is put after. Below would be consistent then: > > > > > > if ( flush_dev_iotlb ) > > > ret = dev_invalidate_iotlb(iommu, did, addr, size_order, > type); > > > rc = dev_invalidate_iotlb_sync(...); > > > if ( !ret ) > > > ret = rc; > > > > > Kevin, > > now I doubt that I should put invalidation sync within > > dev_invalidate_iotlb, which was also your suggestion. > > As the dev_invalidate_iotlb() is invalidation for all of domain's ATS > > devices. If in this consistent way, we couldn't Find which ATS device > > flush timed out, then we need to hide all of domain's ATS devices. > > Do you recall it? > > Also I think it is reluctant to put invalidate_sync within > > queue_invalidate_iotlb() for consistent issue. > > Quan > > Yes I recall this story. > > What about doing this? Let's wrap a _sync version for all flush interfaces, like > below: > > static int queue_invalidate_context_sync(...) > { > queue_invalidate_context(...); > return invalidate_sync(...); > } > > Then invoke _sync version at all callers, e.g.: > static int flush_context_qi(...) > { > ... > if ( qi_ctrl->qinval_maddr != 0 ) > ret = queue_invalidate_context_sync(...); > } > > similarly we'll have dev_invalidate_iotlb_sync for device IOTLB flush. > > It simplifies caller logic and make code more readable. :-) > Agreed, I would follow it and send out v8 ASAP, then I could focus on prereq patch set. :) Quan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue 2016-03-17 7:12 ` [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue Quan Xu 2016-03-17 8:17 ` Tian, Kevin @ 2016-03-18 11:18 ` Jan Beulich 2016-03-18 11:31 ` Xu, Quan 1 sibling, 1 reply; 22+ messages in thread From: Jan Beulich @ 2016-03-18 11:18 UTC (permalink / raw) To: Quan Xu; +Cc: dario.faggioli, feng.wu, kevin.tian, xen-devel >>> On 17.03.16 at 08:12, <quan.xu@intel.com> wrote: > --- a/xen/drivers/passthrough/vtd/qinval.c > +++ b/xen/drivers/passthrough/vtd/qinval.c > @@ -233,6 +233,57 @@ int qinval_device_iotlb(struct iommu *iommu, > return 0; > } > > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did, > + u16 seg, u8 bus, u8 devfn) > +{ > + struct domain *d = NULL; > + struct pci_dev *pdev; > + > + if ( test_bit(did, iommu->domid_bitmap) ) > + d = rcu_lock_domain_by_id(iommu->domid_map[did]); > + > + if ( d == NULL ) > + return; > + > + pcidevs_lock(); > + for_each_pdev(d, pdev) Blank line please between these two, for symmetry with ... > + { > + if ( ( pdev->seg == seg ) && > + ( pdev->bus == bus ) && > + ( pdev->devfn == devfn ) ) > + { > + ASSERT ( pdev->domain ); > + list_del(&pdev->domain_list); > + pdev->domain = NULL; > + pci_hide_existing_device(pdev); > + break; > + } > + } > + > + pcidevs_unlock(); ... this. I assume you're aware that there's pending feedback from Kevin which you didn't reply to so far. Of course there's no need for a reply if you mean to address/incorporate in v8 everything he said. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue 2016-03-18 11:18 ` Jan Beulich @ 2016-03-18 11:31 ` Xu, Quan 2016-03-18 13:35 ` Jan Beulich 0 siblings, 1 reply; 22+ messages in thread From: Xu, Quan @ 2016-03-18 11:31 UTC (permalink / raw) To: Jan Beulich; +Cc: dario.faggioli, Wu, Feng, Tian, Kevin, xen-devel On March 18, 2016 7:19pm, <JBeulich@suse.com> wrote: > >>> On 17.03.16 at 08:12, <quan.xu@intel.com> wrote: > > --- a/xen/drivers/passthrough/vtd/qinval.c > > +++ b/xen/drivers/passthrough/vtd/qinval.c > > @@ -233,6 +233,57 @@ int qinval_device_iotlb(struct iommu *iommu, > > return 0; > > } > > > > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did, > > + u16 seg, u8 bus, u8 devfn) > { > > + struct domain *d = NULL; > > + struct pci_dev *pdev; > > + > > + if ( test_bit(did, iommu->domid_bitmap) ) > > + d = rcu_lock_domain_by_id(iommu->domid_map[did]); > > + > > + if ( d == NULL ) > > + return; > > + > > + pcidevs_lock(); > > + for_each_pdev(d, pdev) > > Blank line please between these two, for symmetry with ... > for_each_pdev( d, pdev ) ?? > > + { > > + if ( ( pdev->seg == seg ) && > > + ( pdev->bus == bus ) && > > + ( pdev->devfn == devfn ) ) > > + { > > + ASSERT ( pdev->domain ); > > + list_del(&pdev->domain_list); > > + pdev->domain = NULL; > > + pci_hide_existing_device(pdev); > > + break; > > + } > > + } > > + > > + pcidevs_unlock(); > > ... this. > > I assume you're aware that there's pending feedback from Kevin which you > didn't reply to so far. Yes, > Of course there's no need for a reply if you mean to > address/incorporate in v8 everything he said. > I will reply and explain much more. It is dinner time, and I will be back soon. Quan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue 2016-03-18 11:31 ` Xu, Quan @ 2016-03-18 13:35 ` Jan Beulich 0 siblings, 0 replies; 22+ messages in thread From: Jan Beulich @ 2016-03-18 13:35 UTC (permalink / raw) To: Quan Xu; +Cc: dario.faggioli, Feng Wu, Kevin Tian, xen-devel >>> On 18.03.16 at 12:31, <quan.xu@intel.com> wrote: > On March 18, 2016 7:19pm, <JBeulich@suse.com> wrote: >> >>> On 17.03.16 at 08:12, <quan.xu@intel.com> wrote: >> > --- a/xen/drivers/passthrough/vtd/qinval.c >> > +++ b/xen/drivers/passthrough/vtd/qinval.c >> > @@ -233,6 +233,57 @@ int qinval_device_iotlb(struct iommu *iommu, >> > return 0; >> > } >> > >> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did, >> > + u16 seg, u8 bus, u8 devfn) >> { >> > + struct domain *d = NULL; >> > + struct pci_dev *pdev; >> > + >> > + if ( test_bit(did, iommu->domid_bitmap) ) >> > + d = rcu_lock_domain_by_id(iommu->domid_map[did]); >> > + >> > + if ( d == NULL ) >> > + return; >> > + >> > + pcidevs_lock(); >> > + for_each_pdev(d, pdev) >> >> Blank line please between these two, for symmetry with ... >> > for_each_pdev( d, pdev ) ?? That's blanks you added, not a blank line. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2016-03-23 5:39 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-03-17 7:12 [PATCH v7 0/2] VT-d Device-TLB flush issue Quan Xu 2016-03-17 7:12 ` [PATCH v7 1/2] VT-d: Reduce spin timeout to 1ms, which can be boot-time changed Quan Xu 2016-03-17 7:45 ` Tian, Kevin 2016-03-17 8:11 ` Xu, Quan 2016-03-17 8:13 ` Jan Beulich 2016-03-17 8:17 ` Xu, Quan 2016-03-17 7:12 ` [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue Quan Xu 2016-03-17 8:17 ` Tian, Kevin 2016-03-17 9:43 ` Jan Beulich 2016-03-17 11:13 ` Tian, Kevin 2016-03-17 11:30 ` Xu, Quan 2016-03-17 11:32 ` Tian, Kevin 2016-03-17 11:33 ` Jan Beulich 2016-03-18 12:21 ` Xu, Quan 2016-03-21 3:27 ` Tian, Kevin 2016-03-23 2:12 ` Xu, Quan 2016-03-23 3:29 ` Xu, Quan 2016-03-23 5:36 ` Tian, Kevin 2016-03-23 5:39 ` Xu, Quan 2016-03-18 11:18 ` Jan Beulich 2016-03-18 11:31 ` Xu, Quan 2016-03-18 13:35 ` Jan Beulich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).