Re: [PATCH rc v7 3/6] iommu: Fix pasid attach in pci_dev_reset_iommu_prepare/done()

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

On 4/19/26 07:41, Nicolin Chen wrote:
> Now the helpers handle per-gdev resets. So use the per-device API properly
> to attach/detach PASIDs. Also add max_pasids check as other callers.
> 
> Fixes: c279e83953d9 ("iommu: Introduce pci_dev_reset_iommu_prepare/done()")
> Cc:stable@vger.kernel.org
> Reported-by: Shuai Xue<xueshuai@linux.alibaba.com>
> Closes:https://lore.kernel.org/all/ad858513-09fc-455e-bbc5- 
> fe38a225cc78@linux.alibaba.com/
> Reviewed-by: Shuai Xue<xueshuai@linux.alibaba.com>
> Reviewed-by: Jason Gunthorpe<jgg@nvidia.com>
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Nicolin Chen<nicolinc@nvidia.com>
> ---
>   drivers/iommu/iommu.c | 25 ++++++++++++++++++-------
>   1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index d5d9102b9d750..e9ffa562b614f 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -4026,9 +4026,14 @@ int pci_dev_reset_iommu_prepare(struct pci_dev *pdev)
>   	 * The pasid_array is mostly fenced by group->mutex, except one reader
>   	 * in iommu_attach_handle_get(), so it's safe to read without xa_lock.
>   	 */
> -	xa_for_each_start(&group->pasid_array, pasid, entry, 1)
> -		iommu_remove_dev_pasid(&pdev->dev, pasid,
> -				       pasid_array_entry_to_domain(entry));
> +	if (pdev->dev.iommu->max_pasids > 0) {
> +		xa_for_each_start(&group->pasid_array, pasid, entry, 1) {
> +			struct iommu_domain *pasid_dom =
> +				pasid_array_entry_to_domain(entry);
> +
> +			iommu_remove_dev_pasid(&pdev->dev, pasid, pasid_dom);
> +		}
> +	}
>   
>   	group->recovery_cnt++;
>   	return ret;
> @@ -4090,10 +4095,16 @@ void pci_dev_reset_iommu_done(struct pci_dev *pdev)
>   	 * The pasid_array is mostly fenced by group->mutex, except one reader
>   	 * in iommu_attach_handle_get(), so it's safe to read without xa_lock.
>   	 */
> -	xa_for_each_start(&group->pasid_array, pasid, entry, 1)
> -		WARN_ON(__iommu_set_group_pasid(
> -			pasid_array_entry_to_domain(entry), group, pasid,
> -			group->blocking_domain));
> +	if (pdev->dev.iommu->max_pasids > 0) {
> +		xa_for_each_start(&group->pasid_array, pasid, entry, 1) {
> +			struct iommu_domain *pasid_dom =
> +				pasid_array_entry_to_domain(entry);
> +
> +			WARN_ON(pasid_dom->ops->set_dev_pasid(
> +				pasid_dom, &pdev->dev, pasid,
> +				group->blocking_domain));
> +		}
> +	}
>   
>   	if (!WARN_ON(group->recovery_cnt == 0))
>   		group->recovery_cnt--;

I don’t think this patch fixes a real-world issue though it has the
"Fixes:" tag. Logically, if pdev->dev.iommu->max_pasids == 0, there
should be no domains attached to any pasid. Consequently,
group->pasid_array should be empty, and the xa_for_each loop is just a
no-op.

I understand this was a result of Sashko's review comments, but I don't
think it worth a formal fix. I admit I might be overlooking something,
so please correct me if I am wrong.

Thanks,
baolu