Re: [PATCH V2 02/10] accel/amdxdna: Support hardware mailbox

[Lizhi Hou <lizhi.hou@amd.com>]

On 8/9/24 09:32, Jeffrey Hugo wrote:
> On 8/5/2024 11:39 AM, Lizhi Hou wrote:
>> +enum aie2_msg_status {
>> +    AIE2_STATUS_SUCCESS                = 0x0,
>> +    /* AIE Error codes */
>> +    AIE2_STATUS_AIE_SATURATION_ERROR        = 0x1000001,
>> +    AIE2_STATUS_AIE_FP_ERROR            = 0x1000002,
>> +    AIE2_STATUS_AIE_STREAM_ERROR            = 0x1000003,
>> +    AIE2_STATUS_AIE_ACCESS_ERROR            = 0x1000004,
>> +    AIE2_STATUS_AIE_BUS_ERROR            = 0x1000005,
>> +    AIE2_STATUS_AIE_INSTRUCTION_ERROR        = 0x1000006,
>> +    AIE2_STATUS_AIE_ECC_ERROR            = 0x1000007,
>> +    AIE2_STATUS_AIE_LOCK_ERROR            = 0x1000008,
>> +    AIE2_STATUS_AIE_DMA_ERROR            = 0x1000009,
>> +    AIE2_STATUS_AIE_MEM_PARITY_ERROR        = 0x100000a,
>> +    AIE2_STATUS_AIE_PWR_CFG_ERROR            = 0x100000b,
>> +    AIE2_STATUS_AIE_BACKTRACK_ERROR            = 0x100000c,
>> +    AIE2_STATUS_MAX_AIE_STATUS_CODE,
>> +    /* MGMT ERT Error codes */
>> +    AIE2_STATUS_MGMT_ERT_SELF_TEST_FAILURE        = 0x2000001,
>> +    AIE2_STATUS_MGMT_ERT_HASH_MISMATCH,
>> +    AIE2_STATUS_MGMT_ERT_NOAVAIL,
>> +    AIE2_STATUS_MGMT_ERT_INVALID_PARAM,
>> +    AIE2_STATUS_MGMT_ERT_ENTER_SUSPEND_FAILURE,
>> +    AIE2_STATUS_MGMT_ERT_BUSY,
>> +    AIE2_STATUS_MGMT_ERT_APPLICATION_ACTIVE,
>> +    MAX_MGMT_ERT_STATUS_CODE,
>> +    /* APP ERT Error codes */
>> +    AIE2_STATUS_APP_ERT_FIRST_ERROR            = 0x3000001,
>> +    AIE2_STATUS_APP_INVALID_INSTR,
>> +    AIE2_STATUS_APP_LOAD_PDI_FAIL,
>> +    MAX_APP_ERT_STATUS_CODE,
>> +    /* NPU RTOS Error Codes */
>> +    AIE2_STATUS_INVALID_INPUT_BUFFER        = 0x4000001,
>> +    AIE2_STATUS_INVALID_COMMAND,
>> +    AIE2_STATUS_INVALID_PARAM,
>> +    AIE2_STATUS_INVALID_OPERATION                    = 0x4000006,
>
> Looks like your alignment is off here
Thanks. I will fix it.
>
>> +    AIE2_STATUS_ASYNC_EVENT_MSGS_FULL,
>> +    AIE2_STATUS_MAX_RTOS_STATUS_CODE,
>> +    MAX_AIE2_STATUS_CODE
>> +};
>> +
>> +struct assign_mgmt_pasid_req {
>> +    u16    pasid;
>> +    u16    reserved;
>> +} __packed;
>> +
>> +struct assign_mgmt_pasid_resp {
>> +    enum aie2_msg_status    status;
>> +} __packed;
>> +
>> +struct map_host_buffer_req {
>> +    u32        context_id;
>> +    u64        buf_addr;
>> +    u64        buf_size;
>> +} __packed;
>
> You define a bunch of structures, but don't use them.  Seems like a 
> lot of dead code to me.
>
> Hard to say since you are not using these, but I'm guessing these are 
> all the message structs to the device (fw).  They should be using __ 
> types, like __u64, since the messages are crossing boundaries.
Yes. These structs are defined by device. I will switch to use __types
>
>
>> +#define MAX_CHAIN_CMDBUF_SIZE 0x1000
>
> SZ_ macro please (here and a few other places)
Sure.
>
>
>> +
>> +struct xdna_msg_header {
>> +    u32 total_size;
>> +    u32 size        : 11;
>> +    u32 rsvd0        : 5;
>> +    u32 protocol_version    : 8;
>> +    u32 rsvd1        : 8;
>
> This bitwise syntax is a really bad idea because it depends on 
> compiler behavior. You should use FIELD_PREP
Oops. I fixed some of them during internal review. I missed this one. I 
will fix it.
>
>> +    u32 id;
>> +    u32 opcode;
>> +} __packed;
>> +
>> +static_assert(sizeof(struct xdna_msg_header) == 16);
>> +
>> +struct mailbox_pkg {
>> +    struct xdna_msg_header    header;
>> +    u32            payload[];
>> +};
>> +
>> +/* The protocol version. */
>> +#define MSG_PROTOCOL_VERSION    0x1
>> +/* The tombstone value. */
>> +#define TOMBSTONE        0xDEADFACE
>> +
>> +struct mailbox_msg {
>> +    void            *handle;
>> +    int            (*notify_cb)(void *handle, const u32 *data, 
>> size_t size);
>> +    size_t            pkg_size; /* package size in bytes */
>> +    struct mailbox_pkg    pkg;
>> +};
>> +
>> +static void mailbox_reg_write(struct mailbox_channel *mb_chann, u32 
>> mbox_reg, u32 data)
>> +{
>> +    struct xdna_mailbox_res *mb_res = &mb_chann->mb->res;
>> +    u64 ringbuf_addr = mb_res->mbox_base + mbox_reg;
>> +
>> +    iowrite32(data, (void *)ringbuf_addr);
>
> Why iowrite32() over writel()?
Will change it to writel
>
>> +static int mailbox_acquire_msgid(struct mailbox_channel *mb_chann, 
>> struct mailbox_msg *mb_msg)
>> +{
>> +    unsigned long flags;
>> +    int msg_id;
>> +
>> +    spin_lock_irqsave(&mb_chann->chan_idr_lock, flags);
>> +    msg_id = idr_alloc_cyclic(&mb_chann->chan_idr, mb_msg, 0,
>> +                  MAX_MSG_ID_ENTRIES, GFP_NOWAIT);
>> +    spin_unlock_irqrestore(&mb_chann->chan_idr_lock, flags);
>
> I think an xa struct would be preferred.

Can we defer this? This is in critical path.


Thanks,

Lizhi

>
>> +    if (msg_id < 0)
>> +        return msg_id;
>> +
>> +    /*
>> +     * The IDR becomes less efficient when dealing with larger IDs.
>> +     * Thus, add MAGIC_VAL to the higher bits.
>> +     */
>> +    msg_id |= MAGIC_VAL;
>> +    return msg_id;
>> +}
>> +