Re: [PATCH v4 2/3] target/riscv: Fixup setting GVA

[Bin Meng]

On Mon, Dec 20, 2021 at 2:49 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> In preperation for adding support for the illegal instruction address

typo: preparation

> let's fixup the Hypervisor extension setting GVA logic and improve the
> variable names.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/cpu_helper.c | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 9eeed38c7e..9e1f5ee177 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -967,6 +967,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>
>      RISCVCPU *cpu = RISCV_CPU(cs);
>      CPURISCVState *env = &cpu->env;
> +    bool write_gva = false;
>      uint64_t s;
>
>      /* cs->exception is 32-bits wide unlike mcause which is XLEN-bits wide
> @@ -975,7 +976,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,7 +1004,7 @@ 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;
> +            write_gva = true;
>              tval = env->badaddr;
>              break;
>          default:
> @@ -1041,18 +1041,6 @@ 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);
> -            }
> -
>              if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) {
>                  /* Trap to VS mode */
>                  /*
> @@ -1063,7 +1051,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>                      cause == IRQ_VS_EXT) {
>                      cause = cause - 1;
>                  }
> -                env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
> +                write_gva = false;
>              } else if (riscv_cpu_virt_enabled(env)) {
>                  /* Trap into HS mode, from virt */
>                  riscv_cpu_swap_hypervisor_regs(env);
> @@ -1072,6 +1060,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>                  env->hstatus = set_field(env->hstatus, HSTATUS_SPV,
>                                           riscv_cpu_virt_enabled(env));
>
> +
>                  htval = env->guest_phys_fault_addr;
>
>                  riscv_cpu_set_virt_enabled(env, 0);
> @@ -1079,7 +1068,9 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>                  /* Trap into HS mode */
>                  env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false);
>                  htval = env->guest_phys_fault_addr;
> +                write_gva = false;
>              }
> +            env->hstatus = set_field(env->hstatus, HSTATUS_GVA, write_gva);

This does not look correct to me.

The env->hstatus[GVA] should remain untouched in the 2nd and 3rd
branch. It only needs to be set in the first branch.

>          }
>
>          s = env->mstatus;

Regards,
Bin