Draft : Added test cases for load address match trigger in sdtrig extension

[anuani21]

Description

-  Handwritten test cases for mcontrol6 trigger using tcontrol register.
-  Test cases for mcontrol6 address  match trigger for load operation with different set of match values(0,1,2,3,4,5).  

Related Issues

NA

Ratified/Unratified Extensions

• Unratified

List Extensions

• Sdtrig

Reference Model Used

• Spike

Mandatory Checklist:

Ran reports are placed here

https://gitlab.com/ptprasanna/actreports/-/tree/main/Sdtrig/RV32/Load_Address_match_trigger?ref_type=heads

[rtwfroody]

I've never written anything for this framework, so some of my comments might stem from lack of understanding. Is there a document that shows how test cases should adapt to different hardware configurations?

[rtwfroody]

Why do you need to set MPP? I don't think this test ever returns from an interrupt handler. Or does one of the macros do that?

[rtwfroody]

What is the expected behavior of these tests when they don't execute in M-Mode? Is there some documentation on the arch tests that talks about this? It seems reasonable to me to not run this test if you don't get to use M-Mode, but if that's an intentional choice the comment should reflect that.

[rtwfroody]

Why read this back?

[rtwfroody]

Suggested change

	li t3,0
	csrw tselect,t3
	csrw tselect, zero

[rtwfroody]

This will make the test fail if the DUT and reference don't implement the identical set of trigger types for this trigger. That might make this test break under conditions that have little to do with mcontrol6 load triggers.

[rtwfroody]

tcontrol isn't required to be implemented.

There are several possible solutions to this, but just assuming it is implemented doesn't seem right.

[rtwfroody]

Section 5.7 of the Debug Spec says:
As a result, a debugger can write any supported trigger as follows:

1. Write 0 to tdata1. (This will result in containing a non-zero value, since the register is WARL.)
2. Write desired values to tdata2 and tdata3.
3. Write desired value to tdata1.

[rtwfroody]

Does this comment apply here?

[rtwfroody]

This falls through to the trap handler if the trigger didn't fire. Not the end of the world because mepc will have the wrong value, but it's unusual. Deserves a comment at least.

[allenjbaum]

I'll have to look at this, but there's a lot that doesn't seem right. The normal reason for writing MPP is generally to return to a different priv mode. This is much trickier than it appears, because if VM is enabled, you'll return to the address in MEPC which will be in a different translation mode than Mmode (usually), The standard trap handler deals with all the complexity setup by the test if you define Mtraproutine and Straproutine. It sets up mtvec/stvec to point to the handler (if possible; xTEVC is WARL so you don't know if the value you want to write is legal) - but if it isn't, the handler will swap out the code that xTVEC points to, and will swap it back in when it returns from the test) It also sets up identity mapped page tables (it assumes 1:1) but there are macros for easily defining different ones (e.g. offset from 1:1) The test will need to initialize the code begin, data begin, and signature begin if different than bare mode Finally, there are predefined macros that will let tests transition from M->lower mode and lower mode->Mmode seamlessly (assuming you've set the begin/end variables correctly). It also takes care of ensuring the signature values are independent of where the code is actually placed by the linker scripts. The trap handler normally returns to the next doubleword offset following the instruction that traps so code can take different paths depending on whether it trapped or not, so if you don't care, always put two noops after an instruction that might trap (whether it is supposed or not)

…

On Fri, Sep 6, 2024 at 12:14 PM Tim Newsome ***@***.***> wrote: ***@***.**** requested changes on this pull request. I've never written anything for this framework, so some of my comments might stem from lack of understanding. Is there a document that shows how test cases should adapt to different hardware configurations? ------------------------------ In riscv-test-suite/rv32i_m/sdtrig/src/load_match0.S <#487 (comment)> : > + csrr t1, mstatus # Read current mstatus + li t0, ~(3 << 11) # Create mask to clear mpp bits (bits 11 and 12) + and t1, t1, t0 # Clear the mpp bits + li t0, (3 << 11) # Prepare value to set mpp to 3 (Machine Mode) + or t1, t1, t0 # Set the mpp bits to 3 + csrw mstatus, t1 # Write back modified mstatus Why do you need to set MPP? I don't think this test ever returns from an interrupt handler. Or does one of the macros do that? ------------------------------ In riscv-test-suite/rv32i_m/sdtrig/src/load_match0.S <#487 (comment)> : > + # Not in Machine mode, handle accordingly (e.g., trap, ecall, etc.) + li a0, 1 # Example action if not in Machine mode + ret What is the expected behavior of these tests when they don't execute in M-Mode? Is there some documentation on the arch tests that talks about this? It seems reasonable to me to not run this test if you don't get to use M-Mode, but if that's an intentional choice the comment should reflect that. ------------------------------ In riscv-test-suite/rv32i_m/sdtrig/src/load_match0.S <#487 (comment)> : > + csrw mstatus, t1 # Write back modified mstatus + RVTEST_SIGUPD(x1,t1) + + csrr t0, mstatus # Read mstatus register + srli t1, t0, 11 # Shift right to get MPP field + andi t1, t1, 3 # Mask to get the MPP value + li t2, 3 # Load value 3 (Machine mode) into t2 + beq t1, t2, setup_trap # If in Machine mode, jump to setup_trap + + # Not in Machine mode, handle accordingly (e.g., trap, ecall, etc.) + li a0, 1 # Example action if not in Machine mode + ret +setup_trap: + la a0, trap_handler + csrw mtvec, a0 + csrr t1, mtvec Why read this back? ------------------------------ In riscv-test-suite/rv32i_m/sdtrig/src/load_match0.S <#487 (comment)> : > + li t3,0 + csrw tselect,t3 ⬇️ Suggested change - li t3,0 - csrw tselect,t3 + csrw tselect, zero ------------------------------ In riscv-test-suite/rv32i_m/sdtrig/src/load_match0.S <#487 (comment)> : > + # Read tinfo and append it to the signature + csrr t6, tinfo + RVTEST_SIGUPD(x1, t6) This will make the test fail if the DUT and reference don't implement the identical set of trigger types for this trigger. That might make this test break under conditions that have little to do with mcontrol6 load triggers. ------------------------------ In riscv-test-suite/rv32i_m/sdtrig/src/load_match0.S <#487 (comment)> : > + li a1, (CSR_TCONTROL_MTE_ENABLED << 3) + csrw tcontrol, a1 + csrr a2,tcontrol + RVTEST_SIGUPD(x1,a2) tcontrol isn't required to be implemented. There are several possible solutions to this, but just assuming it is implemented doesn't seem right. ------------------------------ In riscv-test-suite/rv32i_m/sdtrig/src/load_match0.S <#487 (comment)> : > + csrw tdata1,t2 + RVTEST_SIGUPD(x1,t2) + csrr a3,tdata1 + RVTEST_SIGUPD(x1,a3) + + + # Set the value of tdata2 to the address to match + la t2, 0x80001234 + csrw tdata2, t2 + csrr t6,tdata2 + RVTEST_SIGUPD(x1, t6) Section 5.7 of the Debug Spec says: As a result, a debugger can write any supported trigger as follows: 1. Write 0 to tdata1. (This will result in containing a non-zero value, since the register is WARL.) 2. Write desired values to tdata2 and tdata3. 3. Write desired value to tdata1. ------------------------------ In riscv-test-suite/rv32i_m/sdtrig/src/load_match0.S <#487 (comment)> : > + CSR_MCONTROL6_M |\ + CSR_MCONTROL6_LOAD) + csrw tdata1,t2 + RVTEST_SIGUPD(x1,t2) + csrr a3,tdata1 + RVTEST_SIGUPD(x1,a3) + + + # Set the value of tdata2 to the address to match + la t2, 0x80001234 + csrw tdata2, t2 + csrr t6,tdata2 + RVTEST_SIGUPD(x1, t6) + + # Code that accesses the matched data value (this should trigger the break) + # Execute an instruction at the matching address Does this comment apply here? ------------------------------ In riscv-test-suite/rv32i_m/sdtrig/src/load_match0.S <#487 (comment)> : > + RVTEST_SIGUPD(x1,t2) + csrr a3,tdata1 + RVTEST_SIGUPD(x1,a3) + + + # Set the value of tdata2 to the address to match + la t2, 0x80001234 + csrw tdata2, t2 + csrr t6,tdata2 + RVTEST_SIGUPD(x1, t6) + + # Code that accesses the matched data value (this should trigger the break) + # Execute an instruction at the matching address + la t4,0x80001234 + lw t4, 0(t4) # Load to trigger match (this should trigger the break) + This falls through to the trap handler if the trigger didn't fire. Not the end of the world because mepc will have the wrong value, but it's unusual. Deserves a comment at least. — Reply to this email directly, view it on GitHub <#487 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJSFLXM3CUIUB2GAF73ZVH5IPAVCNFSM6AAAAABNRYJTZGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEOBWHE2DANRYG4> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[rtwfroody]

Since these tests test triggers, but not explicitly test native re-entrant issues, I think it's better not to add this to the signature. Then these tests can compare DUTs and references that have different ways of enabling triggers.

But I might be misunderstanding the philosophy behind these tests.

[rtwfroody]

For similar reasons, do we want tinfo in the signature? I think we only care that the specific trigger type being tested is supported.

[rtwfroody]

This is another one. The spec only requires that the trigger is now disabled, but there are many encodings that could represent a disabled register.

[rtwfroody]

Do you need these?

[rtwfroody]

I think you can also delete these.

[rtwfroody]

I don't understand how the GT/LT execute tests work. You're using 0x80001234 for the compare value for both. But the code that sets the breakpoints must be either before or after that address. Wouldn't one of these two scenarios fire before tdata1/tdata2 are added to the signature?

Also, there's a lot of repeated code here. Refactoring things so there's a single macro/function that just takes the desired tdata1 and tdata2 values would make each test tiny (call the function to set up, then execute whatever code to test triggering/not triggering).

[rtwfroody]

Now this isn't used anywhere.

[rtwfroody]

I think this matches when executing an instruction where bit 15 of the address is set. I don't think you're guaranteed what address this program is placed at. Probably you need to not use a fixed address for your test here, but instead pick an address in your code.

E.g. (untested)

la t2, break_here
li t3, 0xffff
and t4, t2, t3
li t3, 0xffff0000
or t4, t3, t4
csrw tdata2, t4
...
j before_break
...
before_break: nop
break_here: nop

[rtwfroody]

This works for me.

[pdonahue-ventana]

I think that this looks like an OK set of tests for basic Sdtrig functionality. It's difficult to test everything because pretty much everything is optional so the possibilities are endless. But I do appreciate that, for instance, the tests that check match=2 (greater than or equal) will execute properly whether match=2 is supported or not (and the signature will indicate whether it's supported which is what really matters).

[anuani21]

@rtwfroody or @pdonahue-ventana,

I had missed to update the icount test cases in the PR for review. Can you please review it and tell me the comments?