There is no guarantee that we'll able to get a proper opcode to put into mtval for illegal inst exceptions, as demonstrated in [1].
The 'proper opcode' would be retrieved via unwinding the CPU state (via cpu_restore_state(), down to riscv_restore_state_to_opc()) after riscv_raise_exception(). There are cases where that doesn't happen: we'll see a failure in cpu_restore_state(), where in_code_gen_buffer() will return 'false' even for a 'host_pc' that is retrieved via GETPC() and not in an instruction fetch context. Hopefully we don't have to always provide mtval in these cases. The RISC-V priv ISA says that mtval for illegal_inst exceptions are optional, and the faulting instruction address will be reported in env->mepc. The ISA also says that we can set mtval to ILEN/MXLEN bits of the faulting insn address, and we could do that when we're not able to fetch the opcode. But that would add inconsistency in how we behave since mtval would have either the opcode or the insn addr, and no easy way of knowing which one we have, and software would need to check it with mepc regardless. Zeroing mtval when we don't have the opcode is clearer. And yes, zeroing mtval due to an unwind failure isn't ideal either, but it's less worse than reporting a wrong mtval. Until we figure out a way to fix the unwinding in this case, let's clear mtval and let software know that it must find the faulting opcode via other means. [1] https://gitlab.com/qemu-project/qemu/-/issues/3020 Closes: https://gitlab.com/qemu-project/qemu/-/issues/3020 Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com> --- target/riscv/cpu_helper.c | 5 +++++ target/riscv/op_helper.c | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index 3479a62cc7..1cd1849a1d 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -2243,6 +2243,11 @@ void riscv_cpu_do_interrupt(CPUState *cs) break; case RISCV_EXCP_ILLEGAL_INST: case RISCV_EXCP_VIRT_INSTRUCTION_FAULT: + /* + * Note: we'll set tval = 0 for illegal_inst cases + * where we failed to unwind and to get the proper + * opcode. See riscv_raise_exception() for more info. + */ tval = env->bins; break; case RISCV_EXCP_BREAKPOINT: diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c index 15460bf84b..930981a076 100644 --- a/target/riscv/op_helper.c +++ b/target/riscv/op_helper.c @@ -40,6 +40,27 @@ G_NORETURN void riscv_raise_exception(CPURISCVState *env, env->pc); cs->exception_index = exception; + + /* + * There is no guarantee that we'll be able to unwind + * and set env->bins (the opcode for the current PC) + * properly via the cpu_restore_state() path. The RISC-V + * priv ISA says that: + * + * "The mtval register can optionally also be used to return + * the faulting instruction bits on an illegal-instruction + * exception (mepc points to the faulting instruction in + * memory)." + * + * It's not ideal to set mtval != 0 in some cases and zero + * in others due to unwind failures, but it's way better + * than to set mtval to a bogus env->bins opcode from + * the last successful unwinding. + */ + if (cs->exception_index == RISCV_EXCP_ILLEGAL_INST) { + env->bins = 0; + } + cpu_loop_exit_restore(cs, pc); } -- 2.50.1
