Messages in this thread | | | Date | Thu, 11 Jan 2024 10:24:06 +0800 | Subject | Re: [RFC PATCH v10 4/5] iommu/vt-d: don't issue ATS Invalidation request when device is disconnected | From | Baolu Lu <> |
| |
On 1/10/24 4:37 PM, Ethan Zhao wrote: > > On 1/10/2024 1:24 PM, Baolu Lu wrote: >> On 12/29/23 1:05 AM, Ethan Zhao wrote: >>> Except those aggressive hotplug cases - surprise remove a hotplug device >>> while its safe removal is requested and handled in process by: >>> >>> 1. pull it out directly. >>> 2. turn off its power. >>> 3. bring the link down. >>> 4. just died there that moment. >>> >>> etc, in a word, 'gone' or 'disconnected'. >>> >>> Mostly are regular normal safe removal and surprise removal unplug. >>> these hot unplug handling process could be optimized for fix the ATS >>> Invalidation hang issue by calling pci_dev_is_disconnected() in function >>> devtlb_invalidation_with_pasid() to check target device state to avoid >>> sending meaningless ATS Invalidation request to iommu when device is >>> gone. >>> (see IMPLEMENTATION NOTE in PCIe spec r6.1 section 10.3.1) >>> >>> For safe removal, device wouldn't be removed untill the whole software >>> handling process is done, it wouldn't trigger the hard lock up issue >>> caused by too long ATS Invalidation timeout wait. In safe removal path, >>> device state isn't set to pci_channel_io_perm_failure in >>> pciehp_unconfigure_device() by checking 'presence' parameter, calling >>> pci_dev_is_disconnected() in devtlb_invalidation_with_pasid() will >>> return >>> false there, wouldn't break the function. >>> >>> For surprise removal, device state is set to >>> pci_channel_io_perm_failure in >>> pciehp_unconfigure_device(), means device is already gone (disconnected) >>> call pci_dev_is_disconnected() in devtlb_invalidation_with_pasid() will >>> return true to break the function not to send ATS Invalidation >>> request to >>> the disconnected device blindly, thus avoid the further long time >>> waiting >>> triggers the hard lockup. >>> >>> safe removal & surprise removal >>> >>> pciehp_ist() >>> pciehp_handle_presence_or_link_change() >>> pciehp_disable_slot() >>> remove_board() >>> pciehp_unconfigure_device(presence) >>> >>> Tested-by: Haorong Ye <yehaorong@bytedance.com> >>> Signed-off-by: Ethan Zhao <haifeng.zhao@linux.intel.com> >>> --- >>> drivers/iommu/intel/pasid.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c >>> index 715943531091..3d5ed27f39ef 100644 >>> --- a/drivers/iommu/intel/pasid.c >>> +++ b/drivers/iommu/intel/pasid.c >>> @@ -480,6 +480,8 @@ devtlb_invalidation_with_pasid(struct intel_iommu >>> *iommu, >>> if (!info || !info->ats_enabled) >>> return; >>> + if (pci_dev_is_disconnected(to_pci_dev(dev))) >>> + return; >> >> Why do you need the above after changes in PATCH 2/5? It's unnecessary >> and not complete. We have other places where device TLB invalidation is >> issued, right? > > This one could be regarded as optimization, no need to trapped into rabbit > > hole if we could predict the result. because the bad thing is we don't know > > what response to us in the rabbit hole from third party switch (bridges > will > > feedback timeout to requester as PCIe spec mentioned if the endpoint is > > gone).
The IOMMU hardware has its own timeout mechanism. This timeout might happen if:
1) The link to the endpoint is broken, so the invalidation completion message is lost on the way. 2) The device has a longer timeout value, so the device is still busy with handling the cache invalidation when IOMMU's timeout is triggered.
Here, we are doing the following:
For Case 1, we return -ETIMEDOUT directly. For Case 2, we attempt to retry.
Best regards, baolu
| |