Re: [CI 2/2] drm/xe/wopcm: Simplify GuC WOPCM programming

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] drm/xe: call to undefined xe_tile_wopcm_size() breaks
  compilation
- [High] drm/xe: deleting xe_wopcm.h and xe_wopcm_size() breaks
  xe_ggtt.c build
- [Medium] drm/xe: u32 underflow in uc_program_wopcm() locks invalid
  WOPCM regs
--

commit 9298b958426645b9eb36c22dda929d9b3a5d552b
Author: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

drm/xe/wopcm: Simplify GuC WOPCM programming

This commit simplifies GuC WOPCM register programming by returning early
when WOPCM registers are already locked, dropping layout validation checks,
and moving the remaining WOPCM setup into xe_uc.c while removing the
separate xe_wopcm files.

> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index c4fb34e64cd6d..e7e818897bf50 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -47,15 +47,16 @@
>  #include "xe_sleep.h"
>  #include "xe_sriov.h"
>  #include "xe_sriov_pf_migration.h"
> +#include "xe_tile.h"
>  #include "xe_uc.h"
>  #include "xe_uc_fw.h"
>  #include "xe_wa.h"
> -#include "xe_wopcm.h"
>  
>  static u32 guc_bo_ggtt_addr(struct xe_guc *guc,
>  			    struct xe_bo *bo)
>  {
> -	struct xe_device *xe = guc_to_xe(guc);
> +	struct xe_tile *tile = gt_to_tile(guc_to_gt(guc));
> +	struct xe_device *xe = tile_to_xe(tile);
>  	u32 addr;
>  
>  	/*
> @@ -64,10 +65,12 @@ static u32 guc_bo_ggtt_addr(struct xe_guc *guc,
>  	 * might be different for each tile. So, just always ask for the address
>  	 * on the target GuC.
>  	 */
> -	addr = __xe_bo_ggtt_addr(bo, gt_to_tile(guc_to_gt(guc))->id);
> +	addr = __xe_bo_ggtt_addr(bo, tile->id);
> +
> +	if (!IS_SRIOV_VF(xe))
> +		xe_assert(xe, addr >= xe_tile_wopcm_size(tile));

[Severity: High]
Does calling xe_tile_wopcm_size() here and in uc_program_wopcm() break the
build?  xe_tile_wopcm_size() is neither declared in xe_tile.h nor defined
anywhere in the tree.

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
> index b32ba504041c5..501961c2cc8f9 100644
> --- a/drivers/gpu/drm/xe/xe_uc.c
> +++ b/drivers/gpu/drm/xe/xe_uc.c
> @@ -52,6 +55,126 @@ int xe_uc_init_noalloc(struct xe_uc *uc)

[ ... ]

> +static int uc_program_wopcm(struct xe_uc *uc)
> +{
> +	struct xe_gt *gt = uc_to_gt(uc);
> +	struct xe_tile *tile = gt_to_tile(gt);
> +	u32 huc_fw_size = xe_uc_fw_get_upload_size(&gt->uc.huc.fw);
> +	u32 wopcm_size = xe_tile_wopcm_size(tile);
> +	u32 guc_wopcm_base;
> +	u32 guc_wopcm_size;
> +	bool locked;

[ ... ]

> +	/*
> +	 * On legacy HW (pre-PVC) where the driver is responsible for
> +	 * programming the WOPCM registers, the WOPCM is divided into three
> +	 * regions, in order from bottom to top:
> +	 * 1 - The HuC region, which contains the HuC firmware + a 16KB reserved
> +	 *     section.
> +	 * 2 - The GuC region, which contains the GuC firmware + a reserved
> +	 *     section for GuC's usage.
> +	 * 3 - A reserved region for HW contexts, which is 36KB in size.
> +	 *
> +	 * To keep the math simple, we remove the HW context region from the
> +	 * WOPCM size, calculate the size of region 1 and then leave the
> +	 * remaining area for region 2. The official binaries are guaranteed to
> +	 * fit in the WOPCM, so the math below can only go wrong if a user is
> +	 * manipulating the blobs. In such a scenario, we'll catch the error at
> +	 * FW load time, so no need to double verify it here.
> +	 */
> +	wopcm_size -= (SZ_32K + SZ_4K);
> +
> +	guc_wopcm_base = huc_fw_size + SZ_16K;
> +	guc_wopcm_base = ALIGN(guc_wopcm_base, 1UL << GUC_WOPCM_OFFSET_SHIFT);
> +
> +	guc_wopcm_size = wopcm_size - guc_wopcm_base; /* already 4KB aligned */
> +
> +	return __wopcm_init_regs(gt, guc_wopcm_base, guc_wopcm_size);
> +}

[Severity: Medium]
Can wopcm_size and guc_wopcm_size underflow here and lock bogus values into
the write-once WOPCM registers before firmware load?

The firmware header parser in parse_css_header() and parse_cpd_header() only
verifies that the buffer is large enough for the headers, uCode, and RSA
data, without checking ucode_size against WOPCM size, and xe_uc_init() calls
uc_program_wopcm() before any firmware upload happens.  On pre-PVC hardware,
if an overridden HuC blob causes guc_wopcm_base >= wopcm_size (or if
wopcm_size is smaller than 36KB), the u32 subtractions underflow or yield 0.
Because the low 12 bits remain clear, __wopcm_init_regs() passes its mask
assertions and locks the corrupted values into GUC_WOPCM_SIZE and
DMA_GUC_WOPCM_OFFSET, which then cannot be cleared by a GT reset.

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_wopcm.h b/drivers/gpu/drm/xe/xe_wopcm.h
> deleted file mode 100644
> index 0197a282460be..0000000000000
> --- a/drivers/gpu/drm/xe/xe_wopcm.h
> +++ /dev/null
> @@ -1,16 +0,0 @@
> -/* SPDX-License-Identifier: MIT */
> -/*
> - * Copyright © 2022 Intel Corporation
> - */
> -
> -#ifndef _XE_WOPCM_H_
> -#define _XE_WOPCM_H_
> -
> -#include "xe_wopcm_types.h"
> -
> -struct xe_device;
> -
> -int xe_wopcm_init(struct xe_wopcm *wopcm);
> -u32 xe_wopcm_size(struct xe_device *xe);
> -
> -#endif

[Severity: High]
Does deleting xe_wopcm.h and xe_wopcm_size() break compiling xe_ggtt.c?
drivers/gpu/drm/xe/xe_ggtt.c still includes xe_wopcm.h and calls
xe_wopcm_size() in xe_ggtt_init_early() and xe_ggtt_shift_nodes().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113191739.1447747-4-daniele.ceraolospurio@intel.com?part=1