Re: [PATCH V1] accel/amdxdna: Fix incorrect size of ERT_START_NPU commands

["Falkowski, Maciej" <maciej.falkowski@linux.intel.com>]

On 4/9/2025 11:00 PM, Lizhi Hou wrote:

> When multiple ERT_START_NPU commands are combined in one buffer, the
> buffer size calculation is incorrect. Also, the condition to make sure
> the buffer size is not beyond 4K is also fixed.
>
> Fixes: aac243092b70 ("accel/amdxdna: Add command execution")
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
> ---
>   drivers/accel/amdxdna/aie2_message.c  |  6 +++---
>   drivers/accel/amdxdna/aie2_msg_priv.h | 10 ++++------
>   2 files changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/accel/amdxdna/aie2_message.c b/drivers/accel/amdxdna/aie2_message.c
> index bf4219e32cc1..82412eec9a4b 100644
> --- a/drivers/accel/amdxdna/aie2_message.c
> +++ b/drivers/accel/amdxdna/aie2_message.c
> @@ -525,7 +525,7 @@ aie2_cmdlist_fill_one_slot_cf(void *cmd_buf, u32 offset,
>   	if (!payload)
>   		return -EINVAL;
>   
> -	if (!slot_cf_has_space(offset, payload_len))
> +	if (!slot_has_space(*buf, offset, payload_len))
>   		return -ENOSPC;
>   
>   	buf->cu_idx = cu_idx;
> @@ -558,7 +558,7 @@ aie2_cmdlist_fill_one_slot_dpu(void *cmd_buf, u32 offset,
>   	if (payload_len < sizeof(*sn) || arg_sz > MAX_DPU_ARGS_SIZE)
>   		return -EINVAL;
>   
> -	if (!slot_dpu_has_space(offset, arg_sz))
> +	if (!slot_has_space(*buf, offset, arg_sz))
>   		return -ENOSPC;
>   
>   	buf->inst_buf_addr = sn->buffer;
> @@ -569,7 +569,7 @@ aie2_cmdlist_fill_one_slot_dpu(void *cmd_buf, u32 offset,
>   	memcpy(buf->args, sn->prop_args, arg_sz);
>   
>   	/* Accurate buf size to hint firmware to do necessary copy */
> -	*size += sizeof(*buf) + arg_sz;
> +	*size = sizeof(*buf) + arg_sz;
>   	return 0;
>   }
>   
> diff --git a/drivers/accel/amdxdna/aie2_msg_priv.h b/drivers/accel/amdxdna/aie2_msg_priv.h
> index 4e02e744b470..6df9065b13f6 100644
> --- a/drivers/accel/amdxdna/aie2_msg_priv.h
> +++ b/drivers/accel/amdxdna/aie2_msg_priv.h
> @@ -319,18 +319,16 @@ struct async_event_msg_resp {
>   } __packed;
>   
>   #define MAX_CHAIN_CMDBUF_SIZE SZ_4K
> -#define slot_cf_has_space(offset, payload_size) \
> -	(MAX_CHAIN_CMDBUF_SIZE - ((offset) + (payload_size)) > \
> -	 offsetof(struct cmd_chain_slot_execbuf_cf, args[0]))

Could this macro be rewritten as static inline function?
That would provide additional typecheck.

> +#define slot_has_space(slot, offset, payload_size)		\
> +	(MAX_CHAIN_CMDBUF_SIZE >= (offset) + (payload_size) +	\
> +	 sizeof(typeof(slot)))
> +
>   struct cmd_chain_slot_execbuf_cf {
>   	__u32 cu_idx;
>   	__u32 arg_cnt;
>   	__u32 args[] __counted_by(arg_cnt);
>   };
>   
> -#define slot_dpu_has_space(offset, payload_size) \
> -	(MAX_CHAIN_CMDBUF_SIZE - ((offset) + (payload_size)) > \
> -	 offsetof(struct cmd_chain_slot_dpu, args[0]))
>   struct cmd_chain_slot_dpu {
>   	__u64 inst_buf_addr;
>   	__u32 inst_size;

Reviewed-by: Maciej Falkowski <maciej.falkowski@linux.intel.com>

Best regards,
Maciej