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

[Nicolin Chen <nicolinc@nvidia.com>]

On Wed, Oct 21, 2026 at 05:37:07PM +0100, John Garry wrote:
> On 10/5/26 20:30, Nicolin Chen wrote:
> > +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");
> 
> nit: "attempting to adopt ..."

OK.

> > +	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.
> > +		 */
> > +		if (WARN_ON(!(smmu->features & ARM_SMMU_FEAT_2_LVL_STRTAB)))
> > +			ret = -EINVAL;
> > +		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;
> > +	} else {
> > +		dev_err(smmu->dev, "invalid STRTAB format %u\n", fmt);
> > +		ret = -EINVAL;
> > +	}
> > +
> > +	if (ret) {
> > +		arm_smmu_kdump_adopt_cleanup(smmu);
> 
> Why do arm_smmu_kdump_adopt_strtab_2lvl() and
> arm_smmu_kdump_adopt_strtab_linear() not clean up after themselves, which
> would be standard practice?

Well, the cleanup helper is needed here anyway. So, reusing it in
the caller drops a few lines in those two adopt functions.

> > +		goto err;
> > +	}
> > +
> > +	ret = devm_add_action_or_reset(smmu->dev, arm_smmu_kdump_adopt_cleanup,
> > +				       smmu);
> > +	/* devm_add_action_or_reset ran the cleanup upon failure */
> > +	if (ret) {
> > +		dev_warn(smmu->dev, "failed to set up cleanup action\n");
> 
> devm_add_action_or_reset() only fails for ENOMEM, so I don't see a point in
> printing this message.

That's a good point. I will drop the print.

> > +		goto err;
> > +	}
> > +
> > +	return 0;
> > +
> > +err:
> > +	dev_warn(smmu->dev, "falling back to full reset\n");
> 
> nit: "falling back to full reset after adoption failed" or similar

OK.

> > +	/*> +	 * Undo the linear adoption's clearing of FEAT_2_LVL_STRTAB so
> that the
> > +	 * full-reset fallback uses the hardware-supported format.
> > +	 */
> > +	if (was_2lvl)
> > +		smmu->features |= ARM_SMMU_FEAT_2_LVL_STRTAB;
> > +	memset(&smmu->strtab_cfg, 0, sizeof(smmu->strtab_cfg));
> > +	smmu->options &= ~ARM_SMMU_OPT_KDUMP_ADOPT;
> 
> Earlier the event queue feature flag was unset - should we still be going
> with that decision?

Yes. EVETQ and PRIQ are disabled in kdump mode, even today:

	if (is_kdump_kernel())
		enables &= ~(CR0_EVTQEN | CR0_PRIQEN);

> Did you consider to just re-probe the SMMU driver without trying this
> adoption method when problems occur?

You mean -EPROBE_DEFER? I don't think it would be cleaner..

And it would retry the "adopt" mode again? This is a per-smmu
option, no global flag. When it re-probes, smmu pointer would
be reallocated. So it won't disable the adopt path during the
re-probe.

Here, we just have three lines to fallback immediately, which
is much less expensive than a re-probe, IMHO.

Nicolin