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 initial Sail snippets #1838

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

Add initial Sail snippets #1838

wants to merge 1 commit into from

Conversation

Timmmm
Copy link
Contributor

@Timmmm Timmmm commented Jan 31, 2025

Add Makefile targets to download the Sail JSON bundle from Github releases (it's a few MB so this avoids bloating the repo).

I updated a few scalar crypto functions that already had copy & pasted Sail code to use the model source instead. The result is almost identical.

See #1369.

Add Makefile targets to download the Sail JSON bundle from Github releases (it's a few MB so this avoids bloating the repo).

I updated a few scalar crypto functions that already had copy & pasted Sail code to use the model source instead. The result is almost identical.
@Timmmm
Copy link
Contributor Author

Timmmm commented Jan 31, 2025

Note, @Alasdair has demonstrated a fancier version of this with syntax highlighting and hyperlinked code (e.g. so you can click on <<< to see what it means), but that isn't used here yet.

Before:

image

After:

image

(The Sail code has changed a little since that was copy/pasted.)

@wmat
Copy link
Collaborator

wmat commented Feb 3, 2025

Question, do we need to care about the version of the SAIL model we're pulling the index from? I recognize SAIL isn't at v.1.0 yet so perhaps at this point we don't, but in the future should we? Or perhaps we pair a SAIL version with the riscv-isa-manual src when we build a release for publication?

@Timmmm
Copy link
Contributor Author

Timmmm commented Feb 3, 2025

There are a couple of aspects to that:

  1. We obviously want to point to a fixed version so it is repeatable. We don't want to just always use master. That is covered here by the URL being version tracked in Git. We should maybe include the Git hash of the sail-riscv repo, or maybe the hash of the JSON file just in case (a URL can change or disappear). I don't think that is a blocker though.
  2. The spec sometimes changes behaviour - sometimes in backwards incompatible ways - and currently the sail-riscv repo only implements one version. We obviously want the Sail code's behaviour to match the version of the ISA manual it is included in.

For the second point my vague plan was:

  1. Finish adding a way to actually specify the spec versions to the Sail model. I made an attempt here but it needs to be reworked.
  2. Once that is done, use Alasdair's fancy Sail-rewriting system to generate the code for the "current" spec version. Basically you can take code that is like:
if version == 1.12
   foo
else if version == 1.13
   bar
...

and you say "yeah but I know the version is 1.12", and then it will only insert foo.

I don't think Alasdair has published that yet so I'm not exactly sure what you have to do but it will probably end up with us having separate .json bundles for each version.

However, I also don't think that is a blocker. I would suggest initially we just use it for snippets where the behaviour has never changed and is unlikely to change in future.

@wmat
Copy link
Collaborator

wmat commented Feb 3, 2025

Excellent, thanks Tim, I mostly wanted to be sure the versioning question was on the table for discussion. So thanks for confirming to me that it is indeed out there.

@nadime15
Copy link

I'm curious what parts of sail are actually supposed to be integrated into the risc-v manual? Are we just talking about the execution function of each instruction?

@Timmmm
Copy link
Contributor Author

Timmmm commented Feb 14, 2025

Mostly the execute clauses, but I think there are other bits too where the behaviour would be much clearer with the Sail code, e.g. interrupt handling:


/* Interrupts are prioritized in privilege order, and for each
 * privilege, in the order: external, software, timers.
 */
function findPendingInterrupt(ip : xlenbits) -> option(InterruptType) = {
  let ip = Mk_Minterrupts(ip);
  if      ip[MEI] == 0b1 then Some(I_M_External)
  else if ip[MSI] == 0b1 then Some(I_M_Software)
  else if ip[MTI] == 0b1 then Some(I_M_Timer)
  else if ip[SEI] == 0b1 then Some(I_S_External)
  else if ip[SSI] == 0b1 then Some(I_S_Software)
  else if ip[STI] == 0b1 then Some(I_S_Timer)
  else                        None()
}

/* Given the current privilege level, return the pending set
 * of interrupts for the highest privilege that has any pending.
 *
 * We don't use the lowered views of {xie,xip} here, since the spec
 * allows for example the M_Timer to be delegated to the S-mode.
 */
function getPendingSet(priv : Privilege) -> option((xlenbits, Privilege)) = {
  // mideleg can only be non-zero if we support Supervisor mode.
  assert(extensionEnabled(Ext_S) | mideleg.bits == zeros());

  let pending_m = mip.bits & mie.bits & ~(mideleg.bits);
  let pending_s = mip.bits & mie.bits & mideleg.bits;

  let mIE = (priv == Machine    & mstatus[MIE] == 0b1) | priv == Supervisor | priv == User;
  let sIE = (priv == Supervisor & mstatus[SIE] == 0b1) | priv == User;

  if      mIE & (pending_m != zeros()) then Some((pending_m, Machine))
  else if sIE & (pending_s != zeros()) then Some((pending_s, Supervisor))
  else None()
}

/* Examine the current interrupt state and return an interrupt to be *
 * handled (if any), and the privilege it should be handled at.
 */
function dispatchInterrupt(priv : Privilege) -> option((InterruptType, Privilege)) = {
  match getPendingSet(priv) {
    None()       => None(),
    Some(ip, p)  => match findPendingInterrupt(ip) {
                      None()  => None(),
                      Some(i) => Some((i, p)),
                    }
  }
}

(And I think this code could be improved a fair bit too before we include it, e.g. getPendingSet() could be inlined, not having variables called i etc.)

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.

3 participants