Re: [PATCH] PCI/ATS: Disable ATS when ACS TB is set upstream

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc other folks interested in ATS]

On Tue, Oct 20, 2026 at 06:10:52PM +0000, Danny Gonzalez wrote:
> Enabling ATS allows an endpoint to cache translations and initiate DMA
> requests with Translated addresses, which an upstream bridge with ACS
> Translation Blocking (PCI_ACS_TB) set will block as ACS Violations.
> Because pci_enable_acs() preserves existing PCI_ACS_CTRL bits (e.g., from
> firmware, a kexec kernel, or "pci=config_acs=") without relaxing them,
> PCI_ACS_TB may remain set on upstream bridges.
> 
> Update pci_ats_supported() to walk upstream bridges and return false if
> any bridge has PCI_ACS_TB set, leaving the endpoint to use Untranslated
> addresses.
> 
> Tested by booting a machine with an endpoint advertising the ATS Capability
> behind a Root Port with PCI_ACS_TB set; verified ATS is disabled and the
> system boots cleanly.

There was a v2 at
https://lore.kernel.org/all/20261028233651.960835-1-digonzal@google.com,
which added pci_ats_check_acs() in pci_dma_configure().  I thought
that was a non-obvious way to make pci_ats_supported() return "false",
and I proposed doing the checking in pci_ats_supported() directly to
avoid adding the new pci_ats_check_acs() interface.

I'm responding to v1 because it *did* add the checking in
pci_ats_supported().  But sashiko found some issues with it:
https://lore.kernel.org/all/sashiko-outbox-179488@kernel.org.

> Signed-off-by: Danny Gonzalez <digonzal@google.com>
> ---
>  drivers/pci/ats.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index 9cb23780093d..094f4f825acf 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -31,6 +31,24 @@ void pci_ats_init(struct pci_dev *dev)
>  	dev->ats_cap = pos;
>  }
>  
> +static struct pci_dev *pci_acs_tb_path_blocked(struct pci_dev *dev)
> +{
> +	struct pci_dev *bridge;
> +
> +	for (bridge = pci_upstream_bridge(dev); bridge;
> +	     bridge = pci_upstream_bridge(bridge)) {
> +		u16 ctrl;
> +
> +		if (!bridge->acs_cap)
> +			continue;
> +		pci_read_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL,
> +				     &ctrl);

Sashiko noted that if bridge is inaccessible, this read sets ctrl to
~0, so we think PCI_ACS_TB is set.  Maybe worth checking for
PCI_POSSIBLE_ERROR() before using ctrl.

> +		if (ctrl & PCI_ACS_TB)
> +			return bridge;
> +	}
> +	return NULL;
> +}
> +
>  /**
>   * pci_ats_supported - check if the device can use ATS
>   * @dev: the PCI device
> @@ -40,12 +58,22 @@ void pci_ats_init(struct pci_dev *dev)
>   */
>  bool pci_ats_supported(struct pci_dev *dev)
>  {
> +	struct pci_dev *bridge;
> +
>  	if (!dev->ats_cap || dev->untrusted)
>  		return false;
>  
>  	if (dev->is_virtfn)
>  		return pci_ats_supported(pci_physfn(dev));
>  
> +	/* Do not allow ATS if any upstream bridge blocks translation */
> +	bridge = pci_acs_tb_path_blocked(dev);
> +	if (bridge) {
> +		pci_warn_once(dev, "ATS disabled: upstream bridge %s has ACS Translation Blocking enabled\n",
> +			      pci_name(bridge));

I think I suggested it, but now I'm not sure the warning is
worthwhile.  I think this might be too far down in the bowels for it
to be useful.  And sashiko complained about the warn-once bit too,
which is fair.

> +		return false;
> +	}
> +

Sashiko also noted problems if pci_ats_supported() returns different
values in:

  * intel_iommu_probe_device() vs intel_iommu_release_device()

      Different values break rbtree 'info' management: _probe_device()
      can put 'info' in the rbtree and _release_device() may fail to
      remove it.

      I think the fix is for intel_iommu_probe_device() to keep its own
      bit in struct device_domain_info to say whether 'info' is in the
      rbtree so _release_device() doesn't need pci_ats_supported().

  * pci_dev_reset_iommu_prepare() vs pci_dev_reset_iommu_done()

      Different values will make gdev->reset_depth, group->recovery_cnt,
      group->domain, and gdev->blocked get out of sync.

      I think it would be better if _iommu_prepare() and _iommu_done()
      themselves tracked what _iommu_done() needs to do, for example
      they could do something like this:

        int pci_dev_reset_iommu_prepare(struct pci_dev *pdev, bool *restore)
        {
            if (!pci_ats_supported(pdev) || !dev_has_iommu(&pdev->dev)) {
                *restore = false;
                return 0;
            }

            *restore = true;
            ...
        }

      Then pci_dev_reset_iommu_done() wouldn't need to call
      pci_ats_supported() or dev_has_iommu(), and users would look like
      this:

        int pcie_flr(...)
        {
            bool restore;

            ret = pci_dev_reset_iommu_prepare(dev, &restore);
            ...
            if (restore)
                pci_dev_reset_iommu_done(dev);
        }

      Or I guess we could call pci_dev_reset_iommu_done()
      unconditionally and pass it 'restore' and it could test it
      internally.

>  	return true;
>  }
>  EXPORT_SYMBOL_GPL(pci_ats_supported);
> -- 
> 2.56.0.218.g1ec904b173-goog
>