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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3cf235f9-3fda-5fba-ba2e-6ce4185ddf56@amd.com>
Date: Wed, 14 Aug 2024 14:05:57 -0700
From: Lizhi Hou <lizhi.hou@....com>
To: Jeffrey Hugo <quic_jhugo@...cinc.com>, <ogabbay@...nel.org>,
	<dri-devel@...ts.freedesktop.org>
CC: <linux-kernel@...r.kernel.org>, <min.ma@....com>, <max.zhen@....com>,
	<sonal.santan@....com>, <king.tam@....com>, George Yang <George.Yang@....com>
Subject: Re: [PATCH V2 02/10] accel/amdxdna: Support hardware mailbox


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;
>> +}
>> +