iommu/arm-smmu-v3: Quarantine device upon ATC invalidation timeout
Subsystems (6)
v6 cover iommu/arm-smmu-v3: Quarantine device upon ATC invalidation timeout
Hi all,
This series addresses a critical vulnerability and stability issue where an unresponsive PCIe device failing to process ATC (Address Translation Cache) invalidation requests leads to silent data corruption and continuous SMMU CMDQ error spam.
[ As Jason pointed out, because this series fundamentally introduces a new
RAS feature to quarantine and recover from hardware faults and relies on a recently accepted SMMU driver rework, it is not treated as a standard bug fix. Thus, most of the patches here don't carry a "Fixes" tag. ]
Currently, when an ATC invalidation times out, the SMMUv3 driver skips the CMDQ_ERR_CERROR_ATC_INV_IDX error. This leaves the device's ATS cache state desynchronized from the SMMU: the device cache may retain stale ATC entries for memory pages that the OS has already reclaimed and reassigned, creating a direct vector for data corruption. Furthermore, the driver might continue issuing ATC_INV commands, resulting in constant CMDQ errors:
unexpected global error reported (0x00000001), this could be serious
CMDQ error (cons 0x0302bb84): ATC invalidate timeout
unexpected global error reported (0x00000001), this could be serious
CMDQ error (cons 0x0302bb88): ATC invalidate timeout
unexpected global error reported (0x00000001), this could be serious
CMDQ error (cons 0x0302bb8c): ATC invalidate timeout
...To resolve this, introduce a mechanism to quarantine a broken device in the SMMUv3 driver and the IOMMU core. To achieve this, add preparatory changes:
- Pass in PCI reset result to pci_dev_reset_iommu_done()
- Co-clear pending CMDQ_ERR from the cmdq issuer under a raw_spinlock_t, so an ATC_INV timeout flagged in cmdq->atc_sync_timeouts is definitive when the issuer reads its bit after CMD_SYNC poll
On the SMMUv3 driver side, retry the timedout ATC_INV batch to identify the faulty device(s). Perform a surgical STE update, and flag the ATS as broken to reject further ATS/ATC requests at HW level and suppress timeout spam.
This is on Github:
https://github.com/nicolinc/iommufd/commits/smmuv3_atc_timeout-v6
Changelog
v6:
* Rebase on v7.3-rc1
* Use "LLM" for the Assisted-by tags
* Add Reviewed-by tags from Baolu and Jason
* [PCI] Move the port register read to the probe path
* [PCI] Drop the patch propagating error code in quirks.c
* [iommu] Rename enum gdev_blocked to enum blocked_reason
* [iommu] Pass the correct old domain when releasing a blocked device
* [smmuv3] Drop the patch adding arm_smmu_cmdq_batch_force_sync (merged)
v5:
https://lore.kernel.org/all/[email protected]/
* Rebase on v7.2-rc1
* [PCI] Probe the underlying bus reset in cxl_reset_bus_function()
* [PCI] Add quirk_flr_err() to stop the reset cascade on FLR timeout
* [iommu] Drop iommu_report_device_broken() and its preparatory patches
* [smmuv3] Drop master->ats_broken bool
* [smmuv3] Drop master->ats_broken_lock
* [smmuv3] Drop master->ats_invs scratch
* [smmuv3] Introduce INV_TYPE_ATS_BROKEN marker
* [smmuv3] Add arm_smmu_cmdq_batch_force_sync()
* [smmuv3] Don't rb_erase() a never-inserted stream node
* [smmuv3] Add streams_lock for atomic SID->master lookup
* [smmuv3] Drop "Serialize STE.EATS and ats_broken updates"
* [smmuv3] Drop "Move arm_smmu_invs_for_each_entry to header"
* [smmuv3] Recheck CMDQ_ERR in tegra241_vintf0_handle_error()
* [smmuv3] Thread arm_smmu_master_domain on a per-master list
* [smmuv3] Drop pci_disable_ats() from arm_smmu_quarantine_ats()
* [smmuv3] Limit the quarantine() to ARM_SMMU_FEAT_COHERENCY only
* [smmuv3] Rework arm_smmu_quarantine_ats() to walk master_domains
* [smmuv3] Rework arm_smmu_cmdq_batch_retry(): per-unique-SID retry
* [smmuv3] Rework arm_smmu_inv_cmp() to treat ATS variants as one class
* [smmuv3] Rework the issuer-side atc_sync_timeouts test with smp_rmb()
* [smmuv3] Drop "Co-clear pending CMDQ_ERR when queue_has_space() fails"
* [smmuv3] Drop "Keep smmu pointer in arm_smmu_inv but add master for ATS"
v4:
https://lore.kernel.org/all/[email protected]/
* Rebase on Joerg's IOMMU "fixes" branch
* Rebase on Jason's SMMUv3 cmd_ent series
https://lore.kernel.org/all/[email protected]/
* [PCI] Don't suspend IOMMU in probe mode
* [iommu] kfree_rcu() iommu_group
* [iommu] Convert gdev->blocked to enum gdev_blocked
* [iommu] Use disable_work_sync() to fix UAF and ref leak
* [iommu] Gate done() transitions to preserve BLOCKED_BROKEN
* [iommu] Decrement recovery_cnt when unplugging a blocked gdev
* [iommu] Drop racy dev_has_iommu() in iommu_report_device_broken()
* [iommu] Add gdev->broken_pending to skip worker after racing recovery
* [smmuv3] Add master->ats_invs scratch
* [smmuv3] Add arm_smmu_cmdq_batch_issue() wrapper
* [smmuv3] Force per-flush sync for has_ats batches
* [smmuv3] Serialize STE.EATS and ats_broken updates
* [smmuv3] Co-clear pending CMDQ_ERR from cmdq issuer
* [smmuv3] Add invs and has_ats to arm_smmu_cmdq_batch
* [smmuv3] Move arm_smmu_invs_for_each_entry to header
* [smmuv3] Set master->ats_broken after clearing STE.EATS
* [smmuv3] Issue CFGI_STE via arm_smmu_cmdq_issue_cmd_with_sync()
* [smmuv3] Keep "smmu" pointer in arm_smmu_inv but add "master" for ATS
v3:
https://lore.kernel.org/all/[email protected]/
* Rebase on arm/smmu/updates branch + bug fix
* Update commit messages and inline comments
* [iommu] Drop unnecessary ops validation
* [iommu] Add missed function stub when !CONFIG_IOMMU_API
* [iommu] Change iommu_report_device_broken() to per gdev
* [iommu] Separate quarantine from pci_dev_reset_prepare()
* [iommu] Check reset failure in pci_dev_reset_iommu_done()
* [smmuv3] Fix STE update with try_cmpxchg64()
* [smmuv3] Fix "continue" bug when skipping ATC commands
* [smmuv3] Replace atomic_t prod_err with a lockless bitmap
* [smmuv3] Drop master->invs_domain; disable ATS per-master directly
* [smmuv3] Return -EIO for ATC timeout v.s. -ETIMEDOUT for poll timeout
* [smmuv3] Replace INV_TYPE_ATS_DISABLED with per-master ats_broken flag
v2:
https://lore.kernel.org/all/[email protected]/
* Rebase on arm_smmu_invs-v13 series
* Bisect batched atc invalidation commands
* Drop the direct pci_reset_function() call
* Move the work queue from SMMUv3 to the core
* Proceed a surgical STE update to disable EATS
* Wait for pci_dev_reset_iommu_done() to signal a recovery
v1:
https://lore.kernel.org/all/[email protected]/
Thanks
Nicolin
Nicolin Chen (17):
PCI: Don't suspend IOMMU when probing reset capability PCI/CXL: Probe the underlying bus reset in cxl_reset_bus_function()
iommu: Convert gdev->blocked from bool to enum blocked_reason
iommu: Pass in gdev's blocked state to iommu_deinit_device()
iommu: Pass in reset result to pci_dev_reset_iommu_done() iommu/arm-smmu-v3: Don't rb_erase() a never-inserted stream node iommu/arm-smmu-v3: Track ATC invalidation timeouts in a bitmap iommu/arm-smmu-v3: Skip remaining GERROR causes on SFM iommu/arm-smmu-v3: Introduce per-cmdq cmdq_err_handler callback iommu/arm-smmu-v3: Recheck CMDQ_ERR in tegra241_vintf0_handle_error() iommu/arm-smmu-v3: Co-clear pending CMDQ_ERR when CMD_SYNC times out iommu/arm-smmu-v3: Introduce arm_smmu_cmdq_batch_issue() wrapper iommu/arm-smmu-v3: Add streams_lock for atomic-context SID->master
lookup
iommu/arm-smmu-v3: Add has_ats to struct arm_smmu_cmdq_batch iommu/arm-smmu-v3: Add INV_TYPE_ATS_BROKEN for quarantined masters iommu/arm-smmu-v3: Thread arm_smmu_master_domain on a per-master list iommu/arm-smmu-v3: Quarantine ATS after an ATC invalidation timeout
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 33 +- include/linux/iommu.h | 5 +- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 477 ++++++++++++++++-- .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 41 +- drivers/iommu/iommu.c | 87 +++- drivers/pci/pci-acpi.c | 2 +- drivers/pci/pci.c | 37 +- drivers/pci/quirks.c | 13 +- 8 files changed, 606 insertions(+), 89 deletions(-)
base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
Signature · 1 line
2.43.0
PATCH v6 1/17 PCI: Don't suspend IOMMU when probing reset capability
reset_method_store() in drivers/pci/pci-sysfs.c discovers supported reset methods by calling reset_fn(pdev, PCI_RESET_PROBE, ...) without holding a device_lock, since the probe path is expected to query the device's reset capability without changing device state.
However, pci_reset_bus_function() and __pci_dev_specific_reset() violate that contract after pci_dev_reset_iommu_prepare/done() were added, which moves the device into a blocking domain and abruptly aborts any in-flight DMA. Doing this for a probe -- a state-query call that does not even hold device_lock -- can cause driver timeouts and data loss on a DMAing device.
The peer reset helpers all handle this correctly: they short-circuit on a probe input before touching the IOMMU.
Skip pci_dev_reset_iommu_prepare()/_done() entirely when probe is set. The inner reset routines already implement their own probe semantics, and they perform the capability checks and return without changing device state.
--- drivers/pci/pci.c | 13 ++++++++----- drivers/pci/quirks.c | 13 ++++++++----- 2 files changed, 16 insertions(+), 10 deletions(-)
drivers/pci/pci.c +8 -5
@@ -4996,10 +4996,12 @@ static int pci_reset_bus_function(struct pci_dev *dev, bool probe)if (bridge &&pcie_is_cxl (bridge) &&cxl_sbr_masked (bridge))return -ENOTTY ; - rc =pci_dev_reset_iommu_prepare (dev); -if (rc) { -pci_err (dev,"failed to stop IOMMU for a PCI reset: %d\n" , rc); -return rc; +if (!probe) { + rc =pci_dev_reset_iommu_prepare (dev); +if (rc) { +pci_err (dev,"failed to stop IOMMU for a PCI reset: %d\n" , rc); +return rc; + } } rc =pci_dev_reset_slot_function (dev, probe);
@@ -5008,7 +5010,8 @@ static int pci_reset_bus_function(struct pci_dev *dev, bool probe) rc =pci_parent_bus_reset (dev, probe); done: -pci_dev_reset_iommu_done (dev); +if (!probe) +pci_dev_reset_iommu_done (dev);return rc; }
drivers/pci/quirks.c +8 -5
@@ -4253,14 +4253,17 @@ static int __pci_dev_specific_reset(struct pci_dev *dev, bool probe, {int ret; - ret =pci_dev_reset_iommu_prepare (dev); -if (ret) { -pci_err (dev,"failed to stop IOMMU for a PCI reset: %d\n" , ret); -return ret; +if (!probe) { + ret =pci_dev_reset_iommu_prepare (dev); +if (ret) { +pci_err (dev,"failed to stop IOMMU for a PCI reset: %d\n" , ret); +return ret; + } } ret = i->reset (dev, probe); -pci_dev_reset_iommu_done (dev); +if (!probe) +pci_dev_reset_iommu_done (dev);return ret; }
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v6 01/17] PCI: Don't suspend IOMMU when probing reset capability
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 18 lines
reset_method_store()indrivers/pci/pci-sysfs.cdiscovers supported reset methods by callingreset_fn(pdev, PCI_RESET_PROBE, ...)without holding adevice_lock, since the probe path is expected to query the device's reset capability without changing device state.However,
pci_reset_bus_function()and__pci_dev_specific_reset()violate that contract after pci_dev_reset_iommu_prepare/done() were added, which moves the device into a blocking domain and abruptly aborts any in-flight DMA. Doing this for a probe -- a state-query call that does not even holddevice_lock-- can cause driver timeouts and data loss on a DMAing device.The peer reset helpers all handle this correctly: they short-circuit on a probe input before touching the IOMMU.
Skip pci_dev_reset_iommu_prepare()/_done() entirely when probe is set. The inner reset routines already implement their own probe semantics, and they perform the capability checks and return without changing device state.
Quoted 5 lines
Fixes f5b16b802174 ("PCI: Suspend iommu function prior to resetting a device")Reviewed-by Jason Gunthorpe <[email protected]>Assisted-by LLMSigned-off-by Nicolin Chen <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH v6 2/17 PCI/CXL: Probe the underlying bus reset in cxl_reset_bus_function()
In reply to Nicolin Chencxl_reset_bus_function() reports "supported" to a probe after checking only that the upstream bridge carries a CXL port DVSEC. The underlying bus reset can still be unavailable, e.g. on a bus shared with other devices, so both the reset_methods[] array and the reset_method sysfs node end up listing a "cxl_bus" that is guaranteed to fail with -ENOTTY when it is attempted.
Probe the underlying pci_dev_reset_slot_function() and then, if it is not applicable, pci_parent_bus_reset(). These are the same two checks that the actual reset runs, so a shared-bus CXL device no longer advertises a method that can never succeed.
Probing via pci_reset_bus_function() would not work: its cxl_sbr_masked() check rejects every CXL port with a masked SBR, while the do-reset path in this function unmasks the SBR before resetting. Such a port would wrongly probe as unsupported.
Move the port control register read above the probe branch as well, so that probe runs the same checks. That read asserts no reset, and a failure means the device cannot be reset this way, which is a probe-time answer.
Also pass an explicit PCI_RESET_DO_RESET at the do-reset call site, since probe is always false at that point.
--- drivers/pci/pci.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/pci/pci.c +12 -4
@@ -5029,13 +5029,21 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)if (!dvsec)return -ENOTTY ; -if (probe) -return 0 ; - rc =pci_read_config_word (bridge, dvsec +PCI_DVSEC_CXL_PORT_CTL , ®);if (rc)return -ENOTTY ; +/* +* Do not probe via pci_reset_bus_function(), which would reject a +* masked SBR that the do-reset path below unmasks before resetting. +*/ +if (probe) { + rc =pci_dev_reset_slot_function (dev,PCI_RESET_PROBE ); +if (rc != -ENOTTY ) +return rc; +return pci_parent_bus_reset (dev,PCI_RESET_PROBE ); + } + rc =pci_dev_reset_iommu_prepare (dev);if (rc) {pci_err (dev,"failed to stop IOMMU for a PCI reset: %d\n" , rc);
@@ -5050,7 +5058,7 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe) val); } - rc =pci_reset_bus_function (dev, probe); + rc =pci_reset_bus_function (dev,PCI_RESET_DO_RESET );if (reg != val) pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL,
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v6 02/17] PCI/CXL: Probe the underlying bus reset in cxl_reset_bus_function()
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 20 lines
cxl_reset_bus_function()reports "supported" to a probe after checking only that the upstream bridge carries a CXL port DVSEC. The underlying bus reset can still be unavailable, e.g. on a bus shared with other devices, so both the reset_methods[] array and thereset_methodsysfs node end up listing a "cxl_bus" that is guaranteed to fail with-ENOTTYwhen it is attempted.Probe the underlying
pci_dev_reset_slot_function()and then, if it is not applicable,pci_parent_bus_reset(). These are the same two checks that the actual reset runs, so a shared-bus CXL device no longer advertises a method that can never succeed.Probing via
pci_reset_bus_function()would not work: itscxl_sbr_masked()check rejects every CXL port with a masked SBR, while the do-reset path in this function unmasks the SBR before resetting. Such a port would wrongly probe as unsupported.Move the port control register read above the probe branch as well, so that probe runs the same checks. That read asserts no reset, and a failure means the device cannot be reset this way, which is a probe-time answer.
Quoted 6 lines
Also pass an explicit
PCI_RESET_DO_RESETat the do-reset call site, since probe is always false at that point.Fixes 53c49b6e6dd2e ("PCI/CXL: Add 'cxl_bus' reset method for devices below CXL Ports")Assisted-by LLMSigned-off-by Nicolin Chen <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Re: [PATCH v6 02/17] PCI/CXL: Probe the underlying bus reset in cxl_reset_bus_function()
In reply to Nicolin ChenQuoted 20 lines · On Wed, 23 Sep 2026 13:11:21 -0700 Nicolin Chen <[email protected]> wrote:
cxl_reset_bus_function()reports "supported" to a probe after checking only that the upstream bridge carries a CXL port DVSEC. The underlying bus reset can still be unavailable, e.g. on a bus shared with other devices, so both the reset_methods[] array and thereset_methodsysfs node end up listing a "cxl_bus" that is guaranteed to fail with-ENOTTYwhen it is attempted.Probe the underlying
pci_dev_reset_slot_function()and then, if it is not applicable,pci_parent_bus_reset(). These are the same two checks that the actual reset runs, so a shared-bus CXL device no longer advertises a method that can never succeed.Probing via
pci_reset_bus_function()would not work: itscxl_sbr_masked()check rejects every CXL port with a masked SBR, while the do-reset path in this function unmasks the SBR before resetting. Such a port would wrongly probe as unsupported.Move the port control register read above the probe branch as well, so that probe runs the same checks. That read asserts no reset, and a failure means the device cannot be reset this way, which is a probe-time answer.
Quoted 6 lines
Also pass an explicit
PCI_RESET_DO_RESETat the do-reset call site, since probe is always false at that point.Fixes 53c49b6e6dd2e ("PCI/CXL: Add 'cxl_bus' reset method for devices below CXL Ports")Assisted-by LLMSigned-off-by Nicolin Chen <[email protected]>
Nice clear explanation and it looks like the code does what that says, so I'm fine with this.
PATCH v6 3/17 iommu: Convert gdev->blocked from bool to enum blocked_reason
In reply to Nicolin ChenThe gdev->blocked flag tracks whether a device is individually being held in the group->blocking_domain while group->domain is retained. Up to now, a PCI reset in flight is the only producer, so a bool suffices.
Subsequent changes will add more reasons to keep a device blocked, e.g. a failed-reset case that must not auto-unblock, or a driver-side quarantine for a hardware fault. These reasons are cleared by different events, which a single bool cannot encode.
Convert the "bool blocked" into "enum blocked_reason blocked", provisioned with two initial values: BLOCKED_NONE and BLOCKED_RESETTING, covering the existing use cases. All readers keep the "if (gdev->blocked)" form, since BLOCKED_NONE == 0.
This is a pure type change with no behavior change. Follow-on changes will add new enum values along with their producers.
--- drivers/iommu/iommu.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
drivers/iommu/iommu.c +9 -5
@@ -73,16 +73,20 @@ struct iommu_group {void *owner; }; +enum blocked_reason { +BLOCKED_NONE =0 ,/* Not blocked */ +BLOCKED_RESETTING ,/* PCI reset in flight */ +}; +struct group_device {struct list_head list;struct device *dev;char *name;/* * Device is blocked for a pending recovery while its group->domain is -* retained. This can happen when: -* - Device is undergoing a reset +* retained. */ -bool blocked; +enum blocked_reason blocked;unsigned int reset_depth; };
@@ -4072,7 +4076,7 @@ int pci_dev_reset_iommu_prepare(struct pci_dev *pdev)* the correct domain in iommu_driver_get_domain_for_dev() that might be * called in a set_dev_pasid callback function. */ - gdev->blocked = true; + gdev->blocked =BLOCKED_RESETTING ;/* * Stage PASID domains at blocking_domain while retaining pasid_array.
@@ -4198,7 +4202,7 @@ void pci_dev_reset_iommu_done(struct pci_dev *pdev)* the correct domain in iommu_driver_get_domain_for_dev() that might be * called in a set_dev_pasid callback function. */ - gdev->blocked = false; + gdev->blocked =BLOCKED_NONE ;/* * Re-attach PASID domains back to the domains retained in pasid_array.
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v6 03/17] iommu: Convert gdev->blocked from bool to enum blocked_reason
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 14 lines
The
gdev->blockedflag tracks whether a device is individually being held in thegroup->blocking_domainwhilegroup->domainis retained. Up to now, a PCI reset in flight is the only producer, so a bool suffices.Subsequent changes will add more reasons to keep a device blocked, e.g. a failed-reset case that must not auto-unblock, or a driver-side quarantine for a hardware fault. These reasons are cleared by different events, which a single bool cannot encode.
Convert the "bool blocked" into "enum
blocked_reasonblocked", provisioned with two initial values:BLOCKED_NONEandBLOCKED_RESETTING, covering the existing use cases. All readers keep the "if (gdev->blocked)" form, sinceBLOCKED_NONE== 0.
Quoted 7 lines
This is a pure type change with no behavior change. Follow-on changes will add new enum values along with their producers.
Reviewed-by Jason Gunthorpe <[email protected]>Reviewed-by Lu Baolu <[email protected]>Assisted-by LLMSigned-off-by Nicolin Chen <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
PATCH v6 4/17 iommu: Pass in gdev's blocked state to iommu_deinit_device()
In reply to Nicolin Cheniommu_deinit_device() parks the device on the driver's release_domain and passes group->domain to its attach_dev() callback as the old domain. That is only correct while the device is still attached to group->domain.
A subsequent change keeps a device blocked on group->blocking_domain after a failed reset, so the old domain has to follow the device's blocked state. The group_device holding that state is freed by __iommu_group_free_device() before iommu_deinit_device() runs, so read it at the call site and pass it in.
No functional change.
--- drivers/iommu/iommu.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/iommu/iommu.c +5 -3
@@ -550,7 +550,7 @@ static int iommu_init_device(struct device *dev)return ret; } -static void iommu_deinit_device (struct device *dev) +static void iommu_deinit_device (struct device *dev,bool blocked) {struct iommu_group *group = dev->iommu_group;const struct iommu_ops *ops =dev_iommu_ops (dev);
@@ -703,7 +703,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_listlist_del (&gdev->list);__iommu_group_free_device (group, gdev); err_put_group: -iommu_deinit_device (dev); +iommu_deinit_device (dev, false);mutex_unlock (&group->mutex);iommu_group_put (group);
@@ -759,13 +759,15 @@ static void __iommu_group_remove_device(struct device *dev)mutex_lock (&group->mutex);for_each_group_device (group, device) { +bool blocked = device->blocked; +if (device->dev != dev)continue ;list_del (&device->list);__iommu_group_free_device (group, device);if (dev_has_iommu (dev)) -iommu_deinit_device (dev); +iommu_deinit_device (dev, blocked);else dev->iommu_group =NULL ;break ;
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v6 04/17] iommu: Pass in gdev's blocked state to iommu_deinit_device()
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 10 lines
iommu_deinit_device()parks the device on the driver'srelease_domainand passesgroup->domainto itsattach_dev()callback as the old domain. That is only correct while the device is still attached togroup->domain.A subsequent change keeps a device blocked on
group->blocking_domainafter a failed reset, so the old domain has to follow the device's blocked state. Thegroup_deviceholding that state is freed by__iommu_group_free_device()beforeiommu_deinit_device()runs, so read it at the call site and pass it in.
Quoted 4 lines
No functional change.
Assisted-by LLMSigned-off-by Nicolin Chen <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
PATCH v6 5/17 iommu: Pass in reset result to pci_dev_reset_iommu_done()
In reply to Nicolin ChenIOMMU drivers handle ATC cache maintenance. They may encounter ATC-related errors (e.g., ATC invalidation timeout), indicating that the ATC cache may have stale entries that can corrupt the memory. In this case, IOMMU driver has no choice but to block the device's ATS function and wait for a device recovery.
The pci_dev_reset_iommu_done() called at the end of a reset function could serve as a reliable signal to the IOMMU subsystem that the physical device cache is completely clean. However, the function is called unconditionally even if the reset operation had actually failed, which would re-attach the faulty device back to a normal translation domain. And this will leave the system highly exposed, creating vulnerabilities for data corruption:
IOMMU blocks RID/ATS
pci_reset_function():
pci_dev_reset_iommu_prepare(); // Block RID/ATS
__reset(); // Failed (ATC is still stale)
pci_dev_reset_iommu_done(); // Unblock RID/ATS (ah-ha)Instead, pass in @reset_result to pci_dev_reset_iommu_done() from callers:
IOMMU blocks RID/ATS
pci_reset_function():
pci_dev_reset_iommu_prepare(); // Block RID/ATS
rc = __reset();
pci_dev_reset_iommu_done(rc); // Unblock or quarantine
On a successful reset, done() restores the device to its RID/PASID domains and decrements group->recovery_cnt. On failure, the device remains blocked, and concurrent domain attachment will be rejected until a successful reset.
Note: -ENOTTY is overloaded with different meanings by PCI reset functions. Some of them indicate "reset was not attempted", while others indicate "try the next reset method and the current method failed". IOMMU that must react these two outcomes separately has no choice but to keep the device blocked on -ENOTTY as well. Leave an inline FIXME and warning.
This introduces a new situation where a blocked device is being unplugged. Decrement the group->recovery_cnt accordingly. Such a device stays on the group->blocking_domain, so hand that to the release_domain attach_dev() as the old domain in place of group->domain.
--- include/linux/iommu.h | 5 ++-- drivers/iommu/iommu.c | 65 +++++++++++++++++++++++++++++++++++++++--- drivers/pci/pci-acpi.c | 2 +- drivers/pci/pci.c | 10 +++---- drivers/pci/quirks.c | 2 +- 5 files changed, 71 insertions(+), 13 deletions(-)
include/linux/iommu.h +3 -2
@@ -1224,7 +1224,7 @@ void iommu_free_global_pasid(ioasid_t pasid);/* PCI device reset functions */ int pci_dev_reset_iommu_prepare (struct pci_dev *pdev); -void pci_dev_reset_iommu_done (struct pci_dev *pdev); +void pci_dev_reset_iommu_done (struct pci_dev *pdev,int reset_result);#else /* CONFIG_IOMMU_API */ struct iommu_ops {};
@@ -1554,7 +1554,8 @@ static inline int pci_dev_reset_iommu_prepare(struct pci_dev *pdev) return 0; } -static inline void pci_dev_reset_iommu_done (struct pci_dev *pdev) +static inline void pci_dev_reset_iommu_done (struct pci_dev *pdev, +int reset_result) { }#endif /* CONFIG_IOMMU_API */
drivers/iommu/iommu.c +61 -4
131 lines not on this page · raw message
drivers/pci/pci-acpi.c +1 -1
@@ -977,7 +977,7 @@ int pci_dev_acpi_reset(struct pci_dev *dev, bool probe) ret = -ENOTTY ; } -pci_dev_reset_iommu_done (dev); +pci_dev_reset_iommu_done (dev, ret);return ret; }
drivers/pci/pci.c +5 -5
@@ -4423,7 +4423,7 @@ int pcie_flr(struct pci_dev *dev) ret =pci_dev_wait (dev,"FLR" ,PCIE_RESET_READY_POLL_MS ); done: -pci_dev_reset_iommu_done (dev); +pci_dev_reset_iommu_done (dev, ret);return ret; }EXPORT_SYMBOL_GPL (pcie_flr);
@@ -4501,7 +4501,7 @@ static int pci_af_flr(struct pci_dev *dev, bool probe) ret =pci_dev_wait (dev,"AF_FLR" ,PCIE_RESET_READY_POLL_MS ); done: -pci_dev_reset_iommu_done (dev); +pci_dev_reset_iommu_done (dev, ret);return ret; }
@@ -4555,7 +4555,7 @@ static int pci_pm_reset(struct pci_dev *dev, bool probe)pci_dev_d3_sleep (dev); ret =pci_dev_wait (dev,"PM D3hot->D0" ,PCIE_RESET_READY_POLL_MS ); -pci_dev_reset_iommu_done (dev); +pci_dev_reset_iommu_done (dev, ret);return ret; }
@@ -5011,7 +5011,7 @@ static int pci_reset_bus_function(struct pci_dev *dev, bool probe) rc =pci_parent_bus_reset (dev, probe); done:if (!probe) -pci_dev_reset_iommu_done (dev); +pci_dev_reset_iommu_done (dev, rc);return rc; }
@@ -5064,7 +5064,7 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)pci_write_config_word (bridge, dvsec +PCI_DVSEC_CXL_PORT_CTL , reg); -pci_dev_reset_iommu_done (dev); +pci_dev_reset_iommu_done (dev, rc);return rc; }
drivers/pci/quirks.c +1 -1
@@ -4263,7 +4263,7 @@ static int __pci_dev_specific_reset(struct pci_dev *dev, bool probe, ret = i->reset (dev, probe);if (!probe) -pci_dev_reset_iommu_done (dev); +pci_dev_reset_iommu_done (dev, ret);return ret; }
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v6 05/17] iommu: Pass in reset result to pci_dev_reset_iommu_done()
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 18 lines
IOMMU drivers handle ATC cache maintenance. They may encounter ATC-related errors (e.g., ATC invalidation timeout), indicating that the ATC cache may have stale entries that can corrupt the memory. In this case, IOMMU driver has no choice but to block the device's ATS function and wait for a device recovery.
The
pci_dev_reset_iommu_done()called at the end of a reset function could serve as a reliable signal to the IOMMU subsystem that the physical device cache is completely clean. However, the function is called unconditionally even if the reset operation had actually failed, which would re-attach the faulty device back to a normal translation domain. And this will leave the system highly exposed, creating vulnerabilities for data corruption:
IOMMU blocks RID/ATS
pci_reset_function():
pci_dev_reset_iommu_prepare(); // Block RID/ATS
[ ... ]
the old domain in place ofgroup->domain.
Quoted 3 lines
Suggested-by Kevin Tian <[email protected]>Reviewed-by Lu Baolu <[email protected]>Signed-off-by Nicolin Chen <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
PATCH v6 6/17 iommu/arm-smmu-v3: Don't rb_erase() a never-inserted stream node
In reply to Nicolin Chenarm_smmu_insert_master() skips inserting a stream whose StreamID duplicates one the same master already owns (bridged PCI devices can present duplicate IDs), leaving that master->streams[i].node zeroed and unlinked from the smmu->streams rb-tree.
Both the insert error-rollback loop and arm_smmu_remove_master() then call rb_erase() on every master->streams[i].node unconditionally. rb_erase() on a zeroed node sees a NULL parent, treats the node as the tree root and sets root->rb_node = NULL, silently emptying the whole SID tree and breaking SID lookups (and DMA) for every other master on the SMMU.
Mark each node with RB_CLEAR_NODE() after sort_nonatomic() reorders the array, since sorting relocates the entries and would leave the earlier self-referential RB_CLEAR_NODE() pointer stale. An un-inserted node then stays RB_EMPTY_NODE() and is skipped in both erase loops; inserted nodes are linked by rb_find_add() and erased as before.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +12 -2
@@ -4122,6 +4122,13 @@ static int arm_smmu_insert_master(struct arm_smmu_device *smmu,sizeof (master->streams[0 ]), arm_smmu_stream_id_cmp, NULL); +/* +* Clear after sorting: RB_CLEAR_NODE() records the node's own address, +* which sort_nonatomic() invalidates by relocating the entries. +*/ +for (i =0 ; i < fwspec->num_ids; i++) +RB_CLEAR_NODE (&master->streams[i].node); +mutex_lock (&smmu->streams_mutex);for (i =0 ; i < fwspec->num_ids; i++) {struct arm_smmu_stream *new_stream = &master->streams[i];
@@ -4154,7 +4161,9 @@ static int arm_smmu_insert_master(struct arm_smmu_device *smmu,if (ret) {for (i--; i >=0 ; i--) -rb_erase (&master->streams[i].node, &smmu->streams); +if (!RB_EMPTY_NODE (&master->streams[i].node)) +rb_erase (&master->streams[i].node, + &smmu->streams);kfree (master->streams);kfree (master->build_invs); }
@@ -4174,7 +4183,8 @@ static void arm_smmu_remove_master(struct arm_smmu_master *master)mutex_lock (&smmu->streams_mutex);for (i =0 ; i < fwspec->num_ids; i++) -rb_erase (&master->streams[i].node, &smmu->streams); +if (!RB_EMPTY_NODE (&master->streams[i].node)) +rb_erase (&master->streams[i].node, &smmu->streams);mutex_unlock (&smmu->streams_mutex);kfree (master->streams);
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v6 06/17] iommu/arm-smmu-v3: Don't rb_erase() a never-inserted stream node
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 17 lines
arm_smmu_insert_master()skips inserting a stream whose StreamID duplicates one the same master already owns (bridged PCI devices can present duplicate IDs), leaving that master->streams[i].node zeroed and unlinked from thesmmu->streamsrb-tree.Both the insert error-rollback loop and
arm_smmu_remove_master()then callrb_erase()on every master->streams[i].node unconditionally.rb_erase()on a zeroed node sees a NULL parent, treats the node as the tree root and setsroot->rb_node= NULL, silently emptying the whole SID tree and breaking SID lookups (and DMA) for every other master on the SMMU.Mark each node with
RB_CLEAR_NODE()aftersort_nonatomic()reorders the array, since sorting relocates the entries and would leave the earlier self-referentialRB_CLEAR_NODE()pointer stale. An un-inserted node then staysRB_EMPTY_NODE()and is skipped in both erase loops; inserted nodes are linked byrb_find_add()and erased as before.
Quoted 3 lines
Fixes b00d24997a11 ("iommu/arm-smmu-v3: Fix iommu_device_probe bug due to duplicated stream ids")Assisted-by LLMSigned-off-by Nicolin Chen <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
Re: [PATCH v6 06/17] iommu/arm-smmu-v3: Don't rb_erase() a never-inserted stream node
In reply to Nicolin ChenHi Will,
Quoted 17 lines · Nicolin Chen
arm_smmu_insert_master()skips inserting a stream whose StreamID duplicates one the same master already owns (bridged PCI devices can present duplicate IDs), leaving that master->streams[i].node zeroed and unlinked from thesmmu->streamsrb-tree.Both the insert error-rollback loop and
arm_smmu_remove_master()then callrb_erase()on every master->streams[i].node unconditionally.rb_erase()on a zeroed node sees a NULL parent, treats the node as the tree root and setsroot->rb_node= NULL, silently emptying the whole SID tree and breaking SID lookups (and DMA) for every other master on the SMMU.Mark each node with
RB_CLEAR_NODE()aftersort_nonatomic()reorders the array, since sorting relocates the entries and would leave the earlier self-referentialRB_CLEAR_NODE()pointer stale. An un-inserted node then staysRB_EMPTY_NODE()and is skipped in both erase loops; inserted nodes are linked byrb_find_add()and erased as before.
Quoted 3 lines
Fixes b00d24997a11 ("iommu/arm-smmu-v3: Fix iommu_device_probe bug due to duplicated stream ids")Assisted-by LLMSigned-off-by Nicolin Chen <[email protected]>
There have been two other versions of this fix being sent by other developers in this cycle; both of them were seemingly for the same Sashiko finding as this one.
To avoid more versions sent by more folks, maybe we can prioritize this patch considering that this series might need more time going through the reviews?
Thanks
Nicolin
Re: [PATCH v6 06/17] iommu/arm-smmu-v3: Don't rb_erase() a never-inserted stream node
In reply to Nicolin ChenQuoted 17 lines · Nicolin Chen
arm_smmu_insert_master()skips inserting a stream whose StreamID duplicates one the same master already owns (bridged PCI devices can present duplicate IDs), leaving that master->streams[i].node zeroed and unlinked from thesmmu->streamsrb-tree.Both the insert error-rollback loop and
arm_smmu_remove_master()then callrb_erase()on every master->streams[i].node unconditionally.rb_erase()on a zeroed node sees a NULL parent, treats the node as the tree root and setsroot->rb_node= NULL, silently emptying the whole SID tree and breaking SID lookups (and DMA) for every other master on the SMMU.Mark each node with
RB_CLEAR_NODE()aftersort_nonatomic()reorders the array, since sorting relocates the entries and would leave the earlier self-referentialRB_CLEAR_NODE()pointer stale. An un-inserted node then staysRB_EMPTY_NODE()and is skipped in both erase loops; inserted nodes are linked byrb_find_add()and erased as before.
Quoted 3 lines
Fixes b00d24997a11 ("iommu/arm-smmu-v3: Fix iommu_device_probe bug due to duplicated stream ids")Assisted-by LLMSigned-off-by Nicolin Chen <[email protected]>
Thanks,
Mostafa
Quoted 46 lines
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +12
-2@@ -4122,6 +4122,13 @@ static int arm_smmu_insert_master(struct arm_smmu_device *smmu,sizeof (master->streams[0 ]), arm_smmu_stream_id_cmp, NULL); +/* +* Clear after sorting: RB_CLEAR_NODE() records the node's own address, +* which sort_nonatomic() invalidates by relocating the entries. +*/ +for (i =0 ; i < fwspec->num_ids; i++) +RB_CLEAR_NODE (&master->streams[i].node); +mutex_lock (&smmu->streams_mutex);for (i =0 ; i < fwspec->num_ids; i++) {struct arm_smmu_stream *new_stream = &master->streams[i];@@ -4154,7 +4161,9 @@ static int arm_smmu_insert_master(struct arm_smmu_device *smmu,if (ret) {for (i--; i >=0 ; i--) -rb_erase (&master->streams[i].node, &smmu->streams); +if (!RB_EMPTY_NODE (&master->streams[i].node)) +rb_erase (&master->streams[i].node, + &smmu->streams);kfree (master->streams);kfree (master->build_invs); }@@ -4174,7 +4183,8 @@ static void arm_smmu_remove_master(struct arm_smmu_master *master)mutex_lock (&smmu->streams_mutex);for (i =0 ; i < fwspec->num_ids; i++) -rb_erase (&master->streams[i].node, &smmu->streams); +if (!RB_EMPTY_NODE (&master->streams[i].node)) +rb_erase (&master->streams[i].node, &smmu->streams);mutex_unlock (&smmu->streams_mutex);kfree (master->streams);Signature · 1 line
2.43.0
PATCH v6 7/17 iommu/arm-smmu-v3: Track ATC invalidation timeouts in a bitmap
In reply to Nicolin ChenAn ATC invalidation timeout is a fatal error. While the SMMUv3 hardware is aware of the timeout via a GERROR interrupt, the driver thread issuing the commands lacks a direct mechanism to verify whether its specific batch was the cause or not, as polling the CMD_SYNC status doesn't natively return a failure code, making it very difficult to coordinate per-device recovery.
Introduce an atc_sync_timeouts bitmap in the cmdq structure to bridge this gap. When the ISR detects an ATC timeout, set the bit corresponding to the physical CMDQ index of the faulting CMD_SYNC command.
On the issuer side, test the bit after the poll completes or times out, and return -EIO when it is set, so the caller can quarantine the device. Order the test after the poll with an smp_rmb(), since the poll may use a relaxed load. Test before clearing, to spare the shared cache line an atomic RMW in the common no-timeout case.
Clear any stale bit before publishing a new CMD_SYNC in a reused slot, so the GERROR handler can only mark the new command.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 70 ++++++++++++++++++++- 2 files changed, 70 insertions(+), 1 deletion(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -0
@@ -703,6 +703,7 @@ struct arm_smmu_cmdq {atomic_long_t *valid_map;atomic_t owner_prod;atomic_t lock; +unsigned long *atc_sync_timeouts;bool (*supports_cmd)(struct arm_smmu_cmd *cmd); };
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +69 -1
97 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v6 07/17] iommu/arm-smmu-v3: Track ATC invalidation timeouts in a bitmap
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 16 lines
An ATC invalidation timeout is a fatal error. While the SMMUv3 hardware is aware of the timeout via a GERROR interrupt, the driver thread issuing the commands lacks a direct mechanism to verify whether its specific batch was the cause or not, as polling the
CMD_SYNCstatus doesn't natively return a failure code, making it very difficult to coordinate per-device recovery.Introduce an
atc_sync_timeoutsbitmap in the cmdq structure to bridge this gap. When the ISR detects an ATC timeout, set the bit corresponding to the physical CMDQ index of the faultingCMD_SYNCcommand.On the issuer side, test the bit after the poll completes or times out, and return
-EIOwhen it is set, so the caller can quarantine the device. Order the test after the poll with ansmp_rmb(), since the poll may use a relaxed load. Test before clearing, to spare the shared cache line an atomic RMW in the common no-timeout case.
Quoted 4 lines
Clear any stale bit before publishing a new
CMD_SYNCin a reused slot, so the GERROR handler can only mark the new command.Signed-off-by Nicolin Chen <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
PATCH v6 8/17 iommu/arm-smmu-v3: Skip remaining GERROR causes on SFM
In reply to Nicolin ChenWhen the SMMU enters Service Failure Mode (SFM), arm_smmu_device_disable() clears CR0 and the SMMU stops processing requests entirely. The remaining GERROR causes (MSI write aborts, PRIQ/EVTQ aborts, CMDQ_ERR) are moot at that point: the cmdq is dead so arm_smmu_cmdq_skip_err() would just twiddle bookkeeping for a queue nobody's reading, and the per-cause dev_warn lines add little diagnostic value beyond the SFM message itself.
Ack the GERROR before arm_smmu_device_disable() and return. Acking before the multi-ms disable wait keeps a level-triggered IRQ source from re-firing the handler. The writel+return here duplicates the non-SFM tail because a subsequent commit will give the two paths different locking. SFM is one-way and the SMMU does not generate new GERROR causes, so the ack is final.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3 -0
@@ -2380,8 +2380,11 @@ static irqreturn_t arm_smmu_gerror_handler(int irq, void *dev) active);if (active &GERROR_SFM_ERR ) { +/* SMMU is being disabled, so other errors don't matter */ +writel (gerror, smmu->base +ARM_SMMU_GERRORN );dev_err (smmu->dev,"device has entered Service Failure Mode!\n" );arm_smmu_device_disable (smmu); +return IRQ_HANDLED ; }if (active &GERROR_MSI_GERROR_ABT_ERR )
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v6 08/17] iommu/arm-smmu-v3: Skip remaining GERROR causes on SFM
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 7 lines
When the SMMU enters Service Failure Mode (SFM),
arm_smmu_device_disable()clears CR0 and the SMMU stops processing requests entirely. The remaining GERROR causes (MSI write aborts, PRIQ/EVTQ aborts,CMDQ_ERR) are moot at that point: the cmdq is dead soarm_smmu_cmdq_skip_err()would just twiddle bookkeeping for a queue nobody's reading, and the per-causedev_warnlines add little diagnostic value beyond the SFM message itself.
Quoted 8 lines
Ack the GERROR before
arm_smmu_device_disable()and return. Acking before the multi-ms disable wait keeps a level-triggered IRQ source from re-firing the handler. The writel+return here duplicates the non-SFM tail because a subsequent commit will give the two paths different locking. SFM is one-way and the SMMU does not generate new GERROR causes, so the ack is final.Assisted-by LLMSigned-off-by Nicolin Chen <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
PATCH v6 9/17 iommu/arm-smmu-v3: Introduce per-cmdq cmdq_err_handler callback
In reply to Nicolin ChenA subsequent change will need arm_smmu_cmdq_issue_cmdlist() to co-clear a pending CMDQ_ERR after a CMD_SYNC poll timeout. And this needs to be done for both smmu->cmdq and tegra241-cmdq.
Add a cmdq_err_handler and a paired cmdq_err_lock to struct arm_smmu_cmdq. arm_smmu_gerror_handler() now takes the per-cmdq cmdq_err_lock when acking CMDQ_ERR. It already covers a concurrent ack from cmdq_err_handler via its existing early-exit on no-active-bits.
Impl functions and caller will be added in the subsequent change.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 12 ++++++++++-- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 17 +++++++++++++---- drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 2 +- 3 files changed, 24 insertions(+), 7 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +10 -2
@@ -698,6 +698,10 @@ struct arm_smmu_queue_poll {bool wfe; }; +struct arm_smmu_cmdq ; +typedef void (*arm_smmu_cmdq_err_fn )(struct arm_smmu_device *smmu, +struct arm_smmu_cmdq *cmdq); +struct arm_smmu_cmdq {struct arm_smmu_queue q;atomic_long_t *valid_map;
@@ -705,6 +709,10 @@ struct arm_smmu_cmdq {atomic_t lock;unsigned long *atc_sync_timeouts;bool (*supports_cmd)(struct arm_smmu_cmd *cmd); + +/* Drain a pending CMDQ_ERR; will hold cmdq_err_lock with irqsave */ +arm_smmu_cmdq_err_fn cmdq_err_handler; +raw_spinlock_t cmdq_err_lock; }; static inline bool arm_smmu_cmdq_supports_cmd(struct arm_smmu_cmdq *cmdq,
@@ -1170,8 +1178,8 @@ int arm_smmu_init_one_queue(struct arm_smmu_device *smmu, struct arm_smmu_queue *q, void __iomem *page, unsigned long prod_off, unsigned long cons_off, size_t dwords, const char *name); -int arm_smmu_cmdq_init (struct arm_smmu_device *smmu, -struct arm_smmu_cmdq *cmdq); +int arm_smmu_cmdq_init (struct arm_smmu_device *smmu,struct arm_smmu_cmdq *cmdq, +arm_smmu_cmdq_err_fn cmdq_err_handler);static inline bool arm_smmu_master_canwbs (struct arm_smmu_master *master) {
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +13 -4
@@ -2367,13 +2367,18 @@ static irqreturn_t arm_smmu_gerror_handler(int irq, void *dev) {u32 gerror, gerrorn, active;struct arm_smmu_device *smmu = dev; +unsigned long flags; + +raw_spin_lock_irqsave (&smmu->cmdq.cmdq_err_lock, flags); gerror =readl_relaxed (smmu->base +ARM_SMMU_GERROR ); gerrorn =readl_relaxed (smmu->base +ARM_SMMU_GERRORN ); active = gerror ^ gerrorn; -if (!(active &GERROR_ERR_MASK )) +if (!(active &GERROR_ERR_MASK )) { +raw_spin_unlock_irqrestore (&smmu->cmdq.cmdq_err_lock, flags);return IRQ_NONE ;/* No errors pending */ + } dev_warn(smmu->dev, "unexpected global error reported (0x%08x), this could be serious\n",
@@ -2382,6 +2387,7 @@ static irqreturn_t arm_smmu_gerror_handler(int irq, void *dev)if (active &GERROR_SFM_ERR ) {/* SMMU is being disabled, so other errors don't matter */ writel (gerror, smmu->base +ARM_SMMU_GERRORN ); +raw_spin_unlock_irqrestore (&smmu->cmdq.cmdq_err_lock, flags);dev_err (smmu->dev,"device has entered Service Failure Mode!\n" );arm_smmu_device_disable (smmu);return IRQ_HANDLED ;
@@ -2409,6 +2415,7 @@ static irqreturn_t arm_smmu_gerror_handler(int irq, void *dev)arm_smmu_cmdq_skip_err (smmu);writel (gerror, smmu->base +ARM_SMMU_GERRORN ); +raw_spin_unlock_irqrestore (&smmu->cmdq.cmdq_err_lock, flags);return IRQ_HANDLED ; }
@@ -4529,13 +4536,15 @@ int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,return 0 ; } -int arm_smmu_cmdq_init (struct arm_smmu_device *smmu, -struct arm_smmu_cmdq *cmdq) +int arm_smmu_cmdq_init (struct arm_smmu_device *smmu,struct arm_smmu_cmdq *cmdq, +arm_smmu_cmdq_err_fn cmdq_err_handler) {unsigned int nents =1 << cmdq->q.llq.max_n_shift;atomic_set (&cmdq->owner_prod,0 );atomic_set (&cmdq->lock,0 ); +raw_spin_lock_init (&cmdq->cmdq_err_lock); + cmdq->cmdq_err_handler = cmdq_err_handler; cmdq->valid_map = (atomic_long_t *)devm_bitmap_zalloc (smmu->dev, nents,GFP_KERNEL );
@@ -4575,7 +4584,7 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu)if (ret)return ret; - ret =arm_smmu_cmdq_init (smmu, &smmu->cmdq); + ret =arm_smmu_cmdq_init (smmu, &smmu->cmdq,NULL );if (ret)return ret;
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +1 -1
@@ -676,7 +676,7 @@ static int tegra241_vcmdq_alloc_smmu_cmdq(struct tegra241_vcmdq *vcmdq) q->q_base = q->base_dma &VCMDQ_ADDR ; q->q_base |=FIELD_PREP (VCMDQ_LOG2SIZE , q->llq.max_n_shift); -return arm_smmu_cmdq_init (smmu, cmdq); +return arm_smmu_cmdq_init (smmu, cmdq,NULL ); }/* VINTF Logical VCMDQ Resource Helpers */
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v6 09/17] iommu/arm-smmu-v3: Introduce per-cmdq cmdq_err_handler callback
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 9 lines
A subsequent change will need
arm_smmu_cmdq_issue_cmdlist()to co-clear a pendingCMDQ_ERRafter aCMD_SYNCpoll timeout. And this needs to be done for bothsmmu->cmdqand tegra241-cmdq.Add a
cmdq_err_handlerand a pairedcmdq_err_lockto structarm_smmu_cmdq.arm_smmu_gerror_handler()now takes the per-cmdqcmdq_err_lockwhen ackingCMDQ_ERR. It already covers a concurrent ack fromcmdq_err_handlervia its existing early-exit on no-active-bits.
Quoted 4 lines
Impl functions and caller will be added in the subsequent change.
Assisted-by LLMSigned-off-by Nicolin Chen <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9
PATCH v6 10/17 iommu/arm-smmu-v3: Recheck CMDQ_ERR in tegra241_vintf0_handle_error()
In reply to Nicolin ChenA subsequent change will allow cmdq_err_handler to ack a pending CMDQ_ERR concurrently with tegra241_vintf0_handle_error(). Take cmdq_err_lock around the gerror read and ack, and recheck (gerror ^ gerrorn) & GERROR_CMDQ_ERR before calling __arm_smmu_cmdq_skip_err() so a concurrent ack doesn't cause us to skip_err on an already-handled error.
arm_smmu_gerror_handler() already covers this via its existing early-exit on no-active-bits.
tegra241_vcmdq_hw_deinit() acks the same GERROR/GERRORN pair unlocked, and the error IRQ is live from probe, so a latched-error ISR can race a VCMDQ deinit during a device reset. Take the lock around that ack as well. Since a user-owned VCMDQ never goes through arm_smmu_cmdq_init() yet does reach tegra241_vcmdq_hw_deinit(), initialize its cmdq_err_lock at allocation.
--- drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +15 -2
@@ -323,7 +323,8 @@ static void tegra241_vintf0_handle_error(struct tegra241_vintf *vintf)unsigned long map_bit =__ffs64 (map);unsigned long lidx =64 * i + map_bit;struct tegra241_vcmdq *vcmdq; -u32 gerror; +unsigned long flags; +u32 gerror, gerrorn; map &= ~BIT_ULL (map_bit);
@@ -335,9 +336,16 @@ static void tegra241_vintf0_handle_error(struct tegra241_vintf *vintf)if (!vcmdq)continue ; +raw_spin_lock_irqsave (&vcmdq->cmdq.cmdq_err_lock, flags); gerror =readl_relaxed (REG_VCMDQ_PAGE0 (vcmdq,GERROR )); -__arm_smmu_cmdq_skip_err (&cmdqv->smmu, &vcmdq->cmdq); + gerrorn =readl_relaxed (REG_VCMDQ_PAGE0 (vcmdq,GERRORN )); + +if ((gerror ^ gerrorn) &GERROR_CMDQ_ERR ) +__arm_smmu_cmdq_skip_err (&cmdqv->smmu, + &vcmdq->cmdq);writel (gerror,REG_VCMDQ_PAGE0 (vcmdq,GERRORN )); +raw_spin_unlock_irqrestore (&vcmdq->cmdq.cmdq_err_lock, + flags); } } }
@@ -477,6 +485,7 @@ static void tegra241_vcmdq_hw_deinit(struct tegra241_vcmdq *vcmdq) {char header[64 ], *h =lvcmdq_error_header (vcmdq, header,64 );u32 gerrorn, gerror; +unsigned long flags;if (vcmdq_write_config (vcmdq,0 )) { dev_err(vcmdq->cmdqv->dev,
@@ -492,6 +501,7 @@ static void tegra241_vcmdq_hw_deinit(struct tegra241_vcmdq *vcmdq)writeq_relaxed (0 ,REG_VCMDQ_PAGE1 (vcmdq,BASE ));writeq_relaxed (0 ,REG_VCMDQ_PAGE1 (vcmdq,CONS_INDX_BASE )); +raw_spin_lock_irqsave (&vcmdq->cmdq.cmdq_err_lock, flags); gerrorn =readl_relaxed (REG_VCMDQ_PAGE0 (vcmdq,GERRORN )); gerror =readl_relaxed (REG_VCMDQ_PAGE0 (vcmdq,GERROR ));if (gerror != gerrorn) {
@@ -499,6 +509,7 @@ static void tegra241_vcmdq_hw_deinit(struct tegra241_vcmdq *vcmdq) "%suncleared error detected, resetting\n", h);writel (gerror,REG_VCMDQ_PAGE0 (vcmdq,GERRORN )); } +raw_spin_unlock_irqrestore (&vcmdq->cmdq.cmdq_err_lock, flags);dev_dbg (vcmdq->cmdqv->dev,"%sdeinited\n" , h); }
@@ -1206,6 +1217,8 @@ static int tegra241_vintf_alloc_lvcmdq_user(struct iommufd_hw_queue *hw_queue, vcmdq->cmdq.q.q_base = base_addr_pa &VCMDQ_ADDR ; vcmdq->cmdq.q.q_base |= log2size; +raw_spin_lock_init (&vcmdq->cmdq.cmdq_err_lock); + ret =tegra241_vcmdq_hw_init_user (vcmdq);if (ret) goto unmap_lvcmdq;
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v6 10/17] iommu/arm-smmu-v3: Recheck CMDQ_ERR in tegra241_vintf0_handle_error()
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 9 lines
A subsequent change will allow
cmdq_err_handlerto ack a pendingCMDQ_ERRconcurrently withtegra241_vintf0_handle_error(). Takecmdq_err_lockaround the gerror read and ack, and recheck (gerror ^ gerrorn) &GERROR_CMDQ_ERRbefore calling__arm_smmu_cmdq_skip_err()so a concurrent ack doesn't cause us toskip_erron an already-handled error.
arm_smmu_gerror_handler()already covers this via its existing early-exit on no-active-bits.
Quoted 7 lines
tegra241_vcmdq_hw_deinit()acks the same GERROR/GERRORN pair unlocked, and the error IRQ is live from probe, so a latched-error ISR can race a VCMDQ deinit during a device reset. Take the lock around that ack as well. Since a user-owned VCMDQ never goes througharm_smmu_cmdq_init()yet does reachtegra241_vcmdq_hw_deinit(), initialize itscmdq_err_lockat allocation.Signed-off-by Nicolin Chen <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10
PATCH v6 11/17 iommu/arm-smmu-v3: Co-clear pending CMDQ_ERR when CMD_SYNC times out
In reply to Nicolin ChenOnce arm_smmu_cmdq_poll_until_sync() returns, arm_smmu_cmdq_issue_cmdlist() tests its CMD_SYNC slot in atc_sync_timeouts to decide whether there was an ATC_INV timeout.
On the other hand, when that poll timed out, the GERROR ISR might have been delayed past the poll deadline, so the atc_sync_timeouts test could miss an ATC_INV timeout, classifying it as a generic CMD_SYNC timeout and bypassing the per-device quarantine.
Add two cmdq_err_handler impl functions:
- arm_smmu_cmdq_err_handler() reads SMMU GERROR/GERRORN.
- tegra241_vcmdq_handle_cmdq_err() reads VCMDQ GERROR/GERRORN.
Co-clear any pending CMDQ_ERR in the issuer, when the polling on a CMD_SYNC times out. Each cmdq impl serializes the synchronous drain against its own IRQ handler with cmdq->cmdq_err_lock.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 31 ++++++++++++++++++- .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 24 +++++++++++++- 2 files changed, 53 insertions(+), 2 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +30 -1
@@ -822,6 +822,15 @@ int __arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu, sync_prod = llq.prod; ret =arm_smmu_cmdq_poll_until_sync (smmu, cmdq, &llq); +/* +* When the poll above timed out, the GERROR ISR might have been +* delayed past the poll deadline, so the atc_sync_timeouts test +* below could miss our ATC_INV timeout. Thus, drain any pending +* CMDQ_ERR synchronously first via the per-cmdq callback. +*/ +if (ret && cmdq->cmdq_err_handler) + cmdq->cmdq_err_handler (smmu, cmdq); +/* * Ensure that the read in __arm_smmu_cmdq_poll_until_msi() or * __arm_smmu_cmdq_poll_until_consumed() are completed, before
@@ -2363,6 +2372,26 @@ static irqreturn_t arm_smmu_priq_thread(int irq, void *dev)static int arm_smmu_device_disable (struct arm_smmu_device *smmu); +/* Drain a pending CMDQ_ERR, used by arm_smmu_cmdq_issue_cmdlist() */ +static void arm_smmu_cmdq_err_handler (struct arm_smmu_device *smmu, +struct arm_smmu_cmdq *cmdq) +{ +u32 gerror, gerrorn; + +guard (raw_spinlock_irqsave)(&cmdq->cmdq_err_lock); + + gerror =readl_relaxed (smmu->base +ARM_SMMU_GERROR ); + gerrorn =readl_relaxed (smmu->base +ARM_SMMU_GERRORN ); + +if (!((gerror ^ gerrorn) &GERROR_CMDQ_ERR )) +return ; + +__arm_smmu_cmdq_skip_err (smmu, cmdq); + +/* Toggle only the CMDQ_ERR bit; other bits are left for the ISR. */ +writel (gerrorn ^GERROR_CMDQ_ERR , smmu->base +ARM_SMMU_GERRORN ); +} +static irqreturn_t arm_smmu_gerror_handler (int irq,void *dev) {u32 gerror, gerrorn, active;
@@ -4584,7 +4613,7 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu)if (ret)return ret; - ret =arm_smmu_cmdq_init (smmu, &smmu->cmdq,NULL ); + ret =arm_smmu_cmdq_init (smmu, &smmu->cmdq, arm_smmu_cmdq_err_handler);if (ret)return ret;
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +23 -1
@@ -350,6 +350,28 @@ static void tegra241_vintf0_handle_error(struct tegra241_vintf *vintf) } } +/* Drain a pending CMDQ_ERR, used by arm_smmu_cmdq_issue_cmdlist() */ +static void tegra241_vcmdq_handle_cmdq_err (struct arm_smmu_device *smmu, +struct arm_smmu_cmdq *cmdq) +{ +struct tegra241_vcmdq *vcmdq = + container_of(cmdq, struct tegra241_vcmdq, cmdq); +u32 gerror, gerrorn; + +guard (raw_spinlock_irqsave)(&cmdq->cmdq_err_lock); + + gerror =readl_relaxed (REG_VCMDQ_PAGE0 (vcmdq,GERROR )); + gerrorn =readl_relaxed (REG_VCMDQ_PAGE0 (vcmdq,GERRORN )); + +if (!((gerror ^ gerrorn) &GERROR_CMDQ_ERR )) +return ; + +__arm_smmu_cmdq_skip_err (smmu, cmdq); + +/* Toggle only the CMDQ_ERR bit on this VCMDQ's GERRORN */ +writel (gerrorn ^GERROR_CMDQ_ERR ,REG_VCMDQ_PAGE0 (vcmdq,GERRORN )); +} +/* * The CMDQV error interrupt is edge-triggered, so a pending VINTF error fires * this ISR once and does not re-assert. An unacked guest therefore cannot
@@ -687,7 +709,7 @@ static int tegra241_vcmdq_alloc_smmu_cmdq(struct tegra241_vcmdq *vcmdq) q->q_base = q->base_dma &VCMDQ_ADDR ; q->q_base |=FIELD_PREP (VCMDQ_LOG2SIZE , q->llq.max_n_shift); -return arm_smmu_cmdq_init (smmu, cmdq,NULL ); +return arm_smmu_cmdq_init (smmu, cmdq, tegra241_vcmdq_handle_cmdq_err); }/* VINTF Logical VCMDQ Resource Helpers */
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v6 11/17] iommu/arm-smmu-v3: Co-clear pending CMDQ_ERR when CMD_SYNC times out
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 13 lines
Once
arm_smmu_cmdq_poll_until_sync()returns,arm_smmu_cmdq_issue_cmdlist()tests itsCMD_SYNCslot inatc_sync_timeoutsto decide whether there was anATC_INVtimeout.On the other hand, when that poll timed out, the GERROR ISR might have been delayed past the poll deadline, so the
atc_sync_timeoutstest could miss anATC_INVtimeout, classifying it as a genericCMD_SYNCtimeout and bypassing the per-device quarantine.Add two
cmdq_err_handlerimpl functions:
-arm_smmu_cmdq_err_handler()reads SMMU GERROR/GERRORN.
-tegra241_vcmdq_handle_cmdq_err()reads VCMDQ GERROR/GERRORN.
Quoted 6 lines
Co-clear any pending
CMDQ_ERRin the issuer, when the polling on aCMD_SYNCtimes out. Each cmdq impl serializes the synchronous drain against its own IRQ handler withcmdq->cmdq_err_lock.Assisted-by LLMSigned-off-by Nicolin Chen <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11
PATCH v6 12/17 iommu/arm-smmu-v3: Introduce arm_smmu_cmdq_batch_issue() wrapper
In reply to Nicolin ChenBoth arm_smmu_cmdq_batch_submit() and arm_smmu_cmdq_batch_add_cmd_p() call arm_smmu_cmdq_issue_cmdlist() to flush batches. A future change will retry the issued commands on -EIO, using the arm_smmu_invs carried in the batch. So, a single hook point is preferred.
Introduce an arm_smmu_cmdq_batch_issue() wrapper, so a retry logic will be simply filled into the wrapper.
No functional changes.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +11 -6
@@ -965,6 +965,14 @@ static void arm_smmu_cmdq_batch_init_cmd(struct arm_smmu_device *smmu, cmds->cmdq =arm_smmu_get_cmdq (smmu, cmd); } +static int arm_smmu_cmdq_batch_issue (struct arm_smmu_device *smmu, +struct arm_smmu_cmdq_batch *cmds, +bool sync) +{ +return arm_smmu_cmdq_issue_cmdlist (smmu, cmds->cmdq, cmds->cmds, + cmds->num, sync); +} +static bool arm_smmu_cmdq_batch_force_sync (struct arm_smmu_device *smmu,struct arm_smmu_cmdq_batch *cmds,struct arm_smmu_cmd *cmd)
@@ -994,14 +1002,12 @@ static void arm_smmu_cmdq_batch_add_cmd_p(struct arm_smmu_device *smmu, struct arm_smmu_cmd *cmd) {if (arm_smmu_cmdq_batch_force_sync (smmu, cmds, cmd)) { -arm_smmu_cmdq_issue_cmdlist (smmu, cmds->cmdq, cmds->cmds, - cmds->num, true); +arm_smmu_cmdq_batch_issue (smmu, cmds, true);arm_smmu_cmdq_batch_init_cmd (smmu, cmds, cmd); }if (cmds->num ==CMDQ_BATCH_ENTRIES ) { -arm_smmu_cmdq_issue_cmdlist (smmu, cmds->cmdq, cmds->cmds, - cmds->num, false); +arm_smmu_cmdq_batch_issue (smmu, cmds, false);arm_smmu_cmdq_batch_init_cmd (smmu, cmds, cmd); }
@@ -1017,8 +1023,7 @@ static void arm_smmu_cmdq_batch_add_cmd_p(struct arm_smmu_device *smmu,static int arm_smmu_cmdq_batch_submit (struct arm_smmu_device *smmu,struct arm_smmu_cmdq_batch *cmds) { -return arm_smmu_cmdq_issue_cmdlist (smmu, cmds->cmdq, cmds->cmds, - cmds->num, true); +return arm_smmu_cmdq_batch_issue (smmu, cmds, true); } static void arm_smmu_page_response(struct device *dev, struct iopf_fault *unused,
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v6 12/17] iommu/arm-smmu-v3: Introduce arm_smmu_cmdq_batch_issue() wrapper
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 5 lines
Both
arm_smmu_cmdq_batch_submit()andarm_smmu_cmdq_batch_add_cmd_p()callarm_smmu_cmdq_issue_cmdlist()to flush batches. A future change will retry the issued commands on-EIO, using thearm_smmu_invscarried in the batch. So, a single hook point is preferred.
Quoted 7 lines
Introduce an
arm_smmu_cmdq_batch_issue()wrapper, so a retry logic will be simply filled into the wrapper.No functional changes.
Assisted-by LLMSigned-off-by Nicolin Chen <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12
PATCH v6 13/17 iommu/arm-smmu-v3: Add streams_lock for atomic-context SID->master lookup
In reply to Nicolin ChenA subsequent change will look up arm_smmu_master entries by SID from inside arm_smmu_cmdq_batch_retry(), which runs with invs->rwlock read_lock held in IRQ-disabled context, and so cannot take the sleeping streams_mutex.
Add a spinlock_t streams_lock that protects rb_root mutations alongside the existing streams_mutex:
- atomic-context readers will hold the spinlock alone
- writers (insert/remove paths) take both
A reader under the streams_lock uses all the streams of the found master, so the insertion has to be all-or-nothing: make arm_smmu_insert_master() initialize all the L2 strtabs first and then insert all the stream nodes in one critical section, making a master found via any single SID always fully initialized.
Update the lockdep assertion in arm_smmu_find_master() to accept either of the locks so the helper is callable from both contexts.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 95 ++++++++++++++------- 2 files changed, 65 insertions(+), 32 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +2 -0
@@ -972,6 +972,8 @@ struct arm_smmu_device {struct rb_root streams;struct mutex streams_mutex; +/* Held during rb_root updates; allows atomic-context lookups */ +spinlock_t streams_lock; };struct arm_smmu_stream {
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +63 -32
131 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v6 13/17] iommu/arm-smmu-v3: Add streams_lock for atomic-context SID->master lookup
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 15 lines
A subsequent change will look up
arm_smmu_masterentries by SID from insidearm_smmu_cmdq_batch_retry(), which runs withinvs->rwlockread_lockheld in IRQ-disabled context, and so cannot take the sleepingstreams_mutex.Add a
spinlock_tstreams_lockthat protectsrb_rootmutations alongside the existingstreams_mutex:
- atomic-context readers will hold the spinlock alone
- writers (insert/remove paths) take bothA reader under the
streams_lockuses all the streams of the found master, so the insertion has to be all-or-nothing: makearm_smmu_insert_master()initialize all the L2 strtabs first and then insert all the stream nodes in one critical section, making a master found via any single SID always fully initialized.
Quoted 6 lines
Update the lockdep assertion in
arm_smmu_find_master()to accept either of the locks so the helper is callable from both contexts.Suggested-by Jason Gunthorpe <[email protected]>Assisted-by LLMSigned-off-by Nicolin Chen <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13
PATCH v6 14/17 iommu/arm-smmu-v3: Add has_ats to struct arm_smmu_cmdq_batch
In reply to Nicolin ChenThe arm_smmu_cmdq_batch_add_cmd_p() might flush a sub-batch mid-way, when the ARM_SMMU_OPT_CMDQ_FORCE_SYNC is set or when a batch is full. To allow a future change to retry these sub-batch flushes on a timeout and identify the broken master, the batch needs to know whether it holds an ATC_INV.
Add a "has_ats" flag, set by arm_smmu_cmdq_batch_add_cmd_p() when it queues an ATC_INV command.
No functional changes.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 ++ drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 +++ 2 files changed, 5 insertions(+)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +2 -0
@@ -724,6 +724,8 @@ static inline bool arm_smmu_cmdq_supports_cmd(struct arm_smmu_cmdq *cmdq,struct arm_smmu_cmdq_batch {struct arm_smmu_cmd cmds[CMDQ_BATCH_ENTRIES ];struct arm_smmu_cmdq *cmdq; +/* Set when an ATC_INV is queued; gates the retry-aware sync decision */ +bool has_ats;int num; };
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3 -0
@@ -963,6 +963,7 @@ static void arm_smmu_cmdq_batch_init_cmd(struct arm_smmu_device *smmu, { cmds->num =0 ; cmds->cmdq =arm_smmu_get_cmdq (smmu, cmd); + cmds->has_ats = false; } static int arm_smmu_cmdq_batch_issue(struct arm_smmu_device *smmu,
@@ -1011,6 +1012,8 @@ static void arm_smmu_cmdq_batch_add_cmd_p(struct arm_smmu_device *smmu,arm_smmu_cmdq_batch_init_cmd (smmu, cmds, cmd); } +if (FIELD_GET (CMDQ_0_OP , cmd->data[0 ]) ==CMDQ_OP_ATC_INV ) + cmds->has_ats = true; cmds->cmds[cmds->num++] = *cmd; }
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v6 14/17] iommu/arm-smmu-v3: Add has_ats to struct arm_smmu_cmdq_batch
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 5 lines
The
arm_smmu_cmdq_batch_add_cmd_p()might flush a sub-batch mid-way, when theARM_SMMU_OPT_CMDQ_FORCE_SYNCis set or when a batch is full. To allow a future change to retry these sub-batch flushes on a timeout and identify the broken master, the batch needs to know whether it holds anATC_INV.
Quoted 7 lines
Add a "
has_ats" flag, set byarm_smmu_cmdq_batch_add_cmd_p()when it queues anATC_INVcommand.No functional changes.
Assisted-by LLMSigned-off-by Nicolin Chen <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14
PATCH v6 15/17 iommu/arm-smmu-v3: Add INV_TYPE_ATS_BROKEN for quarantined masters
In reply to Nicolin ChenClearing STE.EATS makes the SMMU reject ATS requests from any quarantined master, but does not stop the driver from issuing ATC invalidations for it. Those commands would continue to time out.
Introduce INV_TYPE_ATS_BROKEN, which a subsequent change will assign to the master's ATS invalidation entries. Treat it as an ATS type during iteration and ordering, but skip it when issuing commands.
The subsequent quarantine path changes inv->type in place via WRITE_ONCE() while readers hold the invalidation array's read lock. Since a u8 load is already atomic, arm_smmu_inv_type() uses READ_ONCE() to annotate the race for KCSAN. Whole-struct copies in merge and purge cannot use READ_ONCE(), so annotate those copies with data_race().
A stale read at worst issues one more ATC_INV, which times out and causes the master to be quarantined again.
Treat ATS, ATS_FULL, and ATS_BROKEN as one comparator class. This preserves the sort position and matching identity when an entry is changed in place.
No functional change yet; the new type is never set anywhere.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 12 +++++++- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 33 +++++++++++++++------ 2 files changed, 35 insertions(+), 10 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +11 -1
@@ -739,6 +739,7 @@ enum arm_smmu_inv_type {INV_TYPE_S2_VMID_S1_CLEAR ,INV_TYPE_ATS ,INV_TYPE_ATS_FULL , + INV_TYPE_ATS_BROKEN, }; struct arm_smmu_inv {
@@ -755,9 +756,18 @@ struct arm_smmu_inv {int users;/* users=0 to mark as a trash to be purged */ }; +/* cur->type may flip to INV_TYPE_ATS_BROKEN concurrently with readers */ +static inline u8 arm_smmu_inv_type (const struct arm_smmu_inv *inv) +{ +return READ_ONCE (inv->type); +} +static inline bool arm_smmu_inv_is_ats (const struct arm_smmu_inv *inv) { -return inv->type ==INV_TYPE_ATS || inv->type ==INV_TYPE_ATS_FULL ; +u8 type =arm_smmu_inv_type (inv); + +return type ==INV_TYPE_ATS || type ==INV_TYPE_ATS_FULL || + type ==INV_TYPE_ATS_BROKEN ; }/**
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +24 -9
@@ -1094,13 +1094,21 @@ arm_smmu_invs_iter_next(struct arm_smmu_invs *invs, size_t next, size_t *idx)static int arm_smmu_inv_cmp (const struct arm_smmu_inv *inv_l,const struct arm_smmu_inv *inv_r) { +/* +* Treat all ATS types as one class, so an in-place flip to ATS_BROKEN +* preserves the sort order and still matches the original ATS entry. +*/ +bool are_ats =arm_smmu_inv_is_ats (inv_l) &arm_smmu_inv_is_ats (inv_r); +u8 type_l =arm_smmu_inv_type (inv_l); +u8 type_r =arm_smmu_inv_type (inv_r); +if (inv_l->smmu != inv_r->smmu)return cmp_int ((uintptr_t )inv_l->smmu, (uintptr_t )inv_r->smmu); -if (inv_l->type != inv_r->type) -return cmp_int (inv_l->type, inv_r->type); +if (!are_ats && type_l != type_r) +return cmp_int (type_l, type_r);if (inv_l->id != inv_r->id)return cmp_int (inv_l->id, inv_r->id); -if (arm_smmu_inv_is_ats (inv_l)) +if (are_ats)return cmp_int (inv_l->ssid, inv_r->ssid);return 0 ; }
@@ -1186,11 +1194,12 @@ struct arm_smmu_invs *arm_smmu_invs_merge(struct arm_smmu_invs *invs,return ERR_PTR (-ENOMEM ); new = new_invs->inv; +/* data_race(): a racing quarantine may flip ->type; the u8 is safe */ arm_smmu_invs_for_each_cmp (invs, i, to_merge, j, cmp) {if (cmp <0 ) { - *new = invs->inv[i]; + *new =data_race (invs->inv[i]); }else if (cmp ==0 ) { - *new = invs->inv[i]; + *new =data_race (invs->inv[i]);WRITE_ONCE (new->users,READ_ONCE (new->users) +1 ); }else { *new = to_merge->inv[j];
@@ -1312,8 +1321,9 @@ struct arm_smmu_invs *arm_smmu_invs_purge(struct arm_smmu_invs *invs)if (!new_invs)return NULL ; +/* data_race(): a racing quarantine may flip ->type; the u8 is safe */ arm_smmu_invs_for_each_entry (invs, i, inv) { - new_invs->inv[num_invs] = *inv; + new_invs->inv[num_invs] =data_race (*inv);if (arm_smmu_inv_is_ats (inv)) new_invs->has_ats = true; num_invs++;
@@ -2700,8 +2710,8 @@ static inline bool arm_smmu_invs_end_batch(struct arm_smmu_inv *cur,if (cur->smmu != next->smmu)return true;/* The batch for S2 TLBI must be done before nested S1 ASIDs */ -if (cur->type !=INV_TYPE_S2_VMID_S1_CLEAR && - next->type ==INV_TYPE_S2_VMID_S1_CLEAR ) +if (arm_smmu_inv_type (cur) !=INV_TYPE_S2_VMID_S1_CLEAR && +arm_smmu_inv_type (next) ==INV_TYPE_S2_VMID_S1_CLEAR )return true;/* ATS must be after a sync of the S1/S2 invalidations */ if (!arm_smmu_inv_is_ats (cur) &&arm_smmu_inv_is_ats (next))
@@ -2737,7 +2747,7 @@ static void __arm_smmu_domain_inv_range(struct arm_smmu_invs *invs,if (!cmds.num)arm_smmu_cmdq_batch_init_cmd (smmu, &cmds, &cmd); -switch (cur->type) { +switch (arm_smmu_inv_type (cur)) {case INV_TYPE_S1_ASID : cmd =arm_smmu_make_cmd_tlbi (cur->size_opcode, cur->id,0 );
@@ -2771,6 +2781,9 @@ static void __arm_smmu_domain_inv_range(struct arm_smmu_invs *invs,arm_smmu_make_cmd_atc_inv_all (cur->id, IOMMU_NO_PASID));break ; +case INV_TYPE_ATS_BROKEN : +/* Master is quarantined; skip its ATC_INV */ +break ;default :WARN_ON_ONCE (1 );break ;
@@ -3326,6 +3339,8 @@ arm_smmu_master_build_inv(struct arm_smmu_master *master, cur->size_opcode = cur->nsize_opcode =CMDQ_OP_ATC_INV ; cur->ssid = ssid;break ; +case INV_TYPE_ATS_BROKEN : +break ; }return cur;
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v6 15/17] iommu/arm-smmu-v3: Add INV_TYPE_ATS_BROKEN for quarantined masters
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 17 lines
Clearing STE.EATS makes the SMMU reject ATS requests from any quarantined master, but does not stop the driver from issuing ATC invalidations for it. Those commands would continue to time out.
Introduce
INV_TYPE_ATS_BROKEN, which a subsequent change will assign to the master's ATS invalidation entries. Treat it as an ATS type during iteration and ordering, but skip it when issuing commands.The subsequent quarantine path changes
inv->typein place viaWRITE_ONCE()while readers hold the invalidation array's read lock. Since a u8 load is already atomic,arm_smmu_inv_type()usesREAD_ONCE()to annotate the race for KCSAN. Whole-struct copies in merge and purge cannot useREAD_ONCE(), so annotate those copies withdata_race().A stale read at worst issues one more
ATC_INV, which times out and causes the master to be quarantined again.
Quoted 8 lines
Treat ATS,
ATS_FULL, andATS_BROKENas one comparator class. This preserves the sort position and matching identity when an entry is changed in place.No functional change yet; the new type is never set anywhere.
Suggested-by Jason Gunthorpe <[email protected]>Assisted-by LLMSigned-off-by Nicolin Chen <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15
PATCH v6 16/17 iommu/arm-smmu-v3: Thread arm_smmu_master_domain on a per-master list
In reply to Nicolin ChenA subsequent change needs to enumerate, from the CMDQ error path in atomic context, every domain a master is attached to so it can mark this master's ATS entries broken in each domain's invs after an ATC invalidation timeout. The existing per-domain smmu_domain->devices list tracks the inverse direction (masters in a given domain), so introduce a per-master list.
Add a second list_head master_elm to arm_smmu_master_domain, threaded onto a new master->master_domains list under master_domains_lock. The CMDQ error path walks the list while holding smmu->streams_lock; that path runs under the invs->rwlock read side, which is itself sleepable on PREEMPT_RT, so a plain spinlock_t suffices for both. The attach and detach sites now take it with spin_lock(), nested inside the existing devices_lock critical section that already disables IRQs; it is a leaf in the lock order, so no inversion is introduced.
No functional change.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 4 ++++ drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 ++++++++ 2 files changed, 12 insertions(+)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +4 -0
@@ -1035,6 +1035,9 @@ struct arm_smmu_master {struct arm_smmu_vmaster *vmaster;/* use smmu->streams_mutex */ /* Locked by the iommu core using the group mutex */ struct arm_smmu_ctx_desc_cfg cd_table; +struct list_head master_domains; +/* Protects master_domains */ +spinlock_t master_domains_lock;unsigned int num_streams; bool ats_enabled : 1; bool ste_ats_enabled : 1;
@@ -1129,6 +1132,7 @@ struct arm_smmu_invs *arm_smmu_invs_purge(struct arm_smmu_invs *invs);struct arm_smmu_master_domain {struct list_head devices_elm; +struct list_head master_elm;struct arm_smmu_master *master;/* * For nested domains the master_domain is threaded onto the S2 parent,
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +8 -0
@@ -3431,6 +3431,9 @@ static void arm_smmu_remove_master_domain(struct arm_smmu_master *master, ssid, nested_ats_flush);if (master_domain) {list_del (&master_domain->devices_elm); +spin_lock (&master->master_domains_lock); +list_del (&master_domain->master_elm); +spin_unlock (&master->master_domains_lock);if (master->ats_enabled)atomic_dec (&smmu_domain->nr_ats_masters); }
@@ -3681,6 +3684,9 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,if (state->ats_enabled)atomic_inc (&smmu_domain->nr_ats_masters);list_add (&master_domain->devices_elm, &smmu_domain->devices); +spin_lock (&master->master_domains_lock); +list_add (&master_domain->master_elm, &master->master_domains); +spin_unlock (&master->master_domains_lock);spin_unlock_irqrestore (&smmu_domain->devices_lock, flags);arm_smmu_install_new_domain_invs (state);
@@ -4405,6 +4411,8 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev) master->dev = dev; master->smmu = smmu;dev_iommu_priv_set (dev, master); +INIT_LIST_HEAD (&master->master_domains); +spin_lock_init (&master->master_domains_lock); ret =arm_smmu_insert_master (smmu, master);if (ret)
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v6 16/17] iommu/arm-smmu-v3: Thread arm_smmu_master_domain on a per-master list
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 15 lines
A subsequent change needs to enumerate, from the CMDQ error path in atomic context, every domain a master is attached to so it can mark this master's ATS entries broken in each domain's invs after an ATC invalidation timeout. The existing per-domain
smmu_domain->deviceslist tracks the inverse direction (masters in a given domain), so introduce a per-master list.Add a second
list_headmaster_elmtoarm_smmu_master_domain, threaded onto a newmaster->master_domainslist undermaster_domains_lock. The CMDQ error path walks the list while holdingsmmu->streams_lock; that path runs under theinvs->rwlockread side, which is itself sleepable onPREEMPT_RT, so a plainspinlock_tsuffices for both. The attach and detach sites now take it withspin_lock(), nested inside the existingdevices_lockcritical section that already disables IRQs; it is a leaf in the lock order, so no inversion is introduced.
Quoted 4 lines
No functional change.
Assisted-by LLMSigned-off-by Nicolin Chen <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=16
PATCH v6 17/17 iommu/arm-smmu-v3: Quarantine ATS after an ATC invalidation timeout
In reply to Nicolin ChenAn ATC invalidation timeout is reported on the trailing CMD_SYNC, thereby identifying the failed batch but not the CMD_ATC_INV or responsible master.
cmds->cmds is sorted by SID, so when an ATS batch returns -EIO, retry one invalidation for each unique SID to find the unresponsive master. A batch with only one unique SID needs no retry. This is the common case, since it is uncommon for an ATS-capable PCI device to have multiple Stream IDs.
For each identified master, clear STE.EATS on all of its SIDs and mark its ATS invalidation entries in every attached domain as INV_TYPE_ATS_BROKEN, thereby blocking further ATS requests and preventing repeated ATC timeouts.
The marking must span every domain because a master may be attached at its RID and at multiple PASIDs. Marking only the timed-out invs would leave its other invs issuing CMD_ATC_INV commands that continue to time out.
Only mark the entries after the CFGI_STE completes successfully. Otherwise, leave them active so later invalidations retry quarantine instead of being suppressed while ATS may remain enabled.
The marking cannot take invs->rwlock for writing because the timeout path already holds a read lock; taking the write side would ABBA-deadlock this path against a concurrent timeout. Change the type with WRITE_ONCE(); once STE.EATS is cleared, a racing invalidation cannot create any new ATC entry and may at worst time out once more.
Use try_cmpxchg64() to clear STE.EATS without losing a concurrent update to the STE; a non-atomic fallback could revert concurrent changes, such as an S1DSS update. On a non-cacheable stream table, try_cmpxchg64() is unsafe, so limit quarantine to coherent SMMUs. Non-coherent SMMUs retain existing behavior of reporting every ATC_INV timeout.
Finally, force CMD_SYNC on every sub-batch containing an ATC invalidation so the timeout is observed by the call that issued it.
Identification is synchronous; each unresponsive SID adds a CMD_SYNC poll bounded by ARM_SMMU_POLL_TIMEOUT_US. That bound is rarely reached, since a non-responding ATC_INV completes in error once the device's PCIe Completion Timeout expires.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 202 +++++++++++++++++++- 1 file changed, 199 insertions(+), 3 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +199 -3
@@ -118,6 +118,10 @@ static const char * const event_class_str[] = {static int arm_smmu_alloc_cd_tables (struct arm_smmu_master *master);static bool arm_smmu_ats_supported (struct arm_smmu_master *master); +static struct arm_smmu_ste * +arm_smmu_get_step_for_sid (struct arm_smmu_device *smmu,u32 sid); +static struct arm_smmu_domain * +to_smmu_domain_devices (struct iommu_domain *domain);static void parse_driver_options (struct arm_smmu_device *smmu) {
@@ -966,12 +970,25 @@ static void arm_smmu_cmdq_batch_init_cmd(struct arm_smmu_device *smmu, cmds->has_ats = false; } +static void arm_smmu_cmdq_batch_retry (struct arm_smmu_device *smmu, +struct arm_smmu_cmdq_batch *cmds); +static int arm_smmu_cmdq_batch_issue (struct arm_smmu_device *smmu,struct arm_smmu_cmdq_batch *cmds,bool sync) { -return arm_smmu_cmdq_issue_cmdlist (smmu, cmds->cmdq, cmds->cmds, - cmds->num, sync); +int ret =arm_smmu_cmdq_issue_cmdlist (smmu, cmds->cmdq, cmds->cmds, + cmds->num, sync); + +/* +* The CMDQ HW reports an ATC invalidation timeout at the trailing +* CMD_SYNC, not at the failing CMD_ATC_INV. Re-issue each unique ATS +* SID in the batch to identify the unresponsive master and block its +* ATS so subsequent invalidations make forward progress. +*/ +if (ret == -EIO && cmds->has_ats) +arm_smmu_cmdq_batch_retry (smmu, cmds); +return ret; } static bool arm_smmu_cmdq_batch_force_sync(struct arm_smmu_device *smmu,
@@ -995,6 +1012,10 @@ static bool arm_smmu_cmdq_batch_force_sync(struct arm_smmu_device *smmu, arm_smmu_erratum_cmd_needs_repeating(&cmds->cmds[0]))return true; +/* ATC_INV timeout is reported to CMD_SYNC; catch at the call site */ +if (cmds->num ==CMDQ_BATCH_ENTRIES && cmds->has_ats) +return true; +return false; }
@@ -1068,7 +1089,11 @@ static inline struct arm_smmu_inv * arm_smmu_invs_iter_next(struct arm_smmu_invs *invs, size_t next, size_t *idx) { while (true) { - if (next >= invs->num_invs) { +/* +* Lockless readers (arm_smmu_invs_set_ats_broken) pair with the +* WRITE_ONCE() in arm_smmu_invs_unref(); num_invs only shrinks. +*/ +if (next >=READ_ONCE (invs->num_invs)) { *idx = next;return NULL ; }
@@ -2558,6 +2583,177 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master,return arm_smmu_cmdq_batch_submit (master->smmu, &cmds); } +static void arm_smmu_invs_set_ats_broken (struct arm_smmu_invs *invs, +struct arm_smmu_device *smmu,u32 sid) +{ +struct arm_smmu_inv *inv; +size_t i; + +/* arm_smmu_atc_inv_master() submits batches with invs=NULL */ +if (!invs) +return ; + +/* +* invs->rwlock is deliberately not taken: the caller holds one domain's +* read side for the timed-out batch, then taking another domain's write +* side while a concurrent timeout does the reverse would ABBA-deadlock. +* +* This indicates some potential races, but they are harmless since EATS +* was already cleared: +* - WRITE_ONCE() may hit a stale invs copy if an attach just installed +* a new invs, which might result in another ATC_INV timeout. +* - a concurrent invalidation may still issue an ATC_INV that may time +* out again. +*/ +arm_smmu_invs_for_each_entry (invs, i, inv) { +u8 type =arm_smmu_inv_type (inv); + +if (inv->smmu == smmu && inv->id == sid && + (type ==INV_TYPE_ATS || type ==INV_TYPE_ATS_FULL )) +WRITE_ONCE (inv->type,INV_TYPE_ATS_BROKEN ); + } +} + +/* Find the master by SID and block its ATS at the SMMU */ +static void arm_smmu_quarantine_ats (struct arm_smmu_device *smmu,u32 stream_id) +{ +struct arm_smmu_cmd cmd =arm_smmu_make_cmd_op (CMDQ_OP_CFGI_STE ); +struct arm_smmu_master_domain *md; +struct arm_smmu_cmdq_batch cmds; +struct arm_smmu_master *master; +struct arm_smmu_invs *invs; +unsigned long flags; +int i; + +/* +* The in-place STE.EATS clear relies on try_cmpxchg64(), which is UB +* on Non-Cacheable memory. Leave a non-coherent SMMU unquarantined: +* its invalidations keep issuing ATC_INV and reporting the timeouts. +*/ +if (!(smmu->features &ARM_SMMU_FEAT_COHERENCY )) +return ; + +guard (spinlock_irqsave)(&smmu->streams_lock); + master =arm_smmu_find_master (smmu, stream_id); +/* +* A concurrent hot-unplug can release the master while a stale ATS +* entry for it still lingers in the invs snapshot being walked here. +*/ +if (!master) +return ; + +/* Clear STE.EATS for every SID and sync to the SMMU */ +arm_smmu_cmdq_batch_init_cmd (smmu, &cmds, &cmd); + +for (i =0 ; i < master->num_streams; i++) { +u32 sid = master->streams[i].id; +struct arm_smmu_ste *ste =arm_smmu_get_step_for_sid (smmu, sid); +__le64 old, new; + +/* +* A concurrent arm_smmu_write_ste() of a domain attachment may +* overwrite the data[1] and set EATS, which is recoverable by +* another ATC_INV issued by its arm_smmu_attach_commit(). +*/ + old =READ_ONCE (ste->data[1 ]); +do { + new = old & ~cpu_to_le64 (STRTAB_STE_1_EATS ); + }while (!try_cmpxchg64 (&ste->data[1 ], &old, new)); + +arm_smmu_cmdq_batch_add_cmd ( + smmu, &cmds,arm_smmu_make_cmd_cfgi_ste (sid, true)); + } + +/* +* Only proceed to mark the entries broken if the STE.EATS clear above +* is confirmed; otherwise return so invalidations keep issuing ATC_INV +* (and re-quarantine) until ATS is actually disabled. +*/ +if (arm_smmu_cmdq_batch_submit (smmu, &cmds)) { +dev_err_ratelimited (smmu->dev, +"failed to disable ATS for master\n" ); +return ; + } + +/* +* Mark this master's ATS entries broken in every domain it is attached, +* so later invalidations skip the ATC_INV that would time out again. +*/ +rcu_read_lock (); +spin_lock_irqsave (&master->master_domains_lock, flags); +list_for_each_entry (md, &master->master_domains, master_elm) { +struct arm_smmu_domain *smmu_domain = +to_smmu_domain_devices (md->domain); + +if (!smmu_domain) +continue ; + invs =rcu_dereference (smmu_domain->invs); +for (i =0 ; i < master->num_streams; i++) +arm_smmu_invs_set_ats_broken (invs, smmu, + master->streams[i].id); + } +spin_unlock_irqrestore (&master->master_domains_lock, flags); +rcu_read_unlock (); +} + +/* Re-issue every unique ATS SID in @cmds to identify and quarantine masters. */ +static void arm_smmu_cmdq_batch_retry (struct arm_smmu_device *smmu, +struct arm_smmu_cmdq_batch *cmds) +{ +struct arm_smmu_cmd atc = {}; +u32 last_sid =0 ; +int nr_sids =0 ; +int i; + +/* +* Count unique Stream IDs, taking advantage of the sorted commands. An +* ATS-capable PCI device rarely has multiple SIDs, so a batch commonly +* carries a single SID, where a re-issue probe would be pointless. +*/ +for (i =0 ; i < cmds->num; i++) { +u32 sid; + +/* Only ATC_INV commands can time out */ +if (FIELD_GET (CMDQ_0_OP , cmds->cmds[i].data[0 ]) != +CMDQ_OP_ATC_INV ) +continue ; + + sid =FIELD_GET (CMDQ_ATC_0_SID , cmds->cmds[i].data[0 ]); +if (!nr_sids || sid != last_sid) { + nr_sids++; + last_sid = sid; + } + } + +/* The timed-out CMD_SYNC already identifies the lone Stream ID */ +if (nr_sids ==1 ) { +arm_smmu_quarantine_ats (smmu, last_sid); +return ; + } + +for (i =0 ; i < cmds->num; i++) { +u32 sid; + +if (FIELD_GET (CMDQ_0_OP , cmds->cmds[i].data[0 ]) != +CMDQ_OP_ATC_INV ) +continue ; + +/* +* One retry per Stream ID. So, only try the first command since +* commands are sorted. And each dead master costs one CMD_SYNC, +* bounded by its PCIe Completion Timeout (usually <= 250ms). +*/ + sid =FIELD_GET (CMDQ_ATC_0_SID , cmds->cmds[i].data[0 ]); +if (atc.data[0 ] && + sid ==FIELD_GET (CMDQ_ATC_0_SID , atc.data[0 ])) +continue ; + + atc = cmds->cmds[i]; +if (arm_smmu_cmdq_issue_cmd_p (smmu, &atc, true) == -EIO ) +arm_smmu_quarantine_ats (smmu, sid); + } +} +/* IO_PGTABLE API */ static void arm_smmu_tlb_inv_context (void *cookie) {
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v6 17/17] iommu/arm-smmu-v3: Quarantine ATS after an ATC invalidation timeout
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 19 lines
An ATC invalidation timeout is reported on the trailing
CMD_SYNC, thereby identifying the failed batch but not theCMD_ATC_INVor responsible master.
cmds->cmdsis sorted by SID, so when an ATS batch returns-EIO, retry one invalidation for each unique SID to find the unresponsive master. A batch with only one unique SID needs no retry. This is the common case, since it is uncommon for an ATS-capable PCI device to have multiple Stream IDs.For each identified master, clear STE.EATS on all of its SIDs and mark its ATS invalidation entries in every attached domain as
INV_TYPE_ATS_BROKEN, thereby blocking further ATS requests and preventing repeated ATC timeouts.The marking must span every domain because a master may be attached at its RID and at multiple PASIDs. Marking only the timed-out invs would leave its other invs issuing
CMD_ATC_INVcommands that continue to time out. [ ... ]
non-respondingATC_INVcompletes in error once the device's PCIe Completion Timeout expires.
Quoted 2 lines
Assisted-by LLMSigned-off-by Nicolin Chen <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=17
Will Deacon
maintainerQuoted 11 lines · Nicolin Chen
This series addresses a critical vulnerability and stability issue where an unresponsive PCIe device failing to process ATC (Address Translation Cache) invalidation requests leads to silent data corruption and continuous SMMU CMDQ error spam.
[ As Jason pointed out, because this series fundamentally introduces a new
RAS feature to quarantine and recover from hardware faults and relies on a recently accepted SMMU driver rework, it is not treated as a standard bug fix. Thus, most of the patches here don't carry a "Fixes" tag. ][...]
Applied SMMU rbtree fix to iommu (arm/smmu/updates), thanks!
[06/17] iommu/arm-smmu-v3: Don't rb_erase() a never-inserted stream node
https://git.kernel.org/iommu/c/2888520bdcbe
Cheers,
Signature · 5 lines
Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev