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

[Richard Henderson]

On 12/9/21 10:26 PM, Alistair Francis wrote:
> @@ -975,7 +975,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>       bool async = !!(cs->exception_index & RISCV_EXCP_INT_FLAG);
>       target_ulong cause = cs->exception_index & RISCV_EXCP_INT_MASK;
>       target_ulong deleg = async ? env->mideleg : env->medeleg;
> -    bool write_tval = false;
>       target_ulong tval = 0;
>       target_ulong htval = 0;
>       target_ulong mtval2 = 0;
> @@ -1004,9 +1003,11 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>           case RISCV_EXCP_INST_PAGE_FAULT:
>           case RISCV_EXCP_LOAD_PAGE_FAULT:
>           case RISCV_EXCP_STORE_PAGE_FAULT:
> -            write_tval  = true;
>               tval = env->badaddr;
>               break;
...
> @@ -1041,17 +1042,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>           if (riscv_has_ext(env, RVH)) {
>               target_ulong hdeleg = async ? env->hideleg : env->hedeleg;
>   
> -            if (env->two_stage_lookup && write_tval) {
> -                /*
> -                 * If we are writing a guest virtual address to stval, set
> -                 * this to 1. If we are trapping to VS we will set this to 0
> -                 * later.
> -                 */
> -                env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 1);
> -            } else {
> -                /* For other HS-mode traps, we set this to 0. */
> -                env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
> -            }
> +            env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
>   
>               if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) {
>                   /* Trap to VS mode */
> @@ -1063,7 +1054,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>                       cause == IRQ_VS_EXT) {
>                       cause = cause - 1;
>                   }
> -                env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
>               } else if (riscv_cpu_virt_enabled(env)) {
>                   /* Trap into HS mode, from virt */
>                   riscv_cpu_swap_hypervisor_regs(env);
> @@ -1071,6 +1061,13 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>                                            env->priv);
>                   env->hstatus = set_field(env->hstatus, HSTATUS_SPV,
>                                            riscv_cpu_virt_enabled(env));
> +                if (tval) {
> +                    /*
> +                     * If we are writing a guest virtual address to stval, set
> +                     * this to 1.
> +                     */
> +                    env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 1);
> +                }

The thing that concerns me here is the very legitimate badaddr of NULL.  I think you need 
to keep some out-of-band flag for that.

In addition, the out-of-band flag should probably be called write_gva, because tval from 
ILLEGAL_INST is not a Guest Virtual Address, and so should not set GVA.

You could even push the setting of the bit down so that it's only done once, 
e.g.

    hstatus_gva = true;
    tval = env->badaddr;

    if (trap to vs) {
       ...
       hstatus_gva = false;
    } else if (virt enabled) {
       ...
    } else {
       trap into hs
       hstatus_gva = false;
    }
    env->hstatus(set_field(env->hstatus, HSTATUS_GVA, hstatus_gva);


The actual handling of ILLEGAL_INST looks fine.
You may well want to split the GVA handling to a separate patch.


r~