Re: [PATCH v2 2/3] target/riscv: Implement the stval/mtval illegal instruction

[Richard Henderson]

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?

I simply wonder whether it's worthwhile to add the feature and feature test.


r~