Re: [PATCH 09/12] drm/msm/a8xx: devcoredump state capture support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] drm/msm/a8xx: wrong GX power check and early GX read in a8xx_gpu_state_get()
- [High] drm/msm/a8xx: off-by-one slice ID and ignored error in gen8_snapshot_mempool()
- [Medium] drm/msm/a8xx: unrestored DBGC config after gen8_debugbus_internal_read_setup()
- [Medium] drm/msm/a8xx: hardcoded context 0 in gen8_snapshot_dbgahb_regs() SP_READ_SEL
- [Medium] drm/msm/a8xx: broken host aperture setup in legacy AHB snapshot helpers
- [Low] drm/msm/a8xx: 32-bit GENMASK(63, 44) overflow in CD_WRITE() and CD_READ()
--

commit 2b161a7f68dabf614abb5d253ffd0443bf998e9b
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/a8xx: devcoredump state capture support

This commit adds devcoredump GPU state capture and crashdumper support for
Adreno A8xx (Gen8) GPUs, along with snapshot section helpers in
adreno_snapshot.h.

> diff --git a/drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c
> new file mode 100644
> index 0000000000000..e4eb2f335a890
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c
> @@ -0,0 +1,1253 @@

[ ... ]

> +static void gen8_legacy_snapshot_registers(struct msm_gpu *gpu,
> +					   struct adreno_gpu_state *state,
> +					   const struct gen8_reg_list *regs,
> +					   u32 slice_id)
> +{
> +	int count;
> +
> +	/* Figure out how many registers we are going to dump */
> +	count = adreno_snapshot_regs_count(regs->regs);
> +
> +	snapshot_sect (state, MVC_V3, 4 * count, {
> +		.ctxt_id = 0,
> +		.cluster_id = CLUSTER_NONE,
> +		.pipe_id = PIPE_NONE,
> +		.location_id = UINT_MAX,
> +		.sp_id = UINT_MAX,
> +		.usptp_id = UINT_MAX,
> +		.slice_id = HEADER_SLICE_ID(regs->slice_region, slice_id),
> +	}) {
> +		unsigned long flags;
> +
> +		if (regs->sel)
> +			gpu_write(gpu, regs->sel->host_reg, regs->sel->val);
> +
> +		if (regs->slice_region)
> +			a8xx_aperture_acquire_slice(gpu, PIPE_NONE, slice_id, false, &flags);
> +
> +		/* Make sure the previous writes are posted before reading */
> +		mb();
> +
> +		for (const u32 *ptr = regs->regs; ptr[0] != UINT_MAX; ptr += 2) {
> +			snapshot_mvc_regs_v3_subheader(&sect, ptr);
> +
> +			for (int k = ptr[0]; k <= ptr[1]; k++) {
> +				snapshot_sect_u32(&sect, reg_read(gpu, k));
> +			}
> +		}
> +
> +		if (regs->slice_region)
> +			a8xx_aperture_release(gpu, flags);
> +	}
> +}

[ ... ]

> +static bool gen8_snapshot_mempool(struct msm_gpu *gpu,
> +				  struct adreno_gpu_state *state,
> +				  struct adreno_crashdumper *dumper)
> +{
> +	const struct gen8_snapshot_block_list *b = block_list(gpu);
> +	u32 slice_mask = to_a6xx_gpu(to_adreno_gpu(gpu))->slice_mask;
> +	u32 first_slice = ffs(slice_mask);
> +
> +	for (int i = 0; i < b->mempool_index_registers_len; i++) {
> +		const struct gen8_cp_indexed_reg *cp_indexed_reg =
> +			&b->mempool_index_registers[i];
> +		unsigned long flags;
> +		u32 j;
> +
> +		FOR_EACH_SLICE (j, cp_indexed_reg->slice_region, slice_mask) {
> +
> +			/* TODO: Remove hardcoded values */
> +			/* Set CP_CHICKEN_DBG[StabilizeMVC] to stabilize it while dumping */
> +			a8xx_aperture_acquire_slice(gpu, cp_indexed_reg->pipe_id, first_slice, false, &flags);
> +			gpu_rmw(gpu, REG_A8XX_CP_CHICKEN_DBG_PIPE, BIT(2), BIT(2));
> +			a8xx_aperture_release(gpu, flags);
> +
> +			a8xx_aperture_acquire_slice(gpu, cp_indexed_reg->pipe_id, j, true, &flags);
> +			gpu_rmw(gpu, REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE, BIT(2), BIT(2));
> +			a8xx_aperture_release(gpu, flags);
> +
> +			gen8_snapshot_indexed_registers(gpu, state, dumper,
> +				cp_indexed_reg->addr,
> +				cp_indexed_reg->data, 0,
> +				cp_indexed_reg->size,
> +				cp_indexed_reg->pipe_id,
> +				HEADER_SLICE_ID(cp_indexed_reg->slice_region, j));
> +
> +			/* Reset CP_CHICKEN_DBG[StabilizeMVC] once we are done */
> +			a8xx_aperture_acquire_slice(gpu, cp_indexed_reg->pipe_id, first_slice, false, &flags);
> +			gpu_rmw(gpu, REG_A8XX_CP_CHICKEN_DBG_PIPE, BIT(2), 0);
> +			a8xx_aperture_release(gpu, flags);
> +
> +			a8xx_aperture_acquire_slice(gpu, cp_indexed_reg->pipe_id, j, true, &flags);
> +			gpu_rmw(gpu, REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE, BIT(2), 0);
> +			a8xx_aperture_release(gpu, flags);
> +		}
> +	}
> +
> +	/* Clear aperture register */
> +	a8xx_aperture_clear(gpu);
> +
> +	return true;
> +}

[Severity: High]
In drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c, when a8xx_gpu_state_get()
calls gen8_snapshot_mempool(), first_slice is computed as ffs(slice_mask).
Because ffs() returns a 1-based bit index (unlike a8xx_get_first_slice() in
drivers/gpu/drm/msm/adreno/a8xx_gpu.c, which uses ffs(a6xx_gpu->slice_mask) -
1), passing first_slice to a8xx_aperture_acquire_slice() before modifying
REG_A8XX_CP_CHICKEN_DBG_PIPE programs an off-by-one slice ID. On a 1-slice
GPU such as Milos (where slice_mask is 0x1) or when slice ffs(slice_mask) is
fused off in slice_mask, this selects a non-existent or harvested slice.

In addition, for UNSLICE entries in b->mempool_index_registers, FOR_EACH_SLICE
sets j = 0 regardless of slice_mask, and gen8_snapshot_mempool() still
programs the aperture for slice j = 0 with use_slice = true to modify
REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE even if slice 0 is fused off.

Finally, gen8_snapshot_mempool() ignores the return value of
gen8_snapshot_indexed_registers() and always returns true. If the crashdumper
times out during mempool capture, a8xx_gpu_state_get() will not bail out via
goto out and will continue issuing register accesses.

Should first_slice be ffs(slice_mask) - 1, should
REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE only be accessed when
cp_indexed_reg->slice_region == SLICE, and should gen8_snapshot_mempool()
propagate failures from gen8_snapshot_indexed_registers()?

> +static void gen8_legacy_snapshot_mvc(struct msm_gpu *gpu,
> +				     struct adreno_gpu_state *state,
> +				     const struct gen8_cluster_registers *cluster,
> +				     u32 slice_id)
> +{
> +	int count;
> +
> +	/* Figure out how many registers we are going to dump */
> +	count = adreno_snapshot_regs_count(cluster->regs);
> +
> +	snapshot_sect (state, MVC_V3, 4 * count, {
> +		.ctxt_id = (cluster->context_id == STATE_FORCE_CTXT_1) ? 1 : 0,
> +		.cluster_id = cluster->cluster_id,
> +		.pipe_id = cluster->pipe_id,
> +		.location_id = UINT_MAX,
> +		.sp_id = UINT_MAX,
> +		.usptp_id = UINT_MAX,
> +		.slice_id = HEADER_SLICE_ID(cluster->slice_region, slice_id),
> +	}) {
> +		unsigned long flags;
> +
> +		/*
> +		 * Set the AHB control for the Host to read from the
> +		 * cluster/context for this iteration.
> +		 */
> +		a8xx_aperture_acquire_slice(gpu, cluster->pipe_id, slice_id, true, &flags);
> +
> +		if (cluster->sel)
> +			gpu_write(gpu, cluster->sel->host_reg, cluster->sel->val);
> +
> +		/* Make sure the previous writes are posted before reading */
> +		mb();
> +
> +		for (const u32 *ptr = cluster->regs; ptr[0] != UINT_MAX; ptr += 2) {
> +			snapshot_mvc_regs_v3_subheader(&sect, ptr);
> +
> +			for (int k = ptr[0]; k <= ptr[1]; k++) {
> +				snapshot_sect_u32(&sect, reg_read(gpu, k));
> +			}
> +		}
> +
> +		a8xx_aperture_release(gpu, flags);
> +	}
> +}

[Severity: Medium]
Does the host aperture programming in gen8_legacy_snapshot_mvc() and
gen8_legacy_snapshot_registers() in
drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c match what the hardware expects
when dumping registers over AHB (for b->pre_crashdumper_regs and when dumper
is NULL, such as during an SMMU stall fault)?

1. In gen8_legacy_snapshot_mvc(), calling a8xx_aperture_acquire_slice() only
programs PIPEID, SLICEID, and USESLICEID in REG_A8XX_CP_APERTURE_CNTL_HOST,
leaving CLUSTERID and CONTEXTID3D as 0 (whereas the crashdumper path in
gen8_snapshot_mvc_regs() writes cluster->cluster_id and cluster->context_id
via GEN8_CP_APERTURE_REG_VAL()). All cluster and context-banked MVC registers
read on the CPU fallback path will therefore read cluster 0 and context 0.

2. In gen8_legacy_snapshot_registers():

		if (regs->sel)
			gpu_write(gpu, regs->sel->host_reg, regs->sel->val);

		if (regs->slice_region)
			a8xx_aperture_acquire_slice(gpu, PIPE_NONE, slice_id, false, &flags);

regs->sel->host_reg is written before acquiring the aperture lock or setting
the slice, a8xx_aperture_acquire_slice() is called with use_slice = false
instead of true for SLICE regions (omitting
A8XX_CP_APERTURE_CNTL_HOST_USESLICEID), and when a subsequent region has
regs->slice_region == UNSLICE, the aperture is not cleared or updated, leaving
the previous slice selected.

3. Because a8xx_aperture_release() only drops a6xx_gpu->aperture_lock without
clearing REG_A8XX_CP_APERTURE_CNTL_HOST, and the out: path in
a8xx_gpu_state_get() does not call a8xx_aperture_clear(gpu), a CPU-fallback
state capture during a non-reset SMMU stall fault leaves
REG_A8XX_CP_APERTURE_CNTL_HOST and a6xx_gpu->cached_aperture dirty for
subsequent host register accesses.

[ ... ]

> +static void gen8_legacy_snapshot_cluster_dbgahb(
> +	struct msm_gpu *gpu, struct adreno_gpu_state *state,
> +	const struct gen8_sptp_cluster_registers *cluster,
> +	u32 sp, u32 usptp, u32 slice_id)
> +{
> +	int count;
> +
> +	/* Figure out how many registers we are going to dump */
> +	count = adreno_snapshot_regs_count(cluster->regs);
> +
> +	snapshot_sect (state, MVC_V3, 4 * count, {
> +		.ctxt_id = cluster->context_id,
> +		.cluster_id = cluster->cluster_id,
> +		.pipe_id = cluster->pipe_id,
> +		.location_id = cluster->location_id,
> +		.sp_id = sp,
> +		.usptp_id = usptp,
> +		.slice_id = HEADER_SLICE_ID(cluster->slice_region, slice_id),
> +	}) {
> +		u32 read_sel = GEN8_SP_READ_SEL_VAL(
> +			0, slice_id, cluster->location_id, cluster->pipe_id,
> +			cluster->statetype, usptp, sp);
> +
> +		gpu_write(gpu, REG_A7XX_SP_READ_SEL, read_sel);

[ ... ]

> +static bool gen8_snapshot_dbgahb_regs(struct msm_gpu *gpu,
> +				      struct adreno_gpu_state *state,
> +				      struct adreno_crashdumper *dumper)
> +{

[ ... ]

> +					/* Build the crash script */
> +					struct cd_script p = CD_INIT(dumper);
> +
> +					u32 read_sel = GEN8_SP_READ_SEL_VAL(
> +						0, slice_id, cluster->location_id,
> +						cluster->pipe_id, cluster->statetype,
> +						usptp, sp);
> +
> +					/* Program the aperture */
> +					CD_WRITE(&p, REG_A7XX_SP_READ_SEL, read_sel);

[Severity: Medium]
In drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c, both
gen8_legacy_snapshot_cluster_dbgahb() and gen8_snapshot_dbgahb_regs() record
.ctxt_id = cluster->context_id in the MVC_V3 section header, but pass a
hardcoded 0 instead of cluster->context_id as the first argument (_contextid)
to GEN8_SP_READ_SEL_VAL() when programming REG_A7XX_SP_READ_SEL.

For multi-context SPTP cluster entries in the A8xx snapshot tables (where
cluster->context_id ranges from 0 to 15), won't this repeatedly dump context 0
for every context section?

[ ... ]

> +static void gen8_debugbus_internal_read_setup(struct msm_gpu *gpu)
> +{
> +	/* Clear the DBGC logic/events */
> +	gpu_write(gpu, REG_A6XX_DBGC_CFG_DBGBUS_CNTLM, 0x70000000);
> +
> +	/* Enable the internal buffer mode and program to tail mode */
> +	gpu_write(gpu, REG_A6XX_DBGC_CFG_DBGBUS_OPL, 0x28000000);
> +
> +	/* Program the logic unit 0 to high when the side data is 1 */
> +	gpu_write(gpu, REG_A6XX_DBGC_CFG_DBGBUS_BYTEL_0, 0x0000000c);
> +	gpu_write(gpu, REG_A6XX_DBGC_CFG_DBGBUS_MASKL_0, 0x000000f0);
> +
> +	/* Enable the DBGC logic/events */
> +	gpu_write(gpu, REG_A6XX_DBGC_CFG_DBGBUS_CNTLM, 0x07000000);
> +}

[ ... ]

> +static void gen8_snapshot_cx_dbgc_debugbus_block(
> +	struct msm_gpu *gpu, uint32_t block_id, size_t count,
> +	struct adreno_snapshot_sect *sect)
> +{
> +	/* This block requires an out of order access due to a HW limitation.
> +	 * (QCTDD12240539)
> +	 */
> +	if (block_id == DEBUGBUS_CX_GC_US_I_0) {
> +		static u32 data[GEN8_DEBUGBUS_BLOCK_SIZE * 2];
> +		gen8_debugbus_internal_read_setup(gpu);
> +		for (int i = 0; i < GEN8_DEBUGBUS_BLOCK_IDX_1; i++)
> +			gen8_cx_gc_us_i_0_debugbus_read(gpu, block_id, i, &data[i * 2]);
> +		for (int i = GEN8_DEBUGBUS_BLOCK_IDX_2; i < GEN8_DEBUGBUS_BLOCK_SIZE; i++)
> +			gen8_cx_gc_us_i_0_debugbus_read(gpu, block_id, i, &data[i * 2]);
> +		for (int i = GEN8_DEBUGBUS_BLOCK_IDX_1; i < GEN8_DEBUGBUS_BLOCK_IDX_2; i++)
> +			gen8_cx_gc_us_i_0_debugbus_read(gpu, block_id, i, &data[i * 2]);
> +		snapshot_sect_arr(sect, data);
> +	} else {
> +		for (int i = 0; i < count; i++) {
> +			u32 data[2];
> +			gen8_cx_debug_bus_read(gpu, block_id, i, data);
> +			snapshot_sect_arr(sect, data);
> +		}
> +	}
> +}

[Severity: Medium]
In drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c, gen8_get_debugbus() programs
the initial debugbus configuration into REG_A6XX_DBGC_CFG_DBGBUS_CNTLT,
REG_A6XX_DBGC_CFG_DBGBUS_CNTLM, REG_A6XX_DBGC_CFG_DBGBUS_IVTL_0..3,
REG_A6XX_DBGC_CFG_DBGBUS_BYTEL_0..1, and REG_A6XX_DBGC_CFG_DBGBUS_MASKL_0..3
before iterating through b->debugbus_blocks[] and b->gbif_debugbus_blocks[].

When gen8_snapshot_cx_dbgc_debugbus_block() encounters DEBUGBUS_CX_GC_US_I_0
(at index 2 of b->debugbus_blocks[] in the A8xx snapshot tables), it calls
gen8_debugbus_internal_read_setup(), which overwrites
REG_A6XX_DBGC_CFG_DBGBUS_CNTLM, REG_A6XX_DBGC_CFG_DBGBUS_OPL,
REG_A6XX_DBGC_CFG_DBGBUS_BYTEL_0, and REG_A6XX_DBGC_CFG_DBGBUS_MASKL_0 for
internal buffer mode.

Because those registers are not restored after reading DEBUGBUS_CX_GC_US_I_0,
won't all subsequent blocks in b->debugbus_blocks[] and
b->gbif_debugbus_blocks[] be read with the internal buffer mode configuration
still active?

[ ... ]

> +struct msm_gpu_state *a8xx_gpu_state_get(struct msm_gpu *gpu)
> +{
> +	const struct gen8_snapshot_block_list *b = block_list(gpu);
> +	struct adreno_crashdumper _dumper = { 0 }, *dumper = NULL;
> +	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> +	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> +	struct adreno_gpu_state *state;
> +	// TODO need to get status from SMMU on some gen8..
> +	bool stalled = !!(gpu_read(gpu, REG_A8XX_RBBM_MISC_STATUS) &
> +			A8XX_RBBM_MISC_STATUS_SMMU_STALLED_ON_FAULT);
> +
> +	if (!b)
> +		return NULL;
> +
> +	state = kzalloc_obj(*state);
> +	if (!state)
> +		return ERR_PTR(-ENOMEM);
> +
> +	INIT_LIST_HEAD(&state->objs);
> +
> +	/* Get the generic state from the adreno core */
> +	adreno_gpu_state_get(gpu, &state->base);
> +
> +	adreno_snapshot_registers_v2(gpu, state, b->gmu_cx_unsliced_regs);
> +
> +	a8xx_snapshot_gmu_bo(state, &a6xx_gpu->gmu.log, SNAPSHOT_GMU_MEM_LOG);
> +	a8xx_snapshot_gmu_bo(state, &a6xx_gpu->gmu.hfi, SNAPSHOT_GMU_MEM_HFI);
> +	a8xx_snapshot_gmu_bo(state, &a6xx_gpu->gmu.debug, SNAPSHOT_GMU_MEM_DEBUG);
> +
> +	a8xx_snapshot_gmu_hfi_history(gpu, state);
> +
> +	/* If GX isn't on the rest of the data isn't going to be accessible */
> +	if (!a6xx_gmu_gx_is_on(adreno_gpu))
> +		return &state->base;

[Severity: High]
Can this cause unclocked or unpowered register accesses in
a8xx_gpu_state_get() in drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c when state
capture is triggered via debugfs or fault/hang recovery while GX (or GMU MX on
A850) is powered off?

First, at the top of a8xx_gpu_state_get(), REG_A8XX_RBBM_MISC_STATUS is read
before checking whether GX is powered on, even though stalled is only used
after the GX power check.

Second, the GX power check calls a6xx_gmu_gx_is_on(adreno_gpu) instead of
adreno_gpu->funcs->gx_is_on(adreno_gpu) (which points to a8xx_gmu_gx_is_on()).
In drivers/gpu/drm/msm/adreno/a6xx_gmu.c:

bool a8xx_gmu_gx_is_on(struct adreno_gpu *adreno_gpu)
{
...
	if (adreno_is_a850_family(adreno_gpu)) {
		val = a6xx_cx_misc_read(a6xx_gpu, REG_A8XX_CX_MISC_GFX_PWR_CLK_STATUS);

		return !(val &
			(A8XX_CX_MISC_GFX_PWR_CLK_STATUS_GX_GDSC_POWER_OFF |
			 A8XX_CX_MISC_GFX_PWR_CLK_STATUS_GX_CLK_OFF));
	}

	val = gmu_read(gmu, REG_A8XX_GMU_PWR_CLK_STATUS);

	return !(val &
		(A8XX_GMU_PWR_CLK_STATUS_GX_HM_GDSC_POWER_OFF |
		 A8XX_GMU_PWR_CLK_STATUS_GX_HM_CLK_OFF));
}

Calling a6xx_gmu_gx_is_on() reads REG_A6XX_GMU_SPTPRAC_PWR_CLK_STATUS (an
A6xx/A7xx register that is not valid on A8xx and accesses GMU MMIO even when
GMU MX is off on A850) and checks the wrong bits, allowing
a8xx_gpu_state_get() to proceed with GX register accesses when GX is powered
off.

[ ... ]

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_snapshot.h b/drivers/gpu/drm/msm/adreno/adreno_snapshot.h
> index f9956e0bc1d5f..e8ff102a628b7 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_snapshot.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_snapshot.h

[ ... ]

> +static inline void CD_WRITE(struct cd_script *p, u32 reg, u32 val)
> +{
> +	*(p->in++) = val;
> +	*(p->in++) = FIELD_PREP(GENMASK(63, 44), reg) | BIT(21) | BIT(0);
> +	__cd_check_overflow(p);
> +}
> +
> +static inline void CD_READ(struct cd_script *p, u32 reg, u32 dwords)
> +{
> +	*(p->in++) = p->out;
> +	*(p->in++) = FIELD_PREP(GENMASK(63, 44), reg) | dwords;
> +	p->out += sizeof(u32) * dwords;
> +	__cd_check_overflow(p);
> +}

[Severity: Low]
In drivers/gpu/drm/msm/adreno/adreno_snapshot.h, CD_WRITE() and CD_READ() use
GENMASK(63, 44) to build a 64-bit value for u64 *in. On 32-bit architectures
(such as 32-bit ARM builds with CONFIG_DRM_MSM and CONFIG_DRM_MSM_GPU_STATE
enabled), GENMASK() uses unsigned long (32-bit), which overflows the shift
count and triggers a BUILD_BUG_ON_MSG() failure in FIELD_PREP(). Should these
use GENMASK_ULL(63, 44) instead?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020204728.28844-1-robin.clark@oss.qualcomm.com?part=9