iommu/arm-smmu-v3: Quarantine device upon ATC invalidation timeout
Subsystems (6)
v4 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
- Introduce a reset_device_done op, allowing the core to signal the driver when the physical hardware has been cleanly recovered (e.g., via AER or a manual reset) so the quarantine can be lifted
- Utilize a per-group_device WQ via an iommu_report_device_broken() helper
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-v4
Changelog
v4:
* 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 (24):
PCI: Don't suspend IOMMU when probing reset capability
PCI: Propagate FLR return values to callers
iommu: Convert gdev->blocked from bool to enum gdev_blocked
iommu: Pass in reset result to pci_dev_reset_iommu_done()
iommu: Add reset_device_done callback for hardware fault recovery
iommu: Defer iommu_group free via kfree_rcu()
iommu: Defer __iommu_group_free_device() to be outside group->mutex
iommu: Change group->devices to RCU-protected list
iommu: Add group pointer to struct group_device
iommu: Add __iommu_group_block_device helper
iommu: Add iommu_report_device_broken() to quarantine a broken device iommu/arm-smmu-v3: Mark ATC invalidate timeouts via lockless 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: Co-clear pending CMDQ_ERR when CMD_SYNC times out iommu/arm-smmu-v3: Co-clear pending CMDQ_ERR when queue_has_space()
fails
iommu/arm-smmu-v3: Add master in arm_smmu_inv for ATS entries iommu/arm-smmu-v3: Introduce master->ats_broken flag iommu/arm-smmu-v3: Add invs and has_ats to struct arm_smmu_cmdq_batch iommu/arm-smmu-v3: Introduce arm_smmu_cmdq_batch_issue() wrapper iommu/arm-smmu-v3: Move arm_smmu_invs_for_each_entry to header iommu/arm-smmu-v3: Introduce master->ats_invs iommu/arm-smmu-v3: Serialize STE.EATS and ats_broken updates iommu/arm-smmu-v3: Block ATS upon an ATC invalidation timeout
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 72 +++- include/linux/iommu.h | 18 +- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 387 ++++++++++++++--- .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 36 +- drivers/iommu/iommu.c | 406 ++++++++++++++---- drivers/pci/pci-acpi.c | 2 +- drivers/pci/pci.c | 21 +- drivers/pci/quirks.c | 43 +- 8 files changed, 820 insertions(+), 165 deletions(-)
Signature · 1 line
2.43.0
PATCH v4 1/24 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
@@ -4914,10 +4914,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);
@@ -4926,7 +4928,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
@@ -4260,14 +4260,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
PATCH v4 2/24 PCI: Propagate FLR return values to callers
In reply to Nicolin ChenA reset failure implies that the device might be unreliable. E.g. its ATC might still retain stale entries. Thus, the IOMMU layer cannot trust this device to resume its ATS function that can lead to memory corruption. So, the pci_dev_reset_iommu_done() won't recover the device's IOMMU pathway if the device reset fails.
The quirk functions in the pci_dev_reset_methods array invoke pcie_flr(), but do not check the return value. Propagate them correctly.
Also propagate device-internal ack timeouts in reset_hinic_vf_dev().
Note: this change does not introduce any early return on failure, keeping everything status quo. It only propagates the error code properly.
Given that these functions have been running okay, and the return values will only be needed for incoming work. This is not treated as bug fix.
--- drivers/pci/quirks.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-)
drivers/pci/quirks.c +17 -13
@@ -3955,7 +3955,7 @@ static int reset_intel_82599_sfp_virtfn(struct pci_dev *dev, bool probe)* supported. */ if (!probe) -pcie_flr (dev); +return pcie_flr (dev);return 0 ; }
@@ -4013,6 +4013,7 @@ static int reset_chelsio_generic_dev(struct pci_dev *dev, bool probe) {u16 old_command;u16 msix_flags; +int ret;/* * If this isn't a Chelsio T4-based device, return -ENOTTY indicating
@@ -4058,16 +4059,15 @@ static int reset_chelsio_generic_dev(struct pci_dev *dev, bool probe)PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL); -pcie_flr (dev); + ret =pcie_flr (dev);/* * Restore the configuration information (BAR values, etc.) including -* the original PCI Configuration Space Command word, and return -* success. +* the original PCI Configuration Space Command word. */ pci_restore_state (dev);pci_write_config_word (dev,PCI_COMMAND , old_command); -return 0 ; +return ret; }#define PCI_DEVICE_ID_INTEL_82599_SFP_VF 0x10ed
@@ -4150,9 +4150,7 @@ static int nvme_disable_and_flr(struct pci_dev *dev, bool probe)pci_iounmap (dev, bar); -pcie_flr (dev); - -return 0 ; +return pcie_flr (dev); }/*
@@ -4164,14 +4162,17 @@ static int nvme_disable_and_flr(struct pci_dev *dev, bool probe)*/ static int delay_250ms_after_flr (struct pci_dev *dev,bool probe) { +int ret; +if (probe)return pcie_reset_flr (dev,PCI_RESET_PROBE ); -pcie_reset_flr (dev,PCI_RESET_DO_RESET ); + ret =pcie_reset_flr (dev,PCI_RESET_DO_RESET ); +/* Settle the device even on a failed FLR */ msleep (250 ); -return 0 ; +return ret; }#define PCI_DEVICE_ID_HINIC_VF 0x375E
@@ -4187,6 +4188,7 @@ static int reset_hinic_vf_dev(struct pci_dev *pdev, bool probe)unsigned long timeout;void __iomem *bar;u32 val; +int ret;if (probe)return 0 ;
@@ -4207,12 +4209,13 @@ static int reset_hinic_vf_dev(struct pci_dev *pdev, bool probe) val = val |HINIC_VF_FLR_PROC_BIT ;iowrite32be (val, bar +HINIC_VF_OP ); -pcie_flr (pdev); + ret =pcie_flr (pdev);/* * The device must recapture its Bus and Device Numbers after FLR * in order generate Completions. Issue a config write to let the -* device capture this information. +* device capture this information. Note that pcie_flr() can fail +* after the reset is asserted. So, recapture it unconditionally. */ pci_write_config_word (pdev,PCI_VENDOR_ID ,0 );
@@ -4230,11 +4233,12 @@ static int reset_hinic_vf_dev(struct pci_dev *pdev, bool probe) goto reset_complete;pci_warn (pdev,"Reset dev timeout, FLR ack reg: %#010x\n" , val); + ret = -ETIMEDOUT ; reset_complete:pci_iounmap (pdev, bar); -return 0 ; +return ret; }static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
Signature · 1 line
2.43.0
PATCH v4 3/24 iommu: Convert gdev->blocked from bool to enum gdev_blocked
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 "bool blocked" into "enum gdev_blocked blocked", provisioned with two initial values: BLOCKED_NO and BLOCKED_RESETTING, for the existing use cases. All readers keep the "if (gdev->blocked)" form, as BLOCKED_NO == 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 gdev_blocked { +BLOCKED_NO =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 gdev_blocked blocked;unsigned int reset_depth; };
@@ -4083,7 +4087,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.
@@ -4209,7 +4213,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_NO ;/* * Re-attach PASID domains back to the domains retained in pasid_array.
Signature · 1 line
2.43.0
PATCH v4 4/24 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.
--- include/linux/iommu.h | 5 ++-- drivers/iommu/iommu.c | 62 ++++++++++++++++++++++++++++++++++++++++-- drivers/pci/pci-acpi.c | 2 +- drivers/pci/pci.c | 10 +++---- drivers/pci/quirks.c | 2 +- 5 files changed, 69 insertions(+), 12 deletions(-)
include/linux/iommu.h +3 -2
@@ -1195,7 +1195,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 {};
@@ -1525,7 +1525,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 +59 -3
122 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
@@ -4355,7 +4355,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);
@@ -4433,7 +4433,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; }
@@ -4487,7 +4487,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; }
@@ -4929,7 +4929,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; }
@@ -4974,7 +4974,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
@@ -4274,7 +4274,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
PATCH v4 5/24 iommu: Add reset_device_done callback for hardware fault recovery
In reply to Nicolin ChenWhen an IOMMU hardware detects an error due to a faulty device (e.g. an ATS invalidation timeout), IOMMU drivers may quarantine the device by disabling specific hardware features or dropping translation capabilities.
To recover from these states, the IOMMU driver needs a reliable signal that the underlying physical hardware has been cleanly reset (e.g., via PCIe AER or a sysfs Function Level Reset) so as to lift the quarantine.
Introduce a reset_device_done callback in struct iommu_ops. Trigger it from the existing pci_dev_reset_iommu_done() path to notify the underlying IOMMU driver that the device's internal state has been sanitized, when the result indicates a successful physical reset.
As the initial use case, this will be used by ATS-capable PCI devices.
--- include/linux/iommu.h | 4 ++++ drivers/iommu/iommu.c | 12 ++++++++++++ 2 files changed, 16 insertions(+)
include/linux/iommu.h +4 -0
@@ -629,6 +629,9 @@ __iommu_copy_struct_to_user(const struct iommu_user_data *dst_data,* @release_device: Remove device from iommu driver handling * @probe_finalize: Do final setup work after the device is added to an IOMMU * group and attached to the groups domain +* @reset_device_done: Notify the driver that a device has reset successfully. +* Note that the core invokes the callback function while +* holding the group->mutex * @device_group: find iommu group for a particular device * @get_resv_regions: Request list of reserved regions for a device * @of_xlate: add OF master IDs to iommu grouping
@@ -686,6 +689,7 @@ struct iommu_ops {struct iommu_device *(*probe_device)(struct device *dev);void (*release_device)(struct device *dev);void (*probe_finalize)(struct device *dev); +void (*reset_device_done)(struct device *dev);struct iommu_group *(*device_group)(struct device *dev);/* Request/Free a list of reserved regions for a device */
drivers/iommu/iommu.c +12 -0
@@ -4182,12 +4182,14 @@ void pci_dev_reset_iommu_done(struct pci_dev *pdev, int reset_result) {struct iommu_group *group = pdev->dev.iommu_group;enum gdev_blocked old_gdev_blocked; +const struct iommu_ops *ops;struct group_device *gdev;unsigned long pasid;void *entry;if (!pci_ats_supported (pdev) || !dev_has_iommu (&pdev->dev))return ; + ops =dev_iommu_ops (&pdev->dev);guard (mutex)(&group->mutex);
@@ -4249,6 +4251,16 @@ void pci_dev_reset_iommu_done(struct pci_dev *pdev, int reset_result) "DMA-aliased sibling may be prematurely unblocked\n"); } +/* +* A PCI device might have been in an error state, so the IOMMU driver +* had to quarantine the device by disabling specific hardware features +* or dropping translation capability. Here notify the IOMMU driver as +* a reliable signal that the faulty PCI device has been cleanly reset +* so now it can lift its quarantine and restore full functionality. +*/ +if (ops->reset_device_done) + ops->reset_device_done (&pdev->dev); +/* * Re-attach RID domain back to group->domain *
Signature · 1 line
2.43.0
PATCH v4 6/24 iommu: Defer iommu_group free via kfree_rcu()
In reply to Nicolin Chendev->iommu_group will be read in an ISR-context to look up a group_device for fault reporting, in which case mutex cannot be used. For that read to be safe, two things are needed:
(1) The iommu_group memory that dev->iommu_group points to must outlive any in-flight rcu_read_lock section. Add rcu_head to iommu_group and switch iommu_group_release() to calling kfree_rcu().
(2) The publication of dev->iommu_group must pair with rcu_dereference() at the upcoming reader (cannot hold mutex but rcu_read_lock), so the writers must use rcu_assign_pointer().
Existing readers do not use rcu_dereference(); they retain their current synchronization model. Apply a __rcu __force cast at the writer sites to satisfy sparse without forcing every reader to convert.
New reader added by the subsequent change uses rcu_dereference() only, to reach group->devices for a list lookup. And it does not touch group->name and other fields. The kfree_rcu() here is supposed to keep group->devices alive across the read-side critical section; other fields will not affect the reader.
Note: this change alone does not yet make group->devices iteration safe under rcu_read_lock(); a subsequent change will convert the group_device list to RCU and switch struct group_device to kfree_rcu().
--- drivers/iommu/iommu.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
drivers/iommu/iommu.c +9 -5
@@ -71,8 +71,12 @@ struct iommu_group {*/ unsigned int recovery_cnt;void *owner; +struct rcu_head rcu; }; +#define dev_iommu_group_rcu (dev) \ + (*((struct iommu_group __rcu __force **)&(dev)->iommu_group)) +enum gdev_blocked {BLOCKED_NO =0 ,/* Not blocked */ BLOCKED_RESETTING ,/* PCI reset in flight */
@@ -531,7 +535,7 @@ static int iommu_init_device(struct device *dev) ret =PTR_ERR (group); goto err_unlink; } - dev->iommu_group = group; +rcu_assign_pointer (dev_iommu_group_rcu (dev), group); dev->iommu->max_pasids =dev_iommu_get_max_pasids (dev);if (ops->is_attach_deferred)
@@ -613,7 +617,7 @@ static void iommu_deinit_device(struct device *dev) }/* Caller must put iommu_group */ - dev->iommu_group =NULL ; +rcu_assign_pointer (dev_iommu_group_rcu (dev),NULL );module_put (ops->owner);dev_iommu_free (dev);#ifdef CONFIG_IOMMU_DMA
@@ -772,7 +776,7 @@ static void __iommu_group_remove_device(struct device *dev)if (dev_has_iommu (dev))iommu_deinit_device (dev);else - dev->iommu_group =NULL ; +rcu_assign_pointer (dev_iommu_group_rcu (dev),NULL );break ; }mutex_unlock (&group->mutex);
@@ -1059,7 +1063,7 @@ static void iommu_group_release(struct kobject *kobj)WARN_ON (group->blocking_domain);kfree (group->name); -kfree (group); +kfree_rcu (group, rcu); }static const struct kobj_type iommu_group_ktype = {
@@ -1344,7 +1348,7 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)return PTR_ERR (gdev);iommu_group_ref_get (group); - dev->iommu_group = group; +rcu_assign_pointer (dev_iommu_group_rcu (dev), group);mutex_lock (&group->mutex);list_add_tail (&gdev->list, &group->devices);
Signature · 1 line
2.43.0
Re: [PATCH v4 06/24] iommu: Defer iommu_group free via kfree_rcu()
Quoted 3 lines · Nicolin Chen
dev->iommu_groupwill be read in an ISR-context to look up agroup_devicefor fault reporting, in which case mutex cannot be used. For that read to be safe, two things are needed:
What driver does this? iommu_report_device_fault() has to be called in a sleepable context - usually a threaded IRQ handler. So mutex is no problem.
This seems like Sashiko slop - iommu_group does not change while a driver is attached and a driver is not permitted to do any "fault handling" after it has detached, it must flush and synchronize its IRQ if it is using this from a hard IRQ for some bad reason.
You need to be alot more critical about the noise that Sashiko generates, alot is useful, alot is not. It is a not a tool you want to have 0 reports, complaining about hallucinations is expected.
drivers/iommu/iommu.c struct iommu_group { · line 78
Quoted 2 lines
drivers/iommu/iommu.c +2
-0+#define dev_iommu_group_rcu (dev) \ + (*((struct iommu_group __rcu __force **)&(dev)->iommu_group))
Don't do things like this.
Jason
Re: [PATCH v4 06/24] iommu: Defer iommu_group free via kfree_rcu()
Quoted 8 lines · Jason Gunthorpe
Quoted 3 lines · Nicolin Chen
dev->iommu_groupwill be read in an ISR-context to look up agroup_devicefor fault reporting, in which case mutex cannot be used. For that read to be safe, two things are needed:What driver does this?
iommu_report_device_fault()has to be called in a sleepable context - usually a threaded IRQ handler. So mutex is no problem.
It's used in an invalidation context, where mutex is a problem.
Quoted 4 lines
This seems like Sashiko slop -
iommu_groupdoes not change while a driver is attached and a driver is not permitted to do any "fault handling" after it has detached, it must flush and synchronize its IRQ if it is using this from a hard IRQ for some bad reason.
Oh, I probably picked a wrong word. "fault" here means "ATS broken".
IOMMU driver sees ATC timeout during invalidation, and reports "ATS broken" in a lockless context where device could be detached. No?
Quoted 3 lines
You need to be alot more critical about the noise that Sashiko generates, alot is useful, alot is not. It is a not a tool you want to have 0 reports, complaining about hallucinations is expected.
OK. I will be more aware of that in the future.
Thanks
Nicolin
PATCH v4 7/24 iommu: Defer __iommu_group_free_device() to be outside group->mutex
In reply to Nicolin Chen__iommu_group_remove_device() holds group->mutex across the entire call to __iommu_group_free_device() that performs sysfs removals, tracing, and the final kfree(). But in fact, most of these operations don't really need the group->mutex.
Subsequent changes will introduce sleepable operations to this function:
+ synchronize_rcu() to defer the gdev->dev put past a grace period.
+ disable_work_sync() to cancel a future broken_work.
Neither should run while holding group->mutex. Thus, move them outside.
Separate the assertion from __iommu_group_free_device() to another helper __iommu_group_empty_assert_owner_cnt(). While moving it, revise the inline comment a bit to make it clearer.
Defer the __iommu_group_free_device() until the mutex is released.
This is a preparatory refactor with no functional change.
--- drivers/iommu/iommu.c | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-)
drivers/iommu/iommu.c +23 -12
@@ -634,6 +634,19 @@ static struct iommu_domain *pasid_array_entry_to_domain(void *entry)DEFINE_MUTEX (iommu_probe_device_lock); +static void __iommu_group_empty_assert_owner_cnt (struct iommu_group *group) +{ +lockdep_assert_held (&group->mutex); +/* +* If the group has become empty, the ownership must have been released, +* and the current domain must be set back to the default domain (which +* itself can be NULL). +*/ +if (list_empty (&group->devices)) +WARN_ON (group->owner_cnt || + group->domain != group->default_domain); +} +static int __iommu_probe_device (struct device *dev,struct list_head *group_list) {struct iommu_group *group;
@@ -707,10 +720,12 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list err_remove_gdev:list_del (&gdev->list); -__iommu_group_free_device (group, gdev); +__iommu_group_empty_assert_owner_cnt (group); err_put_group:iommu_deinit_device (dev);mutex_unlock (&group->mutex); +if (!IS_ERR (gdev)) +__iommu_group_free_device (group, gdev);iommu_group_put (group);return ret;
@@ -739,20 +754,13 @@ static void __iommu_group_free_device(struct iommu_group *group, {struct device *dev = grp_dev->dev; +lockdep_assert_not_held (&group->mutex); +sysfs_remove_link (group->devices_kobj, grp_dev->name);sysfs_remove_link (&dev->kobj,"iommu_group" );trace_remove_device_from_group (group->id, dev); -/* -* If the group has become empty then ownership must have been -* released, and the current domain must be set back to NULL or -* the default domain. -*/ -if (list_empty (&group->devices)) -WARN_ON (group->owner_cnt || - group->domain != group->default_domain); -kfree (grp_dev->name);kfree (grp_dev); }
@@ -761,7 +769,7 @@ static void __iommu_group_free_device(struct iommu_group *group,static void __iommu_group_remove_device (struct device *dev) {struct iommu_group *group = dev->iommu_group; -struct group_device *device; +struct group_device *device, *to_free =NULL ;mutex_lock (&group->mutex);for_each_group_device (group, device) {
@@ -772,15 +780,18 @@ static void __iommu_group_remove_device(struct device *dev)if (device->blocked && !WARN_ON (group->recovery_cnt ==0 )) group->recovery_cnt--;list_del (&device->list); -__iommu_group_free_device (group, device); +__iommu_group_empty_assert_owner_cnt (group);if (dev_has_iommu (dev))iommu_deinit_device (dev);else rcu_assign_pointer (dev_iommu_group_rcu (dev),NULL ); + to_free = device;break ; }mutex_unlock (&group->mutex); +if (to_free) +__iommu_group_free_device (group, to_free);/* * Pairs with the get in iommu_init_device() or * iommu_group_add_device()
Signature · 1 line
2.43.0
Re: [PATCH v4 07/24] iommu: Defer __iommu_group_free_device() to be outside group->mutex
Quoted 4 lines · Nicolin Chen
__iommu_group_remove_device()holdsgroup->mutexacross the entire call to__iommu_group_free_device()that performs sysfs removals, tracing, and the finalkfree(). But in fact, most of these operations don't really need thegroup->mutex.
Are you sure? sysfs requires unique names, and this mutex is providing a guarentee that will happen. While it shouldn't be possible to race remove and attach, it is easier to reason about if we don't have to make this assumption.
Quoted 2 lines
Subsequent changes will introduce sleepable operations to this function:
+synchronize_rcu()to defer thegdev->devput past a grace period.
I'm not keen on that at all.
Jason
PATCH v4 8/24 iommu: Change group->devices to RCU-protected list
In reply to Nicolin ChenTo allow lockless iterations of the group->devices list in an ISR context that cannot hold the group->mutex, change the list to be RCU protected.
Mark the existing __dev_to_gdev() for group->mutex case only. A subsequent change will add another __dev_to_gdev_rcu() for RCU case.
Hold grp_dev->dev across the RCU grace period using synchronize_rcu(), in __iommu_group_free_device(). Without that, the driver core might free the struct device while an RCU reader is still mid-iteration.
Note: a call_rcu() callback runs in softirq context, but put_device() may sleep -- the device release path can invoke devres_release_all() and ->release callbacks that take mutexes. Use synchronize_rcu() to defer the put_device() to the (sleepable) caller context instead.
Note that in bus_iommu_probe() there is a for_each_group_device marked as FIXME, which can't take either mutex or RCU read lock. Plainly replace it with list_for_each_entry for a status quo.
--- drivers/iommu/iommu.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
drivers/iommu/iommu.c +18 -7
@@ -97,8 +97,10 @@ struct group_device {/* Iterate over each struct group_device in a struct iommu_group */ #define for_each_group_device (group, pos) \ - list_for_each_entry(pos, &(group)->devices, list) + list_for_each_entry_rcu(pos, &(group)->devices, list, \ + lockdep_is_held(&(group)->mutex)) +/* Caller must hold dev->iommu_group->mutex. */ static struct group_device *__dev_to_gdev (struct device *dev) {struct iommu_group *group = dev->iommu_group;
@@ -688,7 +690,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list* The gdev must be in the list before calling * iommu_setup_default_domain() */ -list_add_tail (&gdev->list, &group->devices); +list_add_tail_rcu (&gdev->list, &group->devices);WARN_ON (group->default_domain && !group->domain);if (group->default_domain)iommu_create_device_direct_mappings (group->default_domain, dev);
@@ -719,7 +721,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_listreturn 0 ; err_remove_gdev: -list_del (&gdev->list); +list_del_rcu (&gdev->list);__iommu_group_empty_assert_owner_cnt (group); err_put_group:iommu_deinit_device (dev);
@@ -762,6 +764,10 @@ static void __iommu_group_free_device(struct iommu_group *group,trace_remove_device_from_group (group->id, dev);kfree (grp_dev->name); + +/* Wait for any in-flight reader to drop the reference to gdev->dev */ +synchronize_rcu (); +put_device (grp_dev->dev);kfree (grp_dev); }
@@ -779,7 +785,7 @@ static void __iommu_group_remove_device(struct device *dev)/* Must drop the recovery_cnt when removing a blocked device */ if (device->blocked && !WARN_ON (group->recovery_cnt ==0 )) group->recovery_cnt--; -list_del (&device->list); +list_del_rcu (&device->list);__iommu_group_empty_assert_owner_cnt (group);if (dev_has_iommu (dev))iommu_deinit_device (dev);
@@ -1298,6 +1304,8 @@ static struct group_device *iommu_group_alloc_device(struct iommu_group *group,return ERR_PTR (-ENOMEM ); device->dev = dev; +/* Keep dev alive for any in-flight RCU reader of grp_dev->dev. */ +get_device (dev); ret =sysfs_create_link (&dev->kobj, &group->kobj,"iommu_group" );if (ret)
@@ -1337,6 +1345,7 @@ static struct group_device *iommu_group_alloc_device(struct iommu_group *group, err_remove_link:sysfs_remove_link (&dev->kobj,"iommu_group" ); err_free_device: +put_device (dev);kfree (device);dev_err (dev,"Failed to add to iommu group %d: %d\n" , group->id, ret);return ERR_PTR (ret);
@@ -1362,7 +1371,7 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)rcu_assign_pointer (dev_iommu_group_rcu (dev), group);mutex_lock (&group->mutex); -list_add_tail (&gdev->list, &group->devices); +list_add_tail_rcu (&gdev->list, &group->devices);mutex_unlock (&group->mutex);return 0 ; }
@@ -2011,9 +2020,11 @@ static int bus_iommu_probe(const struct bus_type *bus)* FIXME: Mis-locked because the ops->probe_finalize() call-back * of some IOMMU drivers calls arm_iommu_attach_device() which * in-turn might call back into IOMMU core code, where it tries -* to take group->mutex, resulting in a deadlock. +* to take group->mutex, resulting in a deadlock. Unfortunately, +* as iommu_group_do_probe_finalize() can sleep, rcu_read_lock() +* cannot be held to mitigate this. */ -for_each_group_device (group, gdev) +list_for_each_entry (gdev, &group->devices, list)iommu_group_do_probe_finalize (gdev->dev); }
Signature · 1 line
2.43.0
PATCH v4 9/24 iommu: Add group pointer to struct group_device
In reply to Nicolin ChenThough group pointer is in general available at dev->iommu_group, it would be NULLed by iommu_deinit_device() holding group->mutex.
To introduce an asynchronous worker that would hold the mutex as well, its disable_work_sync() can only get called afterwards outside the mutex. Then, using dev->iommu_group would crash the kernel.
Add a group pointer to the gdev to prepare for that. No functional change.
Drop group arguments next to gdev in function parameters.
--- drivers/iommu/iommu.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
drivers/iommu/iommu.c +10 -9
@@ -84,6 +84,7 @@ enum gdev_blocked { };struct group_device { +struct iommu_group *group;struct list_head list;struct device *dev;char *name;
@@ -177,8 +178,7 @@ static ssize_t iommu_group_store_type(struct iommu_group *group, const char *buf, size_t count);static struct group_device *iommu_group_alloc_device (struct iommu_group *group,struct device *dev); -static void __iommu_group_free_device (struct iommu_group *group, -struct group_device *grp_dev); +static void __iommu_group_free_device (struct group_device *grp_dev);static void iommu_domain_init (struct iommu_domain *domain,unsigned int type,const struct iommu_ops *ops);
@@ -727,7 +727,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_listiommu_deinit_device (dev);mutex_unlock (&group->mutex);if (!IS_ERR (gdev)) -__iommu_group_free_device (group, gdev); +__iommu_group_free_device (gdev);iommu_group_put (group);return ret;
@@ -751,9 +751,9 @@ int iommu_probe_device(struct device *dev)return 0 ; } -static void __iommu_group_free_device (struct iommu_group *group, -struct group_device *grp_dev) +static void __iommu_group_free_device (struct group_device *grp_dev) { +struct iommu_group *group = grp_dev->group;struct device *dev = grp_dev->dev;lockdep_assert_not_held (&group->mutex);
@@ -797,7 +797,7 @@ static void __iommu_group_remove_device(struct device *dev)mutex_unlock (&group->mutex);if (to_free) -__iommu_group_free_device (group, to_free); +__iommu_group_free_device (to_free);/* * Pairs with the get in iommu_init_device() or * iommu_group_add_device()
@@ -1304,6 +1304,7 @@ static struct group_device *iommu_group_alloc_device(struct iommu_group *group,return ERR_PTR (-ENOMEM ); device->dev = dev; + device->group = group;/* Keep dev alive for any in-flight RCU reader of grp_dev->dev. */ get_device (dev);
@@ -4161,9 +4162,9 @@ static int group_device_cmp_dma_alias(struct pci_dev *dev, u16 alias, &alias); } -static bool group_device_dma_alias_is_blocked (struct iommu_group *group, -struct group_device *gdev) +static bool group_device_dma_alias_is_blocked (struct group_device *gdev) { +struct iommu_group *group = gdev->group;struct group_device *sibling;lockdep_assert_held (&group->mutex);
@@ -4263,7 +4264,7 @@ void pci_dev_reset_iommu_done(struct pci_dev *pdev, int reset_result)if (WARN_ON (!gdev->blocked))return ; -if (group_device_dma_alias_is_blocked (group, gdev)) { +if (group_device_dma_alias_is_blocked (gdev)) {/* * FIXME: DMA aliased devices share the same RID, which would be * convoluted to handle, as "gdev->blocked" is not sufficient:
Signature · 1 line
2.43.0
PATCH v4 10/24 iommu: Add __iommu_group_block_device helper
In reply to Nicolin ChenMove the RID/PASID blocking routine into a separate helper, which will be reused by a new function to quarantine the device but does not bother the gdev->reset_depth counter.
Also, document the severity ordering at enum gdev_blocked.
No functional changes.
--- drivers/iommu/iommu.c | 106 ++++++++++++++++++++++++------------------ 1 file changed, 60 insertions(+), 46 deletions(-)
drivers/iommu/iommu.c +60 -46
@@ -77,6 +77,7 @@ struct iommu_group {#define dev_iommu_group_rcu (dev) \ (*((struct iommu_group __rcu __force **)&(dev)->iommu_group)) +/* A bigger number indicates a higher severity */ enum gdev_blocked {BLOCKED_NO =0 ,/* Not blocked */ BLOCKED_RESETTING ,/* PCI reset in flight */
@@ -4053,6 +4054,62 @@ int iommu_replace_group_handle(struct iommu_group *group, }EXPORT_SYMBOL_NS_GPL (iommu_replace_group_handle,"IOMMUFD_INTERNAL" ); +/* Caller can use this function on a blocked @gdev just to update the @reason */ +static int __iommu_group_block_device (struct group_device *gdev, +enum gdev_blocked reason) +{ +struct iommu_group *group = gdev->group; +unsigned long pasid; +void *entry; +int ret; + +lockdep_assert_held (&group->mutex); + +/* Device might be already blocked for a quarantine */ +if (gdev->blocked) { +/* Escalate the severity */ + gdev->blocked =max (gdev->blocked, reason); +return 0 ; + } + + ret =__iommu_group_alloc_blocking_domain (group); +if (ret) +return ret; + +/* Stage RID domain at blocking_domain while retaining group->domain */ +if (group->domain != group->blocking_domain) { + ret =__iommu_attach_device (group->blocking_domain, gdev->dev, + group->domain); +if (ret) +return ret; + } + +/* +* Update gdev->blocked upon the domain change, as it is used to return +* the correct domain in iommu_driver_get_domain_for_dev() that might be +* called in a set_dev_pasid callback function. +*/ + gdev->blocked = reason; + +/* +* Stage PASID domains at blocking_domain while retaining pasid_array. +* +* The pasid_array is mostly fenced by group->mutex, except one reader +* in iommu_attach_handle_get(), so it's safe to read without xa_lock. +*/ +if (gdev->dev->iommu->max_pasids >0 ) { +xa_for_each_start (&group->pasid_array, pasid, entry,1 ) { +struct iommu_domain *pasid_dom = +pasid_array_entry_to_domain (entry); + +iommu_remove_dev_pasid (gdev->dev, pasid, pasid_dom); + } + } + + group->recovery_cnt++; +return 0 ; +} +/** * pci_dev_reset_iommu_prepare() - Block IOMMU to prepare for a PCI device reset * @pdev: PCI device that is going to enter a reset routine
@@ -4086,8 +4143,6 @@ int pci_dev_reset_iommu_prepare(struct pci_dev *pdev) {struct iommu_group *group = pdev->dev.iommu_group;struct group_device *gdev; -unsigned long pasid; -void *entry;int ret;if (!pci_ats_supported (pdev) || !dev_has_iommu (&pdev->dev))
@@ -4102,49 +4157,9 @@ int pci_dev_reset_iommu_prepare(struct pci_dev *pdev)if (gdev->reset_depth++)return 0 ; -/* Device might be already blocked for a quarantine */ -if (gdev->blocked) -return 0 ; - - ret =__iommu_group_alloc_blocking_domain (group); -if (ret) { + ret =__iommu_group_block_device (gdev,BLOCKED_RESETTING ); +if (ret) gdev->reset_depth--; -return ret; - } - -/* Stage RID domain at blocking_domain while retaining group->domain */ -if (group->domain != group->blocking_domain) { - ret =__iommu_attach_device (group->blocking_domain, &pdev->dev, - group->domain); -if (ret) { - gdev->reset_depth--; -return ret; - } - } - -/* -* Update gdev->blocked upon the domain change, as it is used to return -* the correct domain in iommu_driver_get_domain_for_dev() that might be -* called in a set_dev_pasid callback function. -*/ - gdev->blocked =BLOCKED_RESETTING ; - -/* -* Stage PASID domains at blocking_domain while retaining pasid_array. -* -* The pasid_array is mostly fenced by group->mutex, except one reader -* in iommu_attach_handle_get(), so it's safe to read without xa_lock. -*/ -if (pdev->dev.iommu->max_pasids >0 ) { -xa_for_each_start (&group->pasid_array, pasid, entry,1 ) { -struct iommu_domain *pasid_dom = -pasid_array_entry_to_domain (entry); - -iommu_remove_dev_pasid (&pdev->dev, pasid, pasid_dom); - } - } - - group->recovery_cnt++;return ret; }EXPORT_SYMBOL_GPL (pci_dev_reset_iommu_prepare);
@@ -4256,8 +4271,7 @@ void pci_dev_reset_iommu_done(struct pci_dev *pdev, int reset_result)dev_err_ratelimited ( &pdev->dev,"Reset failed. Keep it blocked to protect memory\n" ); -if (gdev->blocked ==BLOCKED_RESETTING ) - gdev->blocked =BLOCKED_RESET_FAILED ; +WARN_ON (__iommu_group_block_device (gdev,BLOCKED_RESET_FAILED ));return ; }
Signature · 1 line
2.43.0
PATCH v4 11/24 iommu: Add iommu_report_device_broken() to quarantine a broken device
In reply to Nicolin ChenWhen an IOMMU hardware detects an error due to a faulty device (e.g. an ATS invalidation timeout), IOMMU drivers may quarantine the device by disabling specific hardware features or dropping translation capabilities.
However, the core-level states of the faulty device are out of sync, as the device can still be attached to a translation domain or even potentially be moved to a new domain that might overwrite the driver-level quarantine.
Given that such an error can likely be triggered from an ISR, introduce an asynchronous broken_work per group_device, and provide a helper function to allow the driver to initiate a quarantine in the core. __dev_to_gdev_rcu() is required here to safely iterate the group_device list.
Note: gdev and gdev->group teardown will be blocked by disable_work_sync(), so it's completely safe for the worker thread to access the group pointer.
--- include/linux/iommu.h | 11 +++- drivers/iommu/iommu.c | 139 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+), 1 deletion(-)
include/linux/iommu.h +10 -1
@@ -631,7 +631,10 @@ __iommu_copy_struct_to_user(const struct iommu_user_data *dst_data,* group and attached to the groups domain * @reset_device_done: Notify the driver that a device has reset successfully. * Note that the core invokes the callback function while -* holding the group->mutex +* holding the group->mutex. Before returning, the driver +* must drain or filter pre-reset fault reports so that +* subsequent calls to iommu_report_device_broken() will +* reflect only post-reset faults. * @device_group: find iommu group for a particular device * @get_resv_regions: Request list of reserved regions for a device * @of_xlate: add OF master IDs to iommu grouping
@@ -896,6 +899,8 @@ static inline struct iommu_device *__iommu_get_iommu_dev(struct device *dev)#define iommu_get_iommu_dev (dev, type, member) \ container_of(__iommu_get_iommu_dev(dev), type, member) +void iommu_report_device_broken (struct device *dev); +static inline void iommu_iotlb_gather_init (struct iommu_iotlb_gather *gather) { *gather = (struct iommu_iotlb_gather ) {
@@ -1211,6 +1216,10 @@ struct iommu_iotlb_gather {};struct iommu_dirty_bitmap {};struct iommu_dirty_ops {}; +static inline void iommu_report_device_broken (struct device *dev) +{ +} +static inline bool device_iommu_capable (struct device *dev,enum iommu_cap cap) {return false;
drivers/iommu/iommu.c +139 -0
187 lines not on this page · raw message
Signature · 1 line
2.43.0
Re: [PATCH v4 11/24] iommu: Add iommu_report_device_broken() to quarantine a broken device
Quoted 6 lines · Nicolin Chen
drivers/iommu/iommu.c +6
-0+void iommu_report_device_broken (struct device *dev) +{ +struct group_device *gdev; + +/* +* We cannot hold group->mutex here. Rely on iommu_group_broken_worker()
drivers/iommu/iommu.c void pci_dev_reset_iommu_done(struct pci_dev *pdev, int reset_result) · line 4474
Quoted 10 lines
drivers/iommu/iommu.c +10
-0+* to validate dev_has_iommu(). The iommu_group memory is RCU-protected +* via kfree_rcu() in iommu_group_release(), and group->devices is an +* RCU-protected list, so the lookup runs entirely under rcu_read_lock. +* +* Note the device might have been concurrently removed from the group +* (list_del_rcu) before iommu_deinit_device() cleared the dev->iommu. +*/ +rcu_read_lock (); + gdev =__dev_to_gdev_rcu (dev); +if (gdev) {
If this is why the RCU is being added it seems like overkill.
Just add the worker to struct dev_iommu and push it there so it can use a mutex but I'm confused why are we even adding this function?
The entire design of this series was supposed to have the IOMMU driver itself adjust it's "STE" to inhibit translated TLPs synchronosly within its fully locked invalidation loop.
Whats the async worker for?
Jason
Re: [PATCH v4 11/24] iommu: Add iommu_report_device_broken() to quarantine a broken device
Quoted 20 lines · Jason Gunthorpe
Quoted 6 lines · Nicolin Chen
drivers/iommu/iommu.c +6
-0+void iommu_report_device_broken (struct device *dev) +{ +struct group_device *gdev; + +/* +* We cannot hold group->mutex here. Rely on iommu_group_broken_worker() drivers/iommu/iommu.c void pci_dev_reset_iommu_done(struct pci_dev *pdev, int reset_result) · line 4474
Quoted 10 lines
drivers/iommu/iommu.c +10
-0+* to validate dev_has_iommu(). The iommu_group memory is RCU-protected +* via kfree_rcu() in iommu_group_release(), and group->devices is an +* RCU-protected list, so the lookup runs entirely under rcu_read_lock. +* +* Note the device might have been concurrently removed from the group +* (list_del_rcu) before iommu_deinit_device() cleared the dev->iommu. +*/ +rcu_read_lock (); + gdev =__dev_to_gdev_rcu (dev); +if (gdev) {If this is why the RCU is being added it seems like overkill.
Quoted 6 lines
Just add the worker to struct
dev_iommuand push it there so it can use a mutex but I'm confused why are we even adding this function?The entire design of this series was supposed to have the IOMMU driver itself adjust it's "STE" to inhibit translated TLPs synchronosly within its fully locked invalidation loop.
Yes. Surgical STE is done in the driver. But, core-level attaching state doesn't reflect correctly. So the driver calls this function to notify the core (this is in an invalidation context -- not able to use mutex).
Quoted 1 line
Whats the async worker for?
Then, the core needs to block the device using the similar routine to the reset prepare(). And that needs to hold group->mutex, so it needs an async worker.
Do you see a much simpler way?
Thanks
Nicolin
Re: [PATCH v4 11/24] iommu: Add iommu_report_device_broken() to quarantine a broken device
Quoted 33 lines · Nicolin Chen
Quoted 20 lines · Jason Gunthorpe
Quoted 6 lines · Nicolin Chen
drivers/iommu/iommu.c +6
-0+void iommu_report_device_broken (struct device *dev) +{ +struct group_device *gdev; + +/* +* We cannot hold group->mutex here. Rely on iommu_group_broken_worker() drivers/iommu/iommu.c void pci_dev_reset_iommu_done(struct pci_dev *pdev, int reset_result) · line 4474
Quoted 10 lines
drivers/iommu/iommu.c +10
-0+* to validate dev_has_iommu(). The iommu_group memory is RCU-protected +* via kfree_rcu() in iommu_group_release(), and group->devices is an +* RCU-protected list, so the lookup runs entirely under rcu_read_lock. +* +* Note the device might have been concurrently removed from the group +* (list_del_rcu) before iommu_deinit_device() cleared the dev->iommu. +*/ +rcu_read_lock (); + gdev =__dev_to_gdev_rcu (dev); +if (gdev) {If this is why the RCU is being added it seems like overkill.
Quoted 6 lines
Just add the worker to struct
dev_iommuand push it there so it can use a mutex but I'm confused why are we even adding this function?The entire design of this series was supposed to have the IOMMU driver itself adjust it's "STE" to inhibit translated TLPs synchronosly within its fully locked invalidation loop.
Yes. Surgical STE is done in the driver. But, core-level attaching state doesn't reflect correctly. So the driver calls this function to notify the core (this is in an invalidation context -- not able to use mutex).
Quoted 7 lines
Quoted 1 line
Whats the async worker for?
Then, the core needs to block the device using the similar routine to the reset
prepare(). And that needs to holdgroup->mutex, so it needs an async worker.Do you see a much simpler way?
Put the work on the dev_iommu and forget about rcu.
But this is all probably better as some later series if at all. The driver can block the ATS and the expectation is something will FLR the device. The FLR will set the blocking and then restore the domain. None of this async work seems functionally necessary, though it would be a nice to have. Lets focus on the bare minimum here it, it is already a difficult enough problem without tacking on these extras..
Jason
Re: [PATCH v4 11/24] iommu: Add iommu_report_device_broken() to quarantine a broken device
Quoted 11 lines · Jason Gunthorpe
Quoted 7 lines · Nicolin Chen
Quoted 1 line · Jason Gunthorpe
On Mon, May 18, 2026 at 08:38:54PM -0700, Nicolin Chen wrote:
Then, the core needs to block the device using the similar routine to the reset
prepare(). And that needs to holdgroup->mutex, so it needs an async worker.Do you see a much simpler way?
Put the work on the
dev_iommuand forget about rcu.
Quoted 7 lines
But this is all probably better as some later series if at all. The driver can block the ATS and the expectation is something will FLR the device. The FLR will set the blocking and then restore the domain. None of this async work seems functionally necessary, though it would be a nice to have. Lets focus on the bare minimum here it, it is already a difficult enough problem without tacking on these extras..
OK. So you are suggesting a quarantine at the driver-level only:
1. Driver detects ATC_INV timeout during an invalidation.
2. Driver retries the commands to identify the master.
3. Driver calls pci_disable_ats() and clears STE.EATS.
4. Driver marks domain->invs ATS entries as BROKEN. (optional since pci_disable_ats() is done?)
5. Driver sets master->ats_broken to fence concurrent attach:
arm_smmu_write_ste() and arm_smmu_ats_supported().
6. Something external triggers an FLR (sysfs or AER).
7. FLR goes through pci_dev_reset_iommu_prepare()/done(). done() reverts 3+4 and calls the reset_device_done callback clearing master->ats_broken (5).
Right?
Then, we'll have very limited work in the core for this series.
Nicolin
Re: [PATCH v4 11/24] iommu: Add iommu_report_device_broken() to quarantine a broken device
Quoted 20 lines · Nicolin Chen
Quoted 11 lines · Jason Gunthorpe
Quoted 7 lines · Nicolin Chen
Quoted 1 line · Jason Gunthorpe
On Mon, May 18, 2026 at 08:38:54PM -0700, Nicolin Chen wrote:
Then, the core needs to block the device using the similar routine to the reset
prepare(). And that needs to holdgroup->mutex, so it needs an async worker.Do you see a much simpler way?
Put the work on the
dev_iommuand forget about rcu.Quoted 7 lines
But this is all probably better as some later series if at all. The driver can block the ATS and the expectation is something will FLR the device. The FLR will set the blocking and then restore the domain. None of this async work seems functionally necessary, though it would be a nice to have. Lets focus on the bare minimum here it, it is already a difficult enough problem without tacking on these extras..
Quoted 4 lines
OK. So you are suggesting a quarantine at the driver-level only:
1. Driver detects
ATC_INVtimeout during an invalidation.
2. Driver retries the commands to identify the master.
I might argue to push even this out to a followup series given it is complex and I suspect it becomes much simpler after the batch removal...
Quoted 3 lines
3. Driver calls
pci_disable_ats()and clears STE.EATS.
4. Driver marksdomain->invsATS entries as BROKEN. (optional sincepci_disable_ats()is done?)
We need to stop sending invs otherwise there will be trouble making forward progress.
Quoted 2 lines
5. Driver sets
master->ats_brokento fence concurrent attach:
arm_smmu_write_ste()andarm_smmu_ats_supported().
Not sure this is needed, if we race some attach then the attach will re-set EATS, get another timeout and clear EATS. Doesn't seem worth trying to optimize for.
Quoted 4 lines
6. Something external triggers an FLR (sysfs or AER).
7. FLR goes through pci_dev_reset_iommu_prepare()/done().done()reverts 3+4 and calls thereset_device_donecallback clearingmaster->ats_broken(5).
It should restore core/driver/hw synchronization of EATS and the pci_enable_ats() by installing a blocking domain. Then it can go on to re-attach a translating domain and everything is back to correct.
We do need to push a pci error event (didn't see that in this series) so the driver can catch it and start the FLR process. I suppose that will still need to bounce through a workqueue, and once you have that it can also set the blocked domain prior to calling out to the driver.
Jason
Re: [PATCH v4 11/24] iommu: Add iommu_report_device_broken() to quarantine a broken device
Quoted 8 lines · Jason Gunthorpe
Quoted 4 lines
OK. So you are suggesting a quarantine at the driver-level only:
1. Driver detects
ATC_INVtimeout during an invalidation.
2. Driver retries the commands to identify the master.I might argue to push even this out to a followup series given it is complex and I suspect it becomes much simpler after the batch removal...
I see you suggest to treat the entire batch as ATS-broken. Just to
confirm: without per-SID retry, that might falsely block a healthy device in the ATC batch, right? The driver now batches all ATC_INV commands via arm_smmu_invs_end_batch().
Quoted 6 lines
Quoted 3 lines
3. Driver calls
pci_disable_ats()and clears STE.EATS.
4. Driver marksdomain->invsATS entries as BROKEN. (optional sincepci_disable_ats()is done?)We need to stop sending invs otherwise there will be trouble making forward progress.
OK. This needs a surgical invs mutation: maybe INV_TYPE_ATS_BROEKN that you suggested.
Quoted 6 lines
Quoted 2 lines
5. Driver sets
master->ats_brokento fence concurrent attach:
arm_smmu_write_ste()andarm_smmu_ats_supported().Not sure this is needed, if we race some attach then the attach will re-set EATS, get another timeout and clear EATS. Doesn't seem worth trying to optimize for.
I didn't see that coming. master->ats_enabled && state->ats_enabled in the commit() for a concurrent attachment would issue an ATC that may timeout again to re-start the step 1.
And since arm_smmu_atc_inv_master() doesn't use domain->invs, it is not affected by INV_TYPE_ATS_BROKEN. So, ATC_INV can continue to be issued in this case.
Ah, I feel that we are walking in the mine field where every single step could be a kaboom. But your insight is clearly a safe pathway.
Quoted 8 lines
Quoted 4 lines
6. Something external triggers an FLR (sysfs or AER).
7. FLR goes through pci_dev_reset_iommu_prepare()/done().done()reverts 3+4 and calls thereset_device_donecallback clearingmaster->ats_broken(5).It should restore core/driver/hw synchronization of EATS and the
pci_enable_ats()by installing a blocking domain. Then it can go on to re-attach a translating domain and everything is back to correct.
Yea. We probably could drop the master->ats_broken, as done() would be seemingly sufficient. I'll do the rework first, and see if there might be some corner case.
Quoted 4 lines
We do need to push a pci error event (didn't see that in this series) so the driver can catch it and start the FLR process. I suppose that will still need to bounce through a workqueue, and once you have that it can also set the blocked domain prior to calling out to the driver.
In the specific case that I am trying to tackle with this series, I do see AER error prints from the device already but there is no FLR process. So, I assume that, even if we push a PCI error event, that wouldn't necessarily trigger an FLR?
Thanks
Nicolin
Re: [PATCH v4 11/24] iommu: Add iommu_report_device_broken() to quarantine a broken device
Quoted 6 lines · Nicolin Chen
Quoted 5 lines · Jason Gunthorpe
Quoted 4 lines
OK. So you are suggesting a quarantine at the driver-level only:
1. Driver detects
ATC_INVtimeout during an invalidation.
2. Driver retries the commands to identify the master.
Quoted 8 lines
Quoted 3 lines
I might argue to push even this out to a followup series given it is complex and I suspect it becomes much simpler after the batch removal...
I see you suggest to treat the entire batch as ATS-broken. Just to
confirm: without per-SID retry, that might falsely block a healthy device in the ATC batch, right? The driver now batches allATC_INVcommands viaarm_smmu_invs_end_batch().
Yes, it is not good, but a giant complex series is not reviewable. So I'd start with trashing all the devices, then come with a narrowing.
Quoted 11 lines
Quoted 6 lines
Quoted 2 lines
5. Driver sets
master->ats_brokento fence concurrent attach:
arm_smmu_write_ste()andarm_smmu_ats_supported().Not sure this is needed, if we race some attach then the attach will re-set EATS, get another timeout and clear EATS. Doesn't seem worth trying to optimize for.
I didn't see that coming.
master->ats_enabled&&state->ats_enabledin thecommit()for a concurrent attachment would issue an ATC that may timeout again to re-start the step 1.
Quoted 6 lines
And since
arm_smmu_atc_inv_master()doesn't usedomain->invs, it is not affected byINV_TYPE_ATS_BROKEN. So,ATC_INVcan continue to be issued in this case.Ah, I feel that we are walking in the mine field where every single step could be a kaboom. But your insight is clearly a safe pathway.
We cannot eliminate parallel ATS invalidation. Two threads could be concurrently processing the invs list. So it has handle it, the driver is going to have to tolerate a number of redundant error events. It's OK if the unlikely case of parallel attach also generates redundant error events.
Quoted 8 lines
Quoted 4 lines
We do need to push a pci error event (didn't see that in this series) so the driver can catch it and start the FLR process. I suppose that will still need to bounce through a workqueue, and once you have that it can also set the blocked domain prior to calling out to the driver.
In the specific case that I am trying to tackle with this series, I do see AER error prints from the device already but there is no FLR process.
It depends on the driver, mlx5 has a FLR RAS flow for instance.
A driver with a device that can blow up ATS should implement the FLR flow if it wants automatic RAS. It requires driver co-ordination.
But I wasn't thinking we can rely on existing AER events here, yes probably there will be AERs associated with the device exploding so badly it cannot do ATS, but also maybe not..
This is also a problem if we shoot healthy devices as the first stage, there will not be an AER from heathly..
So I guess we need to decide which is better to tackle, the dedicated event or the single invalidation sequence..
Jason
Re: [PATCH v4 11/24] iommu: Add iommu_report_device_broken() to quarantine a broken device
Quoted 11 lines · Jason Gunthorpe
Quoted 10 lines · Nicolin Chen
Quoted 8 lines · Jason Gunthorpe
Quoted 4 lines
OK. So you are suggesting a quarantine at the driver-level only:
1. Driver detects
ATC_INVtimeout during an invalidation.
2. Driver retries the commands to identify the master.I might argue to push even this out to a followup series given it is complex and I suspect it becomes much simpler after the batch removal...
Quoted 7 lines
Quoted 4 lines
I see you suggest to treat the entire batch as ATS-broken. Just to
confirm: without per-SID retry, that might falsely block a healthy device in the ATC batch, right? The driver now batches allATC_INVcommands viaarm_smmu_invs_end_batch().Yes, it is not good, but a giant complex series is not reviewable. So I'd start with trashing all the devices, then come with a narrowing.
I can take that path for now and leave a FIXME.
Another option is to not batch multiple devices, until we support retry (which shouldn't be hard to add since we've already done the coding)?
Quoted 15 lines
Quoted 15 lines
Quoted 6 lines
Quoted 2 lines
5. Driver sets
master->ats_brokento fence concurrent attach:
arm_smmu_write_ste()andarm_smmu_ats_supported().Not sure this is needed, if we race some attach then the attach will re-set EATS, get another timeout and clear EATS. Doesn't seem worth trying to optimize for.
I didn't see that coming.
master->ats_enabled&&state->ats_enabledin thecommit()for a concurrent attachment would issue an ATC that may timeout again to re-start the step 1.And since
arm_smmu_atc_inv_master()doesn't usedomain->invs, it is not affected byINV_TYPE_ATS_BROKEN. So,ATC_INVcan continue to be issued in this case.
Quoted 6 lines
Quoted 2 lines
Ah, I feel that we are walking in the mine field where every single step could be a kaboom. But your insight is clearly a safe pathway.
We cannot eliminate parallel ATS invalidation. Two threads could be concurrently processing the invs list. So it has handle it, the driver is going to have to tolerate a number of redundant error events.
OK. That sounds like we still need a flag or locking so that at least pci_disable_ats() would not be called again. I will see what I can do.
Quoted 10 lines
Quoted 8 lines
Quoted 4 lines
We do need to push a pci error event (didn't see that in this series) so the driver can catch it and start the FLR process. I suppose that will still need to bounce through a workqueue, and once you have that it can also set the blocked domain prior to calling out to the driver.
In the specific case that I am trying to tackle with this series, I do see AER error prints from the device already but there is no FLR process.
It depends on the driver, mlx5 has a FLR RAS flow for instance.
I assume a driver like that would trigger FLR flow on its own?
Quoted 2 lines
A driver with a device that can blow up ATS should implement the FLR flow if it wants automatic RAS. It requires driver co-ordination.
Or FLR via sysfs, which I have been doing...
Quoted 3 lines
But I wasn't thinking we can rely on existing AER events here, yes probably there will be AERs associated with the device exploding so badly it cannot do ATS, but also maybe not..
So, should I put the AER injection on hold for a future work? To be honest, I am still not very clear how AER injection could help here; or is it for a case where ATC times out while device isn't aware of any AER fault?
Quoted 5 lines
This is also a problem if we shoot healthy devices as the first stage, there will not be an AER from heathly..
So I guess we need to decide which is better to tackle, the dedicated event or the single invalidation sequence..
I feel it safer to not break healthy devices. Otherwise, would a nesting parent invalidation falsely block all devices, if one of them times out?
Thanks
Nicolin
Re: [PATCH v4 11/24] iommu: Add iommu_report_device_broken() to quarantine a broken device
Quoted 5 lines · Nicolin Chen
Quoted 5 lines
Quoted 4 lines
I see you suggest to treat the entire batch as ATS-broken. Just to
confirm: without per-SID retry, that might falsely block a healthy device in the ATC batch, right? The driver now batches allATC_INVcommands viaarm_smmu_invs_end_batch().
Quoted 8 lines
Quoted 2 lines
Yes, it is not good, but a giant complex series is not reviewable. So I'd start with trashing all the devices, then come with a narrowing.
I can take that path for now and leave a FIXME.
Another option is to not batch multiple devices, until we support retry (which shouldn't be hard to add since we've already done the coding)?
That's an interesting idea, it undoes some of the meaningful optimization we have recently done though :\
Quoted 7 lines
Quoted 3 lines
We cannot eliminate parallel ATS invalidation. Two threads could be concurrently processing the invs list. So it has handle it, the driver is going to have to tolerate a number of redundant error events.
OK. That sounds like we still need a flag or locking so that at least
pci_disable_ats()would not be called again. I will see what I can do.
I think we can call pci_disable_ats() as many times as we want, we mostly need the driver to merge multiple error notifications for the same event.
Quoted 3 lines
Quoted 1 line
It depends on the driver, mlx5 has a FLR RAS flow for instance.
I assume a driver like that would trigger FLR flow on its own?
Yes
Quoted 4 lines
Quoted 2 lines
A driver with a device that can blow up ATS should implement the FLR flow if it wants automatic RAS. It requires driver co-ordination.
Or FLR via sysfs, which I have been doing...
Yes
Quoted 8 lines
Quoted 3 lines
But I wasn't thinking we can rely on existing AER events here, yes probably there will be AERs associated with the device exploding so badly it cannot do ATS, but also maybe not..
So, should I put the AER injection on hold for a future work? To be honest, I am still not very clear how AER injection could help here; or is it for a case where ATC times out while device isn't aware of any AER fault?
Right, if we don't get an AER fault then we should ensure the ATC is surfaced, but you have a reasonable point that it isn't so likely the get an ATC invalidation timeout without a corresponding related AER..
Still, I'd feel better if it is was definititive and we didn't rely on this. This further points that the driver has to merge multiple error notifications if it gets some AERs and a new "ATC ERROR" all for the same key event.
Jason
Re: [PATCH v4 11/24] iommu: Add iommu_report_device_broken() to quarantine a broken device
Quoted 9 lines · Jason Gunthorpe
Quoted 8 lines · Nicolin Chen
Quoted 7 lines
Quoted 4 lines
I see you suggest to treat the entire batch as ATS-broken. Just to
confirm: without per-SID retry, that might falsely block a healthy device in the ATC batch, right? The driver now batches allATC_INVcommands viaarm_smmu_invs_end_batch().Yes, it is not good, but a giant complex series is not reviewable. So I'd start with trashing all the devices, then come with a narrowing.
Quoted 8 lines
Quoted 5 lines
I can take that path for now and leave a FIXME.
Another option is to not batch multiple devices, until we support retry (which shouldn't be hard to add since we've already done the coding)?
That's an interesting idea, it undoes some of the meaningful optimization we have recently done though :\
I remember you didn't like it. That's why we had the retry(), which I feel we should keep it..
Quoted 9 lines
Quoted 7 lines
Quoted 3 lines
We cannot eliminate parallel ATS invalidation. Two threads could be concurrently processing the invs list. So it has handle it, the driver is going to have to tolerate a number of redundant error events.
OK. That sounds like we still need a flag or locking so that at least
pci_disable_ats()would not be called again. I will see what I can do.I think we can call
pci_disable_ats()as many times as we want
That triggers WARN_ON(!dev->ats_enabled) in pci_disable_ats :-(
Quoted 3 lines
we
mostly need the driver to merge multiple error notifications for the same event.
Yes.
Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
But I wasn't thinking we can rely on existing AER events here, yes probably there will be AERs associated with the device exploding so badly it cannot do ATS, but also maybe not..
So, should I put the AER injection on hold for a future work? To be honest, I am still not very clear how AER injection could help here; or is it for a case where ATC times out while device isn't aware of any AER fault?
Quoted 8 lines
Right, if we don't get an AER fault then we should ensure the ATC is surfaced, but you have a reasonable point that it isn't so likely the get an ATC invalidation timeout without a corresponding related AER..
Still, I'd feel better if it is was definititive and we didn't rely on this. This further points that the driver has to merge multiple error notifications if it gets some AERs and a new "ATC ERROR" all for the same key event.
I feel some race here... Part of the complexity of this v4 is to deal with concurrent device reset during the async report() between IOMMU core and driver. Now, we add AER that could compete on the device side as well...
I will see what I can do here, yet likely would defer it to a followup series, given the direction is to shrink the size of the series.
Thanks
Nicolin
Re: [PATCH v4 11/24] iommu: Add iommu_report_device_broken() to quarantine a broken device
Quoted 11 lines · Nicolin Chen
Quoted 9 lines
Quoted 7 lines
Quoted 3 lines
We cannot eliminate parallel ATS invalidation. Two threads could be concurrently processing the invs list. So it has handle it, the driver is going to have to tolerate a number of redundant error events.
OK. That sounds like we still need a flag or locking so that at least
pci_disable_ats()would not be called again. I will see what I can do.I think we can call
pci_disable_ats()as many times as we wantThat triggers
WARN_ON(!dev->ats_enabled)inpci_disable_ats:-(
IMHO I'd rather take that out than add a bunch of complication in the iommu drivers..
Quoted 9 lines
Quoted 4 lines
Still, I'd feel better if it is was definititive and we didn't rely on this. This further points that the driver has to merge multiple error notifications if it gets some AERs and a new "ATC ERROR" all for the same key event.
I feel some race here... Part of the complexity of this v4 is to deal with concurrent device reset during the async
report()between IOMMU core and driver. Now, we add AER that could compete on the device side as well...
It is always going to have concurrent events, so long as the resets sequence in an orderly way it doesn't matter if they overlap.
Most likely the driver will have locking that prevents it from pushing concurrent resets.
Jason
PATCH v4 12/24 iommu/arm-smmu-v3: Mark ATC invalidate timeouts via lockless 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, after polling completes (or times out), test and clear its dedicated bit. If set, return -EIO to trigger device quarantine.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 42 ++++++++++++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -0
@@ -700,6 +700,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 +41 -1
69 lines not on this page · raw message
Signature · 1 line
2.43.0
PATCH v4 13/24 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.
After arm_smmu_device_disable(), ack GERRORN and return. This intentionally duplicates the last two lines of the function because this SFM path will be slightly different.
--- 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
@@ -2268,8 +2268,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
PATCH v4 14/24 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() and tegra241_vintf0_handle_error() now take the per-cmdq cmdq_err_lock when acking CMDQ_ERR.
tegra241_vintf0_handle_error() also checks (gerror ^ gerrorn) inside the cmdq_err_lock since a concurrent cmdq_err_handler may have already acked the error. arm_smmu_gerror_handler() already covers this 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 | 18 ++++++++++++++---- drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 15 ++++++++++++--- 3 files changed, 36 insertions(+), 9 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +10 -2
@@ -695,6 +695,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;
@@ -702,6 +706,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,
@@ -1163,8 +1171,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 +14 -4
@@ -2255,13 +2255,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",
@@ -2270,6 +2275,8 @@ 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 ); +/* Release before arm_smmu_device_disable() that sleeps */ +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 ;
@@ -2297,6 +2304,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 ; }
@@ -4357,13 +4365,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 );
@@ -4389,7 +4399,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 +12 -3
@@ -319,10 +319,19 @@ static void tegra241_vintf0_handle_error(struct tegra241_vintf *vintf)while (map) {unsigned long lidx =__ffs64 (map);struct tegra241_vcmdq *vcmdq = vintf->lvcmdqs[lidx]; -u32 gerror =readl_relaxed (REG_VCMDQ_PAGE0 (vcmdq,GERROR )); +struct arm_smmu_cmdq *cmdq = &vcmdq->cmdq; +unsigned long flags; +u32 gerror, gerrorn; -__arm_smmu_cmdq_skip_err (&vintf->cmdqv->smmu, &vcmdq->cmdq); +raw_spin_lock_irqsave (&cmdq->cmdq_err_lock, flags); + gerror =readl_relaxed (REG_VCMDQ_PAGE0 (vcmdq,GERROR )); + gerrorn =readl_relaxed (REG_VCMDQ_PAGE0 (vcmdq,GERRORN )); + +if ((gerror ^ gerrorn) &GERROR_CMDQ_ERR ) +__arm_smmu_cmdq_skip_err (&vintf->cmdqv->smmu, + cmdq);writel (gerror,REG_VCMDQ_PAGE0 (vcmdq,GERRORN )); +raw_spin_unlock_irqrestore (&cmdq->cmdq_err_lock, flags); map &= ~BIT_ULL (lidx); } }
@@ -643,7 +652,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
PATCH v4 15/24 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 | 36 ++++++++++++++++++- .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 23 +++++++++++- 2 files changed, 57 insertions(+), 2 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +35 -1
@@ -813,6 +813,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); +/* * Test atc_sync_timeouts first and see if there is ATC timeout * resulted from this cmdlist. Return -EIO to separate from the
@@ -2251,6 +2260,31 @@ 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 on the primary cmdq. Installed as the primary +* cmdq's cmdq_err_handler so arm_smmu_cmdq_issue_cmdlist() can drain after +* a CMD_SYNC poll timeout; serialized against arm_smmu_gerror_handler() by +* cmdq->cmdq_err_lock. +*/ +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;
@@ -4399,7 +4433,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 +22 -1
@@ -337,6 +337,27 @@ static void tegra241_vintf0_handle_error(struct tegra241_vintf *vintf) } } +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 )); +} +static irqreturn_t tegra241_cmdqv_isr (int irq,void *devid) {struct tegra241_cmdqv *cmdqv = (struct tegra241_cmdqv *)devid;
@@ -652,7 +673,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
PATCH v4 16/24 iommu/arm-smmu-v3: Co-clear pending CMDQ_ERR when queue_has_space() fails
In reply to Nicolin ChenIt's unusual when the command queue fails the queue_has_space() test. There must be something stalling the HW so the queue does not advance.
Currently, a possible scenario: arm_smmu_cmdq_issue_cmdlist() may be called in an IRQ context where IRQ is already disabled. E.g., ata_sg_clean() in drivers/ata/libata-core.c
When GERROR is affined to the CPU currently running with IRQs disabled, the GERROR ISR will not run and a CERROR_ILL will not be cleared, which stalls the HW; arm_smmu_cmdq_poll_until_not_full() then either times out or loops without seeing CONS advance.
The window is narrow and it's very difficult to trigger this lockup. Yet, a subsequent change requires serializing the STE update routines between the attach_dev path (mutex-ed) and the invalidation path (non-mutexed), where a spin_lock_irqsave is inevitable. And this would expand the currently narrow window to a wider range -- arm_smmu_write_ste() as well.
Since we have a cmdq_err_handler, call it when queue_has_space() fails, to give the CMDQ hardware a chance to advance its CONS.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 7 +++++++ 1 file changed, 7 insertions(+)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +7 -0
@@ -723,6 +723,13 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,while (!queue_has_space (&llq, n + sync)) {local_irq_restore (flags); +/* +* If the CMDQ is nearly full, it's possible that the HW +* is stalled by an unhandled GERROR_CMDQ_ERR. Thus give +* cmdq_err_handler a chance before each poll. +*/ +if (cmdq->cmdq_err_handler) + cmdq->cmdq_err_handler (smmu, cmdq);if (arm_smmu_cmdq_poll_until_not_full (smmu, cmdq, &llq))dev_err_ratelimited (smmu->dev,"CMDQ timeout\n" );local_irq_save (flags);
Signature · 1 line
2.43.0
PATCH v4 17/24 iommu/arm-smmu-v3: Add master in arm_smmu_inv for ATS entries
In reply to Nicolin ChenStoring the master pointer allows backtracking it from an ATS invalidation entry, which will be useful when handling ATC invalidation timeouts.
Don't simply swap the "smmu" pointer for the "master": a non-ATS entry may be shared across multiple devices (masters). An ATS entry is okay since it is tied to a unique SID.
Master must outlive any concurrent RCU reader iterating the domain->invs, because inv->master is dereferenced inside the read-side critical section.
Add a synchronize_rcu() in arm_smmu_release_device() before freeing master.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 14 +++++++++++--- 2 files changed, 12 insertions(+), 3 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -0
@@ -738,6 +738,7 @@ enum arm_smmu_inv_type {struct arm_smmu_inv {struct arm_smmu_device *smmu; +struct arm_smmu_master *master;/* INV_TYPE_ATS* */ u8 type;u8 size_opcode;u8 nsize_opcode;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +11 -3
@@ -3211,6 +3211,7 @@ arm_smmu_master_build_inv(struct arm_smmu_master *master,case INV_TYPE_ATS_FULL : cur->size_opcode = cur->nsize_opcode =CMDQ_OP_ATC_INV ; cur->ssid = ssid; + cur->master = master;break ; }
@@ -4168,9 +4169,6 @@ static void arm_smmu_remove_master(struct arm_smmu_master *master)for (i =0 ; i < fwspec->num_ids; i++)rb_erase (&master->streams[i].node, &smmu->streams);mutex_unlock (&smmu->streams_mutex); - -kfree (master->streams); -kfree (master->build_invs); }static struct iommu_device *arm_smmu_probe_device (struct device *dev)
@@ -4244,6 +4242,16 @@ static void arm_smmu_release_device(struct device *dev)arm_smmu_remove_master (master);if (arm_smmu_cdtab_allocated (&master->cd_table))arm_smmu_free_cd_tables (master); + +/* +* The iommu core detaches @dev from every iommu domain before invoking +* release_device. So the updated domain->invs no longer references the +* @master; IOW, new RCU readers cannot reach it. Wait one grace period +* for in-flight readers to drop their references. +*/ +synchronize_rcu (); +kfree (master->streams); +kfree (master->build_invs);kfree (master); }
Signature · 1 line
2.43.0
Re: [PATCH v4 17/24] iommu/arm-smmu-v3: Add master in arm_smmu_inv for ATS entries
Quoted 23 lines · Nicolin Chen
Storing the master pointer allows backtracking it from an ATS invalidation entry, which will be useful when handling ATC invalidation timeouts.
Don't simply swap the "smmu" pointer for the "master": a non-ATS entry may be shared across multiple devices (masters). An ATS entry is okay since it is tied to a unique SID.
Master must outlive any concurrent RCU reader iterating the
domain->invs, becauseinv->masteris dereferenced inside the read-side critical section.Add a
synchronize_rcu()inarm_smmu_release_device()before freeing master.Signed-off-by Nicolin Chen <[email protected]>--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 14 +++++++++++--- 2 files changed, 12 insertions(+), 3 deletions(-)drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +0
-0
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h enum arm_smmu_inv_type { · line 741
Quoted 4 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1
-0@@ -738,6 +738,7 @@ enum arm_smmu_inv_type {struct arm_smmu_inv {struct arm_smmu_device *smmu; +struct arm_smmu_master *master;/* INV_TYPE_ATS* */
I don't like this and the locking for just a slow error case.. You should use the SID for this and have a spinlocked ATS master list to search for the SID.
Jason
PATCH v4 18/24 iommu/arm-smmu-v3: Introduce master->ats_broken flag
In reply to Nicolin ChenA subsequent change will set this flag when IOMMU cannot trust device's ATS function. E.g., when ATC invalidation request to the device times out.
Once the flag is set, unsupport the ATS feature to prevent data corruption, and skip further ATC invalidation commands to avoid new timeouts.
Unset the flag when the device finishes a reset for recovery.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 29 +++++++++++++++++++++ 2 files changed, 30 insertions(+)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -0
@@ -1017,6 +1017,7 @@ struct arm_smmu_master {/* Locked by the iommu core using the group mutex */ struct arm_smmu_ctx_desc_cfg cd_table;unsigned int num_streams; +bool ats_broken; bool ats_enabled : 1; bool ste_ats_enabled : 1;bool stall_enabled;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +29 -0
@@ -2429,6 +2429,10 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master,struct arm_smmu_cmd cmd;struct arm_smmu_cmdq_batch cmds; +/* Do not issue ATC_INV that will definitely time out */ +if (READ_ONCE (master->ats_broken)) +return 0 ; + cmd =arm_smmu_make_cmd_atc_inv_all (0 ,IOMMU_NO_PASID );arm_smmu_cmdq_batch_init_cmd (master->smmu, &cmds, &cmd);for (i =0 ; i < master->num_streams; i++)
@@ -2651,12 +2655,18 @@ static void __arm_smmu_domain_inv_range(struct arm_smmu_invs *invs, cur->id));break ;case INV_TYPE_ATS : +/* Do not issue ATC_INV that will definitely time out */ +if (READ_ONCE (cur->master->ats_broken)) +break ;arm_smmu_cmdq_batch_add_cmd ( smmu, &cmds,arm_smmu_atc_inv_to_cmd (cur->id, cur->ssid, iova, size));break ;case INV_TYPE_ATS_FULL : +/* Do not issue ATC_INV that will definitely time out */ +if (READ_ONCE (cur->master->ats_broken)) +break ; arm_smmu_cmdq_batch_add_cmd( smmu, &cmds, arm_smmu_make_cmd_atc_inv_all(cur->id,
@@ -2995,6 +3005,16 @@ void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master, } } +static void arm_smmu_reset_device_done (struct device *dev) +{ +struct arm_smmu_master *master =dev_iommu_priv_get (dev); + +if (WARN_ON (!master)) +return ; +/* Pair with lockless readers */ +WRITE_ONCE (master->ats_broken, false); +} +static bool arm_smmu_ats_supported (struct arm_smmu_master *master) {struct device *dev = master->dev;
@@ -3007,6 +3027,14 @@ static bool arm_smmu_ats_supported(struct arm_smmu_master *master)if (!(fwspec->flags &IOMMU_FWSPEC_PCI_RC_ATS ))return false; +/* +* Do not enable ATS if master->ats_broken is set. The PCI device should +* go through a recovery (reset) that shall notify the SMMUv3 driver via +* a reset_device_done callback. +*/ +if (READ_ONCE (master->ats_broken)) +return false; +return dev_is_pci (dev) &&pci_ats_supported (to_pci_dev (dev)); }
@@ -4345,6 +4373,7 @@ static const struct iommu_ops arm_smmu_ops = { .domain_alloc_paging_flags = arm_smmu_domain_alloc_paging_flags, .probe_device = arm_smmu_probe_device, .release_device = arm_smmu_release_device, + .reset_device_done = arm_smmu_reset_device_done, .device_group = arm_smmu_device_group, .of_xlate = arm_smmu_of_xlate, .get_resv_regions = arm_smmu_get_resv_regions,
Signature · 1 line
2.43.0
Re: [PATCH v4 18/24] iommu/arm-smmu-v3: Introduce master->ats_broken flag
Quoted 16 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4
-0@@ -2429,6 +2429,10 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master,struct arm_smmu_cmd cmd;struct arm_smmu_cmdq_batch cmds; +/* Do not issue ATC_INV that will definitely time out */ +if (READ_ONCE (master->ats_broken)) +return 0 ; + cmd =arm_smmu_make_cmd_atc_inv_all (0 ,IOMMU_NO_PASID );arm_smmu_cmdq_batch_init_cmd (master->smmu, &cmds, &cmd);for (i =0 ; i < master->num_streams; i++)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void __arm_smmu_domain_inv_range(struct arm_smmu_invs *invs, · line 2660
Quoted 6 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3
-0@@ -2651,12 +2655,18 @@ static void __arm_smmu_domain_inv_range(struct arm_smmu_invs *invs, cur->id));break ;case INV_TYPE_ATS : +/* Do not issue ATC_INV that will definitely time out */ +if (READ_ONCE (cur->master->ats_broken)) +break ;
Yuk, this should be a bool in the invs list, not the master.
If the flow is to have the core code always attach a blocked domain before reset_done then the invs list will naturally fix itself.
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static bool arm_smmu_ats_supported(struct arm_smmu_master *master) · line 3034
Quoted 9 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +5
-0@@ -3007,6 +3027,14 @@ static bool arm_smmu_ats_supported(struct arm_smmu_master *master)if (!(fwspec->flags &IOMMU_FWSPEC_PCI_RC_ATS ))return false; +/* +* Do not enable ATS if master->ats_broken is set. The PCI device should +* go through a recovery (reset) that shall notify the SMMUv3 driver via +* a reset_device_done callback. +*/
Should just fail attach of paging domains at this point, the core code should arguably be preventing races like this. Attaching a paging domain in a broken way is going to create more problems.
Jason
Re: [PATCH v4 18/24] iommu/arm-smmu-v3: Introduce master->ats_broken flag
Quoted 26 lines · Jason Gunthorpe
Quoted 16 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4
-0@@ -2429,6 +2429,10 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master,struct arm_smmu_cmd cmd;struct arm_smmu_cmdq_batch cmds; +/* Do not issue ATC_INV that will definitely time out */ +if (READ_ONCE (master->ats_broken)) +return 0 ; + cmd =arm_smmu_make_cmd_atc_inv_all (0 ,IOMMU_NO_PASID );arm_smmu_cmdq_batch_init_cmd (master->smmu, &cmds, &cmd);for (i =0 ; i < master->num_streams; i++)drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void __arm_smmu_domain_inv_range(struct arm_smmu_invs *invs, · line 2660
Quoted 6 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3
-0@@ -2651,12 +2655,18 @@ static void __arm_smmu_domain_inv_range(struct arm_smmu_invs *invs, cur->id));break ;case INV_TYPE_ATS : +/* Do not issue ATC_INV that will definitely time out */ +if (READ_ONCE (cur->master->ats_broken)) +break ;Yuk, this should be a bool in the invs list, not the master.
Quoted 2 lines
If the flow is to have the core code always attach a blocked domain before
reset_donethen the invs list will naturally fix itself.
So I've tried INV_TYPE_ATS_BROKEN: during per-domain invalidation, each batch is built from domain->invs so it can carry the "invs"; if the batch times out, we can immediately mutate its ATS entries.
But I realized a limitation. E.g., if a device attaches to two SVA domains on two SSIDs. An invalidation timing out on one of the SVA domains could mark INV_TYPE_ATS_BROKEN in its own invs, but not in the other SVA domain's invs?
So, it seems that master->ats_broken is still a cleaner solution? In the same spirit of "the invs list will naturally fix itself":
we could clear master->ats_broken on every attach; if the device has not recovered yet, arm_smmu_atc_inv_master() in commit() will time out and set it back to true. Right?
Thanks
Nicolin
Re: [PATCH v4 18/24] iommu/arm-smmu-v3: Introduce master->ats_broken flag
Quoted 30 lines · Nicolin Chen
Quoted 26 lines · Jason Gunthorpe
Quoted 16 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4
-0@@ -2429,6 +2429,10 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master,struct arm_smmu_cmd cmd;struct arm_smmu_cmdq_batch cmds; +/* Do not issue ATC_INV that will definitely time out */ +if (READ_ONCE (master->ats_broken)) +return 0 ; + cmd =arm_smmu_make_cmd_atc_inv_all (0 ,IOMMU_NO_PASID );arm_smmu_cmdq_batch_init_cmd (master->smmu, &cmds, &cmd);for (i =0 ; i < master->num_streams; i++)drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void __arm_smmu_domain_inv_range(struct arm_smmu_invs *invs, · line 2660
Quoted 6 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3
-0@@ -2651,12 +2655,18 @@ static void __arm_smmu_domain_inv_range(struct arm_smmu_invs *invs, cur->id));break ;case INV_TYPE_ATS : +/* Do not issue ATC_INV that will definitely time out */ +if (READ_ONCE (cur->master->ats_broken)) +break ;Yuk, this should be a bool in the invs list, not the master.
Quoted 2 lines
If the flow is to have the core code always attach a blocked domain before
reset_donethen the invs list will naturally fix itself.
Quoted 8 lines
So I've tried
INV_TYPE_ATS_BROKEN: during per-domain invalidation, each batch is built fromdomain->invsso it can carry the "invs"; if the batch times out, we can immediately mutate its ATS entries.But I realized a limitation. E.g., if a device attaches to two SVA domains on two SSIDs. An invalidation timing out on one of the SVA domains could mark
INV_TYPE_ATS_BROKENin its own invs, but not in the other SVA domain's invs?
You'd have to mark all the S1's sharing the STE.
Quoted 1 line
So, it seems that
master->ats_brokenis still a cleaner solution?
I don't want the invs code touching master, that is against the entire design.
Maybe a flag in the invs list itself is sufficient.
Jason
Re: [PATCH v4 18/24] iommu/arm-smmu-v3: Introduce master->ats_broken flag
Quoted 7 lines · Jason Gunthorpe
Quoted 6 lines · Nicolin Chen
Quoted 1 line · Jason Gunthorpe
On Mon, May 18, 2026 at 08:39:01PM -0700, Nicolin Chen wrote:
So I've tried
INV_TYPE_ATS_BROKEN: during per-domain invalidation, each batch is built fromdomain->invsso it can carry the "invs"; if the batch times out, we can immediately mutate its ATS entries.
Quoted 6 lines
Quoted 4 lines
But I realized a limitation. E.g., if a device attaches to two SVA domains on two SSIDs. An invalidation timing out on one of the SVA domains could mark
INV_TYPE_ATS_BROKENin its own invs, but not in the other SVA domain's invs?You'd have to mark all the S1's sharing the STE.
That would be a bit convoluted as we would have to go through all other domains' invs arrays.
A master (that timed out an ATC_INV) might be attached to multiple domains (RID, SVA1, SVA2, ...). Also, we currently don't have any per-master reverse-tracking to its attached domains (master_domain is added to smmu_domain->devices list only for now).
So, two things would be needed on top of what we currently have.
Firstly, we would need another per-master list tracking all the attached smmu_domains. Maybe reuse master_domain? Let's call this master->master_domains for now.
Secondly, locking. We have two paths that can trigger an ATC_INV
timeout: __arm_smmu_domain_inv_range() that takes the rwlock read on the current smmu_domain->invs; arm_smmu_atc_inv_master() that doesn't take any rwlock. When these two paths walk through the master->master_domains, we would need to take different rwlocks for those domains. Also, the __arm_smmu_domain_inv_range() path should skip the invs on the current master_domain, as the rwlock is already held.
I wonder what's your opinion about these?
Given all this complexity, I started to wonder if we could have implemented the invs as an RCU-list than an RCU-array: all IOTLB tag nodes would be still allocated to add/delete/read locklessly; all ATS nodes would be fixed in the master structure to add/del/ read with the rwlock. Then, a timeout occurring to either path can simply mutate the ATS entries on the master directly without going through the list of domains.
Quoted 4 lines
Quoted 1 line
So, it seems that
master->ats_brokenis still a cleaner solution?I don't want the invs code touching master, that is against the entire design.
I think I can understand the idea here: we want the invs design to be in the common code, so anything that's driver-specific (smmu or master) shouldn't be touched.
Quoted 1 line
Maybe a flag in the invs list itself is sufficient.
I think we would have to use INV_TYPE_ATS_BROKEN than a per-invs
flag: e.g., a nesting parent domain will have multiple ATS devices so it cannot use one flag on its big invs to separate the broken devices from all other healthy devices.
Nicolin
Re: [PATCH v4 18/24] iommu/arm-smmu-v3: Introduce master->ats_broken flag
Quoted 13 lines · Nicolin Chen
Quoted 12 lines · Jason Gunthorpe
Quoted 10 lines · Nicolin Chen
Quoted 1 line · Jason Gunthorpe
On Mon, May 18, 2026 at 08:39:01PM -0700, Nicolin Chen wrote:
So I've tried
INV_TYPE_ATS_BROKEN: during per-domain invalidation, each batch is built fromdomain->invsso it can carry the "invs"; if the batch times out, we can immediately mutate its ATS entries.But I realized a limitation. E.g., if a device attaches to two SVA domains on two SSIDs. An invalidation timing out on one of the SVA domains could mark
INV_TYPE_ATS_BROKENin its own invs, but not in the other SVA domain's invs?
Quoted 4 lines
Quoted 1 line
You'd have to mark all the S1's sharing the STE.
That would be a bit convoluted as we would have to go through all other domains' invs arrays.
Ok, that is certainly an annoying problem.
I don't have a better idea than storing the master unfortunately
But I think the locking for that is going to be tricky, I'm not sure it does actually fully work..
Jason
Re: [PATCH v4 18/24] iommu/arm-smmu-v3: Introduce master->ats_broken flag
Quoted 19 lines · Jason Gunthorpe
Quoted 13 lines · Nicolin Chen
Quoted 12 lines · Jason Gunthorpe
Quoted 10 lines · Nicolin Chen
Quoted 1 line · Jason Gunthorpe
On Mon, May 18, 2026 at 08:39:01PM -0700, Nicolin Chen wrote:
So I've tried
INV_TYPE_ATS_BROKEN: during per-domain invalidation, each batch is built fromdomain->invsso it can carry the "invs"; if the batch times out, we can immediately mutate its ATS entries.But I realized a limitation. E.g., if a device attaches to two SVA domains on two SSIDs. An invalidation timing out on one of the SVA domains could mark
INV_TYPE_ATS_BROKENin its own invs, but not in the other SVA domain's invs?Quoted 4 lines
Quoted 1 line
You'd have to mark all the S1's sharing the STE.
That would be a bit convoluted as we would have to go through all other domains' invs arrays.
Quoted 6 lines
Ok, that is certainly an annoying problem.
I don't have a better idea than storing the master unfortunately
But I think the locking for that is going to be tricky, I'm not sure it does actually fully work..
Yes, there can be a race that sets STE.EATS back while per-master flag is set, which would skip the ATC_INV in commit(), so no more ATC_INV timeout that resets STE.EATS=0. To close it, we can force STE.EATS=0 at the end of commit() when state->ats_enabled and the per-master flag are both set, which is only possible in a race.
However, after more thought, I found that there's a big trade-off to pay in the invalidation path, if we go on this path. The invs array only has SID, so it needs to convert it to master using the rb_tree for every entry (holding streams_lock).
I asked AI to list and compare the three solutions that we have:
(1) Per-invs marker: INV_TYPE_ATS_BROKEN + master_domains disable_ats() in the timeout path walks master->master_domains and flips matching ATS invs entries to the BROKEN type.
+ invs walker is free (one case label in the existing type switch).
+ No lock or pointer deref in the invs walker.
+ No master pointer stored in invs; no lifetime concern.
- disable_ats() walks every (master, domain) and marks each invs set; the list needs locking usable from atomic.
- No master-level flag, so atc_inv_master loses the cheap early skip and attach has nothing to gate ats_enabled on. Concurrent quarantine self-heals only after the next ATC_INV times out (~1s), unless we also keep a master flag (i.e. a hybrid of (1) and (2)).
(2) Per-master flag + streams_lock
invs walker resolves SID -> master via streams_lock and reads
master->ats_broken. + Single source of truth on the master.
+ disable_ats() is one WRITE_ONCE.
+ atc_inv_master early-skips via one READ_ONCE.
+ attach gates ats_enabled on the flag; a concurrent quarantine race can be closed by a short post-attach re-check in commit()
+ No master pointer in invs; no lifetime concern.
- invs walker pays streams_lock + rb_find(SID) per ATS entry on every invalidation. Measurable on ATS-heavy workloads.
(3) Per-master flag + inv->master pointer (v4)
invs entry carries a master pointer; the invs walker reads cur->master->ats_broken directly.
+ invs walker is one READ_ONCE through a cached pointer.
+ disable_ats is one WRITE_ONCE.
+ atc_inv_master early-skip via one READ_ONCE.
+ attach gate + post-attach re-check, same as (2).
- invs holds a master ptr, so release_device must synchronize_rcu() before freeing the master to drain walkers under rcu_read_lock(). We dropped this from v4 for that reason.
Side-by-side:
(1) (2) (3) invs walker fastest slow fast disable_ats slow fast fast atc_inv skip slow fast fast attach race slow fast fast master free fast fast slow
FWIW, AI picks (3), but this would reintroduce synchronize_rcu() that you dislike. My personal feeling is that (1) is the cleanest shirt in the dirty laundry?
I'd like to hear your thought before finalizing the design.
Thanks
Nicolin
Re: [PATCH v4 18/24] iommu/arm-smmu-v3: Introduce master->ats_broken flag
Quoted 24 lines · Nicolin Chen
Quoted 23 lines · Jason Gunthorpe
Quoted 13 lines · Nicolin Chen
Quoted 12 lines · Jason Gunthorpe
Quoted 10 lines · Nicolin Chen
Quoted 1 line · Jason Gunthorpe
On Mon, May 18, 2026 at 08:39:01PM -0700, Nicolin Chen wrote:
So I've tried
INV_TYPE_ATS_BROKEN: during per-domain invalidation, each batch is built fromdomain->invsso it can carry the "invs"; if the batch times out, we can immediately mutate its ATS entries.But I realized a limitation. E.g., if a device attaches to two SVA domains on two SSIDs. An invalidation timing out on one of the SVA domains could mark
INV_TYPE_ATS_BROKENin its own invs, but not in the other SVA domain's invs?Quoted 4 lines
Quoted 1 line
You'd have to mark all the S1's sharing the STE.
That would be a bit convoluted as we would have to go through all other domains' invs arrays.
Ok, that is certainly an annoying problem.
I don't have a better idea than storing the master unfortunately
Quoted 8 lines
Quoted 2 lines
But I think the locking for that is going to be tricky, I'm not sure it does actually fully work..
Yes, there can be a race that sets STE.EATS back while per-master flag is set, which would skip the
ATC_INVincommit(), so no moreATC_INVtimeout that resets STE.EATS=0. To close it, we can force STE.EATS=0 at the end ofcommit()whenstate->ats_enabledand the per-master flag are both set, which is only possible in a race.
I don't see any of these options as appealing. We have to maintain a few key invariants, and I think it cannot be done without a way to find all the domains that are using the STE.
One way or another you have to be using the invs list rw locks to synchronize the EATS state changes.
It is okayish to be sloppy when turning EATS off, but when turning it back on we do need to cycle through every invs list and toggle its lock to ensure that the invalidations are synchronized before EATS=enable happens.
Given you must have a way to go from STE -> master -> all invs lists I'm not sure either option really makes such a large difference.
If so then adjusting the invs to disable the ATS is pretty simple, run over the xarray and set them all off. Yes you could find the master through a SID lookup with some locking adjustment.
Quoted 11 lines
(1) Per-invs marker:
INV_TYPE_ATS_BROKEN+master_domainsdisable_ats()in the timeout path walksmaster->master_domainsand flips matching ATS invs entries to the BROKEN type.+ invs walker is free (one case label in the existing type switch).
+ No lock or pointer deref in the invs walker.
+ No master pointer stored in invs; no lifetime concern.-
disable_ats()walks every (master, domain) and marks each invs set; the list needs locking usable from atomic.
This doesn't seem so bad
Quoted 11 lines
(2) Per-master flag +
streams_lockinvs walker resolves SID -> master via streams_lock and reads master->ats_broken.+ Single source of truth on the master.
+disable_ats()is oneWRITE_ONCE.
+atc_inv_masterearly-skips via oneREAD_ONCE.
+ attach gatesats_enabledon the flag; a concurrent quarantine race can be closed by a short post-attach re-check incommit()
+ No master pointer in invs; no lifetime concern.
Quoted 2 lines
- invs walker pays
streams_lock+rb_find(SID)per ATS entry on every invalidation. Measurable on ATS-heavy workloads.
Doesn't consider how to enable
Quoted 4 lines
(3) Per-master flag +
inv->masterpointer (v4)
invs entry carries a master pointer; the invs walker readscur->master->ats_brokendirectly.
Quoted 8 lines
+ invs walker is one
READ_ONCEthrough a cached pointer.
+disable_atsis oneWRITE_ONCE.
+atc_inv_masterearly-skip via oneREAD_ONCE.
+ attach gate + post-attach re-check, same as (2).- invs holds a master ptr, so
release_devicemustsynchronize_rcu()before freeing the master to drain walkers underrcu_read_lock(). We dropped this from v4 for that reason.
synchronize_rcu is not right because you have to have gone through the rwlock so there can be no readers.
Jason
Re: [PATCH v4 18/24] iommu/arm-smmu-v3: Introduce master->ats_broken flag
Thanks for the reply.
This is indeed a very complex and sophisticated topic..
Quoted 11 lines · Jason Gunthorpe
I don't see any of these options as appealing. We have to maintain a few key invariants, and I think it cannot be done without a way to find all the domains that are using the STE.
One way or another you have to be using the invs list rw locks to synchronize the EATS state changes.
It is okayish to be sloppy when turning EATS off, but when turning it back on we do need to cycle through every invs list and toggle its lock to ensure that the invalidations are synchronized before EATS=enable happens.
I think the core guarantees that "cycle through every invs list"
happens: a PCI reset calls reset_prepare() blocking all the RID and PASID domains and removing ATS entries from every invs list, and then calls reset_done() that re-attach RID/PASID domains so freshly new ATS entries will be installed before EATS=enable.
So, I think the enable path is not an issue, though the disable path or the invalidation path would need "a way to find all the domains that are using the STE".
Quoted 11 lines
Given you must have a way to go from STE -> master -> all invs lists I'm not sure either option really makes such a large difference.
If so then adjusting the invs to disable the ATS is pretty simple, run over the xarray and set them all off. Yes you could find the master through a SID lookup with some locking adjustment.
Quoted 5 lines
(1) Per-invs marker:
INV_TYPE_ATS_BROKEN+master_domainsdisable_ats()in the timeout path walksmaster->master_domainsand flips matching ATS invs entries to the BROKEN type.
Quoted 8 lines
Quoted 6 lines
+ invs walker is free (one case label in the existing type switch).
+ No lock or pointer deref in the invs walker.
+ No master pointer stored in invs; no lifetime concern.-
disable_ats()walks every (master, domain) and marks each invs set; the list needs locking usable from atomic.This doesn't seem so bad
Yea, the only thing is that the disable path has to deal with a complexity from going through a per-device domain list. Maybe it can reuse iommu_group->pasid_array by taking xa_lock?
Quoted 11 lines
Quoted 11 lines
(2) Per-master flag +
streams_lockinvs walker resolves SID -> master via streams_lock and reads master->ats_broken.+ Single source of truth on the master.
+disable_ats()is oneWRITE_ONCE.
+atc_inv_masterearly-skips via oneREAD_ONCE.
+ attach gatesats_enabledon the flag; a concurrent quarantine race can be closed by a short post-attach re-check incommit()
+ No master pointer in invs; no lifetime concern.
Quoted 4 lines
Quoted 2 lines
- invs walker pays
streams_lock+rb_find(SID)per ATS entry on every invalidation. Measurable on ATS-heavy workloads.Doesn't consider how to enable
The enable side is core-driven: when reset_done() re-attaches the device from blocked_domain back to its RID/PASID domains, the new attach_dev callback (old_domain == blocked_domain) can clear the per-master flag. If the device is still broken, then arm_smmu_atc_inv_master() at the end of attach_commit() times out and re-triggers quarantine.
The flaw lives in the invalidation path as it must translate every SID to master using streams_lock + rb_find(SID) per ATS entry, which make it very less attractive.
Quoted 9 lines
Quoted 9 lines
(3) Per-master flag +
inv->masterpointer (v4)
invs entry carries a master pointer; the invs walker readscur->master->ats_brokendirectly.+ invs walker is one
READ_ONCEthrough a cached pointer.
+disable_atsis oneWRITE_ONCE.
+atc_inv_masterearly-skip via oneREAD_ONCE.
+ attach gate + post-attach re-check, same as (2).
Quoted 6 lines
Quoted 3 lines
- invs holds a master ptr, so
release_devicemustsynchronize_rcu()before freeing the master to drain walkers underrcu_read_lock(). We dropped this from v4 for that reason.
synchronize_rcuis not right because you have to have gone through the rwlock so there can be no readers.
Ah, I think you are right! When release_device() is invoked, the device must be already in the release (blocked) domain. So there should be no domain->invs in the system holding its ATS entries. And the enable part would work as (2).
In this case, (3) seems the best? It's fast on every aspect.
And I think it would fit we plan to generalize the invs design:
struct inv {
struct arm_smmu_device *smmu; // => struct iommu_device *iommu;
struct arm_smmu_master *master; // => void *priv;
// (dev->iommu->priv)Thanks
Nicolin
Re: [PATCH v4 18/24] iommu/arm-smmu-v3: Introduce master->ats_broken flag
Quoted 13 lines · Nicolin Chen
Quoted 11 lines · Jason Gunthorpe
I don't see any of these options as appealing. We have to maintain a few key invariants, and I think it cannot be done without a way to find all the domains that are using the STE.
One way or another you have to be using the invs list rw locks to synchronize the EATS state changes.
It is okayish to be sloppy when turning EATS off, but when turning it back on we do need to cycle through every invs list and toggle its lock to ensure that the invalidations are synchronized before EATS=enable happens.
Quoted 5 lines
I think the core guarantees that "cycle through every invs list"
happens: a PCI reset callsreset_prepare()blocking all the RID and PASID domains and removing ATS entries from every invs list, and then callsreset_done()that re-attach RID/PASID domains so freshly new ATS entries will be installed before EATS=enable.
I think this whole thing is so async and racy this is not something we can truely rely on. The driver is going to have to make sure it doesn't get turned on accidentally while the CD is still populated.
Quoted 15 lines
Quoted 15 lines
Given you must have a way to go from STE -> master -> all invs lists I'm not sure either option really makes such a large difference.
If so then adjusting the invs to disable the ATS is pretty simple, run over the xarray and set them all off. Yes you could find the master through a SID lookup with some locking adjustment.
Quoted 9 lines
(1) Per-invs marker:
INV_TYPE_ATS_BROKEN+master_domainsdisable_ats()in the timeout path walksmaster->master_domainsand flips matching ATS invs entries to the BROKEN type.+ invs walker is free (one case label in the existing type switch).
+ No lock or pointer deref in the invs walker.
+ No master pointer stored in invs; no lifetime concern.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
-
disable_ats()walks every (master, domain) and marks each invs set; the list needs locking usable from atomic.This doesn't seem so bad
Yea, the only thing is that the disable path has to deal with a complexity from going through a per-device domain list. Maybe it can reuse
iommu_group->pasid_arrayby takingxa_lock?
Maybe the locking seems tricky as the locks might end up nesting in weird ways.
The streams rb tree and existing master domains linked list seems appealing if the locking can nest acceptably.
Quoted 16 lines
Quoted 9 lines
Quoted 9 lines
(3) Per-master flag +
inv->masterpointer (v4)
invs entry carries a master pointer; the invs walker readscur->master->ats_brokendirectly.+ invs walker is one
READ_ONCEthrough a cached pointer.
+disable_atsis oneWRITE_ONCE.
+atc_inv_masterearly-skip via oneREAD_ONCE.
+ attach gate + post-attach re-check, same as (2).Quoted 6 lines
Quoted 3 lines
- invs holds a master ptr, so
release_devicemustsynchronize_rcu()before freeing the master to drain walkers underrcu_read_lock(). We dropped this from v4 for that reason.
synchronize_rcuis not right because you have to have gone through the rwlock so there can be no readers.
Quoted 6 lines
Ah, I think you are right! When
release_device()is invoked, the device must be already in the release (blocked) domain. So there should be nodomain->invsin the system holding its ATS entries. And the enable part would work as (2).In this case, (3) seems the best? It's fast on every aspect.
I don't like it mainly because of the sketch enable side, and if we tighten that then you can just do 1 which doesn't have a perf impact..
But still, I'm not sure how all the asyncess and races will resolve in any of these cases.
Jason
Re: [PATCH v4 18/24] iommu/arm-smmu-v3: Introduce master->ats_broken flag
Quoted 20 lines · Jason Gunthorpe
Quoted 13 lines · Nicolin Chen
Quoted 11 lines · Jason Gunthorpe
I don't see any of these options as appealing. We have to maintain a few key invariants, and I think it cannot be done without a way to find all the domains that are using the STE.
One way or another you have to be using the invs list rw locks to synchronize the EATS state changes.
It is okayish to be sloppy when turning EATS off, but when turning it back on we do need to cycle through every invs list and toggle its lock to ensure that the invalidations are synchronized before EATS=enable happens.
Quoted 5 lines
I think the core guarantees that "cycle through every invs list"
happens: a PCI reset callsreset_prepare()blocking all the RID and PASID domains and removing ATS entries from every invs list, and then callsreset_done()that re-attach RID/PASID domains so freshly new ATS entries will be installed before EATS=enable.
Quoted 3 lines
I think this whole thing is so async and racy this is not something we can truely rely on. The driver is going to have to make sure it doesn't get turned on accidentally while the CD is still populated.
You mean the driver needs to ensure that the ATS entires in the invs list would not be skipped when STE.EATS is enabled, right?
Otherwise, INV_TYPE_ATS_BROKEN + per-master domain list cannot prevent a concurrent attach() from turning on STE.EATS either.
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Given you must have a way to go from STE -> master -> all invs lists I'm not sure either option really makes such a large difference.
If so then adjusting the invs to disable the ATS is pretty simple, run over the xarray and set them all off. Yes you could find the master through a SID lookup with some locking adjustment.
Quoted 11 lines
(1) Per-invs marker:
INV_TYPE_ATS_BROKEN+master_domainsdisable_ats()in the timeout path walksmaster->master_domainsand flips matching ATS invs entries to the BROKEN type.+ invs walker is free (one case label in the existing type switch).
+ No lock or pointer deref in the invs walker.
+ No master pointer stored in invs; no lifetime concern.-
disable_ats()walks every (master, domain) and marks each invs set; the list needs locking usable from atomic.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
This doesn't seem so bad
Yea, the only thing is that the disable path has to deal with a complexity from going through a per-device domain list. Maybe it can reuse
iommu_group->pasid_arrayby takingxa_lock?Maybe the locking seems tricky as the locks might end up nesting in weird ways.
Oh, that's thoughtful.
Quoted 2 lines
The streams rb tree and existing master domains linked list seems appealing if the locking can nest acceptably.
Yea, limiting the locking at the driver-level is more manageable.
Quoted 21 lines
Quoted 21 lines
Quoted 9 lines
Quoted 9 lines
(3) Per-master flag +
inv->masterpointer (v4)
invs entry carries a master pointer; the invs walker readscur->master->ats_brokendirectly.+ invs walker is one
READ_ONCEthrough a cached pointer.
+disable_atsis oneWRITE_ONCE.
+atc_inv_masterearly-skip via oneREAD_ONCE.
+ attach gate + post-attach re-check, same as (2).Quoted 6 lines
Quoted 3 lines
- invs holds a master ptr, so
release_devicemustsynchronize_rcu()before freeing the master to drain walkers underrcu_read_lock(). We dropped this from v4 for that reason.
synchronize_rcuis not right because you have to have gone through the rwlock so there can be no readers.Ah, I think you are right! When
release_device()is invoked, the device must be already in the release (blocked) domain. So there should be nodomain->invsin the system holding its ATS entries. And the enable part would work as (2).
Quoted 4 lines
Quoted 1 line
In this case, (3) seems the best? It's fast on every aspect.
I don't like it mainly because of the sketch enable side, and if we tighten that then you can just do 1 which doesn't have a perf impact..
I agree that per-master flag alone would be racy. So, for it to work, it would need an extra spinlock. This v4 actually added a pairing ats_broken_lock to fence arm_smmu_write_entry() in the attach path, while the quarantine path is disabling STE.EATS and setting the per-master flag. With that, the driver can ensure:
- no race in STE.EATS update
- consistency between STE.EATS and ATS entries in every invs
Quoted 2 lines
But still, I'm not sure how all the asyncess and races will resolve in any of these cases.
Maybe we can try "INV_TYPE_ATS_BROKEN + per-master domain list" first and see if Sashiko might identify some corner case.
Thanks
Nicolin
PATCH v4 19/24 iommu/arm-smmu-v3: Add invs and 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 carry both the per-domain invs and a per-batch indicator of whether the batch contains an ATC_INV.
Add an "invs" pointer to record the per-domain invalidation array (set via a new arm_smmu_cmdq_batch_init_cmd() parameter), and a "has_ats" flag set in arm_smmu_cmdq_batch_add_cmd_p() when an ATC_INV command is queued. Any caller that does not associate a batch with an invs array can pass NULL.
No functional changes.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 4 ++++ drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 17 +++++++++++------ 2 files changed, 15 insertions(+), 6 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +4 -0
@@ -721,6 +721,10 @@ 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; +/* Per-domain invalidation array, for sub-batch retry-on-EIO lookup */ +struct arm_smmu_invs *invs; +/* 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 +11 -6
@@ -892,10 +892,13 @@ static int arm_smmu_cmdq_issue_cmd_p(struct arm_smmu_device *smmu,static void arm_smmu_cmdq_batch_init_cmd (struct arm_smmu_device *smmu,struct arm_smmu_cmdq_batch *cmds, -struct arm_smmu_cmd *cmd) +struct arm_smmu_cmd *cmd, +struct arm_smmu_invs *invs) { cmds->num =0 ; cmds->cmdq =arm_smmu_get_cmdq (smmu, cmd); + cmds->invs = invs; + cmds->has_ats = false; } static void arm_smmu_cmdq_batch_add_cmd_p(struct arm_smmu_device *smmu,
@@ -910,15 +913,17 @@ static void arm_smmu_cmdq_batch_add_cmd_p(struct arm_smmu_device *smmu,if (force_sync || unsupported_cmd) {arm_smmu_cmdq_issue_cmdlist (smmu, cmds->cmdq, cmds->cmds, cmds->num, true); -arm_smmu_cmdq_batch_init_cmd (smmu, cmds, cmd); +arm_smmu_cmdq_batch_init_cmd (smmu, cmds, cmd, cmds->invs); }if (cmds->num ==CMDQ_BATCH_ENTRIES ) {arm_smmu_cmdq_issue_cmdlist (smmu, cmds->cmdq, cmds->cmds, cmds->num, false); -arm_smmu_cmdq_batch_init_cmd (smmu, cmds, cmd); +arm_smmu_cmdq_batch_init_cmd (smmu, cmds, cmd, cmds->invs); } +if (FIELD_GET (CMDQ_0_OP , cmd->data[0 ]) ==CMDQ_OP_ATC_INV ) + cmds->has_ats = true; cmds->cmds[cmds->num++] = *cmd; }
@@ -1486,7 +1491,7 @@ static void arm_smmu_sync_cd(struct arm_smmu_master *master,struct arm_smmu_device *smmu = master->smmu;struct arm_smmu_cmd cmd =arm_smmu_make_cmd_cfgi_cd (0 , ssid, leaf); -arm_smmu_cmdq_batch_init_cmd (smmu, &cmds, &cmd); +arm_smmu_cmdq_batch_init_cmd (smmu, &cmds, &cmd,NULL );for (i =0 ; i < master->num_streams; i++) arm_smmu_cmdq_batch_add_cmd( smmu, &cmds,
@@ -2434,7 +2439,7 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master,return 0 ; cmd =arm_smmu_make_cmd_atc_inv_all (0 ,IOMMU_NO_PASID ); -arm_smmu_cmdq_batch_init_cmd (master->smmu, &cmds, &cmd); +arm_smmu_cmdq_batch_init_cmd (master->smmu, &cmds, &cmd,NULL );for (i =0 ; i < master->num_streams; i++) arm_smmu_cmdq_batch_add_cmd( master->smmu, &cmds,
@@ -2630,7 +2635,7 @@ static void __arm_smmu_domain_inv_range(struct arm_smmu_invs *invs,struct arm_smmu_inv *next;if (!cmds.num) -arm_smmu_cmdq_batch_init_cmd (smmu, &cmds, &cmd); +arm_smmu_cmdq_batch_init_cmd (smmu, &cmds, &cmd, invs);switch (cur->type) {case INV_TYPE_S1_ASID :
Signature · 1 line
2.43.0
Re: [PATCH v4 19/24] iommu/arm-smmu-v3: Add invs and has_ats to struct arm_smmu_cmdq_batch
Quoted 5 lines · Nicolin Chen
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 carry both the per-domain invs and a per-batch indicator of whether the batch contains anATC_INV.
This seems like too much to tackle in one series. Let's just assume all the ATCs in the batch have failed and blow up everything for this point.
I think retrying will be a lot easier by removing the batch..
This series has become way too big now anyhow..
Jason
PATCH v4 20/24 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
@@ -901,6 +901,14 @@ static void arm_smmu_cmdq_batch_init_cmd(struct arm_smmu_device *smmu, cmds->has_ats = false; } +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 void arm_smmu_cmdq_batch_add_cmd_p (struct arm_smmu_device *smmu,struct arm_smmu_cmdq_batch *cmds,struct arm_smmu_cmd *cmd)
@@ -911,14 +919,12 @@ static void arm_smmu_cmdq_batch_add_cmd_p(struct arm_smmu_device *smmu, unsupported_cmd = !arm_smmu_cmdq_supports_cmd (cmds->cmdq, cmd);if (force_sync || unsupported_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, cmds->invs); }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, cmds->invs); }
@@ -936,8 +942,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
PATCH v4 21/24 iommu/arm-smmu-v3: Move arm_smmu_invs_for_each_entry to header
In reply to Nicolin ChenA subsequent change will use this helper in a lockless context. Since this this is a macro, move it to the header file so other functions can use it.
Also, add READ_ONCE to invs->num_invs for lockless use.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 28 +++++++++++++++++++++ drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 28 --------------------- 2 files changed, 28 insertions(+), 28 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +28 -0
@@ -803,6 +803,34 @@ struct arm_smmu_invs { struct arm_smmu_inv inv[] __counted_by(max_invs); }; +/* Invalidation array manipulation functions */ +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 >=READ_ONCE (invs->num_invs)) { + *idx = next; +return NULL ; + } +if (!READ_ONCE (invs->inv[next].users)) { + next++; +continue ; + } + *idx = next; +return &invs->inv[next]; + } +} + +/** +* arm_smmu_invs_for_each_entry - Iterate over all non-trash entries in invs +* @invs: the base invalidation array +* @idx: a stack variable of 'size_t', to store the array index +* @cur: a stack variable of 'struct arm_smmu_inv *' +*/ +#define arm_smmu_invs_for_each_entry (invs, idx, cur) \ + for (cur = arm_smmu_invs_iter_next(invs, 0, &(idx)); cur; \ + cur = arm_smmu_invs_iter_next(invs, idx + 1, &(idx))) +static inline struct arm_smmu_invs *arm_smmu_invs_alloc (size_t num_invs) {struct arm_smmu_invs *new_invs;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -28
@@ -979,34 +979,6 @@ static void arm_smmu_page_response(struct device *dev, struct iopf_fault *unused*/ } -/* Invalidation array manipulation functions */ -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) { - *idx = next; -return NULL ; - } -if (!READ_ONCE (invs->inv[next].users)) { - next++; -continue ; - } - *idx = next; -return &invs->inv[next]; - } -} - -/** -* arm_smmu_invs_for_each_entry - Iterate over all non-trash entries in invs -* @invs: the base invalidation array -* @idx: a stack variable of 'size_t', to store the array index -* @cur: a stack variable of 'struct arm_smmu_inv *' -*/ -#define arm_smmu_invs_for_each_entry (invs, idx, cur) \ - for (cur = arm_smmu_invs_iter_next(invs, 0, &(idx)); cur; \ - cur = arm_smmu_invs_iter_next(invs, idx + 1, &(idx))) -static int arm_smmu_inv_cmp (const struct arm_smmu_inv *inv_l,const struct arm_smmu_inv *inv_r) {
Signature · 1 line
2.43.0
PATCH v4 22/24 iommu/arm-smmu-v3: Introduce master->ats_invs
In reply to Nicolin ChenSimilar to master->build_invs used by a per-domain invalidation, add a new master->ats_invs to be used by arm_smmu_atc_inv_master().
Since arm_smmu_cmdq_batch_init_cmd() now takes an invs pointer, pass it in.
This will be useful by arm_smmu_cmdq_batch_issue() to backtrack the master pointer from a timed out ATC invalidation command in a subsequent change.
Also replace the streams loop with arm_smmu_invs_for_each_entry() as it is initialized (except ssid) upon allocation.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 46 ++++++++++++++++++--- 2 files changed, 43 insertions(+), 5 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +2 -0
@@ -1045,6 +1045,8 @@ struct arm_smmu_master {* iommu_group mutex. */ struct arm_smmu_invs *build_invs; +/* Scratch memory for arm_smmu_atc_inv_master() to build an ATS array */ +struct arm_smmu_invs *ats_invs;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;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +41 -5
97 lines not on this page · raw message
Signature · 1 line
2.43.0
Re: [PATCH v4 22/24] iommu/arm-smmu-v3: Introduce master->ats_invs
Quoted 7 lines · Nicolin Chen
Similar to
master->build_invsused by a per-domain invalidation, add a newmaster->ats_invsto be used byarm_smmu_atc_inv_master().Since
arm_smmu_cmdq_batch_init_cmd()now takes an invs pointer, pass it in.This will be useful by
arm_smmu_cmdq_batch_issue()to backtrack the master pointer from a timed out ATC invalidation command in a subsequent change.
Again this is a good place to just use the SID and get back to the master through the rbtree under a spinlock.
Jason
PATCH v4 23/24 iommu/arm-smmu-v3: Serialize STE.EATS and ats_broken updates
In reply to Nicolin ChenA subsequent change adding an ATS-broken path will set master->ats_broken flag and overwrite STE.EATS to 0 for a broken master. This will introduce race conditions:
- A concurrent attachment that read ats_broken=false prior to ats_broken being set to true could re-enable STE.EATS.
- A concurrent reset_device_done callback could clear master->ats_broken during the ATS-broken path, leading to iommu_report_device_broken() on a pre-reset fault.
- When the ATS-broken path reads the old_data[1] and writes (old_data[1] & ~EATS), a concurrent attachment could write a new_data[1] in-between.
Due to an ATS-broken path can run in an atomic context (invalidation), it cannot use mutex.
Introduce a per-master spinlock_t ats_broken_lock to fence these cases so as to guarantee that in concurrent cases:
a) A master->ats_broken update is observed by every concurrent attach
b) STE.EATS is never re-enabled while master->ats_broken is true
c) data[1] writes are not lost to a concurrent ATS-broken path
Note IRQ has to be disabled while holding the lock, because an ATS-broken path can be entered via a hardirq that might interrupt another caller and lead to deadlock.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 5 +++++ drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 22 ++++++++++++++++++++- 2 files changed, 26 insertions(+), 1 deletion(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +5 -0
@@ -1048,6 +1048,11 @@ struct arm_smmu_master {/* Scratch memory for arm_smmu_atc_inv_master() to build an ATS array */ struct arm_smmu_invs *ats_invs;struct arm_smmu_vmaster *vmaster;/* use smmu->streams_mutex */ +/* +* Serializes arm_smmu_write_ste(), reset_device_done, and an ATS-broken +* path, preventing races on ats_broken flag and STE updates. +*/ +spinlock_t ats_broken_lock;/* Locked by the iommu core using the group mutex */ struct arm_smmu_ctx_desc_cfg cd_table;unsigned int num_streams;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +21 -1
@@ -1791,7 +1791,23 @@ static void arm_smmu_write_ste(struct arm_smmu_master *master, u32 sid, .sid = sid, }; -arm_smmu_write_entry (&ste_writer.writer, ste->data, target->data); +/* +* Fence against the ATS-broken path concurrently overwriting STE.EATS. +* It's fine if the ATS-broken path writes after arm_smmu_write_entry. +* Otherwise, we must clear STE.EATS before sending a CFGI_STE command. +* +* Must disable IRQs; otherwise a hardirq-context invalidation path on +* this CPU could deadlock at ats_broken_lock on an ATC_INV timeout. +*/ + scoped_guard(spinlock_irqsave, &master->ats_broken_lock) { +struct arm_smmu_ste local_target = *target; + +if (master->ats_broken) + local_target.data[1 ] &= + ~cpu_to_le64 (STRTAB_STE_1_EATS ); +arm_smmu_write_entry (&ste_writer.writer, ste->data, + local_target.data); + }/* It's likely that we'll want to use the new STE soon */ if (!(smmu->options &ARM_SMMU_OPT_SKIP_PREFETCH ))
@@ -3000,6 +3016,9 @@ static void arm_smmu_reset_device_done(struct device *dev)if (WARN_ON (!master))return ; + +/* Ensure the device recovery is seen, to flush any pre-reset fault */ +guard (spinlock_irqsave)(&master->ats_broken_lock);/* Pair with lockless readers */ WRITE_ONCE (master->ats_broken, false); }
@@ -4236,6 +4255,7 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev) master->dev = dev; master->smmu = smmu; +spin_lock_init (&master->ats_broken_lock);dev_iommu_priv_set (dev, master); ret =arm_smmu_insert_master (smmu, master);
Signature · 1 line
2.43.0
PATCH v4 24/24 iommu/arm-smmu-v3: Block ATS upon an ATC invalidation timeout
In reply to Nicolin ChenCurrently, when GERROR_CMDQ_ERR occurs, the arm_smmu_cmdq_skip_err() won't do anything for the CMDQ_ERR_CERROR_ATC_INV_IDX.
When a device wasn't responsive to an ATC invalidation request, this often results 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
...
An ATC invalidation timeout indicates that the device failed to respond to a protocol-critical coherency request, which means that device's internal ATS state is desynchronized from the SMMU.
Furthermore, ignoring the timeout leaves the system in an unsafe state, as the device cache may retain stale ATC entries for memory pages that the OS has already reclaimed and reassigned. This might lead to data corruption.
Isolate the device that is confirmed to be unresponsive by a surgical STE update to unset its EATS bit so as to reject any further ATS transaction, which could corrupt the memory.
Also, set the master->ats_broken flag that is revertible after the device completes a reset. This flag avoids further ATS requests and invalidations from happening.
Finally, report this broken device to the IOMMU core to isolate the device in the core level too.
Since the three steps above are invoked in an invalidation path (which can be an atomic context), hold the ats_broken_lock instead of any mutex.
For batched ATC_INV commands, SMMU hardware only reports a timeout at the CMD_SYNC, which could follow the batch issued for multiple devices. So, it isn't straightforward to identify which command in a batch resulted in the timeout. Fortunately, the invs array has a sorted list of ATC entries. So, the issued batch must be sorted as well. This makes it possible to retry the ATC_INV command for each unique Stream ID in the batch to identify the unresponsive master.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 18 +++ drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 118 +++++++++++++++++++- 2 files changed, 133 insertions(+), 3 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +18 -0
@@ -831,6 +831,24 @@ arm_smmu_invs_iter_next(struct arm_smmu_invs *invs, size_t next, size_t *idx)for (cur =arm_smmu_invs_iter_next (invs,0 , &(idx)); cur; \ cur =arm_smmu_invs_iter_next (invs, idx +1 , &(idx))) +static inline struct arm_smmu_master * +arm_smmu_invs_find_ats_master (struct arm_smmu_invs *invs, +struct arm_smmu_device *smmu,u32 sid) +{ +struct arm_smmu_inv *cur; +size_t i; + +if (!invs->has_ats) +return NULL ; + +arm_smmu_invs_for_each_entry (invs, i, cur) { +if (cur->smmu == smmu &&arm_smmu_inv_is_ats (cur) && + cur->id == sid) +return cur->master; + } +return NULL ; +} +static inline struct arm_smmu_invs *arm_smmu_invs_alloc (size_t num_invs) {struct arm_smmu_invs *new_invs;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +115 -3
144 lines not on this page · raw message
Signature · 1 line
2.43.0
Quoted 20 lines
From: Nicolin Chen <[email protected]>
Sent: Tuesday, May 19, 2026 11:39 AMChangelog
v4:
* Rebase on Joerg's IOMMU "fixes" branch
* Rebase on Jason's SMMUv3cmd_entseries
https://lore.kernel.org/all/0-v2-47b2bf710ad5+716ac-
[email protected]/
* [PCI] Don't suspend IOMMU in probe mode
* [iommu]kfree_rcu()iommu_group
* [iommu] Convertgdev->blockedto enumgdev_blocked
* [iommu] Usedisable_work_sync()to fix UAF and ref leak
* [iommu] Gatedone()transitions to preserveBLOCKED_BROKEN
* [iommu] Decrementrecovery_cntwhen unplugging a blocked gdev
* [iommu] Drop racydev_has_iommu()iniommu_report_device_broken()
* [iommu] Addgdev->broken_pendingto skip worker after racing recovery
* [smmuv3] Addmaster->ats_invsscratch
* [smmuv3] Addarm_smmu_cmdq_batch_issue()wrapper
* [smmuv3] Force per-flush sync forhas_atsbatches
Quoted 8 lines
* [smmuv3] Serialize STE.EATS and
ats_brokenupdates
* [smmuv3] Co-clear pendingCMDQ_ERRfrom cmdq issuer
* [smmuv3] Add invs andhas_atstoarm_smmu_cmdq_batch
* [smmuv3] Movearm_smmu_invs_for_each_entryto header
* [smmuv3] Setmaster->ats_brokenafter clearing STE.EATS
* [smmuv3] IssueCFGI_STEviaarm_smmu_cmdq_issue_cmd_with_sync()
* [smmuv3] Keep "smmu" pointer inarm_smmu_invbut add "master" for
ATS
Not check the detail yet, but this v4 more than doubles the number of patches in v3. Are they all necessary to be in one series? any chance to split to ease the review...
Quoted 29 lines · Kevin
Quoted 20 lines
From: Nicolin Chen <[email protected]>
Sent: Tuesday, May 19, 2026 11:39 AMChangelog
v4:
* Rebase on Joerg's IOMMU "fixes" branch
* Rebase on Jason's SMMUv3cmd_entseries
https://lore.kernel.org/all/0-v2-47b2bf710ad5+716ac-
[email protected]/
* [PCI] Don't suspend IOMMU in probe mode
* [iommu]kfree_rcu()iommu_group
* [iommu] Convertgdev->blockedto enumgdev_blocked
* [iommu] Usedisable_work_sync()to fix UAF and ref leak
* [iommu] Gatedone()transitions to preserveBLOCKED_BROKEN
* [iommu] Decrementrecovery_cntwhen unplugging a blocked gdev
* [iommu] Drop racydev_has_iommu()iniommu_report_device_broken()
* [iommu] Addgdev->broken_pendingto skip worker after racing recovery
* [smmuv3] Addmaster->ats_invsscratch
* [smmuv3] Addarm_smmu_cmdq_batch_issue()wrapper
* [smmuv3] Force per-flush sync forhas_atsbatchesQuoted 8 lines
* [smmuv3] Serialize STE.EATS and
ats_brokenupdates
* [smmuv3] Co-clear pendingCMDQ_ERRfrom cmdq issuer
* [smmuv3] Add invs andhas_atstoarm_smmu_cmdq_batch
* [smmuv3] Movearm_smmu_invs_for_each_entryto header
* [smmuv3] Setmaster->ats_brokenafter clearing STE.EATS
* [smmuv3] IssueCFGI_STEviaarm_smmu_cmdq_issue_cmd_with_sync()
* [smmuv3] Keep "smmu" pointer inarm_smmu_invbut add "master" for
ATS
Quoted 3 lines
Not check the detail yet, but this v4 more than doubles the number of patches in v3. Are they all necessary to be in one series? any chance to split to ease the review...
Ah, sorry. Mostly these are dealing with racing and corner cases in the async design...
Jason has suggested to drop quite a few things. So, I will respin a v5 that should be much smaller. Let's ignore this v4 for now.
Thanks
Nicolin