Add Debug Native Trigger Support

[YazanHussnain-10x]

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

1. riscv_csr_begin.sail: Created CSR map for tinfo and tcontrol.
2. riscv_types.sail: Defined mappings for Trigger Type and Match Options.
3. riscv_sys_regs.sail: Defined CSR read/write and legalization functions.
4. riscv_sys_control.sail: Defined trigger match and trigger firing check functions.
5. riscv_insts_zicsr.sail: Added read/write CSR definitions for the implemented CSRs.
6. riscv_step.sail: Called instruction count, instruction match, and check trigger firing functions.
7. riscv_insts_base.sail: Checked for Data Match trigger on Load/Store instructions.
8. riscv_insts_aext.sail: Checked for Data Match trigger on Load/Store instructions.
9. 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.

[YazanHussnain-10x]

@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]

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...

[YazanHussnain-10x]

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:

1. ext_data_get_addr: Used to get the address (rs1 + imm).
2. 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]

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

No worries at all

[rtwfroody]

@pmundkur Can you approve the workflow for this PR?

[arichardson]

@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

[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.

[rtwfroody]

Still working through this, slowly...

[rtwfroody]

The rest of this looks OK. Thank you.

[tomaird]

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]

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]

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]

@Timmmm Any update on this?

[Timmmm]

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]

@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.