Re: [PATCH] accel/amdxdna: Invalidate the payload of the command that failed

[Lizhi Hou]

On 8/26/26 15:27, Taimuraz Kaitmazov wrote:
> amdxdna_cmd_set_error() computes the failing command's index into
> cc->error_index and then looks up cc->data[0], so the 0xff fill and the
> err_data land on the first command of the chain no matter which one failed.
> Commit 89ff45359abb ("accel/amdxdna: Fill invalid payload for failed
> command") added the index and that lookup together, and describes the fill
> as being for the failed command.
>
> So a chain that fails at a later index leaves that command's payload
> untouched, which is what the fill exists to prevent, and overwrites the
> payload of one that completed. XRT reads the failing command by
> error_index, so on the timeout path the err_data lands where it does not
> look.

User modifies command buffer after submitting it is not supported. In 
this case, it is ok as long as kernel/firmware does not crash.

Lizhi

> Use the index for the lookup. Keep it in a local: the chain lives in a BO
> user space keeps mapped and can rewrite between the store and the load.
> The index itself is firmware supplied and bounded by the command count
> aie2_cmdlist_multi_execbuf() validated at submit.
>
> Fixes: 89ff45359abb ("accel/amdxdna: Fill invalid payload for failed command")
> Signed-off-by: Taimuraz Kaitmazov <taimuraz@xxxxxxxxxxxxx>
> ---
>   drivers/accel/amdxdna/amdxdna_ctx.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 888e857ec558..976c1ad864d7 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
> @@ -177,9 +177,12 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
>   	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
>   
>   	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) {
> +		u32 err_idx;
> +
>   		cc = amdxdna_cmd_get_payload(abo, NULL);
> -		cc->error_index = (cmd_idx < cc->command_count) ? cmd_idx : 0;
> -		abo = amdxdna_gem_get_obj(client, cc->data[0], AMDXDNA_BO_SHARE);
> +		err_idx = (cmd_idx < cc->command_count) ? cmd_idx : 0;
> +		cc->error_index = err_idx;
> +		abo = amdxdna_gem_get_obj(client, cc->data[err_idx], AMDXDNA_BO_SHARE);
>   		if (!abo)
>   			return -EINVAL;
>   		cmd = amdxdna_gem_vmap(abo);