Re: [PATCH v5 05/18] iommu: Pass in reset result to pci_dev_reset_iommu_done()

[Baolu Lu]

On 7/3/2026 12:06 PM, Nicolin Chen wrote:
> IOMMU drivers handle ATC cache maintenance. They may encounter ATC-related
> errors (e.g., ATC invalidation timeout), indicating that the ATC cache may
> have stale entries that can corrupt the memory. In this case, IOMMU driver
> has no choice but to block the device's ATS function and wait for a device
> recovery.
>
> The pci_dev_reset_iommu_done() called at the end of a reset function could
> serve as a reliable signal to the IOMMU subsystem that the physical device
> cache is completely clean. However, the function is called unconditionally
> even if the reset operation had actually failed, which would re-attach the
> faulty device back to a normal translation domain. And this will leave the
> system highly exposed, creating vulnerabilities for data corruption:
>      IOMMU blocks RID/ATS
>      pci_reset_function():
>          pci_dev_reset_iommu_prepare(); // Block RID/ATS
>          __reset(); // Failed (ATC is still stale)
>          pci_dev_reset_iommu_done(); // Unblock RID/ATS (ah-ha)
>
> Instead, pass in @reset_result to pci_dev_reset_iommu_done() from callers:
>      IOMMU blocks RID/ATS
>      pci_reset_function():
>          pci_dev_reset_iommu_prepare(); // Block RID/ATS
>          rc = __reset();
>          pci_dev_reset_iommu_done(rc); // Unblock or quarantine
>
> On a successful reset, done() restores the device to its RID/PASID domains
> and decrements group->recovery_cnt. On failure, the device remains blocked,
> and concurrent domain attachment will be rejected until a successful reset.
>
> Note: -ENOTTY is overloaded with different meanings by PCI reset functions.
> Some of them indicate "reset was not attempted", while others indicate "try
> the next reset method and the current method failed". IOMMU that must react
> these two outcomes separately has no choice but to keep the device blocked
> on -ENOTTY as well. Leave an inline FIXME and warning.
>
> This introduces a new situation where a blocked device is being unplugged.
> Decrement the group->recovery_cnt accordingly.
>
> Suggested-by: Kevin Tian<kevin.tian@xxxxxxxxx>
> Signed-off-by: Nicolin Chen<nicolinc@xxxxxxxxxx>
> ---
>   include/linux/iommu.h  |  5 ++--
>   drivers/iommu/iommu.c  | 62 ++++++++++++++++++++++++++++++++++++++++--
>   drivers/pci/pci-acpi.c |  2 +-
>   drivers/pci/pci.c      | 10 +++----
>   drivers/pci/quirks.c   |  2 +-
>   5 files changed, 69 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index d20aa6f6863ab..59ea7e601a2d7 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -1224,7 +1224,7 @@ void iommu_free_global_pasid(ioasid_t pasid);
>   
>   /* PCI device reset functions */
>   int pci_dev_reset_iommu_prepare(struct pci_dev *pdev);
> -void pci_dev_reset_iommu_done(struct pci_dev *pdev);
> +void pci_dev_reset_iommu_done(struct pci_dev *pdev, int reset_result);
>   #else /* CONFIG_IOMMU_API */
>   
>   struct iommu_ops {};
> @@ -1554,7 +1554,8 @@ static inline int pci_dev_reset_iommu_prepare(struct pci_dev *pdev)
>   	return 0;
>   }
>   
> -static inline void pci_dev_reset_iommu_done(struct pci_dev *pdev)
> +static inline void pci_dev_reset_iommu_done(struct pci_dev *pdev,
> +					    int reset_result)
>   {
>   }
>   #endif /* CONFIG_IOMMU_API */
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 342e8a5ad628c..6e2e607de8d8f 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -76,6 +76,7 @@ struct iommu_group {
>   enum gdev_blocked {
>   	BLOCKED_NO = 0, /* Not blocked */
>   	BLOCKED_RESETTING, /* PCI reset in flight */
> +	BLOCKED_RESET_FAILED, /* PCI reset failed */
>   };
>   
>   struct group_device {
> @@ -762,6 +763,9 @@ static void __iommu_group_remove_device(struct device *dev)
>   		if (device->dev != dev)
>   			continue;
>   
> +		/* Must drop the recovery_cnt when removing a blocked device */
> +		if (device->blocked && !WARN_ON(group->recovery_cnt == 0))
> +			group->recovery_cnt--;

I feel that the change above is unrelated to the purpose of this patch.
It looks like an independent fix that belongs to a separate patch.

Otherwise, this looks good to me.

Reviewed-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>