Re: [PATCH v2 2/2] accel/amdxdna: use READ_ONCE/WRITE_ONCE on the command header

[Lizhi Hou]

On 8/26/26 08:31, Taimuraz Kaitmazov wrote:
> struct amdxdna_cmd::header packs STATE, OPCODE, COUNT and EXTRA_CU_MASK
> into one u32 that lives in a BO user space keeps mapped. The driver
> re-reads it on every accessor call and read-modify-writes STATE in place,
> with plain accesses that let the compiler split or refetch either side.
>
> Annotate them, as the UMQ ring indices already are. This does not make
> the update atomic against user space, it only stops the compiler from
> making it worse.

The user space should not change the BO content after the command is 
submitted. Otherwise, the command could fail.

In another word, driver/hardware treat this as invalid command. It is ok 
as long as kernel/firmware does not crash. So the bad user application 
only messes up itself.


Thanks,

Lizhi

> Signed-off-by: Taimuraz Kaitmazov <taimuraz@xxxxxxxxxxxxx>
> ---
>   drivers/accel/amdxdna/amdxdna_ctx.c | 14 +++++++++-----
>   drivers/accel/amdxdna/amdxdna_ctx.h | 11 +++++++----
>   2 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 9f44e3918bc1..148ae51db0f1 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
> @@ -123,9 +123,10 @@ void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size)
>   	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN)
>   		num_masks = 0;
>   	else
> -		num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, cmd->header);
> +		num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK,
> +					  READ_ONCE(cmd->header));
>   
> -	count = FIELD_GET(AMDXDNA_CMD_COUNT, cmd->header);
> +	count = FIELD_GET(AMDXDNA_CMD_COUNT, READ_ONCE(cmd->header));
>   	if (unlikely(count <= num_masks ||
>   		     count * sizeof(u32) +
>   		     offsetof(struct amdxdna_cmd, data[0]) >
> @@ -173,7 +174,7 @@ u32 amdxdna_cmd_get_cu_idx(struct amdxdna_gem_obj *abo)
>   	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN)
>   		return INVALID_CU_IDX;
>   
> -	num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, cmd->header);
> +	num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, READ_ONCE(cmd->header));
>   	cu_mask = cmd->data;
>   	for (i = 0; i < num_masks; i++) {
>   		if (cu_mask[i])
> @@ -191,12 +192,15 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
>   	struct amdxdna_client *client = job->hwctx->client;
>   	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
>   	struct amdxdna_cmd_chain *cc = NULL;
> +	u32 header;
>   
>   	if (!cmd)
>   		return -ENOMEM;
>   
> -	cmd->header &= ~AMDXDNA_CMD_STATE;
> -	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
> +	header = READ_ONCE(cmd->header);
> +	header &= ~AMDXDNA_CMD_STATE;
> +	header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
> +	WRITE_ONCE(cmd->header, header);
>   
>   	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) {
>   		u32 ccnt;
> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.h b/drivers/accel/amdxdna/amdxdna_ctx.h
> index f6529d512217..48e1fcfd818f 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.h
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.h
> @@ -169,19 +169,22 @@ amdxdna_cmd_get_op(struct amdxdna_gem_obj *abo)
>   	if (!cmd)
>   		return ERT_INVALID_CMD;
>   
> -	return FIELD_GET(AMDXDNA_CMD_OPCODE, cmd->header);
> +	return FIELD_GET(AMDXDNA_CMD_OPCODE, READ_ONCE(cmd->header));
>   }
>   
>   static inline void
>   amdxdna_cmd_set_state(struct amdxdna_gem_obj *abo, enum ert_cmd_state s)
>   {
>   	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
> +	u32 header;
>   
>   	if (!cmd)
>   		return;
>   
> -	cmd->header &= ~AMDXDNA_CMD_STATE;
> -	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, s);
> +	header = READ_ONCE(cmd->header);
> +	header &= ~AMDXDNA_CMD_STATE;
> +	header |= FIELD_PREP(AMDXDNA_CMD_STATE, s);
> +	WRITE_ONCE(cmd->header, header);
>   }
>   
>   static inline enum ert_cmd_state
> @@ -192,7 +195,7 @@ amdxdna_cmd_get_state(struct amdxdna_gem_obj *abo)
>   	if (!cmd)
>   		return ERT_CMD_STATE_INVALID;
>   
> -	return FIELD_GET(AMDXDNA_CMD_STATE, cmd->header);
> +	return FIELD_GET(AMDXDNA_CMD_STATE, READ_ONCE(cmd->header));
>   }
>   
>   void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size);