riscv / sail-riscv

Sail RISC-V model
https://lists.riscv.org/g/tech-golden-model
Other
463 stars 168 forks source link

Various issues around clearing the bottom bits of xepc #593

Open Timmmm opened 1 month ago

Timmmm commented 1 month ago

Basically we should remove pc_alignment_mask():

function pc_alignment_mask() -> xlenbits =
  ~(zero_extend(if misa[C] == 0b1 then 0b00 else 0b10))

The logic here is kind of hard to follow but it returns 0b1111..1111 if C is enabled, otherwise 0b1111...11101. That is weird. I think we got away with it because in all the places it is used the bottom bit is already cleared, but that isn't necessarily the case when we implement CLIC.

It's also weird that it is applied kind of ad-hoc after reading xepc, rather than just having a get_xepc() function that applies the read legalisation.

There is actually a get_xpec type function get_xret_target(). In our code I've renamed it to get_xepc() (clearer, and for CHERI/CLIC reasons). Then we have

function get_xepc(p : Privilege) -> xlenbits =
  match p {
    Machine    => get_mepc(),
    Supervisor => get_sepc(),
    User       => get_uepc(),
  }

...

function get_mepc() -> xlenbits = align_pc(mepc)  // This gets overridden for CHERI.

...

function align_pc(addr : xlenbits) -> xlenbits =
  if misa[C] == 0b1
  then [addr with 0 = bitzero]
  else [addr with 1..0 = zeros()]

This code also needs fixing:

function legalize_xepc(v : xlenbits) -> xlenbits =
  /* allow writing xepc[1] only if misa.C is enabled or could be enabled
     XXX specification says this legalization should be done on read */
  if   (sys_enable_writable_misa() & sys_enable_rvc()) | misa[C] == 0b1
  then [v with 0 = bitzero]
  else v & sign_extend(0b100)

The XXX comment is talking about this exact same issue. There are two issues:

  1. Weird mix of [with] and &.
  2. sys_enable_writable_misa() doesn't matter here.

It should be:

function legalize_xepc(v : xlenbits) -> xlenbits = {
  // allow writing xepc[1] only if misa.C is enabled or could be enabled.
  if   sys_enable_rvc()
  then [v with 0 = bitzero]
  else [v with 1..0 = zeros()]
}
rmn30 commented 6 days ago

I agree that the calculation of pc_alignment_mask is incorrect but we do still need it.

3.1.14. Machine Exception Program Counter (mepc) mepc is an MXLEN-bit read/write register formatted as shown in Figure 21. The low bit of mepc (mepc[0]) is always zero. On implementations that support only IALIGN=32, the two low bits (mepc[1:0]) are always zero. If an implementation allows IALIGN to be either 16 or 32 (by changing CSR misa, for example), then, whenever IALIGN=32, bit mepc[1] is masked on reads so that it appears to be 0. This masking occurs also for the implicit read by the MRET instruction. Though masked, mepc[1] remains writable when IALIGN=32.

I think it is necessary to implement the bolded part of the text above.