Added debug queue support by khalshai-xilinx · Pull Request #1054 · amd/xdna-driver

khalshai-xilinx · 2026-02-06T07:14:33Z

Description
Added debug queue support for ve2 driver.
Merged common functions between HSA queue and DBG queue

TODO
Need to integrate the EXIT_DBG_QUEUE API once available.
Need to work on multi threaded scenario.

Copilot

Pull request overview

This pull request adds debug queue support to the ve2 driver, enabling debug operations through a dedicated queue mechanism similar to the existing HSA queue implementation.

Changes:

Introduced a new debug queue structure (ve2_dbg_queue) with initialization, submission, and cleanup functions
Refactored common queue operations into generic functions that work with both HSA and debug queues
Updated AIE read/write operations to use the debug queue instead of direct partition read/write

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/driver/amdxdna/ve2_of.h	Added debug queue fields to context structure
src/driver/amdxdna/ve2_mgmt.h	Added function declaration for debug queue submission
src/driver/amdxdna/ve2_mgmt.c	Updated partition setup to initialize debug queue addresses
src/driver/amdxdna/ve2_hwctx.c	Implemented debug queue creation, submission, and refactored common queue functions
src/driver/amdxdna/ve2_host_queue.h	Added debug queue structures and command type definitions
src/driver/amdxdna/ve2_debug.c	Modified AIE read/write to use debug queue operations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/driver/amdxdna/ve2_hwctx.c

src/driver/amdxdna/ve2_debug.c

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/driver/amdxdna/ve2_hwctx.c

src/driver/amdxdna/ve2_debug.c

src/driver/amdxdna/ve2_hwctx.c

src/driver/amdxdna/ve2_debug.c

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/driver/amdxdna/ve2_hwctx.c

src/driver/amdxdna/ve2_debug.c

src/driver/amdxdna/ve2_hwctx.c

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/driver/amdxdna/ve2_hwctx.c

src/driver/amdxdna/ve2_mgmt.h

src/driver/amdxdna/ve2_debug.c

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/driver/amdxdna/ve2_hwctx.c

src/driver/amdxdna/ve2_debug.c

chvamshi-xilinx

As discussed , PLease make the debug queue changes under a module param .

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/driver/amdxdna/ve2_debug.c

src/driver/amdxdna/ve2_hwctx.c

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-18T17:16:41Z

src/driver/amdxdna/ve2_hwctx.c

+	slot_id = slot_id & (dbg_queue->dbg_queue_p->hq_header.capacity - 1);
+
+	hdr->common_header.opcode = opcode;
+	hdr->completion_signal =
+		(u64)(dbg_queue->hq_complete.hqc_dma_addr + slot_id * sizeof(u64));
+
+	XDNA_DBG(xdna, "Debug Queue packet opcode: %u", pkt->xrt_header.common_header.opcode);
+
+	hdr->common_header.count = sizeof(struct rw_mem);
+	hdr->common_header.distribute = 0;
+	hdr->common_header.indirect = 0;
+
+	ebp = (struct rw_mem *)pkt->data;
+	ebp->aie_addr = aie_addr;
+	ebp->host_addr_high = upper_32_bits(paddr);
+	ebp->host_addr_low = lower_32_bits(paddr);
+	ebp->length = length;
+
+	ve2_queue_commit_slot(&dbg_queue->hq_lock, &dbg_queue->reserved_write_index,
+			      &dbg_queue->dbg_queue_p->hq_header, dbg_queue->dbg_queue_p->hq_entry,
+			      &dbg_queue->hq_complete, slot_id);


The slot_id is being recalculated to slot_idx at line 588, but then this modified value is passed to ve2_queue_commit_slot at line 609. However, ve2_queue_commit_slot expects the original slot (sequence number), not slot_idx, as it performs its own modulo operation at line 212. This will cause ve2_queue_commit_slot to operate on the wrong slot.

Save the original slot_id in a separate variable before line 588, use slot_idx for the completion_signal calculation at line 593, and pass the original slot_id (not slot_idx) to ve2_queue_commit_slot at line 609.

Copilot · 2026-02-18T17:16:41Z

src/driver/amdxdna/ve2_hwctx.c

+free_hsa_queue:
+	ve2_free_queue(xdna, "HSA", (void **)&hwctx->priv->hwctx_hsa_queue.hsa_queue_p,
+		       &hwctx->priv->hwctx_hsa_queue.alloc_dev,
+		       &hwctx->priv->hwctx_hsa_queue.hsa_queue_mem.dma_addr,
+		       &hwctx->priv->hwctx_hsa_queue.hq_lock,
+		       sizeof(struct hsa_queue) + sizeof(u64) * HOST_QUEUE_ENTRY);


If enable_debug_queue is true and ve2_create_dbg_queue fails, the error path at free_hsa_queue does not clean up the event_timer that was conditionally set up if enable_polling is true. If both enable_polling and enable_debug_queue are enabled and dbg queue creation fails, the event_timer will not be stopped, leading to a use-after-free when the timer fires after cleanup_priv frees the priv structure.

Add a conditional del_timer_sync for event_timer in the free_hsa_queue error path if enable_polling is true.

Copilot · 2026-02-18T17:16:42Z

src/driver/amdxdna/ve2_host_queue.h

+	// hq_lock protects [read | write]_index and reserved_write_index
+	struct mutex			hq_lock;
+	u64				reserved_write_index;
+		/* Device used for host queue allocation */


The comment for the alloc_dev field has incorrect indentation - it has an extra leading tab compared to the field declaration below it. The comment should align with the field it describes.

Suggested change

/* Device used for host queue allocation */

/* Device used for host queue allocation */

Copilot · 2026-02-18T17:16:42Z

src/driver/amdxdna/ve2_debug.c

+		XDNA_ERR(xdev, "Size (%zu) must be a multiple of 4 bytes", size);
+		return -EINVAL;
+	}
+	/*Allocate phy memory and pass it to submit function*/


Missing space after the comment start marker. The comment should be formatted as "/* Allocate phy memory..." with a space after the opening /* for consistency with kernel coding style.

Suggested change

/*Allocate phy memory and pass it to submit function*/

/* Allocate phy memory and pass it to submit function */

Copilot · 2026-02-18T17:16:42Z

src/driver/amdxdna/ve2_hwctx.c

+cleanup_slot_id:
+	/* Reset slot to INVALID after completion (success, timeout, or interruption) */
+	mutex_lock(&dbg_queue->hq_lock);
+	dbg_queue->hq_complete.hqc_mem[slot_id] = ERT_CMD_STATE_INVALID;


After line 588 recalculates slot_id to be the slot index, line 635 uses this slot_id to reset the completion state. However, this is inconsistent - the slot_id used here should match what was passed to ve2_queue_commit_slot. Since there's a bug at line 609 where the wrong slot_id is passed to commit, this cleanup also operates on the wrong slot. This should use the original slot sequence number modulo capacity to get the correct slot_idx.

Suggested change

dbg_queue->hq_complete.hqc_mem[slot_id] = ERT_CMD_STATE_INVALID;

{

u32 slot_idx = slot_id % dbg_queue->capacity;

dbg_queue->hq_complete.hqc_mem[slot_idx] = ERT_CMD_STATE_INVALID;

}

Copilot · 2026-02-18T17:16:42Z

src/driver/amdxdna/ve2_hwctx.c

+		XDNA_ERR(xdna, "Debug queue is not initialized");
+		return -EINVAL;
+	}
+	dbg_queue = (struct ve2_dbg_queue *)&ve2_ctx->hwctx_dbg_queue;


The cast from &ve2_ctx->hwctx_dbg_queue (which has type struct ve2_dbg_queue *) to struct ve2_dbg_queue * is unnecessary. The address-of operator already returns a pointer to the correct type, so the cast can be removed.

Suggested change

dbg_queue = (struct ve2_dbg_queue *)&ve2_ctx->hwctx_dbg_queue;

dbg_queue = &ve2_ctx->hwctx_dbg_queue;

src/driver/amdxdna/ve2_hwctx.c

Signed-off-by: Khaleel Shaik <khaleel.shaik@amd.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-20T00:13:54Z

src/driver/amdxdna/ve2_hwctx.c

+free_hsa_queue:
+	ve2_free_queue(xdna, "HSA", (void **)&hwctx->priv->hwctx_hsa_queue.hsa_queue_p,
+		       &hwctx->priv->hwctx_hsa_queue.alloc_dev,
+		       &hwctx->priv->hwctx_hsa_queue.hsa_queue_mem.dma_addr,
+		       &hwctx->priv->hwctx_hsa_queue.hq_lock,
+		       sizeof(struct hsa_queue) + sizeof(u64) * HOST_QUEUE_ENTRY);


If enable_polling is true and ve2_create_dbg_queue fails, the event_timer (started at line 1466) is not cleaned up in the free_hsa_queue error path. This will leave a timer running that references freed memory, leading to a potential use-after-free. Add del_timer_sync for event_timer in the free_hsa_queue label before freeing the HSA queue, conditional on enable_polling being true.

Copilot · 2026-02-20T00:13:54Z

src/driver/amdxdna/ve2_debug.c

 	/* Write to AIE memory */
-	ret = ve2_partition_write(aie_dev, footer.col, footer.row, footer.addr,
-				  footer.size, local_buf);
+	//TODO This is temporary fix to exit the debug queue.


The TODO comment style is inconsistent with Linux kernel coding style. The comment should be formatted as "/* TODO: This is temporary fix to exit the debug queue. */" with a space after the comment delimiter, colon after TODO, and proper ending.

Suggested change

//TODO This is temporary fix to exit the debug queue.

/* TODO: This is temporary fix to exit the debug queue. */

Copilot · 2026-02-20T00:13:55Z

src/driver/amdxdna/ve2_hwctx.c

+	int nslots = HOST_QUEUE_ENTRY;
+	dma_addr_t dma_handle;
+	size_t alloc_size;
+	int r;


The loop counter 'r' is declared as 'int' but should be 'unsigned int' to match the type used in ve2_create_host_queue (line 652) and to match the unsigned comparison with MAX_MEM_REGIONS. This ensures consistency and avoids potential signed/unsigned comparison issues.

Suggested change

int r;

unsigned int r;

Copilot AI review requested due to automatic review settings February 6, 2026 07:14

Copilot AI reviewed Feb 6, 2026

View reviewed changes

khalshai-xilinx requested review from HimanshuChoudhary-Xilinx, chvamshi-xilinx, Copilot and saifuddin-xilinx February 6, 2026 13:47

Copilot AI reviewed Feb 6, 2026

View reviewed changes

khalshai-xilinx force-pushed the debug_queue_support branch from 84e6a0c to 9ad9f02 Compare February 9, 2026 11:46

khalshai-xilinx requested a review from Copilot February 9, 2026 11:48

Copilot AI reviewed Feb 9, 2026

View reviewed changes

khalshai-xilinx force-pushed the debug_queue_support branch from 9ad9f02 to d33a23e Compare February 9, 2026 17:34

khalshai-xilinx requested a review from Copilot February 9, 2026 17:38

Copilot AI reviewed Feb 9, 2026

View reviewed changes

src/driver/amdxdna/ve2_hwctx.c Outdated Show resolved Hide resolved

src/driver/amdxdna/ve2_hwctx.c Show resolved Hide resolved

src/driver/amdxdna/ve2_mgmt.h Show resolved Hide resolved

src/driver/amdxdna/ve2_debug.c Show resolved Hide resolved

khalshai-xilinx requested a review from Copilot February 9, 2026 18:37

Copilot AI reviewed Feb 9, 2026

View reviewed changes

khalshai-xilinx force-pushed the debug_queue_support branch from d33a23e to 15fc78d Compare February 10, 2026 15:14

chvamshi-xilinx requested changes Feb 11, 2026

View reviewed changes

chvamshi-xilinx added the do-not-merge label Feb 11, 2026

khalshai-xilinx requested review from chvamshi-xilinx and Copilot February 11, 2026 20:06

Copilot AI reviewed Feb 11, 2026

View reviewed changes

src/driver/amdxdna/ve2_debug.c Show resolved Hide resolved

src/driver/amdxdna/ve2_hwctx.c Show resolved Hide resolved

src/driver/amdxdna/ve2_hwctx.c Outdated Show resolved Hide resolved

src/driver/amdxdna/ve2_hwctx.c Outdated Show resolved Hide resolved

chvamshi-xilinx removed the do-not-merge label Feb 12, 2026

khalshai-xilinx added the do-not-merge label Feb 13, 2026

khalshai-xilinx force-pushed the debug_queue_support branch from 73f4cc5 to 66c2aa5 Compare February 15, 2026 11:21

chvamshi-xilinx removed the do-not-merge label Feb 16, 2026

khalshai-xilinx added the do-not-merge label Feb 16, 2026

Copilot AI review requested due to automatic review settings February 18, 2026 17:09

khalshai-xilinx force-pushed the debug_queue_support branch from 66c2aa5 to 395a0ee Compare February 18, 2026 17:09

khalshai-xilinx removed the do-not-merge label Feb 18, 2026

Copilot started reviewing on behalf of khalshai-xilinx February 18, 2026 17:11 View session

Copilot AI reviewed Feb 18, 2026

View reviewed changes

khalshai-xilinx added 10 commits February 19, 2026 23:30

Added debug queue support

b966f40

Signed-off-by: Khaleel Shaik <khaleel.shaik@amd.com>

Updated as per comments and coding style guidelines

adcaa62

Signed-off-by: Khaleel Shaik <khaleel.shaik@amd.com>

Updated as per comments and removed dead code

57b10be

Signed-off-by: Khaleel Shaik <khaleel.shaik@amd.com>

Updated with coding style

ba96187

Signed-off-by: Khaleel Shaik <khaleel.shaik@amd.com>

Updated code modfied to copilot comments

b923c2f

Signed-off-by: Khaleel Shaik <khaleel.shaik@amd.com>

Updated with copilot comments

40baa8c

Signed-off-by: Khaleel Shaik <khaleel.shaik@amd.com>

Added module param for debug queue

1b6f9d0

Signed-off-by: Khaleel Shaik <khaleel.shaik@amd.com>

Solved merge conflicts

55da3b2

Signed-off-by: Khaleel Shaik <khaleel.shaik@amd.com>

2739

coding stle fix

0785f22

Signed-off-by: Khaleel Shaik <khaleel.shaik@amd.com>

Merge conflict

5e9d36e

Signed-off-by: Khaleel Shaik <khaleel.shaik@amd.com>

Copilot AI review requested due to automatic review settings February 20, 2026 00:07

khalshai-xilinx force-pushed the debug_queue_support branch from 26a4d34 to 5e9d36e Compare February 20, 2026 00:07

Copilot started reviewing on behalf of khalshai-xilinx February 20, 2026 00:09 View session

Copilot AI reviewed Feb 20, 2026

View reviewed changes

khalshai-xilinx added the do-not-merge label Mar 10, 2026

	/* Device used for host queue allocation */
	/* Device used for host queue allocation */

	/Allocate phy memory and pass it to submit function/
	/* Allocate phy memory and pass it to submit function */

-	dbg_queue->hq_complete.hqc_mem[slot_id] = ERT_CMD_STATE_INVALID;
+	{
+		u32 slot_idx = slot_id % dbg_queue->capacity;
+		dbg_queue->hq_complete.hqc_mem[slot_idx] = ERT_CMD_STATE_INVALID;
+	}

	dbg_queue = (struct ve2_dbg_queue *)&ve2_ctx->hwctx_dbg_queue;
	dbg_queue = &ve2_ctx->hwctx_dbg_queue;

	//TODO This is temporary fix to exit the debug queue.
	/* TODO: This is temporary fix to exit the debug queue. */

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment