Re: [PATCH v11 06/11] iommu/arm-smmu-v3: Add ARM_SMMU_OPT_KDUMP_ADOPT for kdump kernel

[Mostafa Saleh <smostafa@google.com>]

On Tue, Oct 20, 2026 at 10:36:01AM -0700, Nicolin Chen wrote:
> On Tue, Oct 20, 2026 at 11:38:21AM +0000, Mostafa Saleh wrote:
> > On Mon, Oct 05, 2026 at 12:30:49PM -0700, Nicolin Chen wrote:
> > > +	/*
> > > +	 * According to spec (6.3.24), HW aligns the base down to the table size
> > > +	 * and ignores the low bits, so an unaligned base would make this kernel
> > > +	 * read a different table.
> > > +	 */
> > > +	if (!IS_ALIGNED(base, size)) {
> > > +		dev_err(smmu->dev, "unaligned stream table base %pa\n", &base);
> > 
> > I understand these are best effort, but what happens if the address
> > corrupted and happens to be aligned?
> 
> This is a basic sanity so HW wouldn't immediately fail. A corrupted
> address would be trickier to test out; also we don't have a perfect
> way to do that. Earlier versions had some address range check. I've
> dropped it, following Jason's comments.
> 
> This is also in the kdocs of the whole smmu kdump functions:
> 
>  *  - Only a coherent SMMU (ARM_SMMU_FEAT_COHERENCY) is supported, as the stream
>  *    table adoption is done by memremap with MEMREMAP_WB, which is verified on
>  *    the real hardware. Callers of these functions are responsible for gating
>  *    ARM_SMMU_FEAT_COHERENCY once during the probe.
>  */
> 
> > > +int arm_smmu_kdump_adopt_deferred_l2_strtab(struct arm_smmu_device *smmu,
> > > +					    u32 sid, phys_addr_t base, u32 span,
> > > +					    struct arm_smmu_strtab_l2 **l2table)
> > > +{
> > > +	struct arm_smmu_strtab_l2 *table;
> > > +	size_t size;
> > > +
> > > +	/*
> > > +	 * Retest the span in case the L1 descriptor has been overwritten since
> > > +	 * the 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,
> > > +			"L1[%u] span %u changed since adopt (was %u)\n",
> > > +			arm_smmu_strtab_l1_idx(sid), span, STRTAB_SPLIT + 1);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	size = (1UL << (span - 1)) * sizeof(struct arm_smmu_ste);
> > > +
> > > +	/* Same live-corruption check as the span; reject an overwritten base */
> > > +	if (!base || !IS_ALIGNED(base, size)) {
> > > +		dev_err(smmu->dev, "L1[%u] bad l2 table base %pa\n",
> > > +			arm_smmu_strtab_l1_idx(sid), &base);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > 
> > Those are a bit similar to arm_smmu_kexec_parse_strtab_2lvl() is it
> > possible to have some helpers instead?
> 
> Sure. I can do that.
> 
> > > +int arm_smmu_kdump_adopt_strtab(struct arm_smmu_device *smmu)
> > > +{
> > > +	u32 cfg_reg = readl_relaxed(smmu->base + ARM_SMMU_STRTAB_BASE_CFG);
> > > +	u64 base_reg = readq_relaxed(smmu->base + ARM_SMMU_STRTAB_BASE);
> > > +	bool was_2lvl = smmu->features & ARM_SMMU_FEAT_2_LVL_STRTAB;
> > > +	phys_addr_t base = base_reg & STRTAB_BASE_ADDR_MASK;
> > > +	u32 fmt = FIELD_GET(STRTAB_BASE_CFG_FMT, cfg_reg);
> > > +	int ret;
> > > +
> > > +	dev_dbg(smmu->dev, "adopting crashed kernel's stream table\n");
> > > +
> > > +	if (fmt == STRTAB_BASE_CFG_FMT_2LVL) {
> > > +		/*
> > > +		 * Both kernels run on the same hardware, so it's impossible for
> > > +		 * kdump kernel to see the support for linear stream table only.
> > > +		 */
> > 
> > STRTAB_SPLIT which influences ARM_SMMU_FEAT_2_LVL_STRTAB is a SW choice, so it is
> 
> I missed that.
> 
> > possible to get such mismatch if STRTAB_SPLIT of the newer kernel is larger than the
> > older kernel.
> > However, I agree that in that case it's better to refuse the adoption.
> >
> > > +		if (WARN_ON(!(smmu->features & ARM_SMMU_FEAT_2_LVL_STRTAB)))
> > > +			ret = -EINVAL;
> 
> I can replace the FEAT guard here with
> 
> 	if (WARN_ON(FIELD_GET(IDR0_ST_LVL, idr0) != IDR0_ST_LVL_2LVL))
> 		ret = -EINVAL;
> 

That makes more sense with the comment about for structural validation,
although.

> > > +		else
> > > +			ret = arm_smmu_kdump_adopt_strtab_2lvl(smmu, cfg_reg,
> > > +							       base);
> > > +	} else if (fmt == STRTAB_BASE_CFG_FMT_LINEAR) {
> > > +		/*
> > > +		 * The kdump kernel need not match the crashed kernel. An older
> > > +		 * crashed kernel that predates two-level stream table support
> > > +		 * may have used a linear table on 2-level-capable hardware, so
> > > +		 * enforce the same format here to match the adopted table.
> > > +		 */
> > > +		ret = arm_smmu_kdump_adopt_strtab_linear(smmu, cfg_reg, base);
> > > +		if (!ret)
> > > +			smmu->features &= ~ARM_SMMU_FEAT_2_LVL_STRTAB;
> > 
> > For simplicity, it is better to enforce the same table formats between
> > kernels as the rest of the file, for example,
> > arm_smmu_kexec_parse_strtab_2lvl() refuses different STRTAB_SPLIT.
> > Some checks can be removed and the was_2lvl error path. It's
> > very unlikely to hit such case anyway.
> 
> Sashiko pointed out a corner case that a modified kernel kdumps
> into a vanilla kernel. It is not really a big effort to support
> it. So, I would rather keep this.

I see, I have no strong opinion, but I think we can just ignore sashiko
here, and this can be a limitation as long as it fails gracefully, it
makes the patch slightly easier to reason about.

> 
> > > +	} else {
> > > +		dev_err(smmu->dev, "invalid STRTAB format %u\n", fmt);
> > > +		ret = -EINVAL;
> > > +	}
> > > +
> > > +	if (ret) {
> > > +		arm_smmu_kdump_adopt_cleanup(smmu);
> > 
> > Probably easier to inline the error handling after each call and remove this?
> 
> I don't quite follow this. The clean function is used by three
> sites. How can we remove it?

I see, I missed it is also used in devm_add_action_or_reset()

Thanks,
Mostafa

> 
> Thanks
> Nicolin
>