Re: [PATCH rc v2] iommu/arm-smmu-v3: Fix inconsistent ATS state tracking

[Pranjal Shrivastava <praan@google.com>]

On Mon, May 04, 2026 at 11:01:42AM -0700, Nicolin Chen wrote:
> On Mon, May 04, 2026 at 04:38:42PM +0000, Pranjal Shrivastava wrote:
> > arm_smmu_enable_ats() ignores the return value of pci_enable_ats(). If
> > pci_enable_ats() fails, the driver still updates its internal state
> > master->ats_enabled to true in arm_smmu_attach_commit().
> > 
> > This leads to a state mismatch between the SMMU driver and the PCI core,
> > the SMMU driver operates assuming ATS is enabled. Later, when detaching
> > the device the driver callspci_disable_ats() because it believes ATS is
> 
> Missing space: "calls pci_disable_ats()"

Ack. I'll fix the typo.

> 
> > The issue was exposed under heavy load when running a VFIO-based DMA map
> > stress test: iova_stress [1]
> 
> I wonder what's the real reason for pci_enable_ats() to fail:
> 

Yes, It's the dev->is_virtfn case (the one you suspect below)

> int pci_enable_ats(struct pci_dev *dev, int ps)
> {
>         u16 ctrl;
>         struct pci_dev *pdev;
> 
>         if (!pci_ats_supported(dev))
>                 return -EINVAL;		// unlikely
> 
>         if (WARN_ON(dev->ats_enabled))
>                 return -EBUSY;		// unlikely
> 
>         if (ps < PCI_ATS_MIN_STU)
>                 return -EINVAL;		// unlikely
> 
>         /*
>          * Note that enabling ATS on a VF fails unless it's already enabled
>          * with the same STU on the PF.
>          */
>         ctrl = PCI_ATS_CTRL_ENABLE;
>         if (dev->is_virtfn) {
>                 pdev = pci_physfn(dev);
>                 if (pdev->ats_stu != ps)
>                         return -EINVAL;		// maybe this one?
>         } else {
>                 dev->ats_stu = ps;
>                 ctrl |= PCI_ATS_CTRL_STU(dev->ats_stu - PCI_ATS_MIN_STU);
>         }
>         pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
> 
>         dev->ats_enabled = 1;
>         return 0;
> }
> EXPORT_SYMBOL_GPL(pci_enable_ats);
> 
> > @@ -3051,8 +3051,9 @@ static bool arm_smmu_ats_supported(struct arm_smmu_master *master)
> >  	return dev_is_pci(dev) && pci_ats_supported(to_pci_dev(dev));
> >  }
> >  
> > -static void arm_smmu_enable_ats(struct arm_smmu_master *master)
> > +static int arm_smmu_enable_ats(struct arm_smmu_master *master)
> >  {
> > +	int ret = 0;
> 
> Seems no need to set to 0.
> 

Ack.

> > @@ -3635,7 +3639,8 @@ void arm_smmu_attach_commit(struct arm_smmu_attach_state *state)
> >  	arm_smmu_attach_commit_vmaster(state);
> >  
> >  	if (state->ats_enabled && !master->ats_enabled) {
> > -		arm_smmu_enable_ats(master);
> > +		if (arm_smmu_enable_ats(master))
> > +			state->ats_enabled = false;
> 
> This alone isn't sufficient.
> 
> First, prepare() does:
> 	if (state->ats_enabled)
> 		atomic_inc(&smmu_domain->nr_ats_masters);
> So, unsetting state->ats_enabled would need to balance that:
> 	atomic_dec(&smmu_domain->nr_ats_masters);
> 
> Then, arm_smmu_master_build_invs() adds ATS invalidation entry to
> domain->invs during prepare(), so a per-domain invalidation would
> still send ATC_INV, which is probably ok for the PCI device, IIRC.
> 

Ahh yes, I forgot about invs array here! Nice catch!

> But the device's ATS entry would not be removed from domain->invs
> during detachment since master->ats_enabled is reverted here, which
> would be a memory leak. And reverting that in domain->invs could be
> a bit painful to do in commit().
> 

Hmm.. because we set the ats state in prepare() but turn on ats in
commit.. (no state-change races here due to the asid lock though)

> I am thinking, maybe the call sites of pci_enable/disable_ats() can
> check to_pci_dev(dev)->ats_enabled instead of master->ats_enabled?
> 
> Then, we keep master->ats_enabled as-is, so detach() can revert the
> nr_ats_masters and ATS invalidation entry in domain->invs.

My suggestion in that case would be to update arm_smmu_ats_supported to
check if the client is a VF and check if it's PF enables ATS:

static bool arm_smmu_ats_supported(struct arm_smmu_master *master)
{
    struct device *dev = master->dev;
    struct pci_dev *pdev;

    if (!dev_is_pci(dev))
        return false;

    pdev = to_pci_dev(dev);
    if (!pci_ats_supported(pdev))
        return false;

    /*
     * If this is a VF, ATS only works if the PF has already enabled it
     * with a valid STU.
     */
    if (pdev->is_virtfn && !pci_physfn(pdev)->ats_enabled)
        return false;

    return true;
}

and then in attach_prepare we can add the ats check for safety:

        if (!state->ats_enabled && master->ats_enabled) {
                pci_disable_ats(to_pci_dev(master->dev));

+               if (pdev->ats_enabled)
+                       pci_disable_ats(to_pci_dev(master->dev));

	/* Rest of the condition */

	}

WDYT?

Thanks,
Praan