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

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

[Pranjal Shrivastava]

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
enabled. This triggers a warning in the PCI core since ATS was never
actually enabled on the device:

[  127.925080] ------------[ cut here ]------------
[  127.925084] WARNING: drivers/pci/ats.c:132 at pci_disable_ats+0x94/0xa8, CPU#42: iova_stress/12240
[  127.949761] Modules linked in: vfat fat dummy bridge stp llc cdc_ncm cdc_eem cdc_ether usbnet mii xhci_pci xhci_hcd ehci_pci ehci_hcd
[  127.961760] CPU: 42 UID: 0 PID: 12240 Comm: iova_stress Not tainted 7.1.0-smp-DEV #4 PREEMPTLAZY
[  127.970619] Hardware name: <REDACTED>
[  127.977655] pstate: 61400009 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
[  127.984603] pc : pci_disable_ats+0x94/0xa8
[  127.988687] lr : arm_smmu_attach_prepare+0x104/0x310
[  127.993641] sp : ffff8000ae013ab0
[  127.996943] x29: ffff8000ae013ac0 x28: ffff0001481b0000 x27: ffff000103d17980
[  128.004066] x26: ffff0001093aad48 x25: ffff0001093ab020 x24: ffff0001118200c8
[  128.011189] x23: ffffcf76ddf1ca1c x22: ffffcf76df679020 x21: ffff000148f1df00
[  128.018311] x20: ffff8000ae013c18 x19: ffff8000ae013b28 x18: 0000000000000000
[  128.025434] x17: 0000000000000000 x16: ffff00010945a358 x15: ffff000110ffd080
[  128.032557] x14: 0000000000000200 x13: 0000000000000002 x12: 0000000000000001
[  128.039679] x11: 0000000000004040 x10: 0000000000000001 x9 : ffffcf76dd37f504
[  128.046801] x8 : 0000000000000000 x7 : 0000000000000000 x6 : 0000000000000000
[  128.053924] x5 : 0000000000000000 x4 : 0000000000000000 x3 : ffff0001093ab000
[  128.061046] x2 : 0000000000000000 x1 : 0000000000000001 x0 : ffff000111820000
[  128.068169] Call trace:
[  128.070603]  pci_disable_ats+0x94/0xa8 (P)
[  128.074688]  arm_smmu_attach_prepare+0x104/0x310
[  128.079292]  arm_smmu_attach_dev_ste+0x128/0x1e0
[  128.083899]  arm_smmu_attach_dev_blocked+0x50/0x88
[  128.088677]  __iommu_attach_device+0x30/0x138
[  128.093026]  __iommu_group_set_domain_internal+0xdc/0x228
[  128.098412]  __iommu_take_dma_ownership+0x118/0x150
[  128.103278]  iommu_group_claim_dma_owner+0x48/0x80
[  128.108056]  vfio_container_attach_group+0xc8/0x1b0
[  128.112927]  vfio_group_fops_unl_ioctl+0x578/0x968
[  128.117706]  __arm64_sys_ioctl+0x90/0xe8
[  128.121622]  invoke_syscall+0x50/0x108
[  128.125366]  el0_svc_common+0x94/0xe8
[  128.129016]  do_el0_svc+0x24/0x38
[  128.132319]  el0_svc+0x44/0xf8
[  128.135364]  el0t_64_sync_handler+0x68/0xe0
[  128.139534]  el0t_64_sync+0x1b0/0x1b8
[  128.143185] ---[ end trace 0000000000000000 ]---

The issue was exposed under heavy load when running a VFIO-based DMA map
stress test: iova_stress [1]

Fix this by propagating the error from arm_smmu_enable_ats() and updating
state->ats_enabled to false on failure. This ensures that
master->ats_enabled correctly reflects the actual state of the device.

Fixes: 7497f4211f4f ("iommu/arm-smmu-v3: Make changing domains be hitless for ATS")
Signed-off-by: Pranjal Shrivastava <praan@google.com>

[1] https://github.com/soleen/iova_stress
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index e8d7dbe495f0..50ede399089d 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -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;
 	size_t stu;
 	struct pci_dev *pdev;
 	struct arm_smmu_device *smmu = master->smmu;
@@ -3065,8 +3066,11 @@ static void arm_smmu_enable_ats(struct arm_smmu_master *master)
 	 * ATC invalidation of PASID 0 causes the entire ATC to be flushed.
 	 */
 	arm_smmu_atc_inv_master(master, IOMMU_NO_PASID);
-	if (pci_enable_ats(pdev, stu))
+	ret = pci_enable_ats(pdev, stu);
+	if (ret)
 		dev_err(master->dev, "Failed to enable ATS (STU %zu)\n", stu);
+
+	return ret;
 }
 
 static int arm_smmu_enable_pasid(struct arm_smmu_master *master)
@@ -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;
 	} else if (state->ats_enabled && master->ats_enabled) {
 		/*
 		 * The translation has changed, flush the ATC. At this point the
-- 
2.54.0.545.g6539524ca2-goog

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

[Nicolin Chen]

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

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

[Pranjal Shrivastava]

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

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

[Pranjal Shrivastava]

On Mon, May 04, 2026 at 07:33:07PM +0000, Pranjal Shrivastava wrote:
> 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:
[...]

Accidentally deleted the minus ( - ) below:

> 
> 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)); <--- this
> 
> +               if (pdev->ats_enabled)
> +                       pci_disable_ats(to_pci_dev(master->dev));
> 
> 	/* Rest of the condition */
> 
> 	}
> 
> WDYT?
> 

Thanks,
Praan

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

[Nicolin Chen]

On Mon, May 04, 2026 at 07:33:07PM +0000, Pranjal Shrivastava wrote:
> 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:
> > 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;
> }

I think that's okay to address the issue for now. But it assumes
that pci_enable_ats() will never change so it won't fail for any
other reason.

> 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));

master->ats_enabled is redundant if we check pdev->ats_enabled.

If we add a gate here, should add to pci_enable_ats() too?

Nicolin

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

[Pranjal Shrivastava]

On Mon, May 04, 2026 at 01:23:06PM -0700, Nicolin Chen wrote:
> On Mon, May 04, 2026 at 07:33:07PM +0000, Pranjal Shrivastava wrote:
> > 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:
> > > 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;
> > }
> 
> I think that's okay to address the issue for now. But it assumes
> that pci_enable_ats() will never change so it won't fail for any
> other reason.
> 

Hmm.. in that case, could we factor out the 

	if (dev->is_virtfn) {
                pdev = pci_physfn(dev);
                if (pdev->ats_stu != ps)
                        return -EINVAL;
        }

part into a helper and export it? I think iommu drivers could use it?

> > 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));
> 
> master->ats_enabled is redundant if we check pdev->ats_enabled.
> 
> If we add a gate here, should add to pci_enable_ats() too?

Um.. I accidentally deleted a ` - ` on that line, it was supposed ot be:

-               pci_disable_ats(to_pci_dev(master->dev));

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

Praan

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

[Pranjal Shrivastava]

On Mon, May 04, 2026 at 01:23:06PM -0700, Nicolin Chen wrote:
> On Mon, May 04, 2026 at 07:33:07PM +0000, Pranjal Shrivastava wrote:
> > 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:
> > > 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;
> > }
> 
> I think that's okay to address the issue for now. But it assumes
> that pci_enable_ats() will never change so it won't fail for any
> other reason.
> 
> > 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));
> 
> master->ats_enabled is redundant if we check pdev->ats_enabled.
> 

BTW, is it really redundant? I assume it represents the driver's state 
machine? I believe the condition:

if (!state->ats_enabled && master->ats_enabled) 

translates to: The current state has ATS active, but the new target
state is to have them inactive. Isn't that the case?


> If we add a gate here, should add to pci_enable_ats() too?
> 

I see that pci_enable_ats already checks for it:

	if (WARN_ON(dev->ats_enabled))
		return -EBUSY;

Praan

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

[Nicolin Chen]

On Mon, May 04, 2026 at 08:29:29PM +0000, Pranjal Shrivastava wrote:
> On Mon, May 04, 2026 at 01:23:06PM -0700, Nicolin Chen wrote:
> > On Mon, May 04, 2026 at 07:33:07PM +0000, Pranjal Shrivastava wrote:
> > > 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:
> > > > 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;
> > > }
> > 
> > I think that's okay to address the issue for now. But it assumes
> > that pci_enable_ats() will never change so it won't fail for any
> > other reason.
> > 
> 
> Hmm.. in that case, could we factor out the 
> 
> 	if (dev->is_virtfn) {
>                 pdev = pci_physfn(dev);
>                 if (pdev->ats_stu != ps)
>                         return -EINVAL;
>         }
> 
> part into a helper and export it? I think iommu drivers could use it?

I think having two identical checks in the same attach_dev() path
isn't that necessary..

Also, stu is __ffs(smmu->pgsize_bitmap). stu should match if both
are set. The implication is still that if pf is ats_enabled.

I prefer your previous version.

Nicolin

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

[Nicolin Chen]

On Mon, May 04, 2026 at 08:40:33PM +0000, Pranjal Shrivastava wrote:
> On Mon, May 04, 2026 at 01:23:06PM -0700, Nicolin Chen wrote:
> > On Mon, May 04, 2026 at 07:33:07PM +0000, Pranjal Shrivastava wrote:
> > > 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:
> > > 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));
> > 
> > master->ats_enabled is redundant if we check pdev->ats_enabled.
> > 
> 
> BTW, is it really redundant? I assume it represents the driver's state 
> machine? I believe the condition:
> 
> if (!state->ats_enabled && master->ats_enabled) 
> 
> translates to: The current state has ATS active, but the new target
> state is to have them inactive. Isn't that the case?

That's fair enough.

We would need an dev_is_pci(), so having master->ats_enabled makes
sense.

> > If we add a gate here, should add to pci_enable_ats() too?
> > 
> 
> I see that pci_enable_ats already checks for it:
> 
> 	if (WARN_ON(dev->ats_enabled))
> 		return -EBUSY;

I see, then we could probably leave that so future corner case can
warn us. IOW, yea, I think gating pci_disable_ats() only is fine.

Thanks
Nicolin

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

[Jason Gunthorpe]

On Mon, May 04, 2026 at 07:33:07PM +0000, Pranjal Shrivastava wrote:

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

Oh.... This is a much larger problem then

The VF should not fail to enable ATS, that is a meaningful bug, I'm
not sure why are are just discovering this?

It looks like the PF unconditionally calls pci_prepare_ats() during
arm_smmu_probe_device(), so how does this happen:

> >         if (dev->is_virtfn) {
> >                 pdev = pci_physfn(dev);
> >                 if (pdev->ats_stu != ps)
> >                         return -EINVAL;		// maybe this one?

??

So, I think the right error handling for a case that shouldn't happen is
to fail the attachment not to try to continue it broken?

Jason

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

[Nicolin Chen]

On Tue, May 05, 2026 at 01:11:26PM -0300, Jason Gunthorpe wrote:
> On Mon, May 04, 2026 at 07:33:07PM +0000, Pranjal Shrivastava wrote:
> 
> > > > 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)
> 
> Oh.... This is a much larger problem then
> 
> The VF should not fail to enable ATS, that is a meaningful bug, I'm
> not sure why are are just discovering this?
> 
> It looks like the PF unconditionally calls pci_prepare_ats() during
> arm_smmu_probe_device(), so how does this happen:
> 
> > >         if (dev->is_virtfn) {
> > >                 pdev = pci_physfn(dev);
> > >                 if (pdev->ats_stu != ps)
> > >                         return -EINVAL;		// maybe this one?
> 
> ??
> 
> So, I think the right error handling for a case that shouldn't happen is
> to fail the attachment not to try to continue it broken?

Inspired by your inputs, I found that pci_prepare_ats() demands in
kdocs:

/**
 * pci_prepare_ats - Setup the PS for ATS
 * @dev: the PCI device
 * @ps: the IOMMU page shift
 *
 * This must be done by the IOMMU driver on the PF before any VFs are created to
 * ensure that the VF can have ATS enabled.

But it returns 0 directly on dev->is_virtfn.

I wonder if this function itself should just fail if dev->is_virtfn
and !pci_physfn(dev)->ats_stu?

Then, arm_smmu_probe_device() must fail if pci_prepare_ats() fails.

Nicolin

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

[Pranjal Shrivastava]

On Tue, May 05, 2026 at 01:11:26PM -0300, Jason Gunthorpe wrote:
> On Mon, May 04, 2026 at 07:33:07PM +0000, Pranjal Shrivastava wrote:
> 
> > > > 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)
> 
> Oh.... This is a much larger problem then
> 
> The VF should not fail to enable ATS, that is a meaningful bug, I'm
> not sure why are are just discovering this?
> 
> It looks like the PF unconditionally calls pci_prepare_ats() during
> arm_smmu_probe_device(), so how does this happen:
> 
> > >         if (dev->is_virtfn) {
> > >                 pdev = pci_physfn(dev);
> > >                 if (pdev->ats_stu != ps)
> > >                         return -EINVAL;		// maybe this one?
> 
> ??
> 
> So, I think the right error handling for a case that shouldn't happen is
> to fail the attachment not to try to continue it broken?
> 

Hmm.. you mean we should fail the VF attach if it's PF's ATS isn't
enabled?

I agree prepare_ats is called regardless on every device and we return
early without setting the STU if (dev->is_virtfn) but the catch is we
never check for pci_prepare_ats's return value in probe_device :/
(The PF might be untrusted or non ATS capable).

Here's the EXACT failing case:

1. arm_smmu_probe_device(PF) -> we call pci_prepare_ats & ignore retval
2. The PF attaches to Identity domain (pci_enable_ats skipped, no logs)
3. We create VFs and try to assign them (new group -> unmanaged domain)
4. We try to enable ATS in attach_commit, fail and see the failure log
5. arm-smmu-v3 driver tracks the state as "ATS enabled" for VF
6. We try to disable ATS which was never enabled and hit the WARN

The catch is in step 1, pci_prepare_ats(PF) returned -EINVAL as either
the ATS capability didn't exist OR the device was untrusted but the
driver let it slide (ignored the retval in probe_device).
I'll debug further to see which one is it (because in that case
arm_smmu_ats_supported should've failed too).

Thanks,
Praan

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

[Pranjal Shrivastava]

On Tue, May 05, 2026 at 01:21:37PM -0700, Nicolin Chen wrote:
> On Tue, May 05, 2026 at 01:11:26PM -0300, Jason Gunthorpe wrote:
> > On Mon, May 04, 2026 at 07:33:07PM +0000, Pranjal Shrivastava wrote:
> > 
> > > > > 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)
> > 
> > Oh.... This is a much larger problem then
> > 
> > The VF should not fail to enable ATS, that is a meaningful bug, I'm
> > not sure why are are just discovering this?
> > 
> > It looks like the PF unconditionally calls pci_prepare_ats() during
> > arm_smmu_probe_device(), so how does this happen:
> > 
> > > >         if (dev->is_virtfn) {
> > > >                 pdev = pci_physfn(dev);
> > > >                 if (pdev->ats_stu != ps)
> > > >                         return -EINVAL;		// maybe this one?
> > 
> > ??
> > 
> > So, I think the right error handling for a case that shouldn't happen is
> > to fail the attachment not to try to continue it broken?
> 
> Inspired by your inputs, I found that pci_prepare_ats() demands in
> kdocs:
> 
> /**
>  * pci_prepare_ats - Setup the PS for ATS
>  * @dev: the PCI device
>  * @ps: the IOMMU page shift
>  *
>  * This must be done by the IOMMU driver on the PF before any VFs are created to
>  * ensure that the VF can have ATS enabled.
> 
> But it returns 0 directly on dev->is_virtfn.
> 
> I wonder if this function itself should just fail if dev->is_virtfn
> and !pci_physfn(dev)->ats_stu?
> 

Just wondering if there's a use-case that might break? Otherwise, in
current context I agree that we should fail if ats_stu == 0 / mis-match

> Then, arm_smmu_probe_device() must fail if pci_prepare_ats() fails.

I don't think probe device should fail, we should just mark that ATS
isn't supported for the function? right?

Thanks,
Praan

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

[Nicolin Chen]

On Tue, May 05, 2026 at 09:23:15PM +0000, Pranjal Shrivastava wrote:
> On Tue, May 05, 2026 at 01:21:37PM -0700, Nicolin Chen wrote:
> > /**
> >  * pci_prepare_ats - Setup the PS for ATS
> >  * @dev: the PCI device
> >  * @ps: the IOMMU page shift
> >  *
> >  * This must be done by the IOMMU driver on the PF before any VFs are created to
> >  * ensure that the VF can have ATS enabled.
> > 
> > But it returns 0 directly on dev->is_virtfn.
> > 
> > I wonder if this function itself should just fail if dev->is_virtfn
> > and !pci_physfn(dev)->ats_stu?
> > 
> 
> Just wondering if there's a use-case that might break?

Well, even a break would be what the kdoc statement demands :-/

> Otherwise, in
> current context I agree that we should fail if ats_stu == 0 / mis-match

Yea, should check mis-match. "== 0" doesn't guarantee the enable
success.


> > Then, arm_smmu_probe_device() must fail if pci_prepare_ats() fails.
> 
> I don't think probe device should fail, we should just mark that ATS
> isn't supported for the function? right?

PF must be probed() first with ats_stu set correctly. If VF gets
probed before that or with a different ats_stu, something must be
wrong.

Again, following the kdoc statement, VF should not be "created"
in this case. Or should we read it differently? :-/

Nicolin

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

[Pranjal Shrivastava]

On Tue, May 05, 2026 at 02:44:27PM -0700, Nicolin Chen wrote:
> On Tue, May 05, 2026 at 09:23:15PM +0000, Pranjal Shrivastava wrote:
> > On Tue, May 05, 2026 at 01:21:37PM -0700, Nicolin Chen wrote:
> > > /**
> > >  * pci_prepare_ats - Setup the PS for ATS
> > >  * @dev: the PCI device
> > >  * @ps: the IOMMU page shift
> > >  *
> > >  * This must be done by the IOMMU driver on the PF before any VFs are created to
> > >  * ensure that the VF can have ATS enabled.
> > > 
> > > But it returns 0 directly on dev->is_virtfn.
> > > 
> > > I wonder if this function itself should just fail if dev->is_virtfn
> > > and !pci_physfn(dev)->ats_stu?
> > > 
> > 
> > Just wondering if there's a use-case that might break?
> 
> Well, even a break would be what the kdoc statement demands :-/
> 

Yea

> > Otherwise, in
> > current context I agree that we should fail if ats_stu == 0 / mis-match
> 
> Yea, should check mis-match. "== 0" doesn't guarantee the enable
> success.
> 
> 
> > > Then, arm_smmu_probe_device() must fail if pci_prepare_ats() fails.
> > 
> > I don't think probe device should fail, we should just mark that ATS
> > isn't supported for the function? right?
> 
> PF must be probed() first with ats_stu set correctly. If VF gets
> probed before that or with a different ats_stu, something must be
> wrong.
> 
> Again, following the kdoc statement, VF should not be "created"
> in this case. Or should we read it differently? :-/
> 

Yea, VFs shouldn't be created without PFs.

Praan

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

[Pranjal Shrivastava]

On Tue, May 05, 2026 at 09:14:03PM +0000, Pranjal Shrivastava wrote:
> On Tue, May 05, 2026 at 01:11:26PM -0300, Jason Gunthorpe wrote:
> > On Mon, May 04, 2026 at 07:33:07PM +0000, Pranjal Shrivastava wrote:
> > 
> > > > > 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)
> > 
> > Oh.... This is a much larger problem then
> > 
> > The VF should not fail to enable ATS, that is a meaningful bug, I'm
> > not sure why are are just discovering this?
> > 
> > It looks like the PF unconditionally calls pci_prepare_ats() during
> > arm_smmu_probe_device(), so how does this happen:
> > 
> > > >         if (dev->is_virtfn) {
> > > >                 pdev = pci_physfn(dev);
> > > >                 if (pdev->ats_stu != ps)
> > > >                         return -EINVAL;		// maybe this one?
> > 
> > ??
> > 
> > So, I think the right error handling for a case that shouldn't happen is
> > to fail the attachment not to try to continue it broken?
> > 
> 
> Hmm.. you mean we should fail the VF attach if it's PF's ATS isn't
> enabled?
> 
> I agree prepare_ats is called regardless on every device and we return
> early without setting the STU if (dev->is_virtfn) but the catch is we
> never check for pci_prepare_ats's return value in probe_device :/
> (The PF might be untrusted or non ATS capable).
> 
> Here's the EXACT failing case:
> 
> 1. arm_smmu_probe_device(PF) -> we call pci_prepare_ats & ignore retval
> 2. The PF attaches to Identity domain (pci_enable_ats skipped, no logs)
> 3. We create VFs and try to assign them (new group -> unmanaged domain)
> 4. We try to enable ATS in attach_commit, fail and see the failure log
> 5. arm-smmu-v3 driver tracks the state as "ATS enabled" for VF
> 6. We try to disable ATS which was never enabled and hit the WARN
> 
> The catch is in step 1, pci_prepare_ats(PF) returned -EINVAL as either
> the ATS capability didn't exist OR the device was untrusted but the
> driver let it slide (ignored the retval in probe_device).
> I'll debug further to see which one is it (because in that case
> arm_smmu_ats_supported should've failed too).
> 

I added some logs to check the reason of failure.
To make the matter worse, I believe I spotted this due to a broken HW.
(Although, we can face something similar with untrusted etc. in the mix)

The PF shows no ATS Cap:

[    0.575749] PCI host bridge to bus 000X:00
[    0.575773] pci 000X:00:00.0: set_pcie_untrusted: no parent bridge, removable=0, external_facing=0
[    0.575774] pci 000X:00:00.0: [abab:cdcd] type 00 class 0x060000 conventional PCI endpoint
[    0.575786] pci 000X:00:00.0: pci_ats_supported: NO ATS CAPABILITY FOUND (ats_cap=0)
[    0.575787] pci 000X:00:00.0: pci_ats_supported: NO ATS CAPABILITY FOUND (ats_cap=0)
[    0.575819] pci 000X:00:01.0: set_pcie_untrusted: no parent bridge, removable=0, external_facing=0
[    0.575821] pci 000X:00:01.0: [abab:cdcd] type 01 class 0x060400 PCIe Root Port
[    2.591975] pci 000X:00:00.0: pci_ats_supported: NO ATS CAPABILITY FOUND (ats_cap=0)
[    2.591979] pci 000X:00:00.0: pci_prepare_ats: ps=12, is_virtfn=0, ats_cap=0x0
[    2.591980] pci 000X:00:00.0: pci_ats_supported: NO ATS CAPABILITY FOUND (ats_cap=0)
[    2.591981] pci 000X:00:00.0: pci_prepare_ats failed: not supported
[    2.592001] pci 000X:00:00.0: Adding to iommu group 19
[    2.592007] pci 000X:00:01.0: pci_ats_supported: NO ATS CAPABILITY FOUND (ats_cap=0)
[    2.592009] pci 000X:00:01.0: pci_prepare_ats: ps=12, is_virtfn=0, ats_cap=0x0

Whereas the VF presents ATS cap:

[   16.803702] pci 000X:02:00.0: [abab:cdcd] type 00 class 0x020000 PCIe Endpoint
[   16.803869] pci 000X:02:00.0: pci_prepare_ats: ps=12, is_virtfn=1, ats_cap=0x1e4
[   16.803871] pci 000X:02:00.0: pci_prepare_ats: device is VF, returning success
[   16.803927] pci 000X:02:00.0: Adding to iommu group <redacted>
[   16.803941] pci 000X:02:00.0: arm_smmu_ats_supported: dev is PCI, ats_cap=0x1e4, untrusted=0, pci_ats_supported=1
[   16.803949] pci 000X:02:00.0: Calling pci_enable_ats(STU 12)
[   16.803950] pci 000X:02:00.0: pci_enable_ats: ps=12, is_virtfn=1, ats_cap=0x1e4
[   16.803951] pci 000X:02:00.0: pci_enable_ats: checking PF (000X:01:00.0) -> ats_cap=0x0, ats_enabled=0, ats_stu=0
[   16.803952] pci 000X:02:00.0: pci_enable_ats failed: STU mismatch (PF STU = 0 vs VF STU = 12)
[   16.803954] pci 000X:02:00.0: Failed to enable ATS (STU 12) error -22
[   16.804168] pci 000X:02:00.0: disabling ATS

which means pci_ats_supported returns true for such broken HW. Ideally,
it should return false for VF if the PF doesn't support ATS!

Currently, we don't *really* enable ATS but I believe pci_ats_supported
should fail in such cases.

Thanks,
Praan

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

[Jason Gunthorpe]

On Tue, May 05, 2026 at 10:32:40PM +0000, Pranjal Shrivastava wrote:
> The PF shows no ATS Cap:

Yeah, that's complete broken and unusable. We should be failing things
not trying to patch around it. quirks should be used to deal with
this non-compliant HW.

I like the idea of prepare ats failing on the VF if the PF isn't setup
right and then either failing probe or just disabling ATS on that
device.

I also like the idea of failing attach when ats enable fails so we
keep a consistency, but it should be a WARN_ON path that shouldn't
happen do the prepare_ats dealing with it.

> which means pci_ats_supported returns true for such broken HW. Ideally,
> it should return false for VF if the PF doesn't support ATS!

That's also a possible option, but I also wonder if ats_supported and
prepare_ats should maybe be merged together? Why does the iommu driver
have to call both? If prepare succeeds then allow ATS otherwise HW
does not support ATS, turn it off ?

Jason

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

[Pranjal Shrivastava]

On Wed, May 06, 2026 at 06:46:30AM -0300, Jason Gunthorpe wrote:
> On Tue, May 05, 2026 at 10:32:40PM +0000, Pranjal Shrivastava wrote:
> > The PF shows no ATS Cap:
> 
> Yeah, that's complete broken and unusable. We should be failing things
> not trying to patch around it. quirks should be used to deal with
> this non-compliant HW.
> 
> I like the idea of prepare ats failing on the VF if the PF isn't setup
> right and then either failing probe or just disabling ATS on that
> device.
> 
> I also like the idea of failing attach when ats enable fails so we
> keep a consistency, but it should be a WARN_ON path that shouldn't
> happen do the prepare_ats dealing with it.
> 

I think failing attach would not be the right thing to do, we should let
it attach without enabling ATS (if that's what the ATS spec mandates).

Even if the HW is sane and PF has disabled ATS, the STU is set to 0.
Since, pci_ats_supported returns true for VFs & prepare_ats passes.
Anyway pci_enable_ats currently doesn't allow enabling ATS if PF's STU=0

The whole problem is that the iommu driver's ATS state tracking goes for
a toss as it depends on pci_ats_supported which returns true for a VF 
even if the PF's ATS is disabled.

> > which means pci_ats_supported returns true for such broken HW. Ideally,
> > it should return false for VF if the PF doesn't support ATS!
> 
> That's also a possible option, but I also wonder if ats_supported and
> prepare_ats should maybe be merged together? Why does the iommu driver
> have to call both? If prepare succeeds then allow ATS otherwise HW
> does not support ATS, turn it off ?
> 

They are combined:

int pci_prepare_ats(struct pci_dev *dev, int ps)
{
	u16 ctrl;

	if (!pci_ats_supported(dev))
		return -EINVAL;

	if (WARN_ON(dev->ats_enabled))
		return -EBUSY;

	if (ps < PCI_ATS_MIN_STU)
		return -EINVAL;

	if (dev->is_virtfn)
		return 0;

	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);
	return 0;
}
EXPORT_SYMBOL_GPL(pci_prepare_ats);


I think it really comes down to one of these three possibilities:

 1. If pci_ats_supported() is strictly intended to signal whether the ATS
    capability is present, then the arm-smmu-v3 driver is at fault & it
    shouldn't rely on that check to track its internal ATS state.

 2. Otherwise, if pci_ats_supported() is meant to indicate if a function
    is actually *allowed* to use ATS, then it's currently incoherent IFF
    the ATS spec mandates ATS to be disabled for a capable VF if it's PF
    disables it. 

    The kdoc for pci_ats_supported() mentions "Returns true if the device
    supports ATS and is allowed to use it, false otherwise." [1]

 3. Finally, if the PCIe spec actually allows VFs to enable ATS even
    when the PF's ATS is disabled (but is ATS capable), then we might
    need to ensure the PF is initialized with a valid STU even when its
    ATS Enable bit is disabled, to allow enable_pci_ats() to pass for VFs

I'll dig into the PCIe ATS spec to confirm regarding PF/VF dependencies.

But from an initial understanding, I believe 3 is the correct way to go.
If a device is ATS capable (i.e. PF & VF both present ATS capability),
We shall allow PFs to attach to Identity domain and disable ATS while
while allowing VFs to enable and use ATS.

LMK your thoughts?

Thanks,
Praan

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

[Samiullah Khawaja]

On Tue, May 05, 2026 at 01:21:37PM -0700, Nicolin Chen wrote:
>On Tue, May 05, 2026 at 01:11:26PM -0300, Jason Gunthorpe wrote:
>> On Mon, May 04, 2026 at 07:33:07PM +0000, Pranjal Shrivastava wrote:
>>
>> > > > 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)
>>
>> Oh.... This is a much larger problem then
>>
>> The VF should not fail to enable ATS, that is a meaningful bug, I'm
>> not sure why are are just discovering this?
>>
>> It looks like the PF unconditionally calls pci_prepare_ats() during
>> arm_smmu_probe_device(), so how does this happen:
>>
>> > >         if (dev->is_virtfn) {
>> > >                 pdev = pci_physfn(dev);
>> > >                 if (pdev->ats_stu != ps)
>> > >                         return -EINVAL;		// maybe this one?
>>
>> ??
>>
>> So, I think the right error handling for a case that shouldn't happen is
>> to fail the attachment not to try to continue it broken?
>
>Inspired by your inputs, I found that pci_prepare_ats() demands in
>kdocs:
>
>/**
> * pci_prepare_ats - Setup the PS for ATS
> * @dev: the PCI device
> * @ps: the IOMMU page shift
> *
> * This must be done by the IOMMU driver on the PF before any VFs are created to
> * ensure that the VF can have ATS enabled.
>
>But it returns 0 directly on dev->is_virtfn.
>
>I wonder if this function itself should just fail if dev->is_virtfn
>and !pci_physfn(dev)->ats_stu?

Yes, I think the existing check where ats_stu mismatching with the stu
being set for VF should be enough?

I am actually surprised that this failing, since arm-smmuv3 does set the
stu by calling prepare_ats for the PF during probe. Is there any other
place where we set PF stu to zero after probe?

So the ATS stu of PF should match with VF stu being set as VFs are
created after PF probe.
>
>Then, arm_smmu_probe_device() must fail if pci_prepare_ats() fails.
>
>Nicolin

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

[Pranjal Shrivastava]

On Wed, May 06, 2026 at 06:46:30AM -0300, Jason Gunthorpe wrote:
> On Tue, May 05, 2026 at 10:32:40PM +0000, Pranjal Shrivastava wrote:
> > The PF shows no ATS Cap:
> 
> Yeah, that's complete broken and unusable. We should be failing things
> not trying to patch around it. quirks should be used to deal with
> this non-compliant HW.
> 

IMPORTANT correction, the HW is NOT broken, I mis-read the logs & pasted
the wrong ones. The BDF logs were supposed to be:

WRONG BDF logs: 000X:00:01.0 
RIGHT BDF logs: 000X:01:00.0

Infact we're seeing an in-tree quirk[1] in action:

PF logs:

[    0.575983] pci 000X:01:00.0: [8086:1452] type 00 class 0x020000 PCIe Endpoint
[    0.576003] pci 000X:01:00.0: BAR 0 [mem 0x640900000000-0x64090fffffff 64bit pref]
[    0.576005] pci 000X:01:00.0: BAR 2 [mem 0x640a6cc00000-0x640a6cc3ffff 64bit pref]
[    0.576089] pci 000X:01:00.0: VF BAR 0 [mem 0x640994800000-0x640994bfffff 64bit pref]
[    0.576091] pci 000X:01:00.0: VF BAR 0 [mem 0x640994800000-0x640a547fffff 64bit pref]: contains BAR 0 for 768 VFs
[    0.576093] pci 000X:01:00.0: VF BAR 2 [mem 0x640a6cc80000-0x640a6cc8ffff 64bit pref]
[    0.576094] pci 000X:01:00.0: VF BAR 2 [mem 0x640a6cc80000-0x640a6fc7ffff 64bit pref]: contains BAR 2 for 768 VFs

[    0.832175] pci 000X:01:00.0: quirk_intel_e2000_no_ats: revision 0x11 < 0x20, disabling ATS 
[    0.832177] pci 000X:01:00.0: quirk_no_ats: SETTING ats_cap TO 0 (was 0x2dc) <--- pdev->ats_cap = 0

[    2.365656] pci 000X:01:00.0: pci_prepare_ats: ps=12, is_virtfn=0, ats_cap=0x0
[    2.365663] pci 000X:01:00.0: pci_prepare_ats: MANUAL SCAN FOUND ATS at 0x2dc (header 0x1000f, ctrl 0x0)
[    2.365666] pci 000X:01:00.0: pci_prepare_ats failed: not supported

The PCI ID : Vendor ID => [8086:1452] is for Intel e2000 NICs which has
a quirk for ATS: quirk_intel_e2000_no_ats[1]

VF logs:
[   16.634000] pci 000X:02:00.0: [8086:145c] type 00 class 0x020000 PCIe Endpoint
[   16.634060] pci 000X:02:00.0: pci_ats_init: pci_find_ext_capability(ATS) returned 0x1e4

[   16.634196] pci 000X:02:00.0: pci_prepare_ats: ps=12, is_virtfn=1, ats_cap=0x1e4
[   16.634201] pci 000X:02:00.0: pci_prepare_ats: MANUAL SCAN FOUND ATS at 0x1e4
[   16.634202] pci 000X:02:00.0: pci_prepare_ats: device is VF, returning success

[   16.634272] pci 000X:02:00.0: arm_smmu_ats_supported: dev is PCI, ats_cap=0x1e4, pci_ats_supported=1

[   16.634282] pci 000X:02:00.0: pci_enable_ats entry: ps=12, is_virtfn=1, ats_cap=0x1e4
[   16.634283] pci 000X:02:00.0: pci_enable_ats (VF): checking PF (0005:01:00.0) -> ats_cap=0x0, ats_enabled=0, ats_stu=0
[   16.634285] pci 000X:02:00.0: pci_enable_ats EXIT: STU mismatch (PF 0 vs VF 12)
[   16.634286] pci 000X:02:00.0: Failed to enable ATS (STU 12) error -22

[   16.634513] pci 000X:02:00.0: quirk_intel_e2000_no_ats: revision 0x11 < 0x20, disabling ATS
[   16.634515] pci 000X:02:00.0: quirk_no_ats: SETTING ats_cap TO 0 (was 0x1e4)

The PCI ID : Vendor ID => [8086:145c] also has the same: quirk_intel_e2000_no_ats[1]

Thus, this sequence occurs:

1. The Intel quirk sets pdev->ats_cap = 0 for the PF at boot.
2. pci_prepare_ats(PF) fails during arm_smmu_probe_device() as the
   quirk causes pci_ats_supported to return false & PF->ats_stu
   remains 0. (arm-smmu-v3 ignores the failure)

3. PF attaches to identity domain: pci_enable_ats is skipped
4. VF's arm_smmu_probe_device returns early from pci_prepare_ats() due 
   to the if (dev->is_virtfn) return 0; check

5. SMMU calls pci_ats_supported() for the VF in attach_prepare() which
   returns true as the quirk hasn't been called for the VF yet. 
   The SMMU driver sets state->ats_enabled = true

6. We call pci_enable_ats which hits the STU mismatch (PF never sets it)
7. Later while disabling ATS for VF via pci_disable_ats, we hit the WARN

I guess we must think about the following:

1. Check return values of pci_enable_ats / pci_prepare_ats in SMMUv3
2. PCIe core to provide a proper ATS flag that iommu driver can rely on
   and maybe define a policy around pci_ats_supported() to make sure it 
   also handles such quirks in pci_ats_supported().

LMK, your thoughts?

Thanks!
Praan

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

[Pranjal Shrivastava]

On Wed, May 06, 2026 at 08:19:43PM +0000, Pranjal Shrivastava wrote:
> On Wed, May 06, 2026 at 06:46:30AM -0300, Jason Gunthorpe wrote:
> > On Tue, May 05, 2026 at 10:32:40PM +0000, Pranjal Shrivastava wrote:
> > > The PF shows no ATS Cap:
> > 
> 
>  3. Finally, if the PCIe spec actually allows VFs to enable ATS even
>     when the PF's ATS is disabled (but is ATS capable), then we might
>     need to ensure the PF is initialized with a valid STU even when its
>     ATS Enable bit is disabled, to allow enable_pci_ats() to pass for VFs
> 

Correction: This is what happens currently in the happy path, the STU is
set and we only clear the enable bit in the ATS control reg in 
pci_ats_disable.

> I'll dig into the PCIe ATS spec to confirm regarding PF/VF dependencies.
> 
> But from an initial understanding, I believe 3 is the correct way to go.
> If a device is ATS capable (i.e. PF & VF both present ATS capability),
> We shall allow PFs to attach to Identity domain and disable ATS while
> while allowing VFs to enable and use ATS.

The kernel seems to allow this today, during PF probe the PF's STU is
set regardless of it's ATS being enabled. Thus, a VF can use ATS even if
it's PF disables ATS. (Given that the ATS capability is presented
uniformly in both).

We are seeing a weird case due to a pci_ats_quirk coming into action.

Thanks
Praan

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

[Pranjal Shrivastava]

On Wed, May 06, 2026 at 09:57:55PM +0000, Pranjal Shrivastava wrote:
> On Wed, May 06, 2026 at 06:46:30AM -0300, Jason Gunthorpe wrote:
> > On Tue, May 05, 2026 at 10:32:40PM +0000, Pranjal Shrivastava wrote:
> > > The PF shows no ATS Cap:
> > 
> > Yeah, that's complete broken and unusable. We should be failing things
> > not trying to patch around it. quirks should be used to deal with
> > this non-compliant HW.
> > 
> 
> IMPORTANT correction, the HW is NOT broken, I mis-read the logs & pasted
> the wrong ones. The BDF logs were supposed to be:
> 
> WRONG BDF logs: 000X:00:01.0 
> RIGHT BDF logs: 000X:01:00.0
> 
> Infact we're seeing an in-tree quirk[1] in action:
> 
> PF logs:
> 
> [    0.575983] pci 000X:01:00.0: [8086:1452] type 00 class 0x020000 PCIe Endpoint
> [    0.576003] pci 000X:01:00.0: BAR 0 [mem 0x640900000000-0x64090fffffff 64bit pref]
> [    0.576005] pci 000X:01:00.0: BAR 2 [mem 0x640a6cc00000-0x640a6cc3ffff 64bit pref]
> [    0.576089] pci 000X:01:00.0: VF BAR 0 [mem 0x640994800000-0x640994bfffff 64bit pref]
> [    0.576091] pci 000X:01:00.0: VF BAR 0 [mem 0x640994800000-0x640a547fffff 64bit pref]: contains BAR 0 for 768 VFs
> [    0.576093] pci 000X:01:00.0: VF BAR 2 [mem 0x640a6cc80000-0x640a6cc8ffff 64bit pref]
> [    0.576094] pci 000X:01:00.0: VF BAR 2 [mem 0x640a6cc80000-0x640a6fc7ffff 64bit pref]: contains BAR 2 for 768 VFs
> 
> [    0.832175] pci 000X:01:00.0: quirk_intel_e2000_no_ats: revision 0x11 < 0x20, disabling ATS 
> [    0.832177] pci 000X:01:00.0: quirk_no_ats: SETTING ats_cap TO 0 (was 0x2dc) <--- pdev->ats_cap = 0
> 
> [    2.365656] pci 000X:01:00.0: pci_prepare_ats: ps=12, is_virtfn=0, ats_cap=0x0
> [    2.365663] pci 000X:01:00.0: pci_prepare_ats: MANUAL SCAN FOUND ATS at 0x2dc (header 0x1000f, ctrl 0x0)
> [    2.365666] pci 000X:01:00.0: pci_prepare_ats failed: not supported
> 
> The PCI ID : Vendor ID => [8086:1452] is for Intel e2000 NICs which has
> a quirk for ATS: quirk_intel_e2000_no_ats[1]
> 
> VF logs:
> [   16.634000] pci 000X:02:00.0: [8086:145c] type 00 class 0x020000 PCIe Endpoint
> [   16.634060] pci 000X:02:00.0: pci_ats_init: pci_find_ext_capability(ATS) returned 0x1e4
> 
> [   16.634196] pci 000X:02:00.0: pci_prepare_ats: ps=12, is_virtfn=1, ats_cap=0x1e4
> [   16.634201] pci 000X:02:00.0: pci_prepare_ats: MANUAL SCAN FOUND ATS at 0x1e4
> [   16.634202] pci 000X:02:00.0: pci_prepare_ats: device is VF, returning success
> 
> [   16.634272] pci 000X:02:00.0: arm_smmu_ats_supported: dev is PCI, ats_cap=0x1e4, pci_ats_supported=1
> 
> [   16.634282] pci 000X:02:00.0: pci_enable_ats entry: ps=12, is_virtfn=1, ats_cap=0x1e4
> [   16.634283] pci 000X:02:00.0: pci_enable_ats (VF): checking PF (0005:01:00.0) -> ats_cap=0x0, ats_enabled=0, ats_stu=0
> [   16.634285] pci 000X:02:00.0: pci_enable_ats EXIT: STU mismatch (PF 0 vs VF 12)
> [   16.634286] pci 000X:02:00.0: Failed to enable ATS (STU 12) error -22
> 
> [   16.634513] pci 000X:02:00.0: quirk_intel_e2000_no_ats: revision 0x11 < 0x20, disabling ATS
> [   16.634515] pci 000X:02:00.0: quirk_no_ats: SETTING ats_cap TO 0 (was 0x1e4)
> 
> The PCI ID : Vendor ID => [8086:145c] also has the same: quirk_intel_e2000_no_ats[1]
> 
> Thus, this sequence occurs:
> 
> 1. The Intel quirk sets pdev->ats_cap = 0 for the PF at boot.
> 2. pci_prepare_ats(PF) fails during arm_smmu_probe_device() as the
>    quirk causes pci_ats_supported to return false & PF->ats_stu
>    remains 0. (arm-smmu-v3 ignores the failure)
> 
> 3. PF attaches to identity domain: pci_enable_ats is skipped
> 4. VF's arm_smmu_probe_device returns early from pci_prepare_ats() due 
>    to the if (dev->is_virtfn) return 0; check
> 
> 5. SMMU calls pci_ats_supported() for the VF in attach_prepare() which
>    returns true as the quirk hasn't been called for the VF yet. 
>    The SMMU driver sets state->ats_enabled = true
> 
> 6. We call pci_enable_ats which hits the STU mismatch (PF never sets it)
> 7. Later while disabling ATS for VF via pci_disable_ats, we hit the WARN
> 
> I guess we must think about the following:
> 
> 1. Check return values of pci_enable_ats / pci_prepare_ats in SMMUv3
> 2. PCIe core to provide a proper ATS flag that iommu driver can rely on
>    and maybe define a policy around pci_ats_supported() to make sure it 
>    also handles such quirks in pci_ats_supported().
> 
> LMK, your thoughts?
> 
> Thanks!
> Praan

[1] https://elixir.bootlin.com/linux/v7.0.1/source/drivers/pci/quirks.c#L5703

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

[Samiullah Khawaja]

On Wed, May 06, 2026 at 09:57:55PM +0000, Pranjal Shrivastava wrote:
>On Wed, May 06, 2026 at 06:46:30AM -0300, Jason Gunthorpe wrote:
>> On Tue, May 05, 2026 at 10:32:40PM +0000, Pranjal Shrivastava wrote:
>> > The PF shows no ATS Cap:
>>
>> Yeah, that's complete broken and unusable. We should be failing things
>> not trying to patch around it. quirks should be used to deal with
>> this non-compliant HW.
>>
>
>IMPORTANT correction, the HW is NOT broken, I mis-read the logs & pasted
>the wrong ones. The BDF logs were supposed to be:
>
>WRONG BDF logs: 000X:00:01.0
>RIGHT BDF logs: 000X:01:00.0
>
>Infact we're seeing an in-tree quirk[1] in action:
>
>PF logs:
>
>[    0.575983] pci 000X:01:00.0: [8086:1452] type 00 class 0x020000 PCIe Endpoint
>[    0.576003] pci 000X:01:00.0: BAR 0 [mem 0x640900000000-0x64090fffffff 64bit pref]
>[    0.576005] pci 000X:01:00.0: BAR 2 [mem 0x640a6cc00000-0x640a6cc3ffff 64bit pref]
>[    0.576089] pci 000X:01:00.0: VF BAR 0 [mem 0x640994800000-0x640994bfffff 64bit pref]
>[    0.576091] pci 000X:01:00.0: VF BAR 0 [mem 0x640994800000-0x640a547fffff 64bit pref]: contains BAR 0 for 768 VFs
>[    0.576093] pci 000X:01:00.0: VF BAR 2 [mem 0x640a6cc80000-0x640a6cc8ffff 64bit pref]
>[    0.576094] pci 000X:01:00.0: VF BAR 2 [mem 0x640a6cc80000-0x640a6fc7ffff 64bit pref]: contains BAR 2 for 768 VFs
>
>[    0.832175] pci 000X:01:00.0: quirk_intel_e2000_no_ats: revision 0x11 < 0x20, disabling ATS
>[    0.832177] pci 000X:01:00.0: quirk_no_ats: SETTING ats_cap TO 0 (was 0x2dc) <--- pdev->ats_cap = 0
>
>[    2.365656] pci 000X:01:00.0: pci_prepare_ats: ps=12, is_virtfn=0, ats_cap=0x0
>[    2.365663] pci 000X:01:00.0: pci_prepare_ats: MANUAL SCAN FOUND ATS at 0x2dc (header 0x1000f, ctrl 0x0)
>[    2.365666] pci 000X:01:00.0: pci_prepare_ats failed: not supported
>
>The PCI ID : Vendor ID => [8086:1452] is for Intel e2000 NICs which has
>a quirk for ATS: quirk_intel_e2000_no_ats[1]
>
>VF logs:
>[   16.634000] pci 000X:02:00.0: [8086:145c] type 00 class 0x020000 PCIe Endpoint
>[   16.634060] pci 000X:02:00.0: pci_ats_init: pci_find_ext_capability(ATS) returned 0x1e4
>
>[   16.634196] pci 000X:02:00.0: pci_prepare_ats: ps=12, is_virtfn=1, ats_cap=0x1e4
>[   16.634201] pci 000X:02:00.0: pci_prepare_ats: MANUAL SCAN FOUND ATS at 0x1e4
>[   16.634202] pci 000X:02:00.0: pci_prepare_ats: device is VF, returning success
>
>[   16.634272] pci 000X:02:00.0: arm_smmu_ats_supported: dev is PCI, ats_cap=0x1e4, pci_ats_supported=1
>
>[   16.634282] pci 000X:02:00.0: pci_enable_ats entry: ps=12, is_virtfn=1, ats_cap=0x1e4
>[   16.634283] pci 000X:02:00.0: pci_enable_ats (VF): checking PF (0005:01:00.0) -> ats_cap=0x0, ats_enabled=0, ats_stu=0
>[   16.634285] pci 000X:02:00.0: pci_enable_ats EXIT: STU mismatch (PF 0 vs VF 12)
>[   16.634286] pci 000X:02:00.0: Failed to enable ATS (STU 12) error -22
>
>[   16.634513] pci 000X:02:00.0: quirk_intel_e2000_no_ats: revision 0x11 < 0x20, disabling ATS
>[   16.634515] pci 000X:02:00.0: quirk_no_ats: SETTING ats_cap TO 0 (was 0x1e4)
>
>The PCI ID : Vendor ID => [8086:145c] also has the same: quirk_intel_e2000_no_ats[1]
>
>Thus, this sequence occurs:
>
>1. The Intel quirk sets pdev->ats_cap = 0 for the PF at boot.
>2. pci_prepare_ats(PF) fails during arm_smmu_probe_device() as the
>   quirk causes pci_ats_supported to return false & PF->ats_stu
>   remains 0. (arm-smmu-v3 ignores the failure)

Ok that explains how PF stu is zero.
>
>3. PF attaches to identity domain: pci_enable_ats is skipped
>4. VF's arm_smmu_probe_device returns early from pci_prepare_ats() due
>   to the if (dev->is_virtfn) return 0; check
>
>5. SMMU calls pci_ats_supported() for the VF in attach_prepare() which
>   returns true as the quirk hasn't been called for the VF yet.
>   The SMMU driver sets state->ats_enabled = true
>
>6. We call pci_enable_ats which hits the STU mismatch (PF never sets it)
>7. Later while disabling ATS for VF via pci_disable_ats, we hit the WARN
>
>I guess we must think about the following:
>
>1. Check return values of pci_enable_ats / pci_prepare_ats in SMMUv3
>2. PCIe core to provide a proper ATS flag that iommu driver can rely on
>   and maybe define a policy around pci_ats_supported() to make sure it
>   also handles such quirks in pci_ats_supported().

It seems like that ats_supported() checking only ats_cap of the VF is
not enough, since ATS can be enabled for a VF only when the stu matches
with the PF? So even if ats_supported() for the VF returns true, its not
really supported if the stu doesn't match. And the stu would be set
properly if the PF had ats_cap. Should the ats_supported() check for
the ats_cap for the PF also?

We can check the PF ats_cap in the ats_supported() or handle the
enable_ats() error in arm smmuv3. Intel and AMD drivers seems to be
handling the error.
>
>LMK, your thoughts?
>
>Thanks!
>Praan

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

[Pranjal Shrivastava]

On Wed, May 06, 2026 at 10:20:15PM +0000, Samiullah Khawaja wrote:
> > 
> > Thus, this sequence occurs:
> > 
> > 1. The Intel quirk sets pdev->ats_cap = 0 for the PF at boot.
> > 2. pci_prepare_ats(PF) fails during arm_smmu_probe_device() as the
> >   quirk causes pci_ats_supported to return false & PF->ats_stu
> >   remains 0. (arm-smmu-v3 ignores the failure)
> 
> Ok that explains how PF stu is zero.
> > 
> > 3. PF attaches to identity domain: pci_enable_ats is skipped
> > 4. VF's arm_smmu_probe_device returns early from pci_prepare_ats() due
> >   to the if (dev->is_virtfn) return 0; check
> > 
> > 5. SMMU calls pci_ats_supported() for the VF in attach_prepare() which
> >   returns true as the quirk hasn't been called for the VF yet.
> >   The SMMU driver sets state->ats_enabled = true
> > 
> > 6. We call pci_enable_ats which hits the STU mismatch (PF never sets it)
> > 7. Later while disabling ATS for VF via pci_disable_ats, we hit the WARN
> > 
> > I guess we must think about the following:
> > 
> > 1. Check return values of pci_enable_ats / pci_prepare_ats in SMMUv3
> > 2. PCIe core to provide a proper ATS flag that iommu driver can rely on
> >   and maybe define a policy around pci_ats_supported() to make sure it
> >   also handles such quirks in pci_ats_supported().
> 
> It seems like that ats_supported() checking only ats_cap of the VF is
> not enough, since ATS can be enabled for a VF only when the stu matches
> with the PF? So even if ats_supported() for the VF returns true, its not
> really supported if the stu doesn't match. And the stu would be set
> properly if the PF had ats_cap. Should the ats_supported() check for
> the ats_cap for the PF also?
> 

I think yes. We should add that OR we should be checking for PF's ATS
cap while probing VFs to handle such quirks. The main problem is that
the quirk isn't applied to the VF *before* the IOMMU attach. 

> We can check the PF ats_cap in the ats_supported() or handle the
> enable_ats() error in arm smmuv3. Intel and AMD drivers seems to be
> handling the error.

I guess if pci_ats_supported() is fixed arm smmuv3 would stop hitting
that warning. Although, I agree that we should be checking the retval of
prepare_ats & enable_ats in arm-smmu-v3.

Thanks,
Praan

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

[Jason Gunthorpe]

On Wed, May 06, 2026 at 10:04:38PM +0000, Pranjal Shrivastava wrote:

> [1] https://elixir.bootlin.com/linux/v7.0.1/source/drivers/pci/quirks.c#L5703

The quirk is broken. It clearly says the device has mis-implemented
the ATS invalidation message so ATS must *NEVER* be enabled.

Thus the quirk should cover the VFs too and also disable ATS there.

So I still think my original suggestion is appropriate, fail to probe
the iommu device if ats prepare fails (serious PCI layer bug, like
broken quirks) and fail to attach the domain of ats enable fails
(serious internal kernel malfunction since enable must succeed if
prepare succeeded)

Jason

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

[Pranjal Shrivastava]

On Sat, May 09, 2026 at 02:14:51PM -0300, Jason Gunthorpe wrote:
> On Wed, May 06, 2026 at 10:04:38PM +0000, Pranjal Shrivastava wrote:
> 
> > [1] https://elixir.bootlin.com/linux/v7.0.1/source/drivers/pci/quirks.c#L5703
> 
> The quirk is broken. It clearly says the device has mis-implemented
> the ATS invalidation message so ATS must *NEVER* be enabled.
> 

I agree that the ATS must never be enable on this device. 

> Thus the quirk should cover the VFs too and also disable ATS there.

I observe the quirk does apply VFs but after the VF's iommu attach
because of how the pci_iov_add_virtfn() is written:

int pci_iov_add_virtfn(...)
{

	...
	pci_device_add(virtfn, virtfn->bus); // <------ This is where the iommu attach happens within device_add()
	rc = pci_iov_sysfs_link(dev, virtfn, id);
	if (rc)
		goto failed1;

	pci_bus_add_device(virtfn); // <---- This is where the quirk gets applied

}

i.e. in pci_device_add it calls pci_fixup_device(pci_fixup_header, dev)
whereas this ATS quirk is of `pci_fixup_final` type & gets applied within
pci_bus_add_device(). 

While, we can move this quirk to be in fixup_early / fixup_header.
I still think we should add checking the PF->ats_cap within 
pci_ats_supported before bailing out happily (return 0) for VFs.

> So I still think my original suggestion is appropriate, fail to probe
> the iommu device if ats prepare fails (serious PCI layer bug, like
> broken quirks) and fail to attach the domain of ats enable fails
> (serious internal kernel malfunction since enable must succeed if
> prepare succeeded)

I’m concerned that refusing to attach a device because it isn't ATS 
capable is bit too aggressive.

Even if pci_enable_ats() fails, the device is still perfectly capable of
performing standard DMA. We should allow the IOMMU attach to succeed but
in a 'Degraded' mode—where ATS is disabled for all it's functions, i.e. 
configured to treat it as a standard non-ATS device.

Thanks,
Praan

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

[Jason Gunthorpe]

On Mon, May 11, 2026 at 12:07:04PM +0000, Pranjal Shrivastava wrote:
> 
> > Thus the quirk should cover the VFs too and also disable ATS there.
> 
> I observe the quirk does apply VFs but after the VF's iommu attach
> because of how the pci_iov_add_virtfn() is written:

Ah, that seems to be the issue then :\

> While, we can move this quirk to be in fixup_early / fixup_header.
> I still think we should add checking the PF->ats_cap within 
> pci_ats_supported before bailing out happily (return 0) for VFs.

I feel like this should stay in prepare... supported is a differerent
thing. Maybe the quirk should be more directly tied to ats_supported
instead of whatever it is doing now? The ACS code calls directly into
quirks for example?
 
> > So I still think my original suggestion is appropriate, fail to probe
> > the iommu device if ats prepare fails (serious PCI layer bug, like
> > broken quirks) and fail to attach the domain of ats enable fails
> > (serious internal kernel malfunction since enable must succeed if
> > prepare succeeded)
> 
> I’m concerned that refusing to attach a device because it isn't ATS 
> capable is bit too aggressive.

Sorry, I was thinking prepare fails because it *fails*, prepare
succeeds if ATS is just not supported and won't ever be enabled.

But these odd cases where prepare sees ATS as available but can't
prepare it should fail and that should fail to probe the device.
That's exposing some kind of kernel or device bug that someone should
be fixing - exactly like you discovered here.

Jason

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

[Pranjal Shrivastava]

On Mon, May 11, 2026 at 11:16:07AM -0300, Jason Gunthorpe wrote:
> On Mon, May 11, 2026 at 12:07:04PM +0000, Pranjal Shrivastava wrote:
> > 
> > > Thus the quirk should cover the VFs too and also disable ATS there.
> > 
> > I observe the quirk does apply VFs but after the VF's iommu attach
> > because of how the pci_iov_add_virtfn() is written:
> 
> Ah, that seems to be the issue then :\
> 

Yea :/ we need something to ensure that quirk orders don't impact sanity

> > While, we can move this quirk to be in fixup_early / fixup_header.
> > I still think we should add checking the PF->ats_cap within 
> > pci_ats_supported before bailing out happily (return 0) for VFs.
> 
> I feel like this should stay in prepare... supported is a differerent
> thing. Maybe the quirk should be more directly tied to ats_supported
> instead of whatever it is doing now? The ACS code calls directly into
> quirks for example?
>  
> > > So I still think my original suggestion is appropriate, fail to probe
> > > the iommu device if ats prepare fails (serious PCI layer bug, like
> > > broken quirks) and fail to attach the domain of ats enable fails
> > > (serious internal kernel malfunction since enable must succeed if
> > > prepare succeeded)
> > 
> > I’m concerned that refusing to attach a device because it isn't ATS 
> > capable is bit too aggressive.
> 
> Sorry, I was thinking prepare fails because it *fails*, prepare
> succeeds if ATS is just not supported and won't ever be enabled.
> 
> But these odd cases where prepare sees ATS as available but can't
> prepare it should fail and that should fail to probe the device.
> That's exposing some kind of kernel or device bug that someone should
> be fixing - exactly like you discovered here.

This is actually a little weird/funny...

For PF: pci_prepare_ats fails as it sees a null ats_cap (quirk applied)
        and fails at pci_ats_supported().

For VF: pci_prepare_ats bails out early if (dev->is_virtfn):

/**
 * pci_prepare_ats - Setup the PS for ATS
 * @dev: the PCI device
 * @ps: the IOMMU page shift
 *
 * This must be done by the IOMMU driver on the PF before any VFs are created to
 * ensure that the VF can have ATS enabled.
 *
 * Returns 0 on success, or negative on failure.
 */
int pci_prepare_ats(struct pci_dev *dev, int ps)
{
	u16 ctrl;

	if (!pci_ats_supported(dev))
		return -EINVAL;

	if (WARN_ON(dev->ats_enabled))
		return -EBUSY;

	if (ps < PCI_ATS_MIN_STU)
		return -EINVAL;

	if (dev->is_virtfn) <---- THIS
		return 0;

	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);
	return 0;
}
EXPORT_SYMBOL_GPL(pci_prepare_ats);

Ideally, in such cases I'd want VFs to fail at pci_ats_supported itself

We could either:

1. Add this directly in pci_ats_supported:

if (dev->is_virtfn && !pci_ats_supported(pci_physfn(dev)))
         return false;

OR 2. we could have pci_ats_supported call into some ats quirk helper.

We need pci_ats_supported to fail here because if we contain the failure
in pci_prepare_ats, we'll see pci_ats_supported return success in attach
which will make arm_smmu_v3 believe that ATS is supported..

Thanks,
Praan

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

[David Matlack]

On Mon, May 11, 2026 at 9:07 AM Pranjal Shrivastava <praan@google.com> wrote:
>
> On Mon, May 11, 2026 at 11:16:07AM -0300, Jason Gunthorpe wrote:
> > On Mon, May 11, 2026 at 12:07:04PM +0000, Pranjal Shrivastava wrote:
> > >
> > > > Thus the quirk should cover the VFs too and also disable ATS there.
> > >
> > > I observe the quirk does apply VFs but after the VF's iommu attach
> > > because of how the pci_iov_add_virtfn() is written:
> >
> > Ah, that seems to be the issue then :\
>
> Yea :/ we need something to ensure that quirk orders don't impact sanity

I haven't read through this whole thread, but I think this patch is
related to the issue you are discussing:

  https://lore.kernel.org/linux-pci/20260403222750.1215002-1-dmatlack@google.com/

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

[Pranjal Shrivastava]

On Mon, May 11, 2026 at 09:30:49AM -0700, David Matlack wrote:
> On Mon, May 11, 2026 at 9:07 AM Pranjal Shrivastava <praan@google.com> wrote:
> >
> > On Mon, May 11, 2026 at 11:16:07AM -0300, Jason Gunthorpe wrote:
> > > On Mon, May 11, 2026 at 12:07:04PM +0000, Pranjal Shrivastava wrote:
> > > >
> > > > > Thus the quirk should cover the VFs too and also disable ATS there.
> > > >
> > > > I observe the quirk does apply VFs but after the VF's iommu attach
> > > > because of how the pci_iov_add_virtfn() is written:
> > >
> > > Ah, that seems to be the issue then :\
> >
> > Yea :/ we need something to ensure that quirk orders don't impact sanity
> 
> I haven't read through this whole thread, but I think this patch is
> related to the issue you are discussing:
> 
>   https://lore.kernel.org/linux-pci/20260403222750.1215002-1-dmatlack@google.com/

Yup, This should be the proper fix to the quirk issue. It moves the ats
quirk from final -> header and makes ats_init quirk-aware. I'll test out
this patch. Thanks for sharing it!

That said, I think even if we solve the quirk timing, pci_enable_ats() &
pci_prepare_ats can still fail for runtime reasons & the arm-smmu-v3
driver should check their return values for correctly tracking ATS state

Thanks,
Praan

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

[Jason Gunthorpe]

On Mon, May 11, 2026 at 04:07:10PM +0000, Pranjal Shrivastava wrote:
> This is actually a little weird/funny...
> 
> For PF: pci_prepare_ats fails as it sees a null ats_cap (quirk applied)
>         and fails at pci_ats_supported().
> 
> For VF: pci_prepare_ats bails out early if (dev->is_virtfn):
> 
> /**
>  * pci_prepare_ats - Setup the PS for ATS
>  * @dev: the PCI device
>  * @ps: the IOMMU page shift
>  *
>  * This must be done by the IOMMU driver on the PF before any VFs are created to
>  * ensure that the VF can have ATS enabled.
>  *
>  * Returns 0 on success, or negative on failure.
>  */
> int pci_prepare_ats(struct pci_dev *dev, int ps)
> {
> 	u16 ctrl;
> 
> 	if (!pci_ats_supported(dev))
> 		return -EINVAL;
> 
> 	if (WARN_ON(dev->ats_enabled))
> 		return -EBUSY;
> 
> 	if (ps < PCI_ATS_MIN_STU)
> 		return -EINVAL;
> 
> 	if (dev->is_virtfn) <---- THIS
> 		return 0;

I would check the PF STU here and fail if wrong, like pci_enable_ats()
does. The purpose of prepare is to make the STU right, so it is
completely logical.

> Ideally, in such cases I'd want VFs to fail at pci_ats_supported itself

The quirk should cause pci_ats_supported to fail.

My observation is that pci_prepare_ats() shuld also always fail if it
cannot setup the STU, meaning the VF should do more than just return 0
since enable_ats is doing more.

> if (dev->is_virtfn && !pci_ats_supported(pci_physfn(dev)))
>          return false;
> 
> OR 2. we could have pci_ats_supported call into some ats quirk helper.

Both of these seem like good options to fix the quirk. The first is
probably OK

> We need pci_ats_supported to fail here because if we contain the failure
> in pci_prepare_ats, we'll see pci_ats_supported return success in attach
> which will make arm_smmu_v3 believe that ATS is supported..

Yes, that's right for the quirk issue.

Jason