Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for Smrnmi extension #431

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

ved-rivos
Copy link
Contributor

@ved-rivos ved-rivos commented Jan 27, 2024

Description

Add test for Smrnmi extension

Related Issues

N/A

Ratified/Unratified Extensions

  • Ratified
  • Unratified

List Extensions

https://github.com/riscv/riscv-isa-manual/blob/main/src/rnmi.adoc

Reference Model Used

  • SAIL
  • Spike
  • Other - < SPECIFY HERE >

Mandatory Checklist:

  • All tests are compliant with the test-format spec present in this repo ?
  • Ran the new tests on RISCOF with SAIL/Spike as reference model successfully ?
  • Ran the new tests on RISCOF in coverage mode
  • Link to Google-Drive folder containing the new coverage reports (See this for more info): < SPECIFY HERE >
  • Link to PR in RISCV-ISAC from which the reports were generated : < SPECIFY HERE >
  • Changelog entry created with a minor patch

Optional Checklist:

  • RISCV-V CTG PR link if tests were generated using it : < SPECIFY HERE >
  • Were the tests hand-written/modified ?
  • Have you run these on any hard DUT model ? Please specify name and provide link if possible in the description
  • If you have modified arch_test.h Please provide a detailed description of the changes in the Description section above.

Copy link
Collaborator

@allenjbaum allenjbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, this doesn't follow the structure of any other tests I've seen (e.g. doesn't use SIGUPD to update the signature, etc and doesn't use the standard trap handler (which make sense because this defines a new priv level for handler, though we could augment it to do that reasonably easy, especially if MPRV is ignored)). It isn't clear from the spec how custom the encodings of cause are, or if they are exactly the same as all the other priv levels

@ved-rivos
Copy link
Contributor Author

In general, this doesn't follow the structure of any other tests I've seen (e.g. doesn't use SIGUPD to update the signature, etc

Please see ecall.S and ebreak.S - this test follows that Privileged test structure. The trap handlers in the arch test also do not use SIGUPD (since they are not sequential and cannot use a redefined .offset macro).

and doesn't use the standard trap handler (which make sense because this defines a new priv level for handler, though we could augment it to do that reasonably easy, especially if MPRV is ignored)). It isn't clear from the spec how custom the encodings of cause are, or if they are exactly the same as all the other priv levels

The cause encodings are not specified.

@ved-rivos
Copy link
Contributor Author

I have updated the tests to use the SIGUPD macros and to use the common exception trap handler.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Jan 28, 2024 via email

@ved-rivos
Copy link
Contributor Author

ved-rivos commented Jan 28, 2024

You are right that we cannot have an ACT for the NMI trap handler itself because there is no architecturally defined way to set it up. There is no architecturally defined way to cause an NMI either. Since such handlers may be embedded in the platform in a manner where they cannot be modified it may not be possible to test for exceptions in the NMI handler. I needed that code to test the Sail model but should not have been part of ACT. I have removed that.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Jan 29, 2024 via email

@ved-rivos
Copy link
Contributor Author

Well, my reading of the spec is you can cause an exception, but not an interrupt, though I'm not completely clear on the details.

If an exception occurs then the exception is vectored to a platform specific address.

What needs to be provided to the model is still the platform-defined interrupt and vector addresses. Eventually we will have an event generator that models can use to stimulate external events (with predictable timing) such as interrupts, and even snoops.

But that is outside the scope of ACT. ACTs cannot rely on testbench functions like event generators.

The individual platforms will need to provide a shim to interface those events to their specific models, just as the need shims to start and stop tests, and to extract signatures.

There is no architecturally defined cause for an NMI. The NMI vectors to a platform specific address. The architecture does not require that the platform specific address be configurable. It also does not require that the NMI handler be modifiable - it can be held in a ROM.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Jan 29, 2024 via email

@ved-rivos
Copy link
Contributor Author

NMI are different from other interrupts in that they represent error conditions. It may not be possible to cause error conditions in silicon. Further the handler may be embedded in a ROM and its address not mutable. Without the RNMI handler address being programmable, a test cannot replace the NMI handler address. Since an NMI handlers address may not be programmable and the NMI handler replaceable with a test handler, it may not be possible to add test code such as causing an exception in the NMI handler code. Further, the exceptions when NMIE=0 vector to a platform specific address and such exception handlers may also be hardwired in the ROM - so replacing them with the test handler in arch_test.h may not be feasible.

@jamesbeyond jamesbeyond changed the base branch from main to dev May 28, 2024 16:54
@allenjbaum
Copy link
Collaborator

allenjbaum commented Jun 13, 2024 via email

@ved-rivos
Copy link
Contributor Author

NMI may represent error conditions, but if RTL can't assert an error condition in some way - then it can't be tested by the vendor either. If the vendor asserts that it supports NMI, we would require a callback to assert the NMI as well

Thats not always true - especially for products that are not soft IPs. NMIs may be triggered in the platform by sources such as thermal diodes when they detect a catastrophic thermal excursion impending. Other reasons for a NMI getting triggered may be things like the DDR command/data parity errors to request the NMI handler which runs out of ROM and special SRAM and hence not affected by a DDR failure to be able to do a failover to a mirror DIMM. Many cores have tons of DFT features - including ability to inject hardware errors that trigger NMI - using DFT hooks. But these DFT hooks are not exposed to general software - including M-mode software and using these facilities usually needs silicon vendor level debug authorization.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Jun 14, 2024 via email

@ved-rivos ved-rivos changed the title Add tests for unratified Smrnmi extension Add tests for Smrnmi extension Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants