Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

arm: Support hardware debugging of ARM architecture #15616

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

W-M-R
Copy link
Contributor

@W-M-R W-M-R commented Jan 20, 2025

Note: Please adhere to Contributing Guidelines.

Summary

Support hardware debugging of ARM architecture, and support smp mode
We can use "up_debugpoint_add" or "up_debugpoint_remove" to add breakpoints, and the hardware will jump into the interrupt after detecting it.
TODO:
Support singlestep

Impact

It will not affect our program.
Registration callback is not supported yet,the debug of arm32 jumps into the exception vector table, and it is also necessary to develop the jump function of the exception vector table in debug mode

Testing

Use arm32 boards, call "up_debugpoint_add", "up_debugpoint_remove" api, and set the breakpoint address.

    if (up_debugpoint_add(DEBUGPOINT_WATCHPOINT_WO, p-1, 1, NULL, NULL))
        {
            printf("Watchpoint added error\n");
        }
    if (up_debugpoint_add(DEBUGPOINT_WATCHPOINT_WO, p+4, 1, NULL, NULL))
        {
            printf("Watchpoint added error\n");
        }
    p[8]++;
    printf("p[8]: %d\n", p[8]);
    p[7]++;
    printf("p[7]: %d\n", p[7]);
    p[4]++;
    printf("p[4]: %d\n", p[4]);
    p[2]++;
    printf("p[2]: %d\n", p[2]);
    p[1]++;
    printf("p[1]: %d\n", p[1]);
    p[0]++;
    printf("p[0]: %d\n", p[0]);

log:

arm_enable_dbgmonitor: found 5 (+1 reserved) breakpoint and 4 watchpoint registers.
arm_enable_dbgmonitor: maximum watchpoint size is 8 bytes.
add_debugpoint: add debugpoints[0]: type = 2 addr = 0x40208f9f
add_debugpoint: add debugpoints[1]: type = 2 addr = 0x40208fa4
p[8]: 1
p[7]: 65
arm_dataabort: Data abort. PC: 0061f6a0 DFAR: 40208fa4 DFSR: 00000002
dump_assert_info: Current Version: NuttX  0.0.0 9d33530389-dirty Jan 21 2025 13:13:34 arm
dump_assert_info: Assertion failed panic: at file: armv7-a/arm_dataabort.c:159 task: hello process: hello 0x61f568
up_dump_register: R0: 00000009 R1: 00000000 R2: 00000049  R3: 40208fa4
up_dump_register: R4: 0061f568 R5: 00000001 R6: 40207fa8  R7: 00000000
up_dump_register: R8: 00000000 SB: 00000000 SL: 00000000  FP: 00000000
up_dump_register: IP: 6000005f SP: 40208f58 LR: 0061f68c  PC: 0061f6a0
up_dump_register: CPSR: 6000005f
dump_tasks:    PID GROUP PRI POLICY   TYPE    NPX STATE   EVENT      SIGMASK          STACKBASE  STACKSIZE      USED   FILLED    COMMAND
dump_tasks:   ----   --- --- -------- ------- --- ------- ---------- ---------------- 0x402003e0      2048       172     8.3%    irq
dump_task:       0     0   0 FIFO     Kthread -   Ready              0000000000000000 0x40204010      4080       712    17.4%    Idle_Task
dump_task:       1     0 192 RR       Kthread -   Waiting Semaphore  0000000000000000 0x40205438      4032       400     9.9%    hpwork 0x40200000 0x40200030
dump_task:       2     2 100 RR       Task    -   Waiting Semaphore  0000000000000000 0x40206850      4056      1072    26.4%    nsh_main
dump_task:       3     3 100 RR       Task    -   Running            0000000000000000 0x40207fb8      4064      1156    28.4%    hello

DFSR: 00000002 indicates that the abnormal debugging mode is entered

@W-M-R W-M-R marked this pull request as draft January 20, 2025 10:28
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Size: L The size of the change in this PR is large labels Jan 20, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 20, 2025

[Experimental Bot, please feedback here]

This PR template itself meets the NuttX requirements by providing sections for all the requested information. However, a user submitting a PR needs to fill out all the sections completely and accurately for the PR to truly meet the requirements. Simply including the template without filling it out is insufficient.

Specifically, the template addresses the requirements by prompting for:

  • Summary: Reason for the change, affected code, mechanism of the change, and related issue references.
  • Impact: Detailed breakdown of the change's effects on various aspects of the system (user experience, build process, hardware, documentation, security, and compatibility).
  • Testing: Confirmation of testing, details on the build host and target environments, and ideally, before/after testing logs.

The inclusion of the contributing guidelines link further reinforces the importance of adhering to the broader project standards.

Copy link
Contributor

@acassis acassis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include commit message with more information

@W-M-R W-M-R force-pushed the dev-hwdebug branch 4 times, most recently from 9d33530 to 4de7c4d Compare January 21, 2025 05:14
@W-M-R W-M-R changed the title arm: Support hardware debug arm: Support hardware debugging of ARM architecture Jan 21, 2025
@W-M-R W-M-R marked this pull request as ready for review January 21, 2025 05:20
@W-M-R
Copy link
Contributor Author

W-M-R commented Jan 21, 2025

Please include commit message with more information

OK, I have improved the description

arch/arm/include/arch.h Outdated Show resolved Hide resolved
arch/arm/src/armv7-a/arm_dbgmonitor.c Outdated Show resolved Hide resolved
arch/arm/src/armv7-a/arm_dbgmonitor.c Outdated Show resolved Hide resolved
arch/arm/src/armv7-a/arm_dbgmonitor.c Outdated Show resolved Hide resolved
arch/arm/include/arch.h Outdated Show resolved Hide resolved
arch/arm/src/armv7-a/arm_dbgmonitor.c Outdated Show resolved Hide resolved
arch/arm/src/armv7-a/arm_dbgmonitor.c Outdated Show resolved Hide resolved
arch/arm/src/armv7-a/arm_dbgmonitor.c Outdated Show resolved Hide resolved
arch/arm/src/armv7-a/arm_dbgmonitor.c Outdated Show resolved Hide resolved
arch/arm/src/armv7-a/arm_dbgmonitor.c Outdated Show resolved Hide resolved
arch/arm/src/armv7-a/arm_dbgmonitor.c Outdated Show resolved Hide resolved
@W-M-R W-M-R force-pushed the dev-hwdebug branch 7 times, most recently from 0b152b2 to bb1b94f Compare January 22, 2025 08:45
	Support hardware debugging of ARM architecture, and support smp mode.We can use "up_debugpoint_add" or "up_debugpoint_remove" to add breakpoints, and the hardware will jump into the interrupt after detecting it.
	It will not affect our program. Registration callback is not supported yet,the debug of arm32 jumps into the exception vector table, and it is also necessary to develop the jump function of the exception vector table in debug mode

TODO:
Support singlestep

Signed-off-by: wangmingrong1 <[email protected]>
@@ -43,7 +43,7 @@ CMN_ASRCS += arm_cpuhead.S arm_vectors.S arm_saveusercontext.S
# Common C source files

CMN_CSRCS += arm_cache.c arm_cpuinfo.c arm_dataabort.c
CMN_CSRCS += arm_doirq.c arm_gicv2.c arm_gicv2_dump.c
CMN_CSRCS += arm_hwdebug.c arm_doirq.c arm_gicv2.c arm_gicv2_dump.c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add CONFIG_ARCH_HAVE_DEBUG to control

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary because this file is in armv7a and it definitely supports debug functionality

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or should we add select ARCH_HAVE_DEBUG for the ARMv7a arch

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, need select ARCH_HAVE_DEBUG, since gdbstub depend on this.

@@ -43,7 +43,7 @@ CMN_ASRCS += arm_cpuhead.S arm_vectors.S arm_saveusercontext.S
# Common C source files

CMN_CSRCS += arm_cache.c arm_cpuinfo.c arm_dataabort.c
CMN_CSRCS += arm_doirq.c arm_gicv2.c arm_gicv2_dump.c
CMN_CSRCS += arm_hwdebug.c arm_doirq.c arm_gicv2.c arm_gicv2_dump.c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, need select ARCH_HAVE_DEBUG, since gdbstub depend on this.

#define ARM_ADDBRP_EVENT 0
#define ARM_ADDWRP_EVENT 1

/* Accessor macros for the debug registers. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add blank line after EACH comment

static int arm_get_num_brp_resources(void)
{
uint32_t didr;
ARM_DBG_READ(c0, c0, 0, didr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static int arm_get_num_brps(void)
{
int brps = arm_get_num_brp_resources();
return core_has_mismatch_brps() ? brps - 1 : brps;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove one breakpoint if hardware suport more than one.

{
uint32_t didr;
ARM_DBG_READ(c0, c0, 0, didr);
return ((didr >> 28) & 0xf) + 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's define macro like cp15.h instead using magic number

(bp->ctrl.len == ARM_BREAKPOINT_LEN_2) ? 2 :
(bp->ctrl.len == ARM_BREAKPOINT_LEN_4) ? 4 : 8));

memcpy(p, bp, sizeof(struct arm_hw_breakpoint));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move beore line 329 and remove line 333-336

size = arm_get_num_wrps();
}

for (i = 0; i < size; i++, p += sizeof(struct arm_hw_breakpoint))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

(bp->ctrl.len == ARM_BREAKPOINT_LEN_2) ? 2 :
(bp->ctrl.len == ARM_BREAKPOINT_LEN_4) ? 4 : 8));

memset(p, 0, sizeof(struct arm_hw_breakpoint));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto


bp->ctrl.enabled = 1;

if (bp->ctrl.type == ARM_BREAKPOINT_EXECUTE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not reuse:

#define DEBUGPOINT_NONE          0x00
#define DEBUGPOINT_WATCHPOINT_RO 0x01
#define DEBUGPOINT_WATCHPOINT_WO 0x02
#define DEBUGPOINT_WATCHPOINT_RW 0x03
#define DEBUGPOINT_BREAKPOINT    0x04
#define DEBUGPOINT_STEPPOINT     0x05

{
memset(bp, 0, sizeof(struct arm_hw_breakpoint));

bp->addr = (uint32_t)addr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need mask the low 2bits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture Size: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants