Remove pc alignment mask #618

rez5427 · 2024-11-19T09:23:10Z

Timmmm · 2024-11-19T09:33:01Z

model/riscv_sys_control.sail

@@ -293,7 +293,7 @@ function exception_handler(cur_priv : Privilege, ctl : ctl_result,
      if   get_config_print_platform()
      then print_platform("ret-ing from " ^ to_str(prev_priv) ^ " to " ^ to_str(cur_privilege));

-      prepare_xret_target(Machine) & pc_alignment_mask()
+      legalize_xepc(prepare_xret_target(Machine))


Since the value is already legalized in the write functions (set_xret_target) you don't need to also legalize it on read, so you can remove the legalize_xepcs here, and also for the read_CSR clauses.

As per comment below I think this was correct previously except for the calculation of pc_alignment_mask which should be something like sign_extend(if misa.C then 0b110 else 0b100).

Timmmm · 2024-11-19T09:33:50Z

model/riscv_sys_exceptions.sail

@@ -89,9 +89,9 @@ function clause is_CSR_defined(0x305) = true // mtvec
 function clause is_CSR_defined(0x341) = true // mepc

 function clause read_CSR(0x105) = get_stvec()
-function clause read_CSR(0x141) = get_xret_target(Supervisor) & pc_alignment_mask()
+function clause read_CSR(0x141) = legalize_xepc(get_xret_target(Supervisor))


I would maybe also rename get/set_xret_target to get/set_xepc. It's a bit clearer IMO.

github-actions · 2024-11-19T09:36:55Z

Test Results

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

Results for commit 42052ce. ± Comparison against base commit 8a2e0f5.

♻️ This comment has been updated with latest results.

rmn30 · 2024-11-19T10:29:07Z

There is a bear trap here. From the RISC-V privileged spec:

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.

(emphasis mine)

So the legalization that occurs on write is not necessarily the same as the one for the implicit read in MRET. Please check whether we're getting this right.

rmn30 · 2024-11-19T17:42:33Z

model/riscv_sys_control.sail

@@ -310,7 +310,7 @@ function exception_handler(cur_priv : Privilege, ctl : ctl_result,
      then print_platform("ret-ing from " ^ to_str(prev_priv)
                          ^ " to " ^ to_str(cur_privilege));

-      prepare_xret_target(Supervisor) & pc_alignment_mask()
+      prepare_xret_target(Supervisor) & sign_extend(if misa[C] then 0b110 else 0b100)


Please don't repeat this in two places. You could use @Timmmm 's align_pc function from #593 .

Is there a reason one should not just reuse legalize_xepc? Perhaps because we want to capture the fact that the LSB isn't ever set in this case, it's only the second bit that needs handling specially?

rmn30 · 2024-11-19T18:03:02Z

model/riscv_sys_exceptions.sail

@@ -89,11 +89,11 @@ function clause is_CSR_defined(0x305) = true // mtvec
 function clause is_CSR_defined(0x341) = true // mepc

 function clause read_CSR(0x105) = get_stvec()
-function clause read_CSR(0x141) = get_xret_target(Supervisor) & pc_alignment_mask()
+function clause read_CSR(0x141) = get_xepc(Supervisor)


I think there should still be a mask here (e.g. using align_pc). It's weird but the way I understand the spec if a CPU supports C but can disable it via misa.C then you could have the following sequence:

write misa.C <- 0 write mepc <- 0b11 read mepc -> 0b00 write misa.C <- 1 read mepc -> 0b10

I believe that's true, though the slightly less weird sequence that's definitely true is:

write mepc <- 0b11 read mepc -> 0b10 write misa.C <- 0 read mepc -> 0b00 write misa.C <- 1 read mepc -> 0b10

That alone requires masking on read, something which is explicitly mentioned in the privileged spec.

jrtc27 · 2024-11-19T18:10:59Z

I don't know what you've done to cause it, but your commit history is an inscrutable mess. Please clean it up per the second paragraph in https://github.com/riscv/sail-riscv/blob/master/CONTRIBUTING.md (and just because it's the expectation for contributing to any project).

jrtc27 · 2024-11-19T18:15:14Z

model/riscv_sys_exceptions.sail

-val get_xret_target : Privilege -> xlenbits
-function get_xret_target(p) =
+val get_xepc : Privilege -> xlenbits
+function get_xepc(p) =


Should this just not legalise the result? Every use of get_xret_target (possibly indirectly via prepare_xret_target) previously masked with pc_alignment_mask, all of which were correct.

jrtc27 · 2024-11-20T05:30:46Z

This is getting worse not better; you're piling on more commits to an already mess of a history, you're committing some random test log file, you're adding in debug prints and you're removing more code that needs to be there.

rez5427 · 2024-11-20T05:44:53Z

This is getting worse not better; you're piling on more commits to an already mess of a history, you're committing some random test log file, you're adding in debug prints and you're removing more code that needs to be there.

sorry... I'm try to remove the mess commit.... but it didn't work Is there a way to remove these commit in a pushed pr. I've learn how to better commit, It won't happen again.. really sorry..

jrtc27 · 2024-11-20T06:11:01Z

This is getting worse not better; you're piling on more commits to an already mess of a history, you're committing some random test log file, you're adding in debug prints and you're removing more code that needs to be there.

sorry... I'm try to remove the mess commit.... but it didn't work Is there a way to remove these commit in a pushed pr. I've learn how to better commit, It won't happen again.. really sorry..

Look up how to use git rebase, e.g. https://about.gitlab.com/blog/2020/11/23/keep-git-history-clean-with-interactive-rebase/ looks like it could be a decent introduction

rez5427 · 2024-11-20T10:02:51Z

This is getting worse not better; you're piling on more commits to an already mess of a history, you're committing some random test log file, you're adding in debug prints and you're removing more code that needs to be there.

sorry... I'm try to remove the mess commit.... but it didn't work Is there a way to remove these commit in a pushed pr. I've learn how to better commit, It won't happen again.. really sorry..

Look up how to use git rebase, e.g. https://about.gitlab.com/blog/2020/11/23/keep-git-history-clean-with-interactive-rebase/ looks like it could be a decent introduction

Copy that Sir

rmn30 · 2024-11-20T15:29:47Z

It's worth pointing out that after a rebase you will have to use git push -f to force update the branch for this PR. That goes slightly against what the article Jessica linked says, namely "should NOT be used on commit history that has already been pushed", but it is pretty normal for PRs prior to merging. It can cause slight pain for anyone who has pulled the PR branch and wants to update but it's possible to recover.

Timmmm · 2024-11-21T14:40:14Z

So I got permission to push most of our Sail code in one big go, to support some CHERI stuff (long story)... anyway the point is you can see what we've done here:

/* The xepc legalization zeroes xepc[1:0] when misa.C is hardwired to 0.
 * When misa.C is writable, it zeroes only xepc[0].
 */
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()]
}

// Align value to min supported PC alignment. This is used to
// legalize xepc reads.
function align_pc(addr : xlenbits) -> xlenbits = {
  if misa[C] == 0b1
  then [addr with 0 = bitzero]
  else [addr with 1..0 = zeros()]
}

And elsewhere:

function get_mepc() -> xlenbits = align_pc(mepc)

function set_mepc(value : xlenbits) -> unit = {
  mepc = legalize_xepc(convertInvalidAddr(value));
}

function get_sepc() -> xlenbits = align_pc(sepc)

function set_sepc(value : xlenbits) -> unit = {
  sepc = legalize_xepc(convertInvalidAddr(value));
}

...

function clause read_CSR(0x341, _) = get_mepc()

function clause write_CSR(0x341, value) = { set_mepc(value); get_mepc() }

...

function get_xepc(p : Privilege) -> xlenbits =
  match p {
    Machine    => get_mepc(),
    Supervisor => get_sepc(),
    User       => internal_error(__FILE__, __LINE__, "N extension not supported"),
  }

...

function prepare_xret_target(p : Privilege) -> xlenbits =
  if inSmclicMode() & get_xcause(p)[Xinhv] == 0b1
  then prepare_clic_xret_vectored_target(p)
  else get_xepc(p)

Ignore my earlier comment about removing prepare_xret_target; let's leave it in so when we add CLIC it's easier.

Also it was maybe overkill having both get_mepc and get_xepc. I would just have get_xepc I think. IIRC it kind ended up that way because of the pain of handling CHERI CSRs.

rez5427 · 2024-11-22T04:44:44Z

So I got permission to push most of our Sail code in one big go, to support some CHERI stuff (long story)... anyway the point is you can see what we've done here:

/* The xepc legalization zeroes xepc[1:0] when misa.C is hardwired to 0.
 * When misa.C is writable, it zeroes only xepc[0].
 */
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()]
}

// Align value to min supported PC alignment. This is used to
// legalize xepc reads.
function align_pc(addr : xlenbits) -> xlenbits = {
  if misa[C] == 0b1
  then [addr with 0 = bitzero]
  else [addr with 1..0 = zeros()]
}

And elsewhere:

function get_mepc() -> xlenbits = align_pc(mepc)

function set_mepc(value : xlenbits) -> unit = {
  mepc = legalize_xepc(convertInvalidAddr(value));
}

function get_sepc() -> xlenbits = align_pc(sepc)

function set_sepc(value : xlenbits) -> unit = {
  sepc = legalize_xepc(convertInvalidAddr(value));
}

...

function clause read_CSR(0x341, _) = get_mepc()

function clause write_CSR(0x341, value) = { set_mepc(value); get_mepc() }

...

function get_xepc(p : Privilege) -> xlenbits =
  match p {
    Machine    => get_mepc(),
    Supervisor => get_sepc(),
    User       => internal_error(__FILE__, __LINE__, "N extension not supported"),
  }

...

function prepare_xret_target(p : Privilege) -> xlenbits =
  if inSmclicMode() & get_xcause(p)[Xinhv] == 0b1
  then prepare_clic_xret_vectored_target(p)
  else get_xepc(p)

Ignore my earlier comment about removing prepare_xret_target; let's leave it in so when we add CLIC it's easier.

Also it was maybe overkill having both get_mepc and get_xepc. I would just have get_xepc I think. IIRC it kind ended up that way because of the pain of handling CHERI CSRs.

I'm not sure. Is the new commit ok?

Timmmm · 2024-11-22T09:52:10Z

Yes looks good to me with one change: move the align_pc() calls into get_xepc(), since that legalisation is always done when reading it.

val get_xepc : Privilege -> xlenbits
function get_xepc(p) =
  match p {
    Machine    => align_pc(mepc),
    Supervisor => align_pc(sepc),
    User       => internal_error(__FILE__, __LINE__, "Invalid privilege level"),
  }

(Or you can put the align_pc around the whole match but I think it makes it slightly less readable; up to you.)

model/riscv_sys_control.sail

Timmmm · 2024-11-27T11:58:56Z

model/riscv_sys_exceptions.sail

@@ -89,11 +89,11 @@ function clause is_CSR_defined(0x305) = true // mtvec
 function clause is_CSR_defined(0x341) = true // mepc

 function clause read_CSR(0x105) = get_stvec()
-function clause read_CSR(0x141) = get_xret_target(Supervisor) & pc_alignment_mask()
+function clause read_CSR(0x141) = align_pc(get_xepc(Supervisor))


Oh sorry you also don't need align_pc here, or on line 94.

Timmmm

LGTM

Timmmm · 2024-11-29T11:54:16Z

@rmn30 @jrtc27 does this look ok to you?

rmn30 · 2024-11-29T13:10:23Z

Yes, I think it looks fine but the commit history is a bit messy still. Could use squash merge but would need to update PR with proper description first.

Timmmm · 2024-11-29T15:25:58Z

Ok cool, I will merge this in a few days with a nice squashed commit if nobody objects. Thanks for reviewing!

jrtc27 · 2024-11-29T22:53:55Z

I'll take a look over the weekend

jrtc27

Minor comment otherwise I think the change is correct

jrtc27 · 2024-11-30T20:38:05Z

model/riscv_sys_exceptions.sail

+ * get_xepc:     used to read the value of the xret target  (no control flow transfer)
+ * set_xepc:     used to write a value of the xret target   (no control flow transfer)
 * prepare_xret_target: used to get the value for control transfer to the xret target


Suggested change

* get_xepc: used to read the value of the xret target (no control flow transfer)

* set_xepc: used to write a value of the xret target (no control flow transfer)

* prepare_xret_target: used to get the value for control transfer to the xret target

* get_xepc: used to read the value of the xret target (no control flow transfer)

* set_xepc: used to write a value of the xret target (no control flow transfer)

* prepare_xret_target: used to get the value for control transfer to the xret target

Timmmm requested changes Nov 19, 2024

View reviewed changes

rmn30 reviewed Nov 19, 2024

View reviewed changes

jrtc27 reviewed Nov 19, 2024

View reviewed changes

rez5427 force-pushed the remove_pc_alignment_mask branch from e824e9e to 0893def Compare November 21, 2024 07:48

rez5427 closed this Nov 21, 2024

rez5427 reopened this Nov 21, 2024

Remove legalize_xepc

a85c988

rez5427 force-pushed the remove_pc_alignment_mask branch from 0893def to a85c988 Compare November 21, 2024 08:31

Rename pc_alignment_mask

63141dc

Rename pc_alignment_mask

61d670c

Timmmm reviewed Nov 26, 2024

View reviewed changes

model/riscv_sys_control.sail Outdated Show resolved Hide resolved

Remove redundent align_pc

7f5d491

Timmmm reviewed Nov 27, 2024

View reviewed changes

Remove redundent align_pc

42052ce

Timmmm approved these changes Nov 27, 2024

View reviewed changes

Timmmm added the will be merged Scheduled to be merged in a few days if nobody objects label Nov 29, 2024

jrtc27 reviewed Nov 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove pc alignment mask #618

Remove pc alignment mask #618

rez5427 commented Nov 19, 2024

Timmmm Nov 19, 2024

rmn30 Nov 19, 2024

Timmmm Nov 19, 2024

github-actions bot commented Nov 19, 2024 •

edited

Loading

rmn30 commented Nov 19, 2024

rmn30 Nov 19, 2024 •

edited

Loading

jrtc27 Nov 19, 2024

rmn30 Nov 19, 2024 •

edited

Loading

jrtc27 Nov 19, 2024

jrtc27 commented Nov 19, 2024

jrtc27 Nov 19, 2024

jrtc27 commented Nov 20, 2024

rez5427 commented Nov 20, 2024 •

edited

Loading

jrtc27 commented Nov 20, 2024

rez5427 commented Nov 20, 2024

rmn30 commented Nov 20, 2024

Timmmm commented Nov 21, 2024

rez5427 commented Nov 22, 2024

Timmmm commented Nov 22, 2024

Timmmm Nov 27, 2024

Timmmm left a comment

Timmmm commented Nov 29, 2024

rmn30 commented Nov 29, 2024

Timmmm commented Nov 29, 2024

jrtc27 commented Nov 29, 2024

jrtc27 left a comment

jrtc27 Nov 30, 2024

Remove pc alignment mask #618

Are you sure you want to change the base?

Remove pc alignment mask #618

Conversation

rez5427 commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 19, 2024 • edited Loading

Test Results

rmn30 commented Nov 19, 2024

rmn30 Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmn30 Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrtc27 commented Nov 19, 2024

Choose a reason for hiding this comment

jrtc27 commented Nov 20, 2024

rez5427 commented Nov 20, 2024 • edited Loading

jrtc27 commented Nov 20, 2024

rez5427 commented Nov 20, 2024

rmn30 commented Nov 20, 2024

Timmmm commented Nov 21, 2024

rez5427 commented Nov 22, 2024

Timmmm commented Nov 22, 2024

Choose a reason for hiding this comment

Timmmm left a comment

Choose a reason for hiding this comment

Timmmm commented Nov 29, 2024

rmn30 commented Nov 29, 2024

Timmmm commented Nov 29, 2024

jrtc27 commented Nov 29, 2024

jrtc27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 19, 2024 •

edited

Loading

rmn30 Nov 19, 2024 •

edited

Loading

rmn30 Nov 19, 2024 •

edited

Loading

rez5427 commented Nov 20, 2024 •

edited

Loading