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

[John Garry <john.garry@arm.com>]

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

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

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

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

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

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

> +	return ret;
> +}