iommu/arm-smmu-v3: Fix device crash on kdump kernel
Subsystems (3)
v5 cover iommu/arm-smmu-v3: Fix device crash on kdump kernel
When transitioning to a kdump kernel, the primary kernel might have crashed while endpoint devices were actively bus-mastering DMA. Currently, the SMMU driver aggressively resets the hardware during probe by clearing CR0_SMMUEN and setting the Global Bypass Attribute (GBPA) to ABORT.
In a kdump scenario, this aggressive reset is highly destructive:
a) If GBPA is set to ABORT, in-flight DMA will be aborted, generating fatal PCIe AER or SErrors that may panic the kdump kernel
b) If GBPA is set to BYPASS, in-flight DMA targeting some IOVAs will bypass the SMMU and corrupt the physical memory at those 1:1 mapped IOVAs.
To safely absorb in-flight DMA, the kdump kernel must leave SMMUEN=1 intact and avoid modifying STRTAB_BASE. This allows HW to continue translating in- flight DMA using the crashed kernel's page tables until the endpoint device drivers probe and quiesce their respective hardware.
However, the ARM SMMUv3 architecture specification states that updating the SMMU_STRTAB_BASE register while SMMUEN == 1 is UNPREDICTABLE or ignored.
This leaves a kdump kernel no choice but to adopt the stream table from the crashed kernel.
In this series:
- Introduce an ARM_SMMU_OPT_KDUMP_ADOPT
- Skip SMMUEN and STRTAB_BASE resets in arm_smmu_device_reset()
- Skip EVENTQ and PRIQ setups including interrupts and their handlers
- Memremap the crashed kernel's stream tables into the kdump kernel [*]
- Defer any default domain attachment to retain STEs until device drivers explicitly request it.
[*] For verification reason, this series only fixes coherent SMMUs.
For non-ARM_SMMU_OPT_KDUMP_ADOPT cases, keep a status quo since the commit 3f54c447df34f ("iommu/arm-smmu-v3: Don't disable SMMU in kdump kernel"):
full reset followed by driver-initiated reattach, potentially rejecting any in-flight DMA.
Note that the series requires Jason's work that was merged in v6.12: commit 85196f54743d ("iommu/arm-smmu-v3: Reorganize struct arm_smmu_strtab_cfg"). I have a backported version that is verified with a v6.8 kernel. I can send if we see a strong need after this version is accepted.
This is on Github:
https://github.com/nicolinc/iommufd/commits/smmuv3_kdump-v5
Changelog
v5
* Add Reviewed-by from Kevin
* Drop READ_ONCE on lazy-attach L1 read
* Split "Skip EVTQ/PRIQ setup" into two patches
* Tighten kdump probe comment and dev_warn message
* Use MEM + BUSY in arm_smmu_kdump_phys_is_corrupted
v4
https://lore.kernel.org/all/[email protected]/
* Rebase v7.1-rc1
* s/arm_smmu_adopt/arm_smmu_kdump_adopt
* Revert alloc/memremap/fmt on fallback
* Reorder patches to avoid bisect regression
* Use IRQ_NONE for spurious evtq/priq entries
* Cap linear log2size by kdump's allocation bound
* Defer clearing FEAT_2_LVL_STRTAB on linear adopt
* Add arm_smmu_kdump_phys_is_corrupted() validation
* Defer l2 stream table memremap till master inserts
* Re-validate L1 desc on master insert with READ_ONCE
v3
https://lore.kernel.org/all/[email protected]/
* s/OPT_KDUMP/OPT_KDUMP_ADOPT
* Do not adopt if GERROR_SFM_ERR
* Retain CR0_ATSCHK beside CR0_SMMUEN
* Clear latched GERROR bits (e.g. CMDQ_ERR)
* Assert ARM_SMMU_FEAT_COHERENCY in adopt functions
* Add STE.Cfg check in arm_smmu_is_attach_deferred()
* Fix validations on return codes from devm_memremap()
* Sanitize crashed kernel register values in adopt functions
* Drop unnecessary l2ptrs guard in arm_smmu_is_attach_deferred()
* Don't enable PRIQ/EVTQ irqs and guard the irq functions for combined irq cases
v2
https://lore.kernel.org/all/[email protected]/
* Add warning in non-coherent SMMU cases
* Keep eventq/priq disabled v.s. enabling-and-disabling-later
* Check KDUMP option in the beginning of arm_smmu_device_reset()
* Validate STRTAB format matches HW capability instead of forcing flags
v1:
https://lore.kernel.org/all/[email protected]/
Nicolin Chen (6):
iommu/arm-smmu-v3: Add arm_smmu_kdump_adopt_strtab() for kdump iommu/arm-smmu-v3: Implement is_attach_deferred() for kdump iommu/arm-smmu-v3: Suppress EVTQ/PRIQ events in kdump kernel iommu/arm-smmu-v3: Skip EVTQ/PRIQ setup in kdump kernel iommu/arm-smmu-v3: Retain CR0_SMMUEN during kdump device reset iommu/arm-smmu-v3: Detect ARM_SMMU_OPT_KDUMP_ADOPT in probe()
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 473 +++++++++++++++++++- 2 files changed, 450 insertions(+), 24 deletions(-)
Signature · 1 line
2.43.0
PATCH v5 1/6 iommu/arm-smmu-v3: Add arm_smmu_kdump_adopt_strtab() for kdump
When transitioning to a kdump kernel, the primary kernel might have crashed while endpoint devices were actively bus-mastering DMA. Currently, the SMMU driver aggressively resets the hardware during probe by clearing CR0_SMMUEN and setting the Global Bypass Attribute (GBPA) to ABORT.
In a kdump scenario, this aggressive reset is highly destructive:
a) If GBPA is set to ABORT, in-flight DMA will be aborted, generating fatal PCIe AER or SErrors that may panic the kdump kernel
b) If GBPA is set to BYPASS, in-flight DMA targeting some IOVAs will bypass the SMMU and corrupt the physical memory at those 1:1 mapped IOVAs.
To safely absorb in-flight DMAs, a kdump kernel will have to leave SMMUEN=1 intact and avoid modifying STRTAB_BASE, allowing HW to continue translating in-flight DMAs reusing the crashed kernel's page tables until the endpoint device drivers probe and quiesce their respective hardware.
However, the ARM SMMUv3 architecture specification states that updating the SMMU_STRTAB_BASE register while SMMUEN == 1 is UNPREDICTABLE or ignored.
This leaves a kdump kernel no choice but to adopt the stream table from the crashed kernel.
Introduce ARM_SMMU_OPT_KDUMP_ADOPT and adopt functions memremapping all the stream tables extracted from STRTAB_BASE and STRTAB_BASE_CFG.
Note that the adoption of the crashed kernel's stream table follows certain strict rules, since the old stream table might be compromised. Thus, apply a series of validations against the values read from the registers. If any address or size doesn't pass the test, it means the stream table cannot be trusted, so toss it entirely. To avoid OOM due to a deeply corrupted stream table, the memremap for l2 tables is done on the kdump kernel's demand.
The new option will be set in a following change.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 305 +++++++++++++++++++- 2 files changed, 303 insertions(+), 3 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -0
@@ -861,6 +861,7 @@ struct arm_smmu_device {#define ARM_SMMU_OPT_MSIPOLL (1 << 2)#define ARM_SMMU_OPT_CMDQ_FORCE_SYNC (1 << 3)#define ARM_SMMU_OPT_TEGRA241_CMDQV (1 << 4) +#define ARM_SMMU_OPT_KDUMP_ADOPT (1 << 5)u32 options;struct arm_smmu_cmdq cmdq;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +302 -3
348 lines not on this page · raw message
Signature · 1 line
2.43.0
Re: [PATCH v5 1/6] iommu/arm-smmu-v3: Add arm_smmu_kdump_adopt_strtab() for kdump
Quoted 1 line · Nicolin Chen
+#include <
linux/dma-direct.h>
Nope, never do this, it is an internal header.
Quoted 23 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +23
-0+/* +* Adopting the crashed kernel's stream table has risks: the physical addresses +* read from ARM_SMMU_STRTAB_BASE / L1 descriptors may be corrupted. Reject any +* range that overlaps the kdump kernel's critical regions. +*/ +static bool arm_smmu_kdump_phys_is_corrupted (phys_addr_t base,size_t size) +{ +/* +* On arm64 kdump, iomem_resource entries are typically: +* ------------------------------------------------------------ +* | Entry | IORESOURCE_ Flags | IORES_DESC_ Desc | +* ------------------------------------------------------------ +* | System RAM | MEM + BUSY + SYSRAM | NONE | +* | MMIO regions | MEM + BUSY | NONE | +* | Reserved memory | MEM | NONE | +* ------------------------------------------------------------ +* +* Test and reject any overlap with MEM + BUSY, covering/excluding: +* + System RAM: silent corruption of kdump kernel's own memory +* + MMIO regions: fatal SError on cacheable speculative access +* - Reserved memory: crashed kernel's stream table might reside +*/ + if (region_intersects(base, size, IORESOURCE_MEM | IORESOURCE_BUSY,
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_init_initial_stes(struct arm_smmu_ste *strtab, · line 2076
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +10
-0+ IORES_DESC_NONE) != REGION_DISJOINT) +return true; + +/* +* Note: physical holes are absent from iomem_resource, so a corrupted +* address pointing into one will not be caught here. Closing that gap +* requires a firmware memory map and is left as a future improvement. +*/ +return false; +}
Something like this should not be in the smmu driver, this is some core kdump code. I'd drop it, I don't see other drivers doing this?
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_init_initial_stes(struct arm_smmu_ste *strtab, · line 2082
Quoted 5 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +5
-0+static int arm_smmu_kdump_adopt_l2_strtab (struct arm_smmu_device *smmu,u32 sid, +u32 l1_idx,u64 l2_dma,u32 span, +struct arm_smmu_strtab_l2 **l2table) +{ +phys_addr_t base =dma_to_phys (smmu->dev, l2_dma);
The thing stored in the L2PTR is a *phys*, the HW doesn't support any kind of translation. When using dma_alloc_coherent we never get a phys so it uses the dma_addr_t and assumes it is == phys.
But on this flow this is *phys* and should remain phys. Never touch dma_addr_t.
Quoted 30 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +24
-0+struct arm_smmu_strtab_l2 *table; +size_t size; + +/* +* Only a coherent SMMU is supported at this moment. For a non-coherent +* SMMU that wants to support ARM_SMMU_OPT_KDUMP_ADOPT, try MEMREMAP_WC. +*/ +if (WARN_ON (!(smmu->features &ARM_SMMU_FEAT_COHERENCY ))) +return -EOPNOTSUPP ; + +/* +* Retest the memremap inputs in case the L1 descriptor was overwritten +* since adopt. Reject this master's insert; panic or SMMU-disable would +* either lose the vmcore or cascade aborts. Do not try to fix it, as it +* would break all other SIDs in the same bus (PCI case). The corruption +* blast radius is already bounded to that bus range. +*/ +if (span !=STRTAB_SPLIT +1 ) { +dev_err (smmu->dev, +"kdump: L1[%u] span %u changed since adopt (was %u)\n" , + l1_idx, span,STRTAB_SPLIT +1 ); +return -EINVAL ; + }static int arm_smmu_init_l2_strtab (struct arm_smmu_device *smmu,u32 sid) {dma_addr_t l2ptr_dma;struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;struct arm_smmu_strtab_l2 **l2table; +u32 l1_idx =arm_smmu_strtab_l1_idx (sid);
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_init_initial_stes(struct arm_smmu_ste *strtab, · line 2141
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +5
-1- l2table = &cfg->l2.l2ptrs[arm_smmu_strtab_l1_idx (sid)]; + l2table = &cfg->l2.l2ptrs[l1_idx];if (*l2table)return 0 ; +/* Deferred adoption of the crashed kernel's L2 table */ +if (smmu->options &ARM_SMMU_OPT_KDUMP_ADOPT ) { +u64 l2ptr =le64_to_cpu (cfg->l2.l1tab[l1_idx].l2ptr); +dma_addr_t l2_dma = l2ptr &STRTAB_L1_DESC_L2PTR_MASK ;
Like here, this should by phys_addr_t
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu) · line 4655
Quoted 2 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2
-0+static int arm_smmu_kdump_adopt_strtab_2lvl (struct arm_smmu_device *smmu, +u32 cfg_reg,dma_addr_t dma)
Same issues with dma_addr_t
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu) · line 4747
Quoted 3 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3
-0+static int arm_smmu_kdump_adopt_strtab_linear (struct arm_smmu_device *smmu, +u32 cfg_reg,dma_addr_t dma) +{
Same issues with dma_addr_t
Quoted 4 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4
-0+static void arm_smmu_kdump_adopt_cleanup (struct arm_smmu_device *smmu,u32 fmt) +{ +struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg; +
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu) · line 4809
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +10
-0+if (fmt ==STRTAB_BASE_CFG_FMT_2LVL ) { +if (cfg->l2.l2ptrs) +devm_kfree (smmu->dev, cfg->l2.l2ptrs); +if (!IS_ERR_OR_NULL (cfg->l2.l1tab)) +devm_memunmap (smmu->dev, cfg->l2.l1tab); + }else if (fmt ==STRTAB_BASE_CFG_FMT_LINEAR ) { +if (!IS_ERR_OR_NULL (cfg->linear.table)) +devm_memunmap (smmu->dev, cfg->linear.table); + } +}
If we have a cleanup function why is it using devm? Call the cleanup function during remove too?
Jason
Re: [PATCH v5 1/6] iommu/arm-smmu-v3: Add arm_smmu_kdump_adopt_strtab() for kdump
Quoted 5 lines · Jason Gunthorpe
Quoted 1 line · Nicolin Chen
+#include <
linux/dma-direct.h>Nope, never do this, it is an internal header.
Hmm, I have included it for a wrong reason, yet it does mention "IOMMU drivers".
/*
* Internals of the DMA direct mapping implementation. Only for use by the
* DMA mapping code and IOMMU drivers.
*/
Quoted 6 lines
Quoted 6 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +6
-0+/* +* Adopting the crashed kernel's stream table has risks: the physical addresses +* read from ARM_SMMU_STRTAB_BASE / L1 descriptors may be corrupted. Reject any +* range that overlaps the kdump kernel's critical regions. +*/ +static bool arm_smmu_kdump_phys_is_corrupted (phys_addr_t base,size_t size)
[..]
Quoted 2 lines
Something like this should not be in the smmu driver, this is some core kdump code. I'd drop it, I don't see other drivers doing this?
OK.
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_init_initial_stes(struct arm_smmu_ste *strtab, · line 2082
Quoted 5 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +5
-0+static int arm_smmu_kdump_adopt_l2_strtab (struct arm_smmu_device *smmu,u32 sid, +u32 l1_idx,u64 l2_dma,u32 span, +struct arm_smmu_strtab_l2 **l2table) +{ +phys_addr_t base =dma_to_phys (smmu->dev, l2_dma);The thing stored in the L2PTR is a *phys*, the HW doesn't support any kind of translation. When using
dma_alloc_coherentwe never get a phys so it uses thedma_addr_tand assumes it is == phys.
Quoted 2 lines
But on this flow this is *phys* and should remain phys. Never touch
dma_addr_t.
Fixing that and other places too.
Quoted 9 lines
Quoted 9 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +9
-0+static void arm_smmu_kdump_adopt_cleanup (struct arm_smmu_device *smmu,u32 fmt) +{ +struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg; + +if (fmt ==STRTAB_BASE_CFG_FMT_2LVL ) { +if (cfg->l2.l2ptrs) +devm_kfree (smmu->dev, cfg->l2.l2ptrs); +if (!IS_ERR_OR_NULL (cfg->l2.l1tab)) +devm_memunmap (smmu->dev, cfg->l2.l1tab);
Quoted 8 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu) · line 4809
Quoted 5 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +5
-0+ }else if (fmt ==STRTAB_BASE_CFG_FMT_LINEAR ) { +if (!IS_ERR_OR_NULL (cfg->linear.table)) +devm_memunmap (smmu->dev, cfg->linear.table); + } +}If we have a cleanup function why is it using devm? Call the cleanup function during remove too?
Dropping "devm_"s.
Thanks
Nicolin
PATCH v5 2/6 iommu/arm-smmu-v3: Implement is_attach_deferred() for kdump
In reply to Nicolin ChenThough the kdump kernel adopts the crashed kernel's stream table, the iommu core will still try to attach each probed device to a default domain, which overwrites the adopted STE and breaks in-flight DMA from that device.
Implement an is_attach_deferred() callback to prevent this. For each device that has STE.V=1 and STE.Cfg!=Abort in the adopted table, defer the default domain attachment, until the device driver explicitly requests it.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 24 +++++++++++++++++++++ 1 file changed, 24 insertions(+)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +24 -0
@@ -4310,6 +4310,29 @@ static void arm_smmu_remove_master(struct arm_smmu_master *master)kfree (master->build_invs); } +static bool arm_smmu_is_attach_deferred (struct device *dev) +{ +struct arm_smmu_master *master =dev_iommu_priv_get (dev); +struct arm_smmu_device *smmu = master->smmu; +int i; + +if (!(smmu->options &ARM_SMMU_OPT_KDUMP_ADOPT )) +return false; + +for (i =0 ; i < master->num_streams; i++) { +struct arm_smmu_ste *ste = +arm_smmu_get_step_for_sid (smmu, master->streams[i].id); +u64 ent0 =le64_to_cpu (ste->data[0 ]); + +/* Defer only when there might be in-flight DMAs */ +if ((ent0 &STRTAB_STE_0_V ) && +FIELD_GET (STRTAB_STE_0_CFG , ent0) !=STRTAB_STE_0_CFG_ABORT ) +return true; + } + +return false; +} +static struct iommu_device *arm_smmu_probe_device (struct device *dev) {int ret;
@@ -4472,6 +4495,7 @@ static const struct iommu_ops arm_smmu_ops = { .hw_info = arm_smmu_hw_info, .domain_alloc_sva = arm_smmu_sva_domain_alloc, .domain_alloc_paging_flags = arm_smmu_domain_alloc_paging_flags, + .is_attach_deferred = arm_smmu_is_attach_deferred, .probe_device = arm_smmu_probe_device, .release_device = arm_smmu_release_device, .device_group = arm_smmu_device_group,
Signature · 1 line
2.43.0
Re: [PATCH v5 2/6] iommu/arm-smmu-v3: Implement is_attach_deferred() for kdump
Quoted 8 lines · Nicolin Chen
Though the kdump kernel adopts the crashed kernel's stream table, the iommu core will still try to attach each probed device to a default domain, which overwrites the adopted STE and breaks in-flight DMA from that device.
Implement an
is_attach_deferred()callback to prevent this. For each device that has STE.V=1 and STE.Cfg!=Abort in the adopted table, defer the default domain attachment, until the device driver explicitly requests it.
Quoted 7 lines
Fixes b63b3439b856 ("iommu/arm-smmu-v3: Abort all transactions if SMMU is enabled in kdump kernel")Cc [email protected] # v6.12+Signed-off-by Nicolin Chen <[email protected]>Reviewed-by Kevin Tian <[email protected]>--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 24 +++++++++++++++++++++ 1 file changed, 24 insertions(+)
Jason
PATCH v5 3/6 iommu/arm-smmu-v3: Suppress EVTQ/PRIQ events in kdump kernel
In reply to Nicolin ChenIn kdump cases, the crashed kernel's CDs and page tables can be corrupted, which could trigger event spamming. Also, we cannot serve page requests.
Skip the IRQ setup for EVTQ/PRIQ in arm_smmu_setup_irqs(), and guard the thread functions against being entered via a combined-IRQ delivery while the queue is disabled.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 23 +++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +21 -2
@@ -2364,6 +2364,14 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev) static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST); +/* +* A combined IRQ might call into this function with the queue disabled. +* E.g. kdump, where stale HW PROD vs SW CONS would drive a bogus drain +* and a CONS write to a disabled queue. +*/ +if (!(readl_relaxed (smmu->base +ARM_SMMU_CR0 ) &CR0_EVTQEN )) +return IRQ_NONE ; + do { while (!queue_remove_raw(q, evt)) { arm_smmu_decode_event(smmu, evt, &event);
@@ -2432,6 +2440,14 @@ static irqreturn_t arm_smmu_priq_thread(int irq, void *dev)struct arm_smmu_ll_queue *llq = &q->llq;u64 evt[PRIQ_ENT_DWORDS ]; +/* +* A combined IRQ might call into this function with the queue disabled. +* E.g. kdump, where stale HW PROD vs SW CONS would drive a bogus drain +* and a CONS write to a disabled queue. +*/ +if (!(readl_relaxed (smmu->base +ARM_SMMU_CR0 ) &CR0_PRIQEN )) +return IRQ_NONE ; + do { while (!queue_remove_raw(q, evt)) arm_smmu_handle_ppr(smmu, evt);
@@ -5056,7 +5072,10 @@ static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)static int arm_smmu_setup_irqs (struct arm_smmu_device *smmu) {int ret, irq; -u32 irqen_flags =IRQ_CTRL_EVTQ_IRQEN |IRQ_CTRL_GERROR_IRQEN ; +u32 irqen_flags =IRQ_CTRL_GERROR_IRQEN ; + +if (!is_kdump_kernel ()) + irqen_flags |=IRQ_CTRL_EVTQ_IRQEN ;/* Disable IRQs first */ ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
@@ -5082,7 +5101,7 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) }else arm_smmu_setup_unique_irqs (smmu); -if (smmu->features &ARM_SMMU_FEAT_PRI ) +if (!is_kdump_kernel () && (smmu->features &ARM_SMMU_FEAT_PRI )) irqen_flags |=IRQ_CTRL_PRIQ_IRQEN ;/* Enable interrupt generation on the SMMU */
Signature · 1 line
2.43.0
Re: [PATCH v5 3/6] iommu/arm-smmu-v3: Suppress EVTQ/PRIQ events in kdump kernel
Quoted 20 lines · Nicolin Chen
In kdump cases, the crashed kernel's CDs and page tables can be corrupted, which could trigger event spamming. Also, we cannot serve page requests.
Skip the IRQ setup for EVTQ/PRIQ in
arm_smmu_setup_irqs(), and guard the thread functions against being entered via a combined-IRQ delivery while the queue is disabled.Fixes b63b3439b856 ("iommu/arm-smmu-v3: Abort all transactions if SMMU is enabled in kdump kernel")Cc [email protected] # v6.12+Reviewed-by Kevin Tian <[email protected]>Signed-off-by Nicolin Chen <[email protected]>--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 23 +++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0
-0
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev) · line 2373
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +7
-0@@ -2364,6 +2364,14 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev) static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST); +/* +* A combined IRQ might call into this function with the queue disabled. +* E.g. kdump, where stale HW PROD vs SW CONS would drive a bogus drain +* and a CONS write to a disabled queue. +*/ +if (!(readl_relaxed (smmu->base +ARM_SMMU_CR0 ) &CR0_EVTQEN )) +return IRQ_NONE ;
I don't think we should be doing register reads on these paths.
Why not load a different irq function instead?
Jason
Re: [PATCH v5 3/6] iommu/arm-smmu-v3: Suppress EVTQ/PRIQ events in kdump kernel
Quoted 15 lines · Jason Gunthorpe
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev) · line 2373
Quoted 11 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +7
-0@@ -2364,6 +2364,14 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev) static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST); +/* +* A combined IRQ might call into this function with the queue disabled. +* E.g. kdump, where stale HW PROD vs SW CONS would drive a bogus drain +* and a CONS write to a disabled queue. +*/ +if (!(readl_relaxed (smmu->base +ARM_SMMU_CR0 ) &CR0_EVTQEN )) +return IRQ_NONE ;I don't think we should be doing register reads on these paths.
Quoted 1 line
Why not load a different irq function instead?
Yea. Perhaps we could even entirely skip their IRQ requests. Only gerror should be kept in kdump case.
Thanks
Nicolin
Re: [PATCH v5 3/6] iommu/arm-smmu-v3: Suppress EVTQ/PRIQ events in kdump kernel
Quoted 16 lines · Nicolin Chen
Quoted 15 lines · Jason Gunthorpe
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev) · line 2373
Quoted 11 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +7
-0@@ -2364,6 +2364,14 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev) static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST); +/* +* A combined IRQ might call into this function with the queue disabled. +* E.g. kdump, where stale HW PROD vs SW CONS would drive a bogus drain +* and a CONS write to a disabled queue. +*/ +if (!(readl_relaxed (smmu->base +ARM_SMMU_CR0 ) &CR0_EVTQEN )) +return IRQ_NONE ;I don't think we should be doing register reads on these paths.
Quoted 4 lines
Quoted 1 line
Why not load a different irq function instead?
Yea. Perhaps we could even entirely skip their IRQ requests. Only gerror should be kept in kdump case.
Yeah, if the irq handlers don't do anything then don't register them makes alot of sense..
Jason
PATCH v5 4/6 iommu/arm-smmu-v3: Skip EVTQ/PRIQ setup in kdump kernel
In reply to Nicolin ChenIn kdump cases, the crashed kernel's CDs and page tables can be corrupted, which could trigger event spamming. Also, we cannot serve page requests.
Skip the EVTQ/PRIQ setup entirely rather than enabling then disabling them.
Also add some inline comments explaining that.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 43 +++++++++++++-------- 1 file changed, 27 insertions(+), 16 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +27 -16
@@ -5211,21 +5211,35 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu) cmd.opcode =CMDQ_OP_TLBI_NSNH_ALL ;arm_smmu_cmdq_issue_cmd_with_sync (smmu, &cmd); -/* Event queue */ -writeq_relaxed (smmu->evtq.q.q_base, smmu->base +ARM_SMMU_EVTQ_BASE ); -writel_relaxed (smmu->evtq.q.llq.prod, smmu->page1 +ARM_SMMU_EVTQ_PROD ); -writel_relaxed (smmu->evtq.q.llq.cons, smmu->page1 +ARM_SMMU_EVTQ_CONS ); - - enables |=CR0_EVTQEN ; - ret =arm_smmu_write_reg_sync (smmu, enables,ARM_SMMU_CR0 , -ARM_SMMU_CR0ACK ); -if (ret) { -dev_err (smmu->dev,"failed to enable event queue\n" ); -return ret; +/* +* Event queue +* +* Do not enable in a kdump case, as the crashed kernel's CDs and page +* tables might be corrupted, triggering event spamming. +*/ +if (!is_kdump_kernel ()) { +writeq_relaxed (smmu->evtq.q.q_base, + smmu->base +ARM_SMMU_EVTQ_BASE ); +writel_relaxed (smmu->evtq.q.llq.prod, + smmu->page1 +ARM_SMMU_EVTQ_PROD ); +writel_relaxed (smmu->evtq.q.llq.cons, + smmu->page1 +ARM_SMMU_EVTQ_CONS ); + + enables |=CR0_EVTQEN ; + ret =arm_smmu_write_reg_sync (smmu, enables,ARM_SMMU_CR0 , +ARM_SMMU_CR0ACK ); +if (ret) { +dev_err (smmu->dev,"failed to enable event queue\n" ); +return ret; + } } -/* PRI queue */ -if (smmu->features &ARM_SMMU_FEAT_PRI ) { +/* +* PRI queue +* +* Do not enable in a kdump case, as we cannot serve page requests. +*/ +if (!is_kdump_kernel () && (smmu->features &ARM_SMMU_FEAT_PRI )) {writeq_relaxed (smmu->priq.q.q_base, smmu->base +ARM_SMMU_PRIQ_BASE ); writel_relaxed(smmu->priq.q.llq.prod,
@@ -5258,9 +5272,6 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu)return ret; } -if (is_kdump_kernel ()) - enables &= ~(CR0_EVTQEN |CR0_PRIQEN ); -/* Enable the SMMU interface */ enables |=CR0_SMMUEN ; ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
Signature · 1 line
2.43.0
Re: [PATCH v5 4/6] iommu/arm-smmu-v3: Skip EVTQ/PRIQ setup in kdump kernel
Quoted 7 lines · Nicolin Chen
In kdump cases, the crashed kernel's CDs and page tables can be corrupted, which could trigger event spamming. Also, we cannot serve page requests.
Skip the EVTQ/PRIQ setup entirely rather than enabling then disabling them.
Also add some inline comments explaining that.
Quoted 8 lines
Fixes b63b3439b856 ("iommu/arm-smmu-v3: Abort all transactions if SMMU is enabled in kdump kernel")Cc [email protected] # v6.12+Suggested-by Kevin Tian <[email protected]>Reviewed-by Kevin Tian <[email protected]>Signed-off-by Nicolin Chen <[email protected]>--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 43 +++++++++++++-------- 1 file changed, 27 insertions(+), 16 deletions(-)
Jason
PATCH v5 5/6 iommu/arm-smmu-v3: Retain CR0_SMMUEN during kdump device reset
In reply to Nicolin ChenWhen ARM_SMMU_OPT_KDUMP_ADOPT is detected, do not disable SMMUEN and skip the CR1/CR2/STRTAB_BASE update sequence in arm_smmu_device_reset(). Those register writes are all CONSTRAINED UNPREDICTABLE while CR0_SMMUEN==1, so leaving them intact lets in-flight DMAs continue to be translated by the adopted stream table.
Initialize 'enables' to 0 so it can carry CR0_SMMUEN in kdump case. Then, preserve that when enabling the command queue.
Clear latched gerror bits if necessary.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 47 +++++++++++++++++++-- 1 file changed, 44 insertions(+), 3 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +44 -3
@@ -5151,11 +5151,28 @@ static void arm_smmu_write_strtab(struct arm_smmu_device *smmu)static int arm_smmu_device_reset (struct arm_smmu_device *smmu) {int ret; -u32 reg, enables; +u32 reg, enables =0 ;struct arm_smmu_cmdq_ent cmd; -/* Clear CR0 and sync (disables SMMU and queue processing) */ reg =readl_relaxed (smmu->base +ARM_SMMU_CR0 ); + +/* +* In a kdump case (set when CR0_SMMUEN=1 and !GERROR_SFM_ERR), retain +* CR0_SMMUEN to avoid aborting in-flight DMA, and CR0_ATSCHK to carry +* on the ATS-check policy. +* +* According to spec, updating STRTAB_BASE/CR1/CR2 when CR0_SMMUEN=1 is +* CONSTRAINED UNPREDICTABLE. So, skip those register updates and rely +* on the adopted stream table from the crashed kernel. +*/ +if (smmu->options &ARM_SMMU_OPT_KDUMP_ADOPT ) { +dev_info (smmu->dev, +"kdump: retaining SMMUEN for in-flight DMA\n" ); + enables = reg & (CR0_SMMUEN |CR0_ATSCHK ); + goto reset_queues; + } + +/* Clear CR0 and sync (disables SMMU and queue processing) */ if (reg &CR0_SMMUEN ) {dev_warn (smmu->dev,"SMMU currently enabled! Resetting...\n" );arm_smmu_update_gbpa (smmu,GBPA_ABORT ,0 );
@@ -5185,12 +5202,36 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu)/* Stream table */ arm_smmu_write_strtab (smmu); +reset_queues: +if (smmu->options &ARM_SMMU_OPT_KDUMP_ADOPT ) { +/* Disable queues since arm_smmu_device_disable() was skipped */ + ret =arm_smmu_write_reg_sync (smmu, enables,ARM_SMMU_CR0 , +ARM_SMMU_CR0ACK ); +if (ret) { +dev_err (smmu->dev,"failed to disable queues\n" ); +return ret; + } + } + +/* +* GERROR bits are latched. Read after queue disabling so that unhandled +* errors would be visible. Ack everything prior to re-enabling the CMDQ +* as a stale CMDQ_ERR would halt the CMDQ and new command will timeout. +*/ +if (is_kdump_kernel ()) { +u32 gerror =readl_relaxed (smmu->base +ARM_SMMU_GERROR ); +u32 gerrorn =readl_relaxed (smmu->base +ARM_SMMU_GERRORN ); + +if ((gerror ^ gerrorn) &GERROR_ERR_MASK ) +writel (gerror, smmu->base +ARM_SMMU_GERRORN ); + } +/* Command queue */ writeq_relaxed (smmu->cmdq.q.q_base, smmu->base +ARM_SMMU_CMDQ_BASE );writel_relaxed (smmu->cmdq.q.llq.prod, smmu->base +ARM_SMMU_CMDQ_PROD );writel_relaxed (smmu->cmdq.q.llq.cons, smmu->base +ARM_SMMU_CMDQ_CONS ); - enables =CR0_CMDQEN ; + enables |=CR0_CMDQEN ; ret =arm_smmu_write_reg_sync (smmu, enables,ARM_SMMU_CR0 ,ARM_SMMU_CR0ACK );if (ret) {
Signature · 1 line
2.43.0
PATCH v5 6/6 iommu/arm-smmu-v3: Detect ARM_SMMU_OPT_KDUMP_ADOPT in probe()
In reply to Nicolin Chenarm_smmu_device_hw_probe() runs before arm_smmu_init_structures(), so it's natural to decide whether the kdump kernel must adopt the crashed kernel's stream table.
Given that memremap is used to adopt the old stream table, set this option only on a coherent SMMU.
And make sure SMMU isn't in Service Failure Mode.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 31 +++++++++++++++++++++ 1 file changed, 31 insertions(+)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +31 -0
@@ -5403,6 +5403,33 @@ static void arm_smmu_get_httu(struct arm_smmu_device *smmu, u32 reg) hw_features, fw_features); } +static void arm_smmu_device_hw_probe_kdump (struct arm_smmu_device *smmu) +{ +u32 gerror, gerrorn, active; + +/* No adoption if SMMU is disabled (i.e., there is no in-flight DMA) */ +if (!(readl_relaxed (smmu->base +ARM_SMMU_CR0 ) &CR0_SMMUEN )) +return ; + +/* For now, only support a coherent SMMU that works with MEMREMAP_WB */ +if (!(smmu->features &ARM_SMMU_FEAT_COHERENCY )) { +dev_warn (smmu->dev, +"kdump: non-coherent SMMU unsupported; reset to block all DMAs\n" ); +return ; + } + + gerror =readl_relaxed (smmu->base +ARM_SMMU_GERROR ); + gerrorn =readl_relaxed (smmu->base +ARM_SMMU_GERRORN ); + active = gerror ^ gerrorn; +if (active &GERROR_SFM_ERR ) { +dev_warn (smmu->dev, +"kdump: SMMU in Service Failure Mode, must reset\n" ); +return ; + } + + smmu->options |=ARM_SMMU_OPT_KDUMP_ADOPT ; +} +static int arm_smmu_device_hw_probe (struct arm_smmu_device *smmu) {u32 reg;
@@ -5617,6 +5644,10 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)dev_info (smmu->dev,"oas %lu-bit (features 0x%08x)\n" , smmu->oas, smmu->features); + +if (is_kdump_kernel ()) +arm_smmu_device_hw_probe_kdump (smmu); +return 0 ; }
Signature · 1 line
2.43.0
Re: [PATCH v5 6/6] iommu/arm-smmu-v3: Detect ARM_SMMU_OPT_KDUMP_ADOPT in probe()
Quoted 9 lines · Nicolin Chen
arm_smmu_device_hw_probe()runs beforearm_smmu_init_structures(), so it's natural to decide whether the kdump kernel must adopt the crashed kernel's stream table.Given that memremap is used to adopt the old stream table, set this option only on a coherent SMMU.
And make sure SMMU isn't in Service Failure Mode.
Quoted 7 lines
Fixes b63b3439b856 ("iommu/arm-smmu-v3: Abort all transactions if SMMU is enabled in kdump kernel")Cc [email protected] # v6.12+Reviewed-by Kevin Tian <[email protected]>Signed-off-by Nicolin Chen <[email protected]>--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 31 +++++++++++++++++++++ 1 file changed, 31 insertions(+)
Jason