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

[Pranjal Shrivastava <praan@google.com>]

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