On Wed, Sep 8, 2021 at 4:48 PM Richard Henderson <richard.hender...@linaro.org> wrote: > > On 9/8/21 6:54 AM, Alistair Francis wrote: > > @@ -967,6 +967,16 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > write_tval = true; > > tval = env->badaddr; > > break; > > + case RISCV_EXCP_ILLEGAL_INST: > > + if (riscv_feature(env, RISCV_FEATURE_MTVAL_INST)) { > > + /* > > + * The stval/mtval register can optionally also be used to > > + * return the faulting instruction bits on an illegal > > + * instruction exception. > > + */ > > + tval = env->bins; > > + } > > + break; > > I'll note that write_tval should probably be renamed, and/or eliminated, > because it looks > like it's incorrectly unset here. If you move the adjustment to cause above > this switch, > then you can move the RVH code that needed write_tval into this switch (just > the > HSTATUS_GVA update?). > > But... more specific to this case. Prior to this, was the exception handler > allowed to > assume anything about the contents of stval? Should the value have been > zero? Would it > be wrong to write to stval unconditionally? How does the guest OS know that > it can rely > on stval being set?
As we didn't support writing the illegal instruction stval should be zero before this patch. > > I simply wonder whether it's worthwhile to add the feature and feature test. Do you just mean have it enabled all the time? Alistair > > > r~