Add Debug Native Trigger Support #573

YazanHussnain-10x · 2024-10-04T06:14:33Z

Debug native trigger support

This PR adds the debug native trigger support in sail RISC-V golden model. RISC-V Debug Specification

Parameter	Details
CSRs Implemented	Trigger Select, Trigger Data 1, Trigger Data 2, Trigger Data 3, Trigger Info, Trigger Control
No. of Supported Triggers	7
Supported Actions	0 - Currently Sail-RISC-V does not have support for debug mode, therefore, only breakpoint exception action is supported
Value of dmode	0 - Both Debug and M-mode can write the `tdata` registers
Supported Trigger Types	Instruction Count, Interrupt Trigger, Exception Trigger, Match Control Type 6 Trigger, Disabled
Re-entrancy Solution	Both solutions supported - Use `--reent-opt1` flag to enable the second solution. Only one solution can be enabled at a time
Supported Match Options in MControl6	0: Equal, 1: NAPOT, 2: GE, 3: LT, 4: Mask Low, 5: Mask High, 6: Not Equal, 8: Not Equal, 9: Not NAPOT, 12: Not Mask Low, 13: Not Mask High

Implemented Functions

tdata1_write: Writes the tdata1 register at the selected tselect.
legalize_tdata1: Legalizes the values in tdata1 for the following triggers: icount, itrigger, etrigger, and mcontrol6.
tdata1_read: Reads the tdata1 register at the selected tselect.
tdata2_write: Writes the tdata2 register at the selected tselect.
tdata2_read: Reads the tdata2 register at the selected tselect.
tdata3_write: Writes the tdata3 register at the selected tselect.
tdata3_read: Reads the tdata3 register at the selected tselect.
legalize_tcontrol: Legalizes the tcontrol register: only mte and mpte fields are writable.
TriggerFireMatch: Checks re-entrancy and returns true if trigger matching and firing are valid.
icount_trigger_match: Checks for a match in the instruction count trigger.
IETrigger_match: Checks for a match in the interrupt and exception triggers.
instrDataMatch: Checks for matches in: Execute Address, Execute Instruction, Load Address, Load Data, Store Address, Store Data. Fires the trigger except in the Load Data case.
check_trigger_firing: Before fetching the next instruction, checks for any trigger firing: icount, itrigger, etrigger, and mcontrol6.

Files Changed

riscv_csr_begin.sail: Created CSR map for tinfo and tcontrol.
riscv_types.sail: Defined mappings for Trigger Type and Match Options.
riscv_sys_regs.sail: Defined CSR read/write and legalization functions.
riscv_sys_control.sail: Defined trigger match and trigger firing check functions.
riscv_insts_zicsr.sail: Added read/write CSR definitions for the implemented CSRs.
riscv_step.sail: Called instruction count, instruction match, and check trigger firing functions.
riscv_insts_base.sail: Checked for Data Match trigger on Load/Store instructions.
riscv_insts_aext.sail: Checked for Data Match trigger on Load/Store instructions.
riscv_insts_zicboz.sail: Checked for Data Match trigger for cbo.zero.

rtwfroody

I've never written anything for sail, so take this feedback with a large grain of salt.

Looks like you're modifying each instruction that does a memory access, but I don't see this for vector operations. Did those get missed?

I only looked at the first half of this change so far. I'll read through the rest later.

c_emulator/riscv_sim.c

YazanHussnain-10x · 2024-10-09T10:08:30Z

@rtwfroody, Thanks for the initial feedback

Actually, I have never worked on the vector extension, so I planned to do it later. Today, I looked into the vector code in sail-riscv and added the trigger for vector load/store. I asked one of my colleagues to write a basic vector load test and check the load address trigger, and it’s working. However, we need feature complete vector ACTs to verify the functionality.

rtwfroody · 2024-10-09T21:19:01Z

Actually, I have never worked on the vector extension, so I planned to do it later. Today, I looked into the vector code in sail-riscv and added the trigger for vector load/store. I asked one of my colleagues to write a basic vector load test and check the load address trigger, and it’s working. However, we need feature complete vector ACTs to verify the functionality.

That's an interesting topic. The ACTs assume that all memory accesses are treated the same. In this SAIL implementation that is not the case, but I do feel that the SAIL implementation is unusual in that way. (E.g. spike has an MMU object and all memory accesses go through there, regardless of which instruction caused them.) Is there hardware where vector memory accesses are inadvertantly treated different from regular memory accesses?

rtwfroody

Sorry to review so little each day, but I am working on it...

model/riscv_sys_control.sail

YazanHussnain-10x · 2024-10-10T01:53:45Z

That's an interesting topic. The ACTs assume that all memory accesses are treated the same. In this SAIL implementation that is not the case, but I do feel that the SAIL implementation is unusual in that way. (E.g. spike has an MMU object and all memory accesses go through there, regardless of which instruction caused them.) Is there hardware where vector memory accesses are inadvertantly treated different from regular memory accesses?

No, all memory accesses go through a function called translateAddr. However, my only concern in this case is that misaligned load/store addresses have a higher priority than breakpoint exceptions in the SAIL implementation.

There are two functions common to all memory access:

ext_data_get_addr: Used to get the address (rs1 + imm).
translateAddr: Translates VA -> PA.

In the file where ext_data_get_addr is defined, some basic user-defined functions are not accessible, which are required for the implementation. If I implement it inside translateAddr, it will lower the priority of the breakpoint compared to the misaligned exception.

One solution I can think of is to move both the trigger match check and the misaligned exception check inside translateAddr. However, this would be a major change, as it would need to be applied across many files.

I believe we should proceed with the current implementation, but if you have any other suggestions, please let me know.

YazanHussnain-10x · 2024-10-10T12:13:19Z

Sorry to review so little each day, but I am working on it...

No worries at all

rtwfroody · 2024-10-28T21:19:42Z

@pmundkur Can you approve the workflow for this PR?

arichardson · 2024-10-28T21:40:03Z

@pmundkur Can you approve the workflow for this PR?

The workflow is running but it is failing since you have trailing whitespace in the new files. See the log here: https://github.com/riscv/sail-riscv/actions/runs/11274637767/job/32185366539?pr=573

model/riscv_sys_control.sail

pdonahue-ventana

I'm not a SAIL expert so I might miss some of the subtleties. It looks like @rtwfroody already commented on some of the other things I noticed, but here are my additional comments.

model/riscv_types.sail

model/riscv_sys_regs.sail

model/riscv_sys_control.sail

rtwfroody

Still working through this, slowly...

model/riscv_sys_control.sail

rtwfroody

The rest of this looks OK. Thank you.

model/riscv_sys_control.sail

tomaird · 2024-11-08T10:28:15Z

There is trigger matching here for cbo.zero but not for the other cbo.* instructions, which are described in the debug spec as matching as stores

YazanHussnain-10x · 2024-11-08T10:57:43Z

I had not added the trigger earlier due to the TODO comment on the remaining cbo instruction. I have now added the trigger, though it may require modifications once the TODO comment is addressed.

sail-riscv/model/riscv_insts_zicbom.sail

Lines 49 to 51 in 0b9c639

    
           // TODO: This is incorrect since CHERI only requires at least one byte 
        
           // to be in bounds here, whereas `ext_data_get_addr()` checks that all bytes 
        
           // are in bounds. We will need to add a new function, parameter or access type.

github-actions · 2024-11-22T16:09:48Z

Test Results

396 tests ±0 396 ✅ ±0 0s ⏱️ ±0s
4 suites ±0 0 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 5ea9eec. ± Comparison against base commit b102d07.

♻️ This comment has been updated with latest results.

YazanHussnain-10x · 2024-12-18T06:01:43Z

@Timmmm Any update on this?

Co-authored-by: Tim Newsome <[email protected]> Signed-off-by: Yazan Hussnain <[email protected]>

Co-authored-by: Paul Donahue <[email protected]> Signed-off-by: Yazan Hussnain <[email protected]>

Timmmm · 2024-12-24T19:53:48Z

Hey, sorry I'm probably not the best person to review this since I have roughly zero knowledge of the debug/triggers spec.

I can review the low level Sail syntax if you like, but for functionality we probably need someone else.

YazanHussnain-10x · 2024-12-26T06:11:40Z

@Timmmm , the functionality has been reviewed by @rtwfroody. @arichardson requested a review from you, and I believe the focus of this review is on the Sail syntax, Please feel free to proceed with that aspect.

jrtc27

Whilst this may just about implement the spec correctly (some corner cases highlighted), this has not been properly reviewed from a code quality perspective.

From CODE_STYLE.md:

Avoid unnecessary parentheses and curly braces unless doing so seriously hurts readability

These both seem to be in abundance.

Quite a lot of your functions also look, from their names and signatures, as if they're pure functions, but instead have side-effects like updating registers or even taking exceptions (e.g. instrDataMatch -> bool sounds like "is there a match?" not "handle a match").

I've also made quite a few other comments. These are not exhaustive, as some of them will motivate significant refactors, so there's not much point drilling down into all the details until the higher-level design is good. Thus addressing these comments is not sufficient for my approval and I will likely have further points to raise, but need to see the end result of this set before I know exactly what else I think should be changed.

jrtc27 · 2025-01-07T13:02:57Z

c_emulator/riscv_platform.c

@@ -62,6 +62,11 @@ bool sys_enable_zicbom(unit u)
  return rv_enable_zicbom;
 }

+bool sys_reent_opt1(unit u)


This name doesn't mean much unless you know it relates to Sdtrig, which isn't clear. Also, is there a better name that captures what it actually does than "option 1"? Finally, without knowing the Sdtrig spec, the name doesn't make it clear what false, i.e. not "option 1", means.

jrtc27 · 2025-01-07T13:25:29Z

model/riscv_step.sail

+        }
+        else {
+          /* the extension hook interposes on the fetch result */
+          match ext_fetch_hook(fetch()) {


Why is there no split before/after check like for loads? Spike does it that way which makes more sense to me.

jrtc27 · 2025-01-07T13:29:20Z

model/riscv_sys_control.sail

+}
+
+/* Match Control Type 6 Trigger Match */
+function instrDataMatch(cur_priv : Privilege, addr : xlenbits, data : xlenbits, match_size : matchSize, match_type : MatchType) -> bool = {


This is quite a big function. Can we split out the (side-effect-free) checking for a match from actually going and updating state as a result of it?

jrtc27 · 2025-01-07T13:36:53Z

model/riscv_insts_aext.sail

@@ -86,13 +86,20 @@ function clause execute(LOADRES(aq, rl, rs1, width, rd)) = {
      /* "LR faults like a normal load, even though it's in the AMO major opcode space."
        * - Andrew Waterman, isa-dev, 10 Jul 2018.
        */
-      if not(is_aligned(virtaddr_bits(vaddr), width))
+      if (instrDataMatch(cur_privilege, virtaddr_bits(vaddr), zero_extend(0b0), matchSize_of_wordWidth(width), LOAD_MATCH_BEFORE))


Passing 0b0 for every LOAD_MATCH_BEFORE is odd. Does that not mean a trigger on loading 0 will fire for every load? I imagine you instead want the data to be an optional type here and pass None() so it never matches. This is also what Spike implements. But I can't see this detail in the spec, not helped by the match algorithm being split across each field of the CSR.

It may also make sense to have two functions, one for the cases where the exception is raised now, and one for the cases where the exception is raised later. Currently the code here is implicitly assuming that LOAD_MATCH_BEFORE raises an exception, since it returns RETIRE_FAIL, and that LOAD_MATCH_AFTER doesn't, since it continues on to return RETIRE_SUCCESS. Maybe the former should also have the handle_mem_exception call in the caller here like all the other ways this can fault.

jrtc27 · 2025-01-07T13:48:19Z

model/riscv_step.sail

-              (RETIRE_FAIL, true)
+        /* If Etrigger, Icount, Itrigger and mcontrol6 after, matches, fire
+           it before fetching the next instruction */
+        if (check_trigger_firing(cur_privilege)) then {


This handles them after interrupts, which isn't really a good idea given what triggers are for. For most interrupts you can defer them so it is legal, and a good idea, to prioritise triggers. In a few cases (e.g. xRET) deferring them isn't legal, but the Sdtrig spec failed to consider this interaction. There is a proposal to fix this in riscv/riscv-debug-spec#802 but it seems to have stalled. That makes the most sense to me though.

jrtc27 · 2025-01-07T14:14:22Z

model/riscv_sys_control.sail

+          (InstrCount[count] != zero_extend(14, 0b0))           &
+          (TriggerFireMatch(cur_priv))) then { /* Trigger Match valid? check re-enterancy */
+        /* Decrement the counter */
+        if (InstrCount[count] == zero_extend(14, 0b1)) then {


Why hard-code the width for the zero_extends here?

jrtc27 · 2025-01-07T14:14:42Z

model/riscv_sys_control.sail

+           ((InstrCount[u] == 0b1) & (cur_priv == User)) )      & /* Trigger is enabled in U-mode */
+          (InstrCount[count] != zero_extend(14, 0b0))           &
+          (TriggerFireMatch(cur_priv))) then { /* Trigger Match valid? check re-enterancy */
+        /* Decrement the counter */


This comment appears to be in the wrong place

jrtc27 · 2025-01-07T14:16:48Z

model/riscv_sys_regs.sail

+  { tdata2_x.tdata2[index] = v; v } else { o }
+}
+
+/* Read TData2 of current trigger */


Why are these capitalised in some comments?

jrtc27 · 2025-01-07T14:30:07Z

model/riscv_sys_regs.sail

+  let MCtrl6         : MatchControlType6 = Mk_MatchControlType6(v[26 .. 0]);
+  let InstrCount     : InstructionCount  = Mk_InstructionCount(v[26 .. 0]);
+  let IntrptTrigger  : InterruptTrigger  = Mk_InterruptTrigger(v);
+  let ExcepTrigger   : InterruptTrigger  = Mk_InterruptTrigger(v);


These are odd abbreviations for interrupt and exception

jrtc27 · 2025-01-07T14:35:09Z

model/riscv_sys_regs.sail

+register tcontrol : Tcontrol
+
+/*
+ * The seed CSR (entropy source)


YazanHussnain-10x mentioned this pull request Oct 4, 2024

Debug SAIL for Native Triggers riscv-admin/dev-partners#8

Open

13 tasks

YazanHussnain-10x force-pushed the native_triggers branch from a70a138 to 67f737a Compare October 5, 2024 13:23

rtwfroody reviewed Oct 8, 2024

View reviewed changes

c_emulator/riscv_sim.c Outdated Show resolved Hide resolved

YazanHussnain-10x force-pushed the native_triggers branch from 1ec830e to ce3e483 Compare October 9, 2024 09:52

rtwfroody reviewed Oct 10, 2024

View reviewed changes

rtwfroody reviewed Oct 28, 2024

View reviewed changes

model/riscv_sys_control.sail Show resolved Hide resolved

rtwfroody reviewed Oct 28, 2024

View reviewed changes

model/riscv_sys_control.sail Show resolved Hide resolved

model/riscv_sys_control.sail Show resolved Hide resolved

model/riscv_sys_control.sail Outdated Show resolved Hide resolved

billmcspadden-riscv mentioned this pull request Oct 29, 2024

Sdtrig support #486

Closed

pdonahue-ventana reviewed Oct 30, 2024

View reviewed changes

YazanHussnain-10x force-pushed the native_triggers branch from bb2e3bf to ff62e68 Compare October 31, 2024 06:21

rtwfroody reviewed Nov 2, 2024

View reviewed changes

rtwfroody approved these changes Nov 8, 2024

View reviewed changes

tomaird reviewed Nov 8, 2024

View reviewed changes

model/riscv_sys_control.sail Outdated Show resolved Hide resolved

model/riscv_sys_control.sail Outdated Show resolved Hide resolved

YazanHussnain-10x force-pushed the native_triggers branch from c2e31a1 to 970cf85 Compare November 14, 2024 18:10

arichardson requested a review from Timmmm November 14, 2024 19:20

YazanHussnain-10x force-pushed the native_triggers branch from 970cf85 to 142e4b0 Compare December 9, 2024 18:02

YazanHussnain-10x added 4 commits December 21, 2024 15:02

Add Debug Native Trigger Support

0b55df2

add remaining mcontrol6 match options

9f9b2e6

replace sizeof(xlen) with xlen

3421b03

resolve error in NAPOT match option

dd87a8a

YazanHussnain-10x and others added 13 commits December 21, 2024 15:04

Add standard trigger for vector operations

b5c5a92

Update --reent-opt1 description

38a1670

Co-authored-by: Tim Newsome <[email protected]> Signed-off-by: Yazan Hussnain <[email protected]>

resolve reenterancy implementation issues

97e77f5

resolve error in load data match

2a6aa63

Fix comment

60e45de

Co-authored-by: Tim Newsome <[email protected]> Signed-off-by: Yazan Hussnain <[email protected]>

review changes and remove trailing whitespaces

0bd7e49

remove trailing whitespaces

232d922

Apply suggestions from code review

cab0f37

Co-authored-by: Paul Donahue <[email protected]> Signed-off-by: Yazan Hussnain <[email protected]>

change mcontrol6.hit0/1 comment

cd5eb34

resolve error in the interruptType encodings

b46a99a

Co-authored-by: Paul Donahue <[email protected]> Signed-off-by: Yazan Hussnain <[email protected]>

review changes

b587644

review changes cbo.*

40ad532

incorporate new type changes

fb13ad0

YazanHussnain-10x force-pushed the native_triggers branch from 142e4b0 to fb13ad0 Compare December 21, 2024 10:54

remove trailing whitespaces

5ea9eec

Timmmm added the extension Adds support for a RISC-V extension label Jan 6, 2025

jrtc27 requested changes Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Debug Native Trigger Support #573

Add Debug Native Trigger Support #573

YazanHussnain-10x commented Oct 4, 2024 •

edited

Loading

rtwfroody left a comment

YazanHussnain-10x commented Oct 9, 2024

rtwfroody commented Oct 9, 2024

rtwfroody left a comment

YazanHussnain-10x commented Oct 10, 2024 •

edited

Loading

YazanHussnain-10x commented Oct 10, 2024

rtwfroody commented Oct 28, 2024

arichardson commented Oct 28, 2024

pdonahue-ventana left a comment

rtwfroody left a comment

rtwfroody left a comment

tomaird commented Nov 8, 2024

YazanHussnain-10x commented Nov 8, 2024 •

edited

Loading

github-actions bot commented Nov 22, 2024 •

edited

Loading

YazanHussnain-10x commented Dec 18, 2024

Timmmm commented Dec 24, 2024

YazanHussnain-10x commented Dec 26, 2024

jrtc27 left a comment

jrtc27 Jan 7, 2025

jrtc27 Jan 7, 2025

jrtc27 Jan 7, 2025

jrtc27 Jan 7, 2025

jrtc27 Jan 7, 2025

jrtc27 Jan 7, 2025

jrtc27 Jan 7, 2025

jrtc27 Jan 7, 2025

jrtc27 Jan 7, 2025

jrtc27 Jan 7, 2025

jrtc27 Jan 7, 2025

Add Debug Native Trigger Support #573

Are you sure you want to change the base?

Add Debug Native Trigger Support #573

Conversation

YazanHussnain-10x commented Oct 4, 2024 • edited Loading

Debug native trigger support

Implemented Functions

Files Changed

rtwfroody left a comment

Choose a reason for hiding this comment

YazanHussnain-10x commented Oct 9, 2024

rtwfroody commented Oct 9, 2024

rtwfroody left a comment

Choose a reason for hiding this comment

YazanHussnain-10x commented Oct 10, 2024 • edited Loading

YazanHussnain-10x commented Oct 10, 2024

rtwfroody commented Oct 28, 2024

arichardson commented Oct 28, 2024

pdonahue-ventana left a comment

Choose a reason for hiding this comment

rtwfroody left a comment

Choose a reason for hiding this comment

rtwfroody left a comment

Choose a reason for hiding this comment

tomaird commented Nov 8, 2024

YazanHussnain-10x commented Nov 8, 2024 • edited Loading

github-actions bot commented Nov 22, 2024 • edited Loading

Test Results

YazanHussnain-10x commented Dec 18, 2024

Timmmm commented Dec 24, 2024

YazanHussnain-10x commented Dec 26, 2024

jrtc27 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YazanHussnain-10x commented Oct 4, 2024 •

edited

Loading

YazanHussnain-10x commented Oct 10, 2024 •

edited

Loading

YazanHussnain-10x commented Nov 8, 2024 •

edited

Loading

github-actions bot commented Nov 22, 2024 •

edited

Loading