arm: debug: Add GDB stub for aarch32 #58067

marek-vedral · 2023-05-19T11:11:05Z

Add implementation of GDB stub for 32-bit ARM. It has been
tested only on the Zynq-7000 SoC and I would like to get any feedback
from others.

The stub still has these issues:

To implement single stepping, it uses instruction address mismatch
breakpoint, as recommended in ARMv7 reference. The breakpoint control
register is configured (the state control fields) for the "PL0, Supervisor
and System modes only" option. Otherwise the breakpoint would also halt the
processor in abort mode, in which the stub loop runs. Zephyr kernel
runs in the system mode. This works well until the kernel enables
interrupts, as interrupt handlers typically run in Supervisor mode.
Single stepping therefore sometimes "catches" a handler instead of the
next application instruction. I have not tried User mode, because
Cortex-A SoCs do not appear to have the ARCH_HAS_USERSPACE flag.

@wentasah
Cc: Michal Sojka michal.sojka@cvut.cz
Signed-off-by: Marek Vedral marek.vedral@gmail.com

subsys/debug/gdbstub.c

microbuilder · 2023-06-06T06:35:26Z

I'm not sure many of the reviewers here will have a Zync-7000 to test with (I don't). Could this be made to work with qemu_cortex_a9, which is the same core used on the Zync-7000?

marek-vedral · 2023-06-07T09:22:20Z

Yes, it does seem to work on the emulator, except for single stepping. The emulator seems to ignore the mismatch breakpoint and continues the execution. Do you know whether QEMU also emulates these debug registers?

I tried to run the samples/subsys/debug/gdbstub program. The property zephyr,gdbstub-uart = &uart1; needs to be added to the devicetree of qemu_cortex_a9 and the same option -serial tcp:127.0.0.1:5678,server as for qemu_x86 must be specified in CMakeList.txt.

microbuilder · 2023-06-07T10:47:06Z

Yes, it does seem to work on the emulator, except for single stepping. The emulator seems to ignore the mismatch breakpoint and continues the execution. Do you know whether QEMU also emulates these debug registers?

I'm not sure about the internals myself, but if we have an easy to reproduce qemu sample I can ping the people who would know around me. See point below to that effect.

I tried to run the samples/subsys/debug/gdbstub program. The property zephyr,gdbstub-uart = &uart1; needs to be added to the devicetree of qemu_cortex_a9 and the same option -serial tcp:127.0.0.1:5678,server as for qemu_x86 must be specified in CMakeList.txt.

Since we'll probably want some sort of basic testing for CI for this, could you add those changes for the qemu target in a separate commit, and we can figure out how to put together a basic test scenario from there?

marek-vedral · 2023-06-07T15:14:31Z

Since we'll probably want some sort of basic testing for CI for this, could you add those changes for the qemu target in a separate commit, and we can figure out how to put together a basic test scenario from there?

Added a new commit with those changes.

microbuilder · 2023-06-07T15:18:37Z

Added a new commit with those changes.

Thanks. The sample folder is missing some files to be a valid sample, but if you can add either some notes in the sample's readme, plus a useful main.c source file and project config file, or update this PR with some instructions on testing this, I can kick the tires in on it.

EDIT: My mistake, sorry. This is editing the existing sample in samples/subsys/debug/gdbstub 🤦

MaureenHelm · 2023-07-27T15:44:29Z

@povergoing can you take a look?

arch/Kconfig

povergoing

Some minor typos and issues need to be fixed.

also, link to #60031 in case there is an impact. @SgrrZhf @ithinuel would you also take a look?

povergoing · 2023-07-28T05:29:00Z

arch/arm/core/aarch32/gdbstub.c

+/* Wrapper function to save and restore execution context */
+void z_gdb_entry(z_arch_esf_t *esf, unsigned int exc_cause)
+{
+	/* Disable the hardware breapoint in case it was set */


typo: breakpoint

povergoing · 2023-07-28T05:33:13Z

arch/arm/core/aarch32/gdbstub.c

+	uint32_t reg_val = ctx.registers[PC];
+	/* set BVR (Breakpoint value register) to PC, make sure it is word aligned */
+	reg_val &= ~(0x3);
+	__asm__ volatile("mcr p14, 0, %0, c0, c0, 4" ::"r"(reg_val) :);


Can you please use macros to define the registers, e.g. https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/arch/arm/aarch32/cortex_a_r/lib_helpers.h

povergoing · 2023-07-28T05:33:49Z

arch/arm/core/aarch32/gdbstub.c

+	reg_val |= (0xF & DBGDBCR_BYTE_ADDR_MASK) << DBGDBCR_BYTE_ADDR_SHIFT;
+	/* Breakpoint enable */
+	reg_val |= DBGDBCR_BRK_EN_MASK;
+	__asm__ volatile("mcr p14, 0, %0, c0, c0, 5" ::"r"(reg_val) :);


povergoing · 2023-07-28T05:34:30Z

arch/arm/core/aarch32/gdbstub.c

+{
+	int ret = 0;
+	/* Fill the buffer with 'x' in case the stub does not support the required register */
+	memset(buf, "xxxxxxxx", 8);


do you mean memset(buf, "x", 8);?

povergoing · 2023-07-28T05:38:21Z

arch/arm/core/aarch32/gdbstub.c

+	int ret = 0;
+	/* Fill the buffer with 'x' in case the stub does not support the required register */
+	memset(buf, "xxxxxxxx", 8);
+	ret = 8;


What's the meaning of ret = 8, here? Why not int ret = 8 at the beginning? Can you also explain a bit the meaning of the 8? It's the default size of the function read?

povergoing · 2023-07-28T05:41:10Z

arch/arm/core/aarch32/gdbstub.c

+	return ret;
+}
+
+size_t arch_gdb_reg_writeone(struct gdb_ctx *ctx, uint8_t *hex, size_t hexlen, uint32_t regno)


I found that all arch_gdb_reg_read/writexxxs share almost the same logic, would you like to refine them to make them more compact if it's not too difficult?

povergoing · 2023-07-28T05:45:20Z

subsys/debug/Kconfig

@@ -415,6 +415,9 @@ endchoice

 config GDBSTUB_BUF_SZ
 	int "GDB backend send/receive buffer size (in bytes)"
+	# GDB for ARM expects up to 18 4-byte plus 8 12-byte
+	# registers - 336 HEX letters
+	default 350 if ARM


Can this one also be set by arch/soc/board specifically instead of default xxx if ARM

povergoing · 2023-07-28T05:46:33Z

arch/arm/core/aarch32/gdbstub.c

+	/* Enable the monitor debug mode */
+	__asm__ volatile("mrc p14, 0, %0, c0, c2, 2" : "=r"(reg_val)::);
+	reg_val |= DBGDSCR_MONITOR_MODE_EN;
+	__asm__ volatile("mcr p14, 0, %0, c0, c2, 2" ::"r"(reg_val) :);


ditto, registers defined in lib_heler

povergoing · 2023-07-28T06:34:34Z

arch/arm/core/aarch32/cortex_a_r/fault.c

+#if defined(CONFIG_GDBSTUB)
+	z_gdb_entry(esf, GDB_EXCEPTION_INVALID_INSTRUCTION);
+	/* Might not be fatal if GDB stub placed it in the code. */
+	return false;


As I saw you return false after z_gdb_entry, would you like to elaborate on the details if there is a real fault instead of a gdb exception? You are going to send all faults to the gdb clients?

microbuilder · 2023-09-11T16:59:54Z

Still a lot of failures here. Do you have time to look through and sort some of them out to be able to get this merged in eventually? Thanks.

I tried rebasing on the main branch again. Most of the checks are successful, not sure why the two remaining checks fail each time.

This needs to merge first: #62431

microbuilder · 2023-09-14T10:43:23Z

This needs to merge first: #62431

Should pass now if you rebase.

marek-vedral · 2023-09-14T14:32:49Z

This needs to merge first: #62431

Should pass now if you rebase.

Thanks for the notification, I rebased on the main branch again.

SgrrZhf · 2023-10-07T06:02:11Z

It has conflicts now. Would you please rebase it again? @marek-vedral

marek-vedral · 2023-10-07T18:18:18Z

It has conflicts now. Would you please rebase it again? @marek-vedral

Rebased on the main branch, there should not be conflicts anymore.

SgrrZhf · 2023-10-08T06:48:31Z

I have tested it again and I always get stuck in the GDB command line after I type n and the application is running to the end and won't stop. This sample (samples/subsys/debug/gdbstub) works well on the qemu_x86 board well. Could you check and test it again?

marek-vedral · 2023-10-08T07:36:46Z

I have tested it again and I always get stuck in the GDB command line after I type n and the application is running to the end and won't stop. This sample (samples/subsys/debug/gdbstub) works well on the qemu_x86 board well. Could you check and test it again?

We discussed the issue with @microbuilder, see the earlier comments. I am not sure whether QEMU also emulates the debug registers, which would allow single stepping.

SgrrZhf · 2023-10-08T08:13:50Z

We discussed the issue with @microbuilder, see the earlier comments. I am not sure whether QEMU also emulates the debug registers, which would allow single stepping.

Sorry I didn't see it.

github-actions · 2023-12-08T00:30:12Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

microbuilder · 2023-12-08T09:16:35Z

@marek-vedral Sorry, I've been very slow with getting back to this one. I'm OK with the current code, but can you rebase to resolve conflicts since the last push, and we can hopefully get this in? Again, sorry for the delay.

carlescufi · 2023-12-11T08:49:32Z

@marek-vedral please rebase

If GDBSTUB is enabled and the kernel runs in tickless mode, the timer must not convert the delta cycles to a 32-bit data type (cycle_diff_t in this case). The delta_ticks variable would overflow and the next timeout would be set before the current timestamp, thus generating an interrupt right after leaving the handler. As a result, the system would receive tens of thousands of interrupts per second and would not boot. Cc: Michal Sojka <michal.sojka@cvut.cz> Signed-off-by: Marek Vedral <marek.vedral@gmail.com>

This commit adds a devicetree overlay file and an extra condition in the CMakeLists.txt file to allow remote debugging for the qemu_cortex_a9 board. Signed-off-by: Marek Vedral <marek.vedral@gmail.com>

This commit adds implementation of GDB stub for 32-bit ARM. It has been tested only on the Zynq-7000 SoC and I would like to get any feedback from others. The stub still has these issues: - To implement single stepping, it uses instruction address mismatch breakpoint, as recommended in ARMv7 reference. The breakpoint control register is configured (the state control fields) for the "PL0, Supervisor and System modes only" option. Otherwise the breakpoint would also halt the processor in abort mode, in which the stub loop runs. Zephyr kernel runs in the system mode. This works well until the kernel enables interrupts, as interrupt handlers typically run in Supervisor mode. Single stepping therefore sometimes "catches" a handler instead of the next application instruction. I have not tried User mode, because Cortex-A SoCs do not appear to have the ARCH_HAS_USERSPACE flag. Cc: Michal Sojka <michal.sojka@cvut.cz> Signed-off-by: Marek Vedral <marek.vedral@gmail.com>

marek-vedral · 2023-12-11T12:20:30Z

@marek-vedral please rebase

Rebased on main

microbuilder · 2023-12-12T08:56:57Z

@povergoing Do you mind giving this another look? Thanks.

microbuilder · 2023-12-14T08:04:44Z

@carlocaione Do you have time to give this a review?

marek-vedral requested review from ceolin, nashif, MaureenHelm, galak, ioannisg, carlocaione, bbolen and stephanosio as code owners May 19, 2023 11:11

zephyrbot added area: ARM ARM (32-bit) Architecture area: Timer Timer area: Debugging labels May 19, 2023

zephyrbot requested review from andyross, dcpleung, microbuilder, mrkhldn, povergoing and teburd May 19, 2023 11:11

zephyrbot assigned microbuilder May 19, 2023

marek-vedral force-pushed the stub-final branch from 0c0bedd to 625dd3e Compare May 19, 2023 12:11

dcpleung reviewed May 19, 2023

View reviewed changes

subsys/debug/gdbstub.c Outdated Show resolved Hide resolved

marek-vedral force-pushed the stub-final branch from 46ddc82 to 9120845 Compare May 23, 2023 08:16

dcpleung reviewed Jul 27, 2023

View reviewed changes

arch/Kconfig Outdated Show resolved Hide resolved

arch/Kconfig Outdated Show resolved Hide resolved

povergoing requested changes Jul 28, 2023

View reviewed changes

marek-vedral force-pushed the stub-final branch from 85edd45 to 3c89df1 Compare July 28, 2023 08:28

marek-vedral force-pushed the stub-final branch from 8270983 to cef38f2 Compare September 14, 2023 12:07

marek-vedral force-pushed the stub-final branch from cef38f2 to 7b77b1f Compare October 7, 2023 18:13

SgrrZhf previously approved these changes Oct 8, 2023

View reviewed changes

github-actions bot added the Stale label Dec 8, 2023

microbuilder previously approved these changes Dec 8, 2023

View reviewed changes

microbuilder removed the Stale label Dec 8, 2023

marek-vedral added 3 commits December 11, 2023 11:46

samples: debug: Add support for qemu_cortex_a9 in GDB stub sample

8f2a8d8

This commit adds a devicetree overlay file and an extra condition in the CMakeLists.txt file to allow remote debugging for the qemu_cortex_a9 board. Signed-off-by: Marek Vedral <marek.vedral@gmail.com>

marek-vedral dismissed stale reviews from microbuilder and SgrrZhf via cbd7b5a December 11, 2023 10:52

marek-vedral force-pushed the stub-final branch from 7b77b1f to cbd7b5a Compare December 11, 2023 10:52

zephyrbot added the area: Architectures label Dec 11, 2023

microbuilder approved these changes Dec 11, 2023

View reviewed changes

carlocaione approved these changes Dec 18, 2023

View reviewed changes

carlescufi merged commit 93a4287 into zephyrproject-rtos:main Dec 18, 2023
27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

arm: debug: Add GDB stub for aarch32 #58067

arm: debug: Add GDB stub for aarch32 #58067

marek-vedral commented May 19, 2023 •

edited

microbuilder commented Jun 6, 2023

marek-vedral commented Jun 7, 2023

microbuilder commented Jun 7, 2023

marek-vedral commented Jun 7, 2023

microbuilder commented Jun 7, 2023 •

edited

MaureenHelm commented Jul 27, 2023

povergoing left a comment

povergoing Jul 28, 2023

povergoing Jul 28, 2023

povergoing Jul 28, 2023

povergoing Jul 28, 2023

povergoing Jul 28, 2023

povergoing Jul 28, 2023

povergoing Jul 28, 2023

povergoing Jul 28, 2023

povergoing Jul 28, 2023

microbuilder commented Sep 11, 2023

microbuilder commented Sep 14, 2023

marek-vedral commented Sep 14, 2023

SgrrZhf commented Oct 7, 2023

marek-vedral commented Oct 7, 2023

SgrrZhf commented Oct 8, 2023

marek-vedral commented Oct 8, 2023

SgrrZhf commented Oct 8, 2023

github-actions bot commented Dec 8, 2023

microbuilder commented Dec 8, 2023

carlescufi commented Dec 11, 2023

marek-vedral commented Dec 11, 2023

microbuilder commented Dec 12, 2023

microbuilder commented Dec 14, 2023

arm: debug: Add GDB stub for aarch32 #58067

arm: debug: Add GDB stub for aarch32 #58067

Conversation

marek-vedral commented May 19, 2023 • edited

microbuilder commented Jun 6, 2023

marek-vedral commented Jun 7, 2023

microbuilder commented Jun 7, 2023

marek-vedral commented Jun 7, 2023

microbuilder commented Jun 7, 2023 • edited

MaureenHelm commented Jul 27, 2023

povergoing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

microbuilder commented Sep 11, 2023

microbuilder commented Sep 14, 2023

marek-vedral commented Sep 14, 2023

SgrrZhf commented Oct 7, 2023

marek-vedral commented Oct 7, 2023

SgrrZhf commented Oct 8, 2023

marek-vedral commented Oct 8, 2023

SgrrZhf commented Oct 8, 2023

github-actions bot commented Dec 8, 2023

microbuilder commented Dec 8, 2023

carlescufi commented Dec 11, 2023

marek-vedral commented Dec 11, 2023

microbuilder commented Dec 12, 2023

microbuilder commented Dec 14, 2023

marek-vedral commented May 19, 2023 •

edited

microbuilder commented Jun 7, 2023 •

edited