This patchset is used to fix vt-d hard lockup reported when surprise
unplug ATS capable endpoint device connects to system via PCIe switch
as following topology.
+-[0000:15]-+-00.0 Intel Corporation Ice Lake Memory Map/VT-d
| +-00.1 Intel Corporation Ice Lake Mesh 2 PCIe
| +-00.2 Intel Corporation Ice Lake RAS
| +-00.4 Intel Corporation Device 0b23
| \-01.0-[16-1b]----00.0-[17-1b]--+-00.0-[18]----00.0
NVIDIA Corporation Device 2324
| +-01.0-[19]----00.0
Mellanox Technologies MT2910 Family [ConnectX-7]
User brought endpoint device 19:00.0's link down by flapping it's hotplug
capable slot 17:01.0 link control register, as sequence DLLSC response,
pciehp_ist() will unload device driver and power it off, durning device
driver is unloading an iommu device-TLB invalidation (Intel VT-d spec, or
'ATS Invalidation' in PCIe spec) request issued to that link down device,
thus a long time completion/timeout waiting in interrupt context causes
continuous hard lockup warnning and system hang.
Other detail, see every patch commit log.
patch [1&2] were tested by [email protected] on stable v6.7-rc4.
patch [1-5] passed compiling on stable v6.8-rc2.
change log:
v12:
- use base-commit tag to format patch.
- fix building issue on v6.8-rc2 repported by [email protected].
v11:
- update per latest comment and suggestion from Baolu and YiLiu.
- split refactoring patch into two patches, [3/5] for simplify parameters
and [4/5] for pdev parameter passing.
- re-order patches.
- fold target device presence check into qi_check_fault().
- combine patch[2][5] in v10 into one patch[5].
- some commit description correctness.
- add fixes tag to patch[2/5].
- rebased on 6.8rc1
v10:
- refactor qi_submit_sync() and its callers to get pci_dev instance, as
Kevin pointed out add target_flush_dev to iommu is not right.
v9:
- unify all spelling of ATS Invalidation adhere to PCIe spec per Bjorn's
suggestion.
v8:
- add a patch to break the loop for timeout device-TLB invalidation, as
Bjorn said there is possibility device just no response but not gone.
v7:
- reorder patches and revise commit log per Bjorn's guide.
- other code and commit log revise per Lukas' suggestion.
- rebased to stable v6.7-rc6.
v6:
- add two patches to break out device-TLB invalidation if device is gone.
v5:
- add a patch try to fix the rare case (surprise remove a device in
safe removal process). not work because surprise removal handling can't
re-enter when another safe removal is in process.
v4:
- move the PCI device state checking after ATS per Baolu's suggestion.
v3:
- fix commit description typo.
v2:
- revise commit[1] description part according to Lukas' suggestion.
- revise commit[2] description to clarify the issue's impact.
v1:
- https://lore.kernel.org/lkml/20231213034637.2603013-1-haifeng.zhao@
linux.intel.com/T/
Thanks,
Ethan
Ethan Zhao (5):
PCI: make pci_dev_is_disconnected() helper public for other drivers
iommu/vt-d: don't issue ATS Invalidation request when device is
disconnected
iommu/vt-d: simplify parameters of qi_submit_sync() ATS invalidation
callers
iommu/vt-d: pass pdev parameter for qi_check_fault() and refactor
callers
iommu/vt-d: improve ITE fault handling if target device isn't present
drivers/iommu/intel/dmar.c | 71 +++++++++++++++++++++++------
drivers/iommu/intel/iommu.c | 29 +++---------
drivers/iommu/intel/iommu.h | 20 ++++----
drivers/iommu/intel/irq_remapping.c | 2 +-
drivers/iommu/intel/nested.c | 9 +---
drivers/iommu/intel/pasid.c | 12 ++---
drivers/iommu/intel/svm.c | 23 +++++-----
drivers/pci/pci.h | 5 --
include/linux/pci.h | 5 ++
9 files changed, 98 insertions(+), 78 deletions(-)
base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
--
2.31.1
Make pci_dev_is_disconnected() public so that it can be called from
Intel VT-d driver to quickly fix/workaround the surprise removal
unplug hang issue for those ATS capable devices on PCIe switch downstream
hotplug capable ports.
Beside pci_device_is_present() function, this one has no config space
space access, so is light enough to optimize the normal pure surprise
removal and safe removal flow.
Tested-by: Haorong Ye <[email protected]>
Signed-off-by: Ethan Zhao <[email protected]>
---
drivers/pci/pci.h | 5 -----
include/linux/pci.h | 5 +++++
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 2336a8d1edab..94118c4cff54 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -368,11 +368,6 @@ static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
return 0;
}
-static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
-{
- return dev->error_state == pci_channel_io_perm_failure;
-}
-
/* pci_dev priv_flags */
#define PCI_DEV_ADDED 0
#define PCI_DPC_RECOVERED 1
diff --git a/include/linux/pci.h b/include/linux/pci.h
index add9368e6314..19dfe5f16fc9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2512,6 +2512,11 @@ static inline struct pci_dev *pcie_find_root_port(struct pci_dev *dev)
return NULL;
}
+static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
+{
+ return dev->error_state == pci_channel_io_perm_failure;
+}
+
void pci_request_acs(void);
bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
bool pci_acs_path_enabled(struct pci_dev *start,
--
2.31.1
Bjorn,
On 1/29/2024 11:49 AM, Ethan Zhao wrote:
> This patchset is used to fix vt-d hard lockup reported when surprise
> unplug ATS capable endpoint device connects to system via PCIe switch
> as following topology.
>
> +-[0000:15]-+-00.0 Intel Corporation Ice Lake Memory Map/VT-d
> | +-00.1 Intel Corporation Ice Lake Mesh 2 PCIe
> | +-00.2 Intel Corporation Ice Lake RAS
> | +-00.4 Intel Corporation Device 0b23
> | \-01.0-[16-1b]----00.0-[17-1b]--+-00.0-[18]----00.0
> NVIDIA Corporation Device 2324
> | +-01.0-[19]----00.0
> Mellanox Technologies MT2910 Family [ConnectX-7]
>
> User brought endpoint device 19:00.0's link down by flapping it's hotplug
> capable slot 17:01.0 link control register, as sequence DLLSC response,
> pciehp_ist() will unload device driver and power it off, durning device
> driver is unloading an iommu device-TLB invalidation (Intel VT-d spec, or
> 'ATS Invalidation' in PCIe spec) request issued to that link down device,
> thus a long time completion/timeout waiting in interrupt context causes
> continuous hard lockup warnning and system hang.
>
> Other detail, see every patch commit log.
>
> patch [1&2] were tested by [email protected] on stable v6.7-rc4.
> patch [1-5] passed compiling on stable v6.8-rc2.
>
> change log:
> v12:
> - use base-commit tag to format patch.
> - fix building issue on v6.8-rc2 repported by [email protected].
> v11:
> - update per latest comment and suggestion from Baolu and YiLiu.
> - split refactoring patch into two patches, [3/5] for simplify parameters
> and [4/5] for pdev parameter passing.
> - re-order patches.
> - fold target device presence check into qi_check_fault().
> - combine patch[2][5] in v10 into one patch[5].
> - some commit description correctness.
> - add fixes tag to patch[2/5].
> - rebased on 6.8rc1
> v10:
> - refactor qi_submit_sync() and its callers to get pci_dev instance, as
> Kevin pointed out add target_flush_dev to iommu is not right.
> v9:
> - unify all spelling of ATS Invalidation adhere to PCIe spec per Bjorn's
> suggestion.
> v8:
> - add a patch to break the loop for timeout device-TLB invalidation, as
> Bjorn said there is possibility device just no response but not gone.
> v7:
> - reorder patches and revise commit log per Bjorn's guide.
> - other code and commit log revise per Lukas' suggestion.
> - rebased to stable v6.7-rc6.
> v6:
> - add two patches to break out device-TLB invalidation if device is gone.
> v5:
> - add a patch try to fix the rare case (surprise remove a device in
> safe removal process). not work because surprise removal handling can't
> re-enter when another safe removal is in process.
> v4:
> - move the PCI device state checking after ATS per Baolu's suggestion.
> v3:
> - fix commit description typo.
> v2:
> - revise commit[1] description part according to Lukas' suggestion.
> - revise commit[2] description to clarify the issue's impact.
> v1:
> - https://lore.kernel.org/lkml/20231213034637.2603013-1-haifeng.zhao@
> linux.intel.com/T/
>
>
> Thanks,
> Ethan
>
>
>
>
> Ethan Zhao (5):
> PCI: make pci_dev_is_disconnected() helper public for other drivers
> iommu/vt-d: don't issue ATS Invalidation request when device is
> disconnected
> iommu/vt-d: simplify parameters of qi_submit_sync() ATS invalidation
> callers
> iommu/vt-d: pass pdev parameter for qi_check_fault() and refactor
> callers
> iommu/vt-d: improve ITE fault handling if target device isn't present
>
> drivers/iommu/intel/dmar.c | 71 +++++++++++++++++++++++------
> drivers/iommu/intel/iommu.c | 29 +++---------
> drivers/iommu/intel/iommu.h | 20 ++++----
> drivers/iommu/intel/irq_remapping.c | 2 +-
> drivers/iommu/intel/nested.c | 9 +---
> drivers/iommu/intel/pasid.c | 12 ++---
> drivers/iommu/intel/svm.c | 23 +++++-----
> drivers/pci/pci.h | 5 --
> include/linux/pci.h | 5 ++
> 9 files changed, 98 insertions(+), 78 deletions(-)
>
>
> base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
I got mail delivery failure message from Gmail for "unsolicited mail", I
don't
know if you could read it by linux-pci mail list. the vt-d maintainer is
going
to pick up this patchset, may I ask your 'ack' for the pci change if no
more
comment from you ?
Thanks,
Ethan
fold parameters back to struct device_domain_info *info instead of extract
and pass them, thus decrease the number of the parameter passed for
following functions
qi_flush_dev_iotlb()
qi_flush_dev_iotlb_pasid()
quirk_extra_dev_tlb_flush()
no function change.
Signed-off-by: Ethan Zhao <[email protected]>
---
drivers/iommu/intel/dmar.c | 26 ++++++++++++++++++++++----
drivers/iommu/intel/iommu.c | 29 +++++++----------------------
drivers/iommu/intel/iommu.h | 17 ++++++++---------
drivers/iommu/intel/nested.c | 9 ++-------
drivers/iommu/intel/pasid.c | 9 ++-------
drivers/iommu/intel/svm.c | 17 ++++++++---------
6 files changed, 49 insertions(+), 58 deletions(-)
diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index 23cb80d62a9a..ab5e1760bd59 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -1517,11 +1517,20 @@ void qi_flush_iotlb(struct intel_iommu *iommu, u16 did, u64 addr,
qi_submit_sync(iommu, &desc, 1, 0);
}
-void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid,
- u16 qdep, u64 addr, unsigned mask)
+void qi_flush_dev_iotlb(struct intel_iommu *iommu,
+ struct device_domain_info *info, u64 addr,
+ unsigned int mask)
{
+ u16 sid, qdep, pfsid;
struct qi_desc desc;
+ if (!info || !info->ats_enabled)
+ return;
+
+ sid = info->bus << 8 | info->devfn;
+ qdep = info->ats_qdep;
+ pfsid = info->pfsid;
+
/*
* VT-d spec, section 4.3:
*
@@ -1590,11 +1599,20 @@ void qi_flush_piotlb(struct intel_iommu *iommu, u16 did, u32 pasid, u64 addr,
}
/* PASID-based device IOTLB Invalidate */
-void qi_flush_dev_iotlb_pasid(struct intel_iommu *iommu, u16 sid, u16 pfsid,
- u32 pasid, u16 qdep, u64 addr, unsigned int size_order)
+void qi_flush_dev_iotlb_pasid(struct intel_iommu *iommu,
+ struct device_domain_info *info, u64 addr, u32 pasid,
+ unsigned int size_order)
{
unsigned long mask = 1UL << (VTD_PAGE_SHIFT + size_order - 1);
struct qi_desc desc = {.qw1 = 0, .qw2 = 0, .qw3 = 0};
+ u16 sid, qdep, pfsid;
+
+ if (!info || !dev_is_pci(info->dev))
+ return;
+
+ sid = info->bus << 8 | info->devfn;
+ qdep = info->ats_qdep;
+ pfsid = info->pfsid;
/*
* VT-d spec, section 4.3:
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 6fb5f6fceea1..e5902944b3db 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1310,16 +1310,11 @@ static void iommu_disable_pci_caps(struct device_domain_info *info)
static void __iommu_flush_dev_iotlb(struct device_domain_info *info,
u64 addr, unsigned int mask)
{
- u16 sid, qdep;
-
if (!info || !info->ats_enabled)
return;
- sid = info->bus << 8 | info->devfn;
- qdep = info->ats_qdep;
- qi_flush_dev_iotlb(info->iommu, sid, info->pfsid,
- qdep, addr, mask);
- quirk_extra_dev_tlb_flush(info, addr, mask, IOMMU_NO_PASID, qdep);
+ qi_flush_dev_iotlb(info->iommu, info, addr, mask);
+ quirk_extra_dev_tlb_flush(info, addr, IOMMU_NO_PASID, mask);
}
static void iommu_flush_dev_iotlb(struct dmar_domain *domain,
@@ -1342,11 +1337,7 @@ static void iommu_flush_dev_iotlb(struct dmar_domain *domain,
if (!info->ats_enabled)
continue;
- qi_flush_dev_iotlb_pasid(info->iommu,
- PCI_DEVID(info->bus, info->devfn),
- info->pfsid, dev_pasid->pasid,
- info->ats_qdep, addr,
- mask);
+ qi_flush_dev_iotlb_pasid(info->iommu, info, addr, dev_pasid->pasid, mask);
}
spin_unlock_irqrestore(&domain->lock, flags);
}
@@ -4990,22 +4981,16 @@ static void __init check_tylersburg_isoch(void)
*
* As a reminder, #6 will *NEED* this quirk as we enable nested translation.
*/
-void quirk_extra_dev_tlb_flush(struct device_domain_info *info,
- unsigned long address, unsigned long mask,
- u32 pasid, u16 qdep)
+void quirk_extra_dev_tlb_flush(struct device_domain_info *info, u32 pasid,
+ unsigned long address, unsigned long mask)
{
- u16 sid;
-
if (likely(!info->dtlb_extra_inval))
return;
- sid = PCI_DEVID(info->bus, info->devfn);
if (pasid == IOMMU_NO_PASID) {
- qi_flush_dev_iotlb(info->iommu, sid, info->pfsid,
- qdep, address, mask);
+ qi_flush_dev_iotlb(info->iommu, info, address, mask);
} else {
- qi_flush_dev_iotlb_pasid(info->iommu, sid, info->pfsid,
- pasid, qdep, address, mask);
+ qi_flush_dev_iotlb_pasid(info->iommu, info, address, pasid, mask);
}
}
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index d02f916d8e59..f68f17476d85 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1037,18 +1037,17 @@ void qi_flush_context(struct intel_iommu *iommu, u16 did,
u16 sid, u8 fm, u64 type);
void qi_flush_iotlb(struct intel_iommu *iommu, u16 did, u64 addr,
unsigned int size_order, u64 type);
-void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid,
- u16 qdep, u64 addr, unsigned mask);
-
+void qi_flush_dev_iotlb(struct intel_iommu *iommu,
+ struct device_domain_info *info, u64 addr,
+ unsigned int mask);
void qi_flush_piotlb(struct intel_iommu *iommu, u16 did, u32 pasid, u64 addr,
unsigned long npages, bool ih);
-void qi_flush_dev_iotlb_pasid(struct intel_iommu *iommu, u16 sid, u16 pfsid,
- u32 pasid, u16 qdep, u64 addr,
- unsigned int size_order);
-void quirk_extra_dev_tlb_flush(struct device_domain_info *info,
- unsigned long address, unsigned long pages,
- u32 pasid, u16 qdep);
+void qi_flush_dev_iotlb_pasid(struct intel_iommu *iommu,
+ struct device_domain_info *info, u64 addr,
+ u32 pasid, unsigned int size_order);
+void quirk_extra_dev_tlb_flush(struct device_domain_info *info, u32 pasid,
+ unsigned long address, unsigned long mask);
void qi_flush_pasid_cache(struct intel_iommu *iommu, u16 did, u64 granu,
u32 pasid);
diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
index f26c7f1c46cc..d15f72b55940 100644
--- a/drivers/iommu/intel/nested.c
+++ b/drivers/iommu/intel/nested.c
@@ -78,18 +78,13 @@ static void nested_flush_dev_iotlb(struct dmar_domain *domain, u64 addr,
{
struct device_domain_info *info;
unsigned long flags;
- u16 sid, qdep;
spin_lock_irqsave(&domain->lock, flags);
list_for_each_entry(info, &domain->devices, link) {
if (!info->ats_enabled)
continue;
- sid = info->bus << 8 | info->devfn;
- qdep = info->ats_qdep;
- qi_flush_dev_iotlb(info->iommu, sid, info->pfsid,
- qdep, addr, mask);
- quirk_extra_dev_tlb_flush(info, addr, mask,
- IOMMU_NO_PASID, qdep);
+ qi_flush_dev_iotlb(info->iommu, info, addr, mask);
+ quirk_extra_dev_tlb_flush(info, IOMMU_NO_PASID, addr, mask);
}
spin_unlock_irqrestore(&domain->lock, flags);
}
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 953592125e4a..5dacdea3cab7 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -208,7 +208,6 @@ devtlb_invalidation_with_pasid(struct intel_iommu *iommu,
struct device *dev, u32 pasid)
{
struct device_domain_info *info;
- u16 sid, qdep, pfsid;
info = dev_iommu_priv_get(dev);
if (!info || !info->ats_enabled)
@@ -217,10 +216,6 @@ devtlb_invalidation_with_pasid(struct intel_iommu *iommu,
if (pci_dev_is_disconnected(to_pci_dev(dev)))
return;
- sid = info->bus << 8 | info->devfn;
- qdep = info->ats_qdep;
- pfsid = info->pfsid;
-
/*
* When PASID 0 is used, it indicates RID2PASID(DMA request w/o PASID),
* devTLB flush w/o PASID should be used. For non-zero PASID under
@@ -228,9 +223,9 @@ devtlb_invalidation_with_pasid(struct intel_iommu *iommu,
* efficient to flush devTLB specific to the PASID.
*/
if (pasid == IOMMU_NO_PASID)
- qi_flush_dev_iotlb(iommu, sid, pfsid, qdep, 0, 64 - VTD_PAGE_SHIFT);
+ qi_flush_dev_iotlb(iommu, info, 0, 64 - VTD_PAGE_SHIFT);
else
- qi_flush_dev_iotlb_pasid(iommu, sid, pfsid, pasid, qdep, 0, 64 - VTD_PAGE_SHIFT);
+ qi_flush_dev_iotlb_pasid(iommu, info, 0, pasid, 64 - VTD_PAGE_SHIFT);
}
void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 40edd282903f..89168b31bf31 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -181,11 +181,10 @@ static void __flush_svm_range_dev(struct intel_svm *svm,
qi_flush_piotlb(sdev->iommu, sdev->did, svm->pasid, address, pages, ih);
if (info->ats_enabled) {
- qi_flush_dev_iotlb_pasid(sdev->iommu, sdev->sid, info->pfsid,
- svm->pasid, sdev->qdep, address,
+ qi_flush_dev_iotlb_pasid(sdev->iommu, info, address, svm->pasid,
order_base_2(pages));
- quirk_extra_dev_tlb_flush(info, address, order_base_2(pages),
- svm->pasid, sdev->qdep);
+ quirk_extra_dev_tlb_flush(info, svm->pasid, address,
+ order_base_2(pages));
}
}
@@ -227,11 +226,11 @@ static void intel_flush_svm_all(struct intel_svm *svm)
qi_flush_piotlb(sdev->iommu, sdev->did, svm->pasid, 0, -1UL, 0);
if (info->ats_enabled) {
- qi_flush_dev_iotlb_pasid(sdev->iommu, sdev->sid, info->pfsid,
- svm->pasid, sdev->qdep,
- 0, 64 - VTD_PAGE_SHIFT);
- quirk_extra_dev_tlb_flush(info, 0, 64 - VTD_PAGE_SHIFT,
- svm->pasid, sdev->qdep);
+ qi_flush_dev_iotlb_pasid(sdev->iommu, info, 0,
+ svm->pasid,
+ 64 - VTD_PAGE_SHIFT);
+ quirk_extra_dev_tlb_flush(info, svm->pasid, 0,
+ 64 - VTD_PAGE_SHIFT);
}
}
rcu_read_unlock();
--
2.31.1
> From: Ethan Zhao <[email protected]>
> Sent: Tuesday, January 30, 2024 1:26 PM
>
> On 1/29/2024 4:50 PM, Tian, Kevin wrote:
> >> From: Ethan Zhao <[email protected]>
> >> Sent: Monday, January 29, 2024 11:49 AM
> >>
> >> Make pci_dev_is_disconnected() public so that it can be called from
> >> Intel VT-d driver to quickly fix/workaround the surprise removal
> >> unplug hang issue for those ATS capable devices on PCIe switch
> downstream
> >> hotplug capable ports.
> >>
> >> Beside pci_device_is_present() function, this one has no config space
> > s/Beside/Unlike/
> >
> >> space access, so is light enough to optimize the normal pure surprise
> >> removal and safe removal flow.
> > somehow this paragraph belongs to the patch using it. The 1st paragraph
> > is sufficient for the reason of exposure.
>
> pci_device_is_present() is aleardy exported symbol, why not just use it ?
>
> Also try to make it clear, the difference and justification.
>
I misread it.