On 12/25/2023 10:21 AM, Bjorn Helgaas wrote:
On Mon, Dec 25, 2023 at 09:46:26AM +0800, Ethan Zhao wrote:
On 12/25/2023 6:43 AM, Bjorn Helgaas wrote:
On Sun, Dec 24, 2023 at 12:06:55AM -0500, Ethan Zhao wrote:
...
[ 4211.433662] pcieport 0000:17:01.0: pciehp: Slot(108): Link Down
[ 4211.433664] pcieport 0000:17:01.0: pciehp: Slot(108): Card not present
[ 4223.822591] NMI watchdog: Watchdog detected hard LOCKUP on cpu 144
...
[ 4223.822647] Kernel Offset: 0x6400000 from 0xffffffff81000000 (relocation
range: 0xffffffff80000000-0xffffffffbfffffff)
The timestamps don't help understand the problem, so you could remove
them so they aren't a distraction.
Lukas said he see the qi_submit_sync takes up to 12 seconds to trigger the
watchdog.
OK, so the timestamps told us how long the watchdog tolerates. I
don't know how useful that is. I suspect that's not a fixed interval
(probably differs by watchdog and possibly user preference).
Fix it by checking the device's error_state in
devtlb_invalidation_with_pasid() to avoid sending meaningless devTLB flush
request to link down device that is set to pci_channel_io_perm_failure and
then powered off in
A pci_dev_is_disconnected() is racy in this context, so this by itself
doesn't look like a complete "fix".
A quick workaround.
Call it a "quick workaround" then, not a "fix". I'm personally not
usually interested in quick workarounds that are not actually fixes,
but the IOMMU folks would be the ones to decide.
Maybe this is more of an optimization? If patch 4/4 is a real fix (in
the sense that if you disable the watchdog, you would get correct
results after a long timeout), maybe you could reorder the patches so
4/4 comes first, and this one becomes an optimization on top of it? I
Make sense, will reorder them.
haven't worked though the whole path, so I don't know exactly how
these patches work.
pciehp_ist()
pciehp_handle_presence_or_link_change()
pciehp_disable_slot()
remove_board()
pciehp_unconfigure_device()
There are some interesting steps missing here between
pciehp_unconfigure_device() and devtlb_invalidation_with_pasid().
devtlb_invalidation_with_pasid() is Intel-specific. ATS Invalidate
Requests are not Intel-specific, so all IOMMU drivers must have to
deal with the case of an ATS Invalidate Request where we never receive
a corresponding ATS Invalidate Completion. Do other IOMMUs like AMD
and ARM have a similar issue?
So far fix it in Intel vt-d specific path.
If the other IOMMU drivers are vulnerable, I guess they would like to
fix this at the same time and in a similar way if possible.
For SAVE_REMOVAL unplug, link is alive when iommu releases devcie and
issues devTLB invalidate request, wouldn't trigger such issue.
This patch works for all links of SURPPRISE_REMOVAL unplug operations.
It's not completely obvious that a fix that works for the safe removal
case also works for the surprise removal case. Can you briefly
explain why it does?
As I explained to baolu.
For safe_removal, device wouldn't be removed till the whole software
handling process done, so without this fix, it wouldn't trigger the lockup
issue, and in safe_removal path, device state isn't set to
pci_channel_io_perm_failure in pciehp_unconfigure_device() by checking
'presence', patch calling this pci_dev_is_disconnected() will return false
there, wouldn't break the function. so it works.
For suprise_removal, device state is set to pci_channel_io_perm_failure in
pciehp_unconfigure_device(), means device already be in power-off/link-down
/removed state, callpci_dev_is_disconnected() hrere will return true to
break
the function not to send ATS invalidation request anymore, thus avoid the
further long time waiting trigger the hard lockup.
s/safe_removal/safe removal/ (they are not a single word)
s/suprise_removal/surprise removal/ (misspelled, also not a single word)
Do I make it clear enough ?
Needs to be in the commit log, of course.
Okay, append to the commit log.
Thanks,
Ethan
Tested-by: Haorong Ye <yehaorong@xxxxxxxxxxxxx>
Signed-off-by: Ethan Zhao <haifeng.zhao@xxxxxxxxxxxxxxx>
---
drivers/iommu/intel/pasid.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 74e8e4c17e81..7dbee9931eb6 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -481,6 +481,9 @@ devtlb_invalidation_with_pasid(struct intel_iommu *iommu,
if (!info || !info->ats_enabled)
return;
+ if (pci_dev_is_disconnected(to_pci_dev(dev)))
+ return;
+
sid = info->bus << 8 | info->devfn;
qdep = info->ats_qdep;
pfsid = info->pfsid;
This goes on to call qi_submit_sync(), which contains a restart: loop.
I don't know the programming model there, but it looks possible that
qi_submit_sync() and qi_check_fault() might not handle the case of an
unreachable device correctly. There should be a way to exit that
restart: loop in cases where the device doesn't respond at all.
Yes, fix it in patch[4/4] to break it out when device is gone.
