Sdtrig support

[Mudassir10X]

This PR is for the support of sdtrig extension as proof of concept for triggers support in SAIL-RISCV. It is still incomplete and work is being actively done. reviewers and contributors are welcomed.

This PR is related to this issue in RISCV dev partners.

[martinberger]

Thanks for starting to think about debug extensions. Could you say what the plans are for developing Sdtrig? The current draft is very far from complete.

[Mudassir10X]

Yes I am currently working on it thats why its a draft PR. I am trying to complete this ASAP. Will keep you posted about progress.

[github-actions[bot]]

Test Results

712 tests  ±0   712 :white_check_mark: ±0   0s :stopwatch: ±0s   6 suites ±0     0 :zzz: ±0    1 files   ±0     0 :x: ±0 

Results for commit c98efb8b. ± Comparison against base commit 17113857.

:recycle: This comment has been updated with latest results.

[Mudassir10X]

Hi @billmcspadden-riscv, this PR is failing the CI for breakpoint test in riscv test. The way I see it, when the sdtrig registers were absent, the access would just be ignored as there was no register at tdata1 and go on. but as now the register is present and is writable in debug mode only (which is still not present in SAIL-RISCV), so it gives illegal instruction exception.

[jrtc27]

Hi @billmcspadden-riscv, this PR is failing the CI for breakpoint test in riscv test. The way I see it, when the sdtrig registers were absent, the access would just be ignored as there was no register at tdata1 and go on. but as now the register is present and is writable in debug mode only (which is still not present in SAIL-RISCV), so it gives illegal instruction exception.

Either the test is wrong or your implementation is wrong. Which is it?

[martinberger]

Either the test is wrong or your implementation is wrong. Which is it?

At a guess, the test is now wrong, because the expected outcome is "access failure" because the test assumes the register is not there. That has changed. So either remove the failing tests, or change their expected outcome.

@Mudassir10X Could you show us the source of the failing tests?

[allenjbaum]

Is this the problem with the new definition of what happens when you access an unimplemented CSR (that it basically becomes undefined).]? From an architectural test point of view, that means that if you can't discover whether a CSR exists (either programmatically e.g. via something like unified discovery, or passing arguments into the test - which will eventually be the riscv-config) then you have to skip testing the feature. Changing the test won't work. If Sail assumes that this CSR is not present, and is now optionally present, and we have to way to test whether it is present, then it is untestable.

On Fri, Jun 7, 2024 at 11:51 PM Martin Berger @.***> wrote:

Either the test is wrong or your implementation is wrong. Which is it?

At a guess, the test is now wrong, because the expected outcome is that the expected outcome is an access failure because the test assumes the register is not there. That has changed. So either remove those 4 tests, or change the expected outcome.

— Reply to this email directly, view it on GitHub https://github.com/riscv/sail-riscv/pull/486#issuecomment-2155842426, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHPXVJQOYX3S5IUA23546STZGKSWFAVCNFSM6AAAAABIFEZBA6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJVHA2DENBSGY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[UmerShahidengr]

There can be 2 ways to solve this problem.

1. If the test is just checking if some CSR is defined or not, and the passing criteria is "access failure", then test is needed to be modified to change the passing criteria. The test should be considered "pass" if CSR is not defined and giving "access failure" or if CSR is defined then legal read/write should be considered as "pass".
2. Instead of checking if a certain CSR is defined or not, a test should check the valid functionality of that CSR and a flag should be added which chooses the test to be run based on the architecture state (or riscv-config string). Test will be selected only if a certain CSR is defined and accessible but if the CSR is undefined then that test should not be selected.

PS: From ACT point of view, option 2 is viable because if a CSR is not defined then you should not test any feature related to that CSR. But this case in more interesting one. In this case, a CSR (tdata1) has been defined but it is not accessible as Debug mode is not available in Sail, so now there is a CSR whose definition and mapping is available in Sail but you cant read/write it in a test because there is no legal way to access it without Debug mode. In this case, option 1 can be chosen as a short term solution.

[allenjbaum]

I want to echo what Umer said: there is a difference between an unimplemented CSR and an inaccessible CSR. The spec used to say that accessing unimplemented CSR would cause an illegal instruction trap - but that has been loosened to say that the result is Reserved. That has two implications:

• tests must be passed a variable that indicates whether a CSR is implemented or not (as a command line option, extracted from the riscv-config YAML file)
• tests that access a CSR must be conditioned on that variable as part of the TEST_CASE macro.

It is still just fine to test accessbility (and we should) under conditions that explore both allowed and disallowed access ( which could be a function of priv mode, or by type of access (read or write), or a CSR enable/disable bit)

On Tue, Jun 11, 2024 at 4:29 AM Umer Shahid @.***> wrote:

There can be 2 ways to solve this problem.

1. If the test is just checking if some CSR is defined or not, and the passing criteria is "access failure", then test is needed to be modified to change the passing criteria. The test should be considered "pass" if CSR is not defined and giving "access failure" or if CSR is defined then legal read/write should be considered as "pass".
2. Instead of checking if a certain CSR is defined or not, a test should check the valid functionality of that CSR and a flag should be added which chooses the test to be run based on the architecture state (or riscv-config string). Test will be selected only if a certain CSR is defined and accessible but if the CSR is undefined then that test should not be selected.

PS: From ACT point of view, option 2 is viable because if a CSR is not defined then you should not test any feature related to that CSR. But this case in more interesting one. In this case, a CSR (tdata1) has been defined but it is not accessible as Debug mode is not available in Sail, so now there is a CSR whose definition and mapping is available in Sail but you cant read/write it in a test because there is no legal way to access it without Debug mode. In this case, option 1 can be chosen as a short term solution.

— Reply to this email directly, view it on GitHub https://github.com/riscv/sail-riscv/pull/486#issuecomment-2160508336, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHPXVJQQRX6H3F4IDXRAXRTZG3NQPAVCNFSM6AAAAABIFEZBA6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRQGUYDQMZTGY . You are receiving this because you commented.Message ID: @.***>

[UmerShahidengr]

@Mudassir10X can you please close this one, since new PR has been added.

[billmcspadden-riscv]

This PR has been replaced by PR #573