| Message ID | 20231228001646.587653-2-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: Thursday, December 28, 2023 8:17 AM > > @@ -181,6 +181,7 @@ 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) { > + info->iommu->flush_target_dev = info->dev; > qi_flush_dev_iotlb_pasid(sdev->iommu, sdev->sid, info- > >pfsid, > svm->pasid, sdev->qdep, address, > order_base_2(pages)); this is wrong both in concept and function. an iommu instance can be shared by many devices which may all have ongoing ATS invalidation requests to handle. Using a per-iommu field to store the flush target is limiting (and there is no lock protection at all). if there is a real need of passing dev pointer to qi helpers, just change the helper to accept an explicit parameter.
On 12/28/2023 4:10 PM, Tian, Kevin wrote: >> From: Ethan Zhao <haifeng.zhao@linux.intel.com> >> Sent: Thursday, December 28, 2023 8:17 AM >> >> @@ -181,6 +181,7 @@ 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) { >> + info->iommu->flush_target_dev = info->dev; >> qi_flush_dev_iotlb_pasid(sdev->iommu, sdev->sid, info- >>> pfsid, >> svm->pasid, sdev->qdep, address, >> order_base_2(pages)); > this is wrong both in concept and function. Yes, wrong. > > an iommu instance can be shared by many devices which may all have > ongoing ATS invalidation requests to handle. Using a per-iommu field > to store the flush target is limiting (and there is no lock protection at all). > > if there is a real need of passing dev pointer to qi helpers, just change > the helper to accept an explicit parameter. seems the only way is to add parameter and refactor all affected functions. Thanks, Ethan
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 897159dba47d..c3724f1d86dc 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -1461,6 +1461,7 @@ static void __iommu_flush_dev_iotlb(struct device_domain_info *info, sid = info->bus << 8 | info->devfn; qdep = info->ats_qdep; + info->iommu->flush_target_dev = info->dev; qi_flush_dev_iotlb(info->iommu, sid, info->pfsid, qdep, addr, mask); quirk_extra_dev_tlb_flush(info, addr, mask, IOMMU_NO_PASID, qdep); diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h index ce030c5b5772..e892c5c7560a 100644 --- a/drivers/iommu/intel/iommu.h +++ b/drivers/iommu/intel/iommu.h @@ -731,6 +731,8 @@ struct intel_iommu { void *perf_statistic; struct iommu_pmu *pmu; + + struct device *flush_target_dev; /* the target device TLB to be invalidated. */ }; /* PCI domain-device relationship */ diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c index 74e8e4c17e81..1c87fb1b1039 100644 --- a/drivers/iommu/intel/pasid.c +++ b/drivers/iommu/intel/pasid.c @@ -485,6 +485,7 @@ devtlb_invalidation_with_pasid(struct intel_iommu *iommu, qdep = info->ats_qdep; pfsid = info->pfsid; + info->iommu->flush_target_dev = info->dev; /* * 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 diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c index ac12f76c1212..d42a99801cdf 100644 --- a/drivers/iommu/intel/svm.c +++ b/drivers/iommu/intel/svm.c @@ -181,6 +181,7 @@ 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) { + info->iommu->flush_target_dev = info->dev; qi_flush_dev_iotlb_pasid(sdev->iommu, sdev->sid, info->pfsid, svm->pasid, sdev->qdep, address, order_base_2(pages));
As iommu is a pointer member of device_domain_info, so can't play trick like container_of() to get the info and device instance for qi_submit_sync() low level function to check device status, add a flush_target_dev member to struct inte_iommu and pass dev info to all device-TLB invalidation (a.k.a ATS Invalidation) functions. Signed-off-by: Ethan Zhao <haifeng.zhao@linux.intel.com> --- drivers/iommu/intel/iommu.c | 1 + drivers/iommu/intel/iommu.h | 2 ++ drivers/iommu/intel/pasid.c | 1 + drivers/iommu/intel/svm.c | 1 + 4 files changed, 5 insertions(+)