Re: [PATCH rc v5] iommu: Fix nested pci_dev_reset_iommu_prepare/done()

[Nicolin Chen]

On Tue, Apr 07, 2026 at 02:36:21PM +0800, Shuai Xue wrote:
> On 4/4/26 1:02 PM, Nicolin Chen wrote:
> > @@ -3987,7 +4017,11 @@ int pci_dev_reset_iommu_prepare(struct pci_dev *pdev)
> >   		iommu_remove_dev_pasid(&pdev->dev, pasid,
> >   				       pasid_array_entry_to_domain(entry));
> 
> The max_pasids check is indeed missing in
> pci_dev_reset_iommu_prepare(). All other callers of
> iommu_remove_dev_pasid() guard it with a max_pasids > 0 check:
> 
> - __iommu_remove_group_pasid() checks device->dev->iommu->max_pasids > 0
> - __iommu_set_group_pasid() error rollback path also checks it
> 
> But pci_dev_reset_iommu_prepare() calls iommu_remove_dev_pasid()

I think you mean pci_dev_reset_iommu_done().

> unconditionally while iterating pasid_array. Since pasid_array is
> per-group, in a multi-device group where only some devices support
> PASIDs, this could call the driver's set_dev_pasid on a device that
> doesn't support PASIDs.
> 
> Note that this is a pre-existing issue in the
> original code before this patch, not a regression introduced by this
> fix.

That's a good finding. I will add the condition.

> > @@ -4030,6 +4070,7 @@ void pci_dev_reset_iommu_done(struct pci_dev *pdev)
> >   		WARN_ON(__iommu_attach_device(group->domain, &pdev->dev,
> >   					      group->blocking_domain));
> >   	}
> > +	gdev->blocked = false;
> 
> 
> One question about the placement of gdev->blocked = false in done(). In
> the original code, group->resetting_domain is cleared at the very end of
> done(), after both RID re-attach and PASID restore:
> 
> __iommu_attach_device(group->domain, ...);   // RID re-attach
> __iommu_set_group_pasid(...);                 // PASID restore
> group->resetting_domain = NULL;               // cleared last
> 
> So iommu_driver_get_domain_for_dev() returns blocking_domain throughout
> the entire restore sequence.In v5, gdev->blocked is cleared between RID
> re-attach and PASID restore:
> 
> __iommu_attach_device(group->domain, ...);   // blocked=true
> gdev->blocked = false;                        // cleared here
> set_dev_pasid(...);                           // blocked=false
> 
> This means during PASID restore, iommu_driver_get_domain_for_dev() now
> returns group->domain instead of blocking_domain — a behavioral change
> from the original code.
> 
> I traced through the ARM SMMUv3 path: arm_smmu_set_pasid() →
> arm_smmu_update_ste(), and the early return at:
> 
> if (master->cd_table.in_ste && master->ste_ats_enabled == ats_enabled)
>     return;
> 
> is always taken during PASID restore (since RID re-attach already
> installed the CD table and ATS state is consistent), so sid_domain is
> effectively unused. No functional impact on SMMUv3.
> 
> Just want to confirm: is this behavioral change deliberate? Or should
> blocked stay true until after PASID restore to preserve the original
> behavior?

I think it should be updated next to the RID domain change, because
iommu_driver_get_domain_for_dev() must reflect the actual domain in
the STE. I will add some comments in prepare() and done().

You are right that it's status quo for SMMUv3 driver for now in the
done() path. But arm_smmu_blocking_set_dev_pasid() in the prepare()
path checks sid_domain as well.

Thanks
Nicolin