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

[Nicolin Chen <nicolinc@nvidia.com>]

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()"

> 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:

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.

> @@ -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.

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().

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.

Nicolin