Re: [PATCH v5 1/6] iommu/arm-smmu-v3: Add arm_smmu_kdump_adopt_strtab() for kdump

[Jason Gunthorpe <jgg@nvidia.com>]

On Sun, May 10, 2026 at 02:23:00PM -0700, Nicolin Chen wrote:

> +#include <linux/dma-direct.h>

Nope, never do this, it is an internal header.

> +/*
> + * 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,
> +			      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?


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


> +	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);
>  
> -	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

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

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

> +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);
> +	} 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