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

fix inconsistencies on xnxti CSR #438

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hirooih
Copy link

@hirooih hirooih commented Jan 5, 2025

Regarding the definition of xnxti CSR, several inconsistencies have been pointed out in #395, #415, #433, and #434. This PR attempts to resolve them.


In addition to this PR, additional explanations are needed for the following points.

  • Why the csrrs instruction compares clic.level with rs1[23:16] instead of mcause.mpil.
  • Why the csrrs instruction is permitted but not csrrc instruction.
  • Usecases of the csrrs instruction for this CSR.

I removed the following comment after each pseudo code.

When a different CSR instruction is used, the update of mstatus and the test
for whether side-effects should occur are modified accordingly.

This conflicts with the following sentence. (Using different CSR is reserved.)

Accessing the {mnxti} CSR using any other CSR instruction (i.e., CSRRW, CSRRC, or CSRRWI) is reserved.

Regarding the definition of xnxti CSR, several inconsistencies have been pointed out in riscv#395, riscv#415, riscv#433, and riscv#434.
This PR attempts to resolve them.

Signed-off-by: Hiroo HAYASHI <[email protected]>
if (clic.priv==M && clic.level > rs1[23:16] && clic.level > mintthresh.th) {
// There is an available interrupt.
if (rs1[4:0] != 0 && rs1 != x0) { // Side-effects should occur.
if (rs1 != x0) { // Side-effects should occur.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This changes can t be accepted since it is changing the initial behavior.

Currently I m starting a thread of mail with ARC to better understand what we should do.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your response.

This changes can t be accepted since it is changing the initial behavior.

This PR does not propose a change to the specifications.

We have to change either the pseudo code or the following descriptions.

If the CSR instruction that accesses mnxti includes a write, ...

Following the usual convention for CSR instructions, if the CSR instruction does not include write side effects (e.g., csrr t0, mnxti), then no state update on any CSR occurs.

Currently I m starting a thread of mail with ARC to better understand what we should do.

Thank you!


On the other hand, #399, I opened last July, requires changing the behavior. The current specification simply does not work (If I understand correctly).

Copy link
Collaborator

Choose a reason for hiding this comment

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

A thread of mail has been started to understand the best way to proceed with NXTI.
I ll keep you updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants