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

[Lizhi Hou]

On 4/10/25 00:27, Falkowski, Maciej wrote:
> 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@xxxxxxx>
> > ---
> >   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.

Thanks for your suggestion. slot_cf_has_space and slot_dpu_has_space are 
merged into one macro to reduce duplicate code.

The new macro has slot parameter which could be either cf slot or dpu 
slot type. Thus, it may not use inline function.


Lizhi

> > +#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@xxxxxxxxxxxxxxx>
>
> Best regards,
> Maciej