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

[Bin Meng]

On Thu, Jan 6, 2022 at 12:04 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> 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.

Thanks for the info.

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

With these explanations, I feel this patch mixed up two things into one patch.

The changes to 2nd and 3rd branch probably merit a separate patch with
a better commit message as it *explicitely* clear or set GVA field as
per the spec while current codes don't do that.
A follow-up patch can be made to consolidate other "write_tval" cases.

But that's up to you. FWIW:
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Regards,
Bin