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

[Pranjal Shrivastava <praan@google.com>]

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