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

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

[Danny Gonzalez]

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.

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);
+		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));
+		return false;
+	}
+
 	return true;
 }
 EXPORT_SYMBOL_GPL(pci_ats_supported);
-- 
2.56.0.218.g1ec904b173-goog

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

[Bjorn Helgaas]

[+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
> 

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

[Nicolin Chen]

On Thu, Nov 05, 2026 at 05:04:12PM -0600, Bjorn Helgaas wrote:
>   * 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

If the condition here is "an upstream bridge becomes inaccessible
during a reset", then the reset/flr itself fails?

The patch that you acked yesterday handles such a case:
https://lore.kernel.org/linux-iommu/20261105001312.GA1479227@bhelgaas/

Nicolin

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

[Jason Gunthorpe]

On Thu, Nov 05, 2026 at 05:04:12PM -0600, Bjorn Helgaas wrote:

> > +		return false;
> > +	}
> > +
> 
> Sashiko also noted problems if pci_ats_supported() returns different
> values in:

Yeah, I share this concern, if pci_ats_supported() becomes dynamic
that is not how alot of users assume it works. It will need fixing
first.

The general idea seems fine

Jason