[PATCH v2] drm/xe/guc: Fix h2g_write usage of GUC_CTB_MSG_MAX_LEN

[Matthew Brost]

On Thu, Jun 22, 2023 at 09:27:14AM -0700, Alan Previn wrote:
> In the ABI header, GUC_CTB_MSG_MIN_LEN is '1' because
> GUC_CTB_HDR_LEN is 1. This aligns with H2G/G2H CTB specification
> where all command formats are defined in units of dwords so that '1'
> is a dword. Accordingly, GUC_CTB_MSG_MAX_LEN is 256-1 (i.e. 255
> dwords). However, h2g_write was incorrectly assuming that
> GUC_CTB_MSG_MAX_LEN was in bytes. Fix this.
> 
> v2: By correctly treating GUC_CTB_MSG_MAX_LEN as dwords, it causes
>     a local array to consume 4x the stack size. Rework the function
>     to avoid consuming stack even if the action size is large.
> 
> Signed-off-by: Alan Previn <alan.previn.teres.alexis at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc_ct.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> index 22bc9ce846db..45fdc0ebcc0e 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> @@ -401,19 +401,22 @@ static int h2g_write(struct xe_guc_ct *ct, const u32 *action, u32 len,
>  {
>  	struct xe_device *xe = ct_to_xe(ct);
>  	struct guc_ctb *h2g = &ct->ctbs.h2g;
> -	u32 cmd[GUC_CTB_MSG_MAX_LEN / sizeof(u32)];
> -	u32 cmd_len = len + GUC_CTB_HDR_LEN;
> -	u32 cmd_idx = 0, i;
> +#define H2G_CT_HEADERS 2  /* one for CTB header and one for HxG header */

Can you move this define above the function h2g_write? We have been
asked to avoid using defines in the middle of functions.

> +	u32 cmd[H2G_CT_HEADERS];
>  	u32 tail = h2g->info.tail;
> +	u32 full_len;
>  	struct iosys_map map = IOSYS_MAP_INIT_OFFSET(&h2g->cmds,
>  							 tail * sizeof(u32));
>  
> +	--len; /* cmd[1] replaces action[0], so len is one dw less */
> +	full_len = len + H2G_CT_HEADERS;
> +
>  	lockdep_assert_held(&ct->lock);
> -	XE_BUG_ON(len * sizeof(u32) > GUC_CTB_MSG_MAX_LEN);
> +	XE_BUG_ON(len > (GUC_CTB_MSG_MAX_LEN - H2G_CT_HEADERS));
>  	XE_BUG_ON(tail > h2g->info.size);
>  
>  	/* Command will wrap, zero fill (NOPs), return and check credits again */
> -	if (tail + cmd_len > h2g->info.size) {
> +	if (tail + H2G_CT_HEADERS + len > h2g->info.size) {
>  		xe_map_memset(xe, &map, 0, 0,
>  			      (h2g->info.size - tail) * sizeof(u32));
>  		h2g_reserve_space(ct, (h2g->info.size - tail));
> @@ -428,30 +431,30 @@ static int h2g_write(struct xe_guc_ct *ct, const u32 *action, u32 len,
>  	 * dw1: HXG header (including action code)
>  	 * dw2+: action data
>  	 */
> -	cmd[cmd_idx++] = FIELD_PREP(GUC_CTB_MSG_0_FORMAT, GUC_CTB_FORMAT_HXG) |
> +	cmd[0] = FIELD_PREP(GUC_CTB_MSG_0_FORMAT, GUC_CTB_FORMAT_HXG) |
>  		FIELD_PREP(GUC_CTB_MSG_0_NUM_DWORDS, len) |
>  		FIELD_PREP(GUC_CTB_MSG_0_FENCE, ct_fence_value);
>  	if (want_response) {
> -		cmd[cmd_idx++] =
> +		cmd[1] =
>  			FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |
>  			FIELD_PREP(GUC_HXG_EVENT_MSG_0_ACTION |
>  				   GUC_HXG_EVENT_MSG_0_DATA0, action[0]);
>  	} else {
> -		cmd[cmd_idx++] =
> +		cmd[1] =
>  			FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_EVENT) |
>  			FIELD_PREP(GUC_HXG_EVENT_MSG_0_ACTION |
>  				   GUC_HXG_EVENT_MSG_0_DATA0, action[0]);
>  	}
> -	for (i = 1; i < len; ++i)
> -		cmd[cmd_idx++] = action[i];
> +	++action;

NIT move this next to --len as it makes sense to me these operations are
next to each other.

Otherwise LGTM.

Matt

>  
>  	/* Write H2G ensuring visable before descriptor update */
> -	xe_map_memcpy_to(xe, &map, 0, cmd, cmd_len * sizeof(u32));
> +	xe_map_memcpy_to(xe, &map, 0, cmd, H2G_CT_HEADERS * sizeof(u32));
> +	xe_map_memcpy_to(xe, &map, H2G_CT_HEADERS * sizeof(u32), action, len * sizeof(u32));
>  	xe_device_wmb(ct_to_xe(ct));
>  
>  	/* Update local copies */
> -	h2g->info.tail = (tail + cmd_len) % h2g->info.size;
> -	h2g_reserve_space(ct, cmd_len);
> +	h2g->info.tail = (tail + full_len) % h2g->info.size;
> +	h2g_reserve_space(ct, full_len);
>  
>  	/* Update descriptor */
>  	desc_write(xe, h2g, tail, h2g->info.tail);
> 
> base-commit: f0727faae3ac98601f3f4299a967f26542b3e482
> -- 
> 2.39.0
>