VexRiscv icon indicating copy to clipboard operation
VexRiscv copied to clipboard

Enable `catchIllegalAccess` and `catchIllegalInstruction` by default

Open piegamesde opened this issue 2 years ago • 3 comments

Both settings should be enabled by default in all configurations, and the possibility of disabling it should be deprecated. The reason is that compilers rely on the correct trapping behavior during code generation in various ways. For example, the Rust compiler makes use of it to mark unreachable code and when panic = "abort" is set, this is also used to quickly jump to the abort function. I just lost a week debugging the fact that the unimp instruction was a no-op in my processor, which caused some very subtle bugs.

From the RISCV specification (v2.2):

We consider it a feature that any length of instruction containing all zero bits is not legal, as this quickly traps erroneous jumps into zeroed memory regions. Similarly, we also reserve the instruction encoding containing all ones to be an illegal instruction, to catch the other common pattern observed with unprogrammed non-volatile memory devices, disconnected memory buses, or broken memory devices.

Same thing applies to illegal CSR accesses, but I could not easily find the corresponding part in the spec.

piegamesde avatar Mar 23 '22 18:03 piegamesde

omg, I can feel you pain XD

and the possibility of disabling it should be deprecated

Issue is, VexRiscv also support to have very slim / light configuration, on which supporting exception cost many ressources. So deprecating it, i would say it isn't a good idea for those applications.

Both settings should be enabled by default in all configuration

Issue is backward compatibility (FMax / Area). As from some perspective, this not being enabled by default is more a feature than a bug.

What's about we add some warnings durring the generation ? In the style of : "Illegal instruction trap disabled ! Can't run rust code" Or anything in that kind as a reminder for the people using the core ?

Dolu1990 avatar Mar 24 '22 13:03 Dolu1990

How much additional hardware does enabling these produce? Is there a way to generate a code minimal implementation? As in: ditch the mtvec CSR and just halt the CPU on trap? Alternatively, could we at least the enable the simulation checks by default? They wouldn't cost anything in terms of hardware.

piegamesde avatar Mar 24 '22 19:03 piegamesde

How much additional hardware does enabling these produce?

I would say it depend what is already in the CPU. After looking where it was disabled, it seems that

  • GenSmallAndProductive has it disabled
  • GenFullNoMmuNoCache has it enabled

I think this is a good threshold. Adding it into GenSmallAndProductive, will likely consume 200 lut+, as it would also require adding support for exception.

Which configuration where you using ?

Is there a way to generate a code minimal implementation, just halt the CPU on trap

To halt it, so not freezing it, but halting it as if it has a breakpoint ? which would make it debugable via jtag ? It could make quite some sense, and would not require much additional hardware. Sound good to you ?

Alternatively, could we at least the enable the simulation checks by default? They wouldn't cost anything in terms of hardware.

That can be tricky to do it well, basicaly, the behaviour of a uncatched illegal instruction is undefined by the VexRiscv design, so we could add a report if a undecodded reach the last stage of the CPU, but it is not sure it will reach that point without having side effects. Still it can be usefull. So we could implement that. If we add a report in the decode stage, the issue is that we will trigger some false positive, as there is a little bit of speculation that the instruction will not be unscheduled by a branch or something else.

Dolu1990 avatar Mar 25 '22 09:03 Dolu1990