Received-SPF: pass (google.com: domain of linux-kernel+bounces-21721-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <8e16ff9b-4dcd-4bec-a78b-61d90205841f@linux.intel.com>
Date: Wed, 10 Jan 2024 13:24:11 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Cc: baolu.lu@linux.intel.com, linux-pci@vger.kernel.org,
 iommu@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v10 4/5] iommu/vt-d: don't issue ATS Invalidation
 request when device is disconnected
Content-Language: en-US
To: Ethan Zhao <haifeng.zhao@linux.intel.com>, kevin.tian@intel.com,
 bhelgaas@google.com, dwmw2@infradead.org, will@kernel.org,
 robin.murphy@arm.com, lukas@wunner.de
References: <20231228170504.720794-1-haifeng.zhao@linux.intel.com>
 <20231228170504.720794-2-haifeng.zhao@linux.intel.com>
From: Baolu Lu <baolu.lu@linux.intel.com>
In-Reply-To: <20231228170504.720794-2-haifeng.zhao@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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?

>   	/*
>   	 * 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

Best regards,
baolu