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

Refactor chip reset #597

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

Refactor chip reset #597

wants to merge 1 commit into from

Conversation

Timmmm
Copy link
Collaborator

@Timmmm Timmmm commented Oct 15, 2024

This simplifies, clarifies and fixes the reset functionality.

Until now the model conflates reset and initialisation, and does way more than it should on reset. The RISC-V spec only requires a very small number of things to be reset.

This change:

  1. Renames the init functions to reset, to clarify that they correspond to resetting the chip.
  2. Removes the ext_init and ext_rvfi_init functions. The latter is not used and the former is only used by the old CHERI code.
  3. Removes the reset of the X and F registers. These are non-reset.
  4. Removes the reset of various CSRs that are non-reset (mip, mie, mideleg, mtvec, mepc, etc).
  5. Adds reset of mstatus[MIE] and mstatus[MPRV]. As far as I can see they were missing.
  6. Add one-time init of mhartid etc to 0.

I didn't remove the vector register resets yet. That needs a bigger refactor.

Also note that currently there is no way to actually do a chip reset mid-simulation, but that will be needed eventually.

Copy link

github-actions bot commented Oct 15, 2024

Test Results

396 tests  ±0   396 ✅ ±0   0s ⏱️ ±0s
  4 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit baf1374. ± Comparison against base commit 601f3d8.

♻️ This comment has been updated with latest results.

@Alasdair Alasdair self-requested a review December 9, 2024 15:29
model/riscv_sys_control.sail Outdated Show resolved Hide resolved
model/riscv_sys_control.sail Outdated Show resolved Hide resolved
@jrtc27
Copy link
Collaborator

jrtc27 commented Dec 9, 2024

Removes the ext_init and ext_rvfi_init functions. The latter is not used and the former is only used by the old CHERI code.

ext_init should remain (possibly renamed). Our sail-cheri-riscv uses it so this would block us updating if we need to keep it going. But, more importantly, we shouldn't delete hooks just because one possible user isn't using them. Any hook that has a real use case is supposed to be there, it's just the only use case so far is sail-cheri-riscv and thus the hooks that exist are the ones that it needed.

@jrtc27
Copy link
Collaborator

jrtc27 commented Dec 9, 2024

Removes the ext_init and ext_rvfi_init functions. The latter is not used and the former is only used by the old CHERI code.

ext_init should remain (possibly renamed). Our sail-cheri-riscv uses it so this would block us updating if we need to keep it going. But, more importantly, we shouldn't delete hooks just because one possible user isn't using them. Any hook that has a real use case is supposed to be there, it's just the only use case so far is sail-cheri-riscv and thus the hooks that exist are the ones that it needed.

Also, the latter is very much used:

https://github.com/CTSRD-CHERI/sail-cheri-riscv/blob/c93d5eff4132fd52ddad32020cedae93573bfa29/src/cheri_regs.sail#L156-L253

and still used (renamed) in the model for the proposed standard version:

https://github.com/CHERI-Alliance/sail-cheri-riscv/blob/a1ce8f9bebd3397596e683fb90e62e6bbcb8b925/src/cheri_regs.sail#L100-L138

Copy link
Collaborator

@jrtc27 jrtc27 left a comment

Choose a reason for hiding this comment

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

ext_(rvfi_)init should stay as ext_(rvfi_)reset

@jrtc27
Copy link
Collaborator

jrtc27 commented Dec 9, 2024

Removes the reset of the X and F registers. These are non-reset.
Removes the reset of various CSRs that are non-reset (mip, mie, mideleg, mtvec, mepc, etc).

They should be initialised for RVFI though.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Dec 10, 2024

Ah yes you're quite right about the ext_ functions missing. I renamed it to ext_reset here - I'll add that back in.

Our sail-cheri-riscv uses it so this would block us updating if we need to keep it going. But, more importantly, we shouldn't delete hooks just because one possible user isn't using them. Any hook that has a real use case is supposed to be there, it's just the only use case so far is sail-cheri-riscv and thus the hooks that exist are the ones that it needed.

Yeah I'm not exactly sure what the story is on these ext_ hooks and what happens to the CHERI repos. It seems like it's making good progress towards ratification and then it would make sense for it to just be directly in the model. We probably still want to keep its code out of the way of people who don't care about CHERI I guess though. Do you know of anyone else using these hooks?

Could we discuss it in the CHERI Tech chat on Thursday?

They should be initialised for RVFI though.

The generated C code still initialises them to 0 when the model is initialised by model_init() (which is also called when RVFI restarts its stimulus). The difference is that if you do a mid-simulation reset then their values will no longer be changed.

Currently there isn't really an interface to do that, but we do it internally via our lock-step verif system. When the real DUT gets reset, we reset the Sail model by calling sys_reset(). We also have an implementation-specific ext_reset() that does the extra reset that our chip does that isn't required by the standard.

@Timmmm Timmmm self-assigned this Dec 16, 2024
@Timmmm Timmmm requested review from jrtc27 and jordancarlin January 2, 2025 10:49
@jordancarlin
Copy link
Collaborator

@Timmmm I still see the ext_init functions being removed and don't see the replacement ext_reset being added. I'll wait to review in detail again until that is updated.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Jan 3, 2025

Oops totally forgot about that. I've added ext_reset.

@arichardson
Copy link
Collaborator

Not sure why but ci is unhappy:

Type error:
model/riscv_step.sail:132.2-11:
132 |  ext_reset();
    |  ^-------^
    | Function ext_reset not found

@Timmmm
Copy link
Collaborator Author

Timmmm commented Jan 6, 2025

Oops I forgot about RVFI. Should pass now.

Copy link
Collaborator

@arichardson arichardson left a comment

Choose a reason for hiding this comment

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

Overall this looks good, but as far as I can tell this will break the RVFI support. It should reset registers to all zero so you have a reproducible starting point for divergence testing.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Jan 6, 2025

I'm pretty sure that reset is done automatically by model_init() which is called between RVFI sessions. I'll double check though.

@arichardson
Copy link
Collaborator

I'm pretty sure that reset is done automatically by model_init() which is called between RVFI sessions. I'll double check though.

Ah yes it does call model_init in the rvfi loop. Does this re-run all sail global initializers? If so then please ignore my comment. LGTM.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Jan 9, 2025

Ah yes it does call model_init in the rvfi loop. Does this re-run all sail global initializers?

Yep it calls startup_zinitializze_registers() which is

void startup_zinitializze_registers(void)
{
...
  {
    zx1 = UINT64_C(0x0000000000000000);
    unit zgsz317884;
    zgsz317884 = UNIT;
  }
  {
    zx2 = UINT64_C(0x0000000000000000);
    unit zgsz317883;
    zgsz317883 = UNIT;
  }
  {
    zx3 = UINT64_C(0x0000000000000000);
    unit zgsz317882;
    zgsz317882 = UNIT;
  }
...

@Timmmm Timmmm added the tgmm-agenda Tagged for the next Golden Model meeting agenda. label Jan 9, 2025
This simplifies, clarifies and fixes the reset functionality.

Until now the model conflates reset and initialisation, and does way more than it should on reset. The RISC-V spec only requires a very small number of things to be reset.

This change:

1. Renames the `init` functions to `reset`, to clarify that they correspond to resetting the chip.
2. Removes the `ext_init` and `ext_rvfi_init` functions. The latter is not used and the former is only used by the old CHERI code.
2. Removes the reset of the X and F registers. These are non-reset.
3. Removes the reset of various CSRs that are non-reset (`mip`, `mie`, `mideleg`, `mtvec`, `mepc`, etc).
4. Adds reset of `mstatus[MIE]` and `mstatus[MPRV]`. As far as I can see they were missing.
5. Add one-time init of `mhartid` etc to 0.

I didn't remove the vector register resets yet. That needs a bigger refactor.

Also note that currently there is no way to actually do a chip reset mid-simulation, but that will be needed eventually.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tgmm-agenda Tagged for the next Golden Model meeting agenda.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants