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

Various issues around clearing the bottom bits of xepc #593

Closed
Timmmm opened this issue Oct 14, 2024 · 1 comment
Closed

Various issues around clearing the bottom bits of xepc #593

Timmmm opened this issue Oct 14, 2024 · 1 comment

Comments

@Timmmm
Copy link
Collaborator

Timmmm commented Oct 14, 2024

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
Copy link
Collaborator

rmn30 commented Nov 19, 2024

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.

@Timmmm Timmmm closed this as completed Dec 18, 2024
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

No branches or pull requests

2 participants