Re: [PATCH v12 5/5] iommu/vt-d: improve ITE fault handling if target device isn't present

From: Baolu Lu <baolu.lu@linux.intel.com>
To: Jason Gunthorpe <jgg@ziepe.ca>,
	Ethan Zhao <haifeng.zhao@linux.intel.com>
Cc: baolu.lu@linux.intel.com, "Tian, Kevin" <kevin.tian@intel.com>,
	"Liu, Yi L" <yi.l.liu@intel.com>,
	"bhelgaas@google.com" <bhelgaas@google.com>,
	"robin.murphy@arm.com" <robin.murphy@arm.com>,
	"dwmw2@infradead.org" <dwmw2@infradead.org>,
	"will@kernel.org" <will@kernel.org>,
	"lukas@wunner.de" <lukas@wunner.de>,
	"iommu@lists.linux.dev" <iommu@lists.linux.dev>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>
Subject: Re: [PATCH v12 5/5] iommu/vt-d: improve ITE fault handling if target device isn't present
Date: Wed, 31 Jan 2024 14:21:20 +0800	[thread overview]
Message-ID: <6a48f023-2701-4f2f-8077-14fe348794dd@linux.intel.com> (raw)
In-Reply-To: <20240130162958.GF50608@ziepe.ca>

On 2024/1/31 0:29, Jason Gunthorpe wrote:
> On Tue, Jan 30, 2024 at 04:15:33PM +0800, Ethan Zhao wrote:
>> Some tricky situations:
>>
>> 1. The ATS invalidation request is issued from driver driver, while it is
>> in handling, device is removed. this momment, the device instance still
>> exists in the bus list. yes, if searching it by BDF, could get it.
>>
>> 2. The ATS invalidation request is issued from iommu_bus_notifier()
>> for surprise removal reason, as shown in above calltrace, device was
>> already removed from bus list. if searching it by BDF, return NULL.
>>
>> 3. The ATS invlidation request is issued from iommu_bus_notifier()
>> for safe removal, when is in handling, device is removed or link
>> is down. also as #2, device was already removed from bus list.
>> if searching it by BDF. got NULL.
>> ...
>>
>> so, searching device by BDF, only works for the ATS invalidation
>> request is from device driver.
> In the good path, where the hot removal is expected and this is about
> coordinating, the IOMMU driver should do an orderly shutdown of the
> ATS mechanism:
> 
>   1 Write to PCI config space to disable the ATS
>   2 Make the IOMMU respond to ATS requests with UR and set it to BLOCKED
>   3 Issue a flush of the ATC
>   4 Wait for all outstanding ATC flushes to complete
> 
> If it is a bad/surprise path where the device is already gone then:
> 
>   1 should automatically not do anything, possibly timing out
>   2 must succeed
>   3 should time out
>   4 should "complete" in that the ATC flushes are all timed out
> 
> IMHO all you need to do is not crash/lockup while processing the ATC
> timeouts. If this is a surprise path then the ATC timeout might
> already happened before the iommu driver remove notifier event happens.
> 
> If the driver needs to translate from the IOMMU device table index
> into a struct device it is probably best to do that inside the driver.
> 
> eg ARM maintains a rbtree in the iommu dev data. (see
> arm_smmu_insert_master)

An rbtree for IOMMU device data for the VT-d driver would be beneficial.
It also benefits other paths of fault handling, such as the I/O page
fault handling path, where it currently still relies on the PCI
subsystem to convert a RID value into a pci_device structure.

Given that such an rbtree would be helpful for multiple individual
drivers that handle PCI devices, it seems valuable to implement it in
the core?

Best regards,
baolu