On 2/22/2024 7:24 PM, Dan Carpenter wrote:
I'm sorry, I'm coming into this late and this is the first time I have
reviewed this patch. I see that we are at v13, and I hate to come in
with picky comments when a patch has already gone through 13
revisions...
Never mind that. some are totally new.
On Thu, Feb 22, 2024 at 04:02:51AM -0500, Ethan Zhao wrote:
Because surprise removal could happen anytime, e.g. user could request safe
removal to EP(endpoint device) via sysfs and brings its link down to do
surprise removal cocurrently. such aggressive cases would cause ATS
invalidation request issued to non-existence target device, then deadly
loop to retry that request after ITE fault triggered in interrupt context.
this patch aims to optimize the ITE handling by checking the target device
presence state to avoid retrying the timeout request blindly, thus avoid
hard lockup or system hang.
Devices are valid ATS invalidation request target only when they reside
"valid invalidation" is awkward wording. Can we instead say:
If you read them together, sounds like tongue twister. but here "ATS
invalidation request target" is one term in PCIe spec.
Devices should only be invalidated when they are in the
iommu->device_rbtree (probed, not released) and present.
in the iommu->device_rbtre (probed, not released) and present.
^
Missing e in _rbtree.
Yup.
Signed-off-by: Ethan Zhao <haifeng.zhao@xxxxxxxxxxxxxxx>
This patch should have a Fixes tags and be backported to stable kernels.
I think it goes back all the way...
Fixes: 704126ad81b8 ("VT-d: handle Invalidation Queue Error to avoid system hang")
Sounds reasonable.
---
drivers/iommu/intel/dmar.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index d14797aabb7a..d01d68205557 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -1273,6 +1273,9 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
{
u32 fault;
int head, tail;
+ u64 iqe_err, ite_sid;
+ struct device *dev = NULL;
+ struct pci_dev *pdev = NULL;
struct q_inval *qi = iommu->qi;
int shift = qi_shift(iommu);
@@ -1317,6 +1320,13 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
tail = readl(iommu->reg + DMAR_IQT_REG);
tail = ((tail >> shift) - 1 + QI_LENGTH) % QI_LENGTH;
+ /*
+ * SID field is valid only when the ITE field is Set in FSTS_REG
+ * see Intel VT-d spec r4.1, section 11.4.9.9
+ */
+ iqe_err = dmar_readq(iommu->reg + DMAR_IQER_REG);
+ ite_sid = DMAR_IQER_REG_ITESID(iqe_err);
+
writel(DMA_FSTS_ITE, iommu->reg + DMAR_FSTS_REG);
pr_info("Invalidation Time-out Error (ITE) cleared\n");
@@ -1326,6 +1336,21 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
head = (head - 2 + QI_LENGTH) % QI_LENGTH;
} while (head != tail);
+ /*
+ * If got ITE, we need to check if the sid of ITE is one of the
+ * current valid ATS invalidation target devices, if no, or the
+ * target device isn't presnet, don't try this request anymore.
+ * 0 value of ite_sid means old VT-d device, no ite_sid value.
+ */
This comment is kind of confusing.
Really confusing ? this is typo there, resnet-> "present"
/*
* If we have an ITE, then we need to check whether the sid of the ITE
* is in the rbtree (meaning it is probed and not released), and that
* the PCI device is present.
*/
My comment is slightly shorter but I think it has the necessary
information.
+ if (ite_sid) {
+ dev = device_rbtree_find(iommu, ite_sid);
+ if (!dev || !dev_is_pci(dev))
+ return -ETIMEDOUT;
-ETIMEDOUT is weird. The callers don't care which error code we return.
Change this to -ENODEV or something
-ETIMEDOUT means prior ATS invalidation request hit timeout fault, and the
caller really cares about the returned value.
+ pdev = to_pci_dev(dev);
+ if (!pci_device_is_present(pdev) &&
+ ite_sid == pci_dev_id(pci_physfn(pdev)))
The && confused me, but then I realized that probably "ite_sid ==
pci_dev_id(pci_physfn(pdev))" is always true. Can we delete that part?
Here is the fault handling, just double confirm nothing else goes wrong --
beyond the assumption.
pdev = to_pci_dev(dev);
if (!pci_device_is_present(pdev))
return -ENODEV;
+ return -ETIMEDOUT;
-ENODEV.
The ATS invalidation request could be sent from userland in later code,
the userland code will care about the returned value, -ENODEV is one aspect
of the fact (target device not present), while -ETIMEDOUT is another
(timeout happened). we couldn't return them both.
+ }
if (qi->desc_status[wait_index] == QI_ABORT)
return -EAGAIN;
}
Sorry, again for nit picking a v13 patch. I'm not a domain expert but
this patchset seems reasonable to me.
Though this is the v13, it is based on new rbtree code, you are welcome.
Thanks,
Ethan
regards,
dan carpenter
