| Message ID | 20240129034924.817005-5-haifeng.zhao@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix vt-d hard lockup when hotplug ATS capable device | expand |
> From: Ethan Zhao <haifeng.zhao@linux.intel.com> > Sent: Monday, January 29, 2024 11:49 AM > > to check state of ATS capable pci device in qi_check_fault() for surprise > removal case, we need to pass the target pci device of ATS invalidation > request to qi_check_fault(). if pdev is valid, means current request is for > ATS invalidation, vice vesa. > > no function change. qi_submit_sync() is used for all kinds of iotlb/cache/devtlb invalidations. it's a bit weird to see a device pointer (even being NULL) in places where a device doesn't even matter. having a new qi_submit_sync_devtlb() wrapper sounds cleaner to me, with an internal __qi_submit_sync() helper to accept a device pointer. qi_submit_sync() calls __qi_submit_sync() with a null device pointer then non-devtlb paths are intact.
On 1/29/2024 4:58 PM, Tian, Kevin wrote: >> From: Ethan Zhao <haifeng.zhao@linux.intel.com> >> Sent: Monday, January 29, 2024 11:49 AM >> >> to check state of ATS capable pci device in qi_check_fault() for surprise >> removal case, we need to pass the target pci device of ATS invalidation >> request to qi_check_fault(). if pdev is valid, means current request is for >> ATS invalidation, vice vesa. >> >> no function change. > qi_submit_sync() is used for all kinds of iotlb/cache/devtlb invalidations. > it's a bit weird to see a device pointer (even being NULL) in places where > a device doesn't even matter. > > having a new qi_submit_sync_devtlb() wrapper sounds cleaner to me, > with an internal __qi_submit_sync() helper to accept a device pointer. > > qi_submit_sync() calls __qi_submit_sync() with a null device pointer then > non-devtlb paths are intact. Make sense ! That way, could keep about 10 qi_submit_sync() calling intact, while only 2-3 qi_submit_sync_devtlb() wrapper calling needed. Thanks, Ethan
Hi Ethan,
kernel test robot noticed the following build warnings:
url: https://github.com/intel-lab-lkp/linux/commits/Ethan-Zhao/PCI-make-pci_dev_is_disconnected-helper-public-for-other-drivers/20240129-115259
base: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
patch link: https://lore.kernel.org/r/20240129034924.817005-5-haifeng.zhao%40linux.intel.com
patch subject: [PATCH v12 4/5] iommu/vt-d: pass pdev parameter for qi_check_fault() and refactor callers
config: x86_64-randconfig-161-20240207 (https://download.01.org/0day-ci/archive/20240208/202402080321.n77hu71Y-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202402080321.n77hu71Y-lkp@intel.com/
smatch warnings:
drivers/iommu/intel/dmar.c:1533 qi_flush_dev_iotlb() error: we previously assumed 'info->dev' could be null (see line 1533)
vim +1533 drivers/iommu/intel/dmar.c
20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1522 void qi_flush_dev_iotlb(struct intel_iommu *iommu,
20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1523 struct device_domain_info *info, u64 addr,
20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1524 unsigned int mask)
6ba6c3a4cacfd6 drivers/pci/dmar.c Yu Zhao 2009-05-18 1525 {
c830e699e08a6c drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1526 struct pci_dev *pdev = NULL;
20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1527 u16 sid, qdep, pfsid;
6ba6c3a4cacfd6 drivers/pci/dmar.c Yu Zhao 2009-05-18 1528 struct qi_desc desc;
6ba6c3a4cacfd6 drivers/pci/dmar.c Yu Zhao 2009-05-18 1529
20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1530 if (!info || !info->ats_enabled)
20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1531 return;
20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1532
c830e699e08a6c drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 @1533 if (info->dev || !dev_is_pci(info->dev))
Missing ! character. if (!info->dev
c830e699e08a6c drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1534 return;
c830e699e08a6c drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1535
c830e699e08a6c drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1536 pdev = to_pci_dev(info->dev);
20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1537 sid = info->bus << 8 | info->devfn;
20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1538 qdep = info->ats_qdep;
20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1539 pfsid = info->pfsid;
On 2/8/2024 3:15 PM, Dan Carpenter wrote: > Hi Ethan, > > kernel test robot noticed the following build warnings: > > url: https://github.com/intel-lab-lkp/linux/commits/Ethan-Zhao/PCI-make-pci_dev_is_disconnected-helper-public-for-other-drivers/20240129-115259 > base: 41bccc98fb7931d63d03f326a746ac4d429c1dd3 > patch link: https://lore.kernel.org/r/20240129034924.817005-5-haifeng.zhao%40linux.intel.com > patch subject: [PATCH v12 4/5] iommu/vt-d: pass pdev parameter for qi_check_fault() and refactor callers > config: x86_64-randconfig-161-20240207 (https://download.01.org/0day-ci/archive/20240208/202402080321.n77hu71Y-lkp@intel.com/config) > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > | Closes: https://lore.kernel.org/r/202402080321.n77hu71Y-lkp@intel.com/ > > smatch warnings: > drivers/iommu/intel/dmar.c:1533 qi_flush_dev_iotlb() error: we previously assumed 'info->dev' could be null (see line 1533) > > vim +1533 drivers/iommu/intel/dmar.c > > 20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1522 void qi_flush_dev_iotlb(struct intel_iommu *iommu, > 20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1523 struct device_domain_info *info, u64 addr, > 20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1524 unsigned int mask) > 6ba6c3a4cacfd6 drivers/pci/dmar.c Yu Zhao 2009-05-18 1525 { > c830e699e08a6c drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1526 struct pci_dev *pdev = NULL; > 20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1527 u16 sid, qdep, pfsid; > 6ba6c3a4cacfd6 drivers/pci/dmar.c Yu Zhao 2009-05-18 1528 struct qi_desc desc; > 6ba6c3a4cacfd6 drivers/pci/dmar.c Yu Zhao 2009-05-18 1529 > 20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1530 if (!info || !info->ats_enabled) > 20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1531 return; > 20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1532 > c830e699e08a6c drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 @1533 if (info->dev || !dev_is_pci(info->dev)) > > Missing ! character. if (!info->dev Got it, thanks. > > c830e699e08a6c drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1534 return; > c830e699e08a6c drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1535 > c830e699e08a6c drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1536 pdev = to_pci_dev(info->dev); > 20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1537 sid = info->bus << 8 | info->devfn; > 20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1538 qdep = info->ats_qdep; > 20da7293134024 drivers/iommu/intel/dmar.c Ethan Zhao 2024-01-28 1539 pfsid = info->pfsid; >
diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c index ab5e1760bd59..814134e9aa5a 100644 --- a/drivers/iommu/intel/dmar.c +++ b/drivers/iommu/intel/dmar.c @@ -1267,7 +1267,8 @@ static void qi_dump_fault(struct intel_iommu *iommu, u32 fault) (unsigned long long)desc->qw1); } -static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index) +static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index, + struct pci_dev *pdev) { u32 fault; int head, tail; @@ -1344,7 +1345,8 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index) * can be part of the submission but it will not be polled for completion. */ int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc, - unsigned int count, unsigned long options) + unsigned int count, unsigned long options, + struct pci_dev *pdev) { struct q_inval *qi = iommu->qi; s64 devtlb_start_ktime = 0; @@ -1430,7 +1432,7 @@ int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc, * a deadlock where the interrupt context can wait indefinitely * for free slots in the queue. */ - rc = qi_check_fault(iommu, index, wait_index); + rc = qi_check_fault(iommu, index, wait_index, pdev); if (rc) break; @@ -1476,7 +1478,7 @@ void qi_global_iec(struct intel_iommu *iommu) desc.qw3 = 0; /* should never fail */ - qi_submit_sync(iommu, &desc, 1, 0); + qi_submit_sync(iommu, &desc, 1, 0, NULL); } void qi_flush_context(struct intel_iommu *iommu, u16 did, u16 sid, u8 fm, @@ -1490,7 +1492,7 @@ void qi_flush_context(struct intel_iommu *iommu, u16 did, u16 sid, u8 fm, desc.qw2 = 0; desc.qw3 = 0; - qi_submit_sync(iommu, &desc, 1, 0); + qi_submit_sync(iommu, &desc, 1, 0, NULL); } void qi_flush_iotlb(struct intel_iommu *iommu, u16 did, u64 addr, @@ -1514,23 +1516,27 @@ void qi_flush_iotlb(struct intel_iommu *iommu, u16 did, u64 addr, desc.qw2 = 0; desc.qw3 = 0; - qi_submit_sync(iommu, &desc, 1, 0); + qi_submit_sync(iommu, &desc, 1, 0, NULL); } void qi_flush_dev_iotlb(struct intel_iommu *iommu, struct device_domain_info *info, u64 addr, unsigned int mask) { + struct pci_dev *pdev = NULL; u16 sid, qdep, pfsid; struct qi_desc desc; if (!info || !info->ats_enabled) return; + if (info->dev || !dev_is_pci(info->dev)) + return; + + pdev = to_pci_dev(info->dev); sid = info->bus << 8 | info->devfn; qdep = info->ats_qdep; pfsid = info->pfsid; - /* * VT-d spec, section 4.3: * @@ -1554,7 +1560,7 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, desc.qw2 = 0; desc.qw3 = 0; - qi_submit_sync(iommu, &desc, 1, 0); + qi_submit_sync(iommu, &desc, 1, 0, pdev); } /* PASID-based IOTLB invalidation */ @@ -1595,7 +1601,7 @@ void qi_flush_piotlb(struct intel_iommu *iommu, u16 did, u32 pasid, u64 addr, QI_EIOTLB_AM(mask); } - qi_submit_sync(iommu, &desc, 1, 0); + qi_submit_sync(iommu, &desc, 1, 0, NULL); } /* PASID-based device IOTLB Invalidate */ @@ -1605,15 +1611,16 @@ void qi_flush_dev_iotlb_pasid(struct intel_iommu *iommu, { unsigned long mask = 1UL << (VTD_PAGE_SHIFT + size_order - 1); struct qi_desc desc = {.qw1 = 0, .qw2 = 0, .qw3 = 0}; + struct pci_dev *pdev = NULL; u16 sid, qdep, pfsid; if (!info || !dev_is_pci(info->dev)) return; + pdev = to_pci_dev(info->dev); sid = info->bus << 8 | info->devfn; qdep = info->ats_qdep; pfsid = info->pfsid; - /* * VT-d spec, section 4.3: * @@ -1657,7 +1664,7 @@ void qi_flush_dev_iotlb_pasid(struct intel_iommu *iommu, desc.qw1 |= QI_DEV_EIOTLB_SIZE; } - qi_submit_sync(iommu, &desc, 1, 0); + qi_submit_sync(iommu, &desc, 1, 0, pdev); } void qi_flush_pasid_cache(struct intel_iommu *iommu, u16 did, @@ -1667,7 +1674,7 @@ void qi_flush_pasid_cache(struct intel_iommu *iommu, u16 did, desc.qw0 = QI_PC_PASID(pasid) | QI_PC_DID(did) | QI_PC_GRAN(granu) | QI_PC_TYPE; - qi_submit_sync(iommu, &desc, 1, 0); + qi_submit_sync(iommu, &desc, 1, 0, NULL); } /* diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h index f68f17476d85..72e1d5fb2114 100644 --- a/drivers/iommu/intel/iommu.h +++ b/drivers/iommu/intel/iommu.h @@ -1052,7 +1052,8 @@ void qi_flush_pasid_cache(struct intel_iommu *iommu, u16 did, u64 granu, u32 pasid); int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc, - unsigned int count, unsigned long options); + unsigned int count, unsigned long options, + struct pci_dev *pdev); /* * Options used in qi_submit_sync: * QI_OPT_WAIT_DRAIN - Wait for PRQ drain completion, spec 6.5.2.8. diff --git a/drivers/iommu/intel/irq_remapping.c b/drivers/iommu/intel/irq_remapping.c index 566297bc87dd..09276bfa127d 100644 --- a/drivers/iommu/intel/irq_remapping.c +++ b/drivers/iommu/intel/irq_remapping.c @@ -153,7 +153,7 @@ static int qi_flush_iec(struct intel_iommu *iommu, int index, int mask) desc.qw2 = 0; desc.qw3 = 0; - return qi_submit_sync(iommu, &desc, 1, 0); + return qi_submit_sync(iommu, &desc, 1, 0, NULL); } static int modify_irte(struct irq_2_iommu *irq_iommu, diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c index 5dacdea3cab7..8bba5721aeba 100644 --- a/drivers/iommu/intel/pasid.c +++ b/drivers/iommu/intel/pasid.c @@ -200,7 +200,7 @@ pasid_cache_invalidation_with_pasid(struct intel_iommu *iommu, desc.qw2 = 0; desc.qw3 = 0; - qi_submit_sync(iommu, &desc, 1, 0); + qi_submit_sync(iommu, &desc, 1, 0, NULL); } static void diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c index 89168b31bf31..c2f4f3822191 100644 --- a/drivers/iommu/intel/svm.c +++ b/drivers/iommu/intel/svm.c @@ -538,7 +538,7 @@ void intel_drain_pasid_prq(struct device *dev, u32 pasid) QI_DEV_IOTLB_PFSID(info->pfsid); qi_retry: reinit_completion(&iommu->prq_complete); - qi_submit_sync(iommu, desc, 3, QI_OPT_WAIT_DRAIN); + qi_submit_sync(iommu, desc, 3, QI_OPT_WAIT_DRAIN, NULL); if (readl(iommu->reg + DMAR_PRS_REG) & DMA_PRS_PRO) { wait_for_completion(&iommu->prq_complete); goto qi_retry; @@ -641,7 +641,7 @@ static void handle_bad_prq_event(struct intel_iommu *iommu, desc.qw3 = 0; } - qi_submit_sync(iommu, &desc, 1, 0); + qi_submit_sync(iommu, &desc, 1, 0, NULL); } static irqreturn_t prq_event_thread(int irq, void *d) @@ -797,7 +797,7 @@ int intel_svm_page_response(struct device *dev, ktime_to_ns(ktime_get()) - prm->private_data[0]); } - qi_submit_sync(iommu, &desc, 1, 0); + qi_submit_sync(iommu, &desc, 1, 0, NULL); } out: return ret;
to check state of ATS capable pci device in qi_check_fault() for surprise removal case, we need to pass the target pci device of ATS invalidation request to qi_check_fault(). if pdev is valid, means current request is for ATS invalidation, vice vesa. no function change. Signed-off-by: Ethan Zhao <haifeng.zhao@linux.intel.com> --- drivers/iommu/intel/dmar.c | 31 ++++++++++++++++++----------- drivers/iommu/intel/iommu.h | 3 ++- drivers/iommu/intel/irq_remapping.c | 2 +- drivers/iommu/intel/pasid.c | 2 +- drivers/iommu/intel/svm.c | 6 +++--- 5 files changed, 26 insertions(+), 18 deletions(-)