Hardware debug API: Extend to aarch64 and add single stepping test #111

alwin-joshy · 2024-01-15T06:18:04Z

seL4's hardware debug API is currently only implemented for ARMv7 and x86. I am currently implementing it for AARCH64 and as a part of this, have ported the software breakpoint test to AARCH64.

I have also implemented a new test to check that single stepping works on ARM. In doing so, I believe I have found a bug in the implementation which I will make another pull request for soon. This test is simpler than the existing x86 equivalent but I think it should be sufficient. If not, I can port the existing x86 test for single stepping to ARM.

Test with: seL4/seL4#1162

Indanz

Looks good, thanks.

seL4/seL4#1162 is about ARMv7, not ARMv8. Wrong pull?

Indanz · 2024-01-19T10:30:59Z

apps/sel4test-tests/arch/arm/arch/debug.h

+#define TEST_SS_LABEL_ONE label_one
+#define TEST_SS_LABEL_TWO label_two


Not sure why this indirection is done, but anyway, I would call it TEST_SS_BREAKPOINT1 and 2.

Indanz · 2024-01-19T10:32:57Z

apps/sel4test-tests/src/arch/arm/tests/breakpoints.c

+    /* NOTHING BELOW SHOULD EVER BE REACHED */
+    int *ptr = NULL;
+    *ptr = 0xBEEF;
+    return 0;


Maybe say something like: "Should not be reached, but if it does, it wakes up the handler thread".

Indanz · 2024-01-19T10:38:45Z

apps/sel4test-tests/src/arch/arm/tests/breakpoints.c

+    if (label == seL4_Fault_DebugException &&
+        fault_data.reason == seL4_SingleStep &&
+        fault_data.vaddr == (seL4_Word) &TEST_SS_LABEL_TWO) {


You could add this check for 'TEST_SS_LABEL_ONE' above too. If the test passes it doesn't matter, but if something went wrong with the initial breakpoint it would make debugging a lot easier.

Indanz · 2024-01-19T10:44:42Z

apps/sel4test-tests/src/arch/arm/tests/breakpoints.c

+    /* Check that it is a breakpoint exception */
+    if (label != seL4_Fault_DebugException ||
+        seL4_GetMR(seL4_DebugException_ExceptionReason) != seL4_InstructionBreakpoint) {
+        ZF_LOGV("Received exception other than instruction breakpoint exception.\n");


Maybe explicitly test for NULL and say that single stepping doesn't work.

Indanz · 2024-01-19T22:12:49Z

With 1162 in mind, I think it would be good to test whether code actually gets executed instead of only checking if the instruction pointer advances, so instead of nops just add a value to a register and check if it's correct at the second label.

alwin-joshy · 2024-01-19T22:24:30Z

With 1162 in mind, I think it would be good to test whether code actually gets executed instead of only checking if the instruction pointer advances, so instead of nops just add a value to a register and check if it's correct at the second label.

Good point. In that case, I'll port the equivalent x86 test, which seems to be more robust.

Indanz · 2024-01-20T10:22:37Z

With 1162 in mind, I think it would be good to test whether code actually gets executed instead of only checking if the instruction pointer advances, so instead of nops just add a value to a register and check if it's correct at the second label.

Good point. In that case, I'll port the equivalent x86 test, which seems to be more robust.

At first glance I don't really see anything x86 specific in there, perhaps make that test cross-platform with some arch specific helper functions or defines to paper over the differences? That definitely seems to be the current intention. You'd need to move the test to some common place and make sure it still gets executed on x86.

alwin-joshy · 2024-01-20T10:25:21Z

With 1162 in mind, I think it would be good to test whether code actually gets executed instead of only checking if the instruction pointer advances, so instead of nops just add a value to a register and check if it's correct at the second label.

Good point. In that case, I'll port the equivalent x86 test, which seems to be more robust.

At first glance I don't really see anything x86 specific in there, perhaps make that test cross-platform with some arch specific helper functions or defines to paper over the differences? That definitely seems to be the current intention. You'd need to move the test to some common place and make sure it still gets executed on x86.

Yeah, should be simple. I do recall seeing a comment somewhere in the code about something being missing or not ready in ARM, but I'll see if I run into any issues. Didn't spot anything that immediately stood out.

alwin-joshy · 2024-01-22T02:52:38Z

@Indanz I moved the x86 single-stepping test into the arch-independent breakpoints file and it just worked.

lsf37 · 2024-01-23T00:05:55Z

It looks like the hw-test trigger isn't working properly on this repo. I'll investigate..

Edit: it doesn't seem to be working on the seL4 repo any more either. Looks like something changed in the infrastructure.

Edit 2: looking more closely, it did actually work. It's just not showing them in the PR summary panel. It is showing them in the "Check" tab of the PR.

Indanz · 2024-01-23T10:06:08Z

The Sabre clang fails with:

   All is well in the universe
  
  
  
  console_run returned 0
  
  0.05user 0.02system 2:48.06elapsed 0%CPU (0avgtext+0avgdata 7936maxresident)k
  0inputs+336outputs (0major+8920minor)pagefaults 0swaps
  +++ python3 ../projects/seL4_libs/libsel4test/tools/extract_results.py -q results.xml parsed_results.xml
  No log to check boot failure on.
  +++ mq.sh sem -signal sabre2 -k seL4/sel4test-seL4Test HW-7619487774-hw-run-5
  Error parsing parsed_results.xml
  -----------[ end test SABRE_debug_MCS_clang_32 ]-----------
SABRE_debug_MCS_clang_32 FAILED

Indanz · 2024-01-23T10:21:47Z

I verified that it still runs and passes on x86 and some 32 bit ARMs, it doesn't yet run on Aarch64 though. I think we should remove the lines around https://github.com/seL4/seL4/blob/cc3205ea486f6ce3da3a64d8a99e166f047b8419/src/arch/arm/config.cmake#L226.

Indanz · 2024-01-23T10:38:11Z

So do we want to remove KernelHardwareDebugAPIUnsupported from aarch64 before merging this, as the aarch64 code isn't tested yet, or merge this first and remove the define later?

@alwin-joshy, did you test this on aarch64 yet?

alwin-joshy · 2024-01-23T10:38:22Z

I verified that it still runs and passes on x86 and some 32 bit ARMs, it doesn't yet run on Aarch64 though. I think we should remove the lines around https://github.com/seL4/seL4/blob/cc3205ea486f6ce3da3a64d8a99e166f047b8419/src/arch/arm/config.cmake#L226.

Yep, that will be necessary for getting the test to run on AARCH64 eventually but there are other substantial changes required for this. I've been working on this and am close to done (alwin-joshy/seL4#7) but there are still a few problems related to ARMv8 32-bit and ARM hyp mode that I want to iron out before submitting a proper PR.

alwin-joshy · 2024-01-23T10:40:11Z

So do we want to remove KernelHardwareDebugAPIUnsupported from aarch64 before merging this, as the aarch64 code isn't tested yet, or merge this first and remove the define later?

@alwin-joshy, did you test this on aarch64 yet?

I think to merge this first and remove the KernelHardwareDebugAPIUnsupported in the PR when I have the hardware debug API fully ported to aarch64

Indanz · 2024-01-23T10:41:54Z

apps/sel4test-tests/arch/arm/arch/debug.h

 #define TEST_SOFTWARE_BREAK_ASM() \
    asm volatile( \
        ".global sbreak, post_sbreak\n\t" \
        ".type post_sbreak, function\n\t" \
        "sbreak:\n\t" \
        "bkpt\n\t")
+#else


Perhaps make it an explicit check for AARCH64, to make it clearer what's happening.

Suggested change

#else

#elif CONFIG_ARCH_AARCH64

If you tested this assembly locally I'm happy to merge this PR.

Yep, the test passes with the changes I've made in alwin-joshy/seL4#7

Ported software breakpoint test to aarch64 and moved single stepping test from x86-only to arch-independent. Signed-off-by: Alwin Joshy <[email protected]>

alwin-joshy force-pushed the hardware_debug_api branch 4 times, most recently from 6483aa2 to e5f009a Compare January 15, 2024 06:28

alwin-joshy mentioned this pull request Jan 16, 2024

Hardware Debug API: Single stepping on ARMv7 seL4/seL4#1162

Merged

Indanz approved these changes Jan 19, 2024

View reviewed changes

alwin-joshy force-pushed the hardware_debug_api branch from 934cef9 to 97eb81b Compare January 22, 2024 02:51

Indanz added hw-build set to run all sel4test hardware builds on this PR hw-test set to run sel4test hardware test for this PR labels Jan 22, 2024

alwin-joshy force-pushed the hardware_debug_api branch from 97eb81b to 5c2aed4 Compare January 23, 2024 00:59

Indanz reviewed Jan 23, 2024

View reviewed changes

hw debug API: aarch64 sw break, single step tests

3028689

Ported software breakpoint test to aarch64 and moved single stepping test from x86-only to arch-independent. Signed-off-by: Alwin Joshy <[email protected]>

alwin-joshy force-pushed the hardware_debug_api branch from 5c2aed4 to 3028689 Compare January 23, 2024 10:51

Indanz merged commit 0a488b9 into seL4:master Jan 23, 2024
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hardware debug API: Extend to aarch64 and add single stepping test #111

Hardware debug API: Extend to aarch64 and add single stepping test #111

alwin-joshy commented Jan 15, 2024 •

edited

Loading

Indanz left a comment •

edited

Loading

Indanz Jan 19, 2024

Indanz Jan 19, 2024

Indanz Jan 19, 2024

Indanz Jan 19, 2024

Indanz commented Jan 19, 2024

alwin-joshy commented Jan 19, 2024

Indanz commented Jan 20, 2024

alwin-joshy commented Jan 20, 2024

alwin-joshy commented Jan 22, 2024 •

edited

Loading

lsf37 commented Jan 23, 2024 •

edited

Loading

Indanz commented Jan 23, 2024

Indanz commented Jan 23, 2024

Indanz commented Jan 23, 2024

alwin-joshy commented Jan 23, 2024

alwin-joshy commented Jan 23, 2024

Indanz Jan 23, 2024

alwin-joshy Jan 23, 2024

		#define TEST_SS_LABEL_ONE label_one
		#define TEST_SS_LABEL_TWO label_two

Hardware debug API: Extend to aarch64 and add single stepping test #111

Hardware debug API: Extend to aarch64 and add single stepping test #111

Conversation

alwin-joshy commented Jan 15, 2024 • edited Loading

Indanz left a comment • edited Loading

Choose a reason for hiding this comment

Indanz Jan 19, 2024

Choose a reason for hiding this comment

Indanz Jan 19, 2024

Choose a reason for hiding this comment

Indanz Jan 19, 2024

Choose a reason for hiding this comment

Indanz Jan 19, 2024

Choose a reason for hiding this comment

Indanz commented Jan 19, 2024

alwin-joshy commented Jan 19, 2024

Indanz commented Jan 20, 2024

alwin-joshy commented Jan 20, 2024

alwin-joshy commented Jan 22, 2024 • edited Loading

lsf37 commented Jan 23, 2024 • edited Loading

Indanz commented Jan 23, 2024

Indanz commented Jan 23, 2024

Indanz commented Jan 23, 2024

alwin-joshy commented Jan 23, 2024

alwin-joshy commented Jan 23, 2024

Indanz Jan 23, 2024

Choose a reason for hiding this comment

alwin-joshy Jan 23, 2024

Choose a reason for hiding this comment

alwin-joshy commented Jan 15, 2024 •

edited

Loading

Indanz left a comment •

edited

Loading

alwin-joshy commented Jan 22, 2024 •

edited

Loading

lsf37 commented Jan 23, 2024 •

edited

Loading