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 missing mstatus.MPP legalization #397

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

ved-rivos
Copy link
Contributor

mstatus.MPP legal values are User (00) (if U-mode implemented), Supervisor (01) (if S-mode implemented) and Machine (11). Encoding 10 is illegal.

model/riscv_sys_regs.sail Outdated Show resolved Hide resolved
model/riscv_sys_regs.sail Outdated Show resolved Hide resolved
model/riscv_sys_regs.sail Outdated Show resolved Hide resolved
model/riscv_sys_regs.sail Outdated Show resolved Hide resolved
@nwf-msr
Copy link

nwf-msr commented Feb 5, 2024

Something also needs to happen at initialization, I think? Right now

function init_sys() -> unit = {
leaves mstatus[MPP] as 0b00, even if that's not a legal value (say, on a M-mode-only core).

@ved-rivos
Copy link
Contributor Author

Something also needs to happen at initialization, I think? Right now

function init_sys() -> unit = {

leaves mstatus[MPP] as 0b00, even if that's not a legal value (say, on a M-mode-only core).

Agree. This was missed. I have committed an update to fix this.

@ved-rivos ved-rivos force-pushed the mpp_legalization branch 2 times, most recently from 19056d8 to ac841b8 Compare February 6, 2024 23:39
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.

LGTM. I slightly prefer using an if/else instead of tuple match (since the first element is only needed for one case), but happy either way.

  if not(haveUsrMode()) then privLevel_to_bits(Machine)
  else match p {
    0b01 => if haveSupMode() then p else privLevel_to_bits(User),
    0b10 => privLevel_to_bits(if haveSupMode() then Supervisor else Machine),
    _ => p
  }

nwf pushed a commit to CHERIoT-Platform/sail-riscv that referenced this pull request Feb 12, 2024
This goes away when riscv#397 lands upstream
nwf pushed a commit to CHERIoT-Platform/sail-riscv that referenced this pull request Feb 12, 2024
This goes away when riscv#397 lands upstream
@Timmmm
Copy link
Collaborator

Timmmm commented Feb 26, 2024

We also came across this, so... alternative implementation, which I think is clearer:

function havePrivilegeMode(priv : priv_level) -> bool =
  match priv {
    0b00 => haveUsrMode(),
    0b01 => haveSupMode(),
    0b10 => false,
    0b11 => true,
  }

// Note, you cannot have Supervisor mode without User mode.
function lowestSupportedPrivilegeMode() -> Privilege =
  if haveUsrMode() then User else Machine

...
  let m = [m with MPP=if havePrivilegeMode(m[MPP]) then m[MPP]
                      else privLevel_to_bits(lowestSupportedPrivilegeMode())];

@ved-rivos
Copy link
Contributor Author

We also came across this, so... alternative implementation, which I think is clearer:

This is nicer. Updated to this.

Copy link

Test Results

712 tests  ±0   712 ✅ ±0   0s ⏱️ ±0s
  6 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 6ef392e. ± Comparison against base commit 9602e3a.

Copy link
Collaborator

@Timmmm Timmmm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@Timmmm Timmmm added the tgmm-agenda Tagged for the next Golden Model meeting agenda. label May 20, 2024
@Timmmm
Copy link
Collaborator

Timmmm commented May 21, 2024

@billmcspadden-riscv We should merge this fix IMO. It's been a few months and there are no issues with it.

@Timmmm
Copy link
Collaborator

Timmmm commented Jun 14, 2024

I'll merge this next week if nobody objects.

@Timmmm Timmmm merged commit 157d4d1 into riscv:master Jun 18, 2024
2 checks passed
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.

5 participants