🐛 fix bug in instruction-misaligned exception handling

[stnolting]

Bug identified by @mikaelsky in #728 (comment).

The current processor version captures unaligned instruction addresses in the pipeline's front-end. Hence, any misaligned instruction will still trigger an instruction fetch and jump-and-link instruction will still update the link register. This PR aims to fix this:

• Branch instructions (unconditional branches or "taken" conditional branches) do not trigger an instruction fetch if the address is misaligned.
• Jump-and-link instruction do not write to the link register if the destination address is misaligned.
• Misaligned instruction address exception are raised by the pipeline's back end (execution) and not by the pipeline's front-end (instruction fetch).

I am not quite sure about the handling of xRET instructions when xEPC contains a misaligned address. The RISC-V priv. spec states:

Machine Exception Program Counter (mepc)
[...]
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.

So I think xEPC[1] should be hardwired to zero if IALIGN=32 (i.e. the C extension is NOT enabled). Hence, xRET cannot raise a misaligned instruction exception. However, I cannot find anything regarding dret/dpc but I assume that we should expect the same behavior there. @mikaelsky can you confirm this?

[mikaelsky]

@stnolting so reading the spec there really isn't an "xRET" instruction, from what I can gather it seems its just short-hand for JAL.
"RISC-V does not have a dedicated instruction for return. The ret is a pseudo-instruction that is expanded to jalr zero, 0(ra), which sets the program counter to ra + 0 and saves the previous program counter’s value plus four to register zero, which is hardwired to zero. This implies that the return value must be copied from the stack into ra before returning."
source: https://infosecwriteups.com/return-oriented-programming-on-risc-v-part-1-dd9817b52d2b

Because of this fixing JAL to handle mis-aligned targets should also fix "xRET". So we shouldn't need to tie xEPC[1] to 0.
For MRET/SRET its an open question what happens as one could imagine xEPC having been modified. Its not clear from the spec vol II. Let me check it and I'll respond.

An additional "fun" detail with JALR is: Spec 2.2 chapter 2.5.1 unconditional branches.
"The indirect jump instruction JALR (jump and link register) uses the I-type encoding. The target
address is obtained by adding the sign-extended 12-bit I-immediate to the register rs1, then setting the
least-significant bit of the result to zero."
This basically makes JALR behave like JAL, as JAL has the LSB implicitly set to 0.

This explains this note for JAL/JALR, as the PC LSB is forced to be 0 disregarding its content.
"The JAL and JALR instructions will generate an instruction-address-misaligned exception if the target
address is not aligned to a four-byte boundary.
Note: Instruction-address-misaligned exceptions are not possible on machines that support
extensions with 16-bit aligned instructions, such as the compressed instruction-set
extension, C."

After thanksgiving I'll take a stab at the v1.9.1.4 release in a local branch and see if it passes our compliance check. Our checker is much more punishing than the riscv-arch test suite.

[stnolting]

so reading the spec there really isn't an "xRET" instruction, from what I can gather it seems its just short-hand for JAL.
"RISC-V does not have a dedicated instruction for return.

Right, I was just referring to the "special" mret and dret instructions here.

So we shouldn't need to tie xEPC[1] to 0.
For MRET/SRET its an open question what happens as one could imagine xEPC having been modified. Its not clear from the spec vol II. Let me check it and I'll respond.

Well, this is what I have found in the specs.:

Priv. spec regarding mret / mepc

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.

Debug spec regarding dret / dpc

The writability of dpc follows the same rules as mepc as defined in the Privileged Spec. In particular, dpc must be able to hold all valid virtual addresses and the writability of the low bits depends on IALIGN.

So I think it should be fine to hardwire bit 1 to zero if IALIGN = 32 (i.e. C is disabled).

An additional "fun" detail with JALR is: Spec 2.2 chapter 2.5.1 unconditional branches. [...]

I need to check that again. I read somewhere that a lot of RISC-V core have some issues with a proper implementation of this...

After thanksgiving I'll take a stab at the v1.9.1.4 release in a local branch and see if it passes our compliance check. Our checker is much more punishing than the riscv-arch test suite.

Thank you very much and happy Thanksgiving! 😉

[mikaelsky]

More a note on the branch misalign fix added in v1.9.1.4.
I've finally gotten to merge up to 1.9.3 and worked through forward fixing the RVVI trace port to be functional with core version 1.9.3.
Initial results show that the misaligned branch fix is functional vs the imperas reference model - and thus the riscv spec - on a per instruction compare full processor state compare basis.

[stnolting]

Initial results show that the misaligned branch fix is functional vs the imperas reference model - and thus the riscv spec - on a per instruction compare full processor state compare basis.

That sounds great! 🎉
Thank you very much for sharing that information!