Re: [RFC PATCH v10 4/5] iommu/vt-d: don't issue ATS Invalidation request when device is disconnected

[Baolu Lu <baolu.lu@linux.intel.com>]

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