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

[Alistair Francis]

On Tue, Dec 21, 2021 at 5:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> 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

Fixed

>
> > 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.

The RISC-V spec says:

""'
Field GVA (Guest Virtual Address) is written by the implementation
whenever a trap is taken
into HS-mode. For any trap (breakpoint, address misaligned, access
fault, page fault, or guest-
page fault) that writes a guest virtual address to stval, GVA is set
to 1. For any other trap into
HS-mode, GVA is set to 0.
"""

So it has to be set in the second and third branches as they are
trapping into HS mode. I guess we could not touch it in the first
branch (Trap to VS mode), but that means we would need to ensure it is
updated later, so it seems easiest to just clear it here.

In the second branch (Trap into HS mode, from virt) we set GVA based
on the trap cause, which seems correct.

In the third case (Trap into HS mode) we are trapping from HS to HS so
we want to clear GVA.

Alistair

>
> >          }
> >
> >          s = env->mstatus;
>
> Regards,
> Bin