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

[Jeffrey Hugo]

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

> +	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.


> +#define MAX_CHAIN_CMDBUF_SIZE 0x1000

SZ_ macro please (here and a few other places)


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

> +	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()?

> +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.

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