* Re: [PATCH] PCI/ATS: Disable ATS when ACS TB is set upstream
2026-10-20 18:10 [PATCH] PCI/ATS: Disable ATS when ACS TB is set upstream Danny Gonzalez
@ 2026-11-05 23:04 ` Bjorn Helgaas
2026-11-05 23:15 ` Nicolin Chen
0 siblings, 1 reply; 3+ messages in thread
From: Bjorn Helgaas @ 2026-11-05 23:04 UTC (permalink / raw)
To: Danny Gonzalez
Cc: Bjorn Helgaas, David Decotigny, Sheena Mohan, linux-pci,
linux-kernel, Jason Gunthorpe, Pranjal Shrivastava, Joerg Roedel,
Nicolin Chen, Lu Baolu, Kevin Tian
[+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
>
^ permalink raw reply [flat|nested] 3+ messages in thread