[v1,15/36] target/riscv: Convert mstatus to pointers
diff mbox series

Message ID 8c0eebc3868757e9ed312ac35e1f5325d5a18e76.1575914822.git.alistair.francis@wdc.com
State New
Headers show
Series
  • Add RISC-V Hypervisor Extension v0.5
Related show

Commit Message

Alistair Francis Dec. 9, 2019, 6:11 p.m. UTC
To handle the new Hypervisor CSR register aliasing let's use pointers.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c        | 11 +++++++++--
 target/riscv/cpu.h        |  9 ++++++++-
 target/riscv/cpu_helper.c | 30 +++++++++++++++---------------
 target/riscv/csr.c        | 20 ++++++++++----------
 target/riscv/op_helper.c  | 14 +++++++-------
 5 files changed, 49 insertions(+), 35 deletions(-)

Comments

Palmer Dabbelt Jan. 8, 2020, 1:30 a.m. UTC | #1
On Mon, 09 Dec 2019 10:11:19 PST (-0800), Alistair Francis wrote:
> To handle the new Hypervisor CSR register aliasing let's use pointers.

For some reason I thought we were making this explicit?  In other words,
requiring that all callers provide which privilege mode they're using when
accessing these CSRs, as opposed to swapping around pointers.  I don't actually
care that much, but IIRC when we were talking with the ARM guys at Plumbers
they were pretty adament that would end up being a much cleaner implementation
as they'd tried this way and later changed over.

> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/cpu.c        | 11 +++++++++--
>  target/riscv/cpu.h        |  9 ++++++++-
>  target/riscv/cpu_helper.c | 30 +++++++++++++++---------------
>  target/riscv/csr.c        | 20 ++++++++++----------
>  target/riscv/op_helper.c  | 14 +++++++-------
>  5 files changed, 49 insertions(+), 35 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index a07c5689b3..e61cf46a73 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -236,7 +236,7 @@ static void riscv_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>      qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "pc      ", env->pc);
>  #ifndef CONFIG_USER_ONLY
>      qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mhartid ", env->mhartid);
> -    qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ", env->mstatus);
> +    qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ", *env->mstatus);
>      if (riscv_has_ext(env, RVH)) {
>          qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "hstatus ", env->hstatus);
>          qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "vsstatus ", env->vsstatus);
> @@ -336,7 +336,7 @@ static void riscv_cpu_reset(CPUState *cs)
>      mcc->parent_reset(cs);
>  #ifndef CONFIG_USER_ONLY
>      env->priv = PRV_M;
> -    env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
> +    *env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
>      env->mcause = 0;
>      env->pc = env->resetvec;
>  #endif
> @@ -465,8 +465,15 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>  static void riscv_cpu_init(Object *obj)
>  {
>      RISCVCPU *cpu = RISCV_CPU(obj);
> +#ifndef CONFIG_USER_ONLY
> +    CPURISCVState *env = &cpu->env;
> +#endif
>
>      cpu_set_cpustate_pointers(cpu);
> +
> +#ifndef CONFIG_USER_ONLY
> +    env->mstatus = &env->mstatus_novirt;
> +#endif
>  }
>
>  static const VMStateDescription vmstate_riscv_cpu = {
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 21ae5a8b19..9dc8303c62 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -122,7 +122,7 @@ struct CPURISCVState {
>      target_ulong resetvec;
>
>      target_ulong mhartid;
> -    target_ulong mstatus;
> +    target_ulong *mstatus;
>
>      target_ulong mip;
>      uint32_t miclaim;
> @@ -145,6 +145,13 @@ struct CPURISCVState {
>      target_ulong mcause;
>      target_ulong mtval;  /* since: priv-1.10.0 */
>
> +    /* The following registers are the "real" versions that the pointer
> +     * versions point to. These should never be used unless you know what you
> +     * are doing. To access these use the pointer versions instead. This is
> +     * required to handle the Hypervisor register swapping.
> +     */
> +    target_ulong mstatus_novirt;
> +
>      /* Hypervisor CSRs */
>      target_ulong hstatus;
>      target_ulong hedeleg;
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index b00f66824a..9684da7f7d 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -37,8 +37,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
>  #ifndef CONFIG_USER_ONLY
>  static int riscv_cpu_local_irq_pending(CPURISCVState *env)
>  {
> -    target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE);
> -    target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE);
> +    target_ulong mstatus_mie = get_field(*env->mstatus, MSTATUS_MIE);
> +    target_ulong mstatus_sie = get_field(*env->mstatus, MSTATUS_SIE);
>      target_ulong pending = env->mip & env->mie;
>      target_ulong mie = env->priv < PRV_M || (env->priv == PRV_M && mstatus_mie);
>      target_ulong sie = env->priv < PRV_S || (env->priv == PRV_S && mstatus_sie);
> @@ -75,7 +75,7 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>  /* Return true is floating point support is currently enabled */
>  bool riscv_cpu_fp_enabled(CPURISCVState *env)
>  {
> -    if (env->mstatus & MSTATUS_FS) {
> +    if (*env->mstatus & MSTATUS_FS) {
>          return true;
>      }
>
> @@ -198,8 +198,8 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>      int mode = mmu_idx;
>
>      if (mode == PRV_M && access_type != MMU_INST_FETCH) {
> -        if (get_field(env->mstatus, MSTATUS_MPRV)) {
> -            mode = get_field(env->mstatus, MSTATUS_MPP);
> +        if (get_field(*env->mstatus, MSTATUS_MPRV)) {
> +            mode = get_field(*env->mstatus, MSTATUS_MPP);
>          }
>      }
>
> @@ -213,11 +213,11 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>
>      hwaddr base;
>      int levels, ptidxbits, ptesize, vm, sum;
> -    int mxr = get_field(env->mstatus, MSTATUS_MXR);
> +    int mxr = get_field(*env->mstatus, MSTATUS_MXR);
>
>      if (env->priv_ver >= PRIV_VERSION_1_10_0) {
>          base = (hwaddr)get_field(env->satp, SATP_PPN) << PGSHIFT;
> -        sum = get_field(env->mstatus, MSTATUS_SUM);
> +        sum = get_field(*env->mstatus, MSTATUS_SUM);
>          vm = get_field(env->satp, SATP_MODE);
>          switch (vm) {
>          case VM_1_10_SV32:
> @@ -237,8 +237,8 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>          }
>      } else {
>          base = (hwaddr)(env->sptbr) << PGSHIFT;
> -        sum = !get_field(env->mstatus, MSTATUS_PUM);
> -        vm = get_field(env->mstatus, MSTATUS_VM);
> +        sum = !get_field(*env->mstatus, MSTATUS_PUM);
> +        vm = get_field(*env->mstatus, MSTATUS_VM);
>          switch (vm) {
>          case VM_1_09_SV32:
>            levels = 2; ptidxbits = 10; ptesize = 4; break;
> @@ -492,8 +492,8 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>      ret = get_physical_address(env, &pa, &prot, address, access_type, mmu_idx);
>
>      if (mode == PRV_M && access_type != MMU_INST_FETCH) {
> -        if (get_field(env->mstatus, MSTATUS_MPRV)) {
> -            mode = get_field(env->mstatus, MSTATUS_MPP);
> +        if (get_field(*env->mstatus, MSTATUS_MPRV)) {
> +            mode = get_field(*env->mstatus, MSTATUS_MPP);
>          }
>      }
>
> @@ -599,12 +599,12 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>      if (env->priv <= PRV_S &&
>              cause < TARGET_LONG_BITS && ((deleg >> cause) & 1)) {
>          /* handle the trap in S-mode */
> -        target_ulong s = env->mstatus;
> +        target_ulong s = *env->mstatus;
>          s = set_field(s, MSTATUS_SPIE, env->priv_ver >= PRIV_VERSION_1_10_0 ?
>              get_field(s, MSTATUS_SIE) : get_field(s, MSTATUS_UIE << env->priv));
>          s = set_field(s, MSTATUS_SPP, env->priv);
>          s = set_field(s, MSTATUS_SIE, 0);
> -        env->mstatus = s;
> +        *env->mstatus = s;
>          env->scause = cause | ((target_ulong)async << (TARGET_LONG_BITS - 1));
>          env->sepc = env->pc;
>          env->sbadaddr = tval;
> @@ -613,12 +613,12 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>          riscv_cpu_set_mode(env, PRV_S);
>      } else {
>          /* handle the trap in M-mode */
> -        target_ulong s = env->mstatus;
> +        target_ulong s = *env->mstatus;
>          s = set_field(s, MSTATUS_MPIE, env->priv_ver >= PRIV_VERSION_1_10_0 ?
>              get_field(s, MSTATUS_MIE) : get_field(s, MSTATUS_UIE << env->priv));
>          s = set_field(s, MSTATUS_MPP, env->priv);
>          s = set_field(s, MSTATUS_MIE, 0);
> -        env->mstatus = s;
> +        *env->mstatus = s;
>          env->mcause = cause | ~(((target_ulong)-1) >> async);
>          env->mepc = env->pc;
>          env->mbadaddr = tval;
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 74e911af08..a4b598d49a 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -136,7 +136,7 @@ static int write_fflags(CPURISCVState *env, int csrno, target_ulong val)
>      if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
>          return -1;
>      }
> -    env->mstatus |= MSTATUS_FS;
> +    *env->mstatus |= MSTATUS_FS;
>  #endif
>      riscv_cpu_set_fflags(env, val & (FSR_AEXC >> FSR_AEXC_SHIFT));
>      return 0;
> @@ -159,7 +159,7 @@ static int write_frm(CPURISCVState *env, int csrno, target_ulong val)
>      if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
>          return -1;
>      }
> -    env->mstatus |= MSTATUS_FS;
> +    *env->mstatus |= MSTATUS_FS;
>  #endif
>      env->frm = val & (FSR_RD >> FSR_RD_SHIFT);
>      return 0;
> @@ -183,7 +183,7 @@ static int write_fcsr(CPURISCVState *env, int csrno, target_ulong val)
>      if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
>          return -1;
>      }
> -    env->mstatus |= MSTATUS_FS;
> +    *env->mstatus |= MSTATUS_FS;
>  #endif
>      env->frm = (val & FSR_RD) >> FSR_RD_SHIFT;
>      riscv_cpu_set_fflags(env, (val & FSR_AEXC) >> FSR_AEXC_SHIFT);
> @@ -313,7 +313,7 @@ static int read_mhartid(CPURISCVState *env, int csrno, target_ulong *val)
>  /* Machine Trap Setup */
>  static int read_mstatus(CPURISCVState *env, int csrno, target_ulong *val)
>  {
> -    *val = env->mstatus;
> +    *val = *env->mstatus;
>      return 0;
>  }
>
> @@ -325,7 +325,7 @@ static int validate_vm(CPURISCVState *env, target_ulong vm)
>
>  static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
>  {
> -    target_ulong mstatus = env->mstatus;
> +    target_ulong mstatus = *env->mstatus;
>      target_ulong mask = 0;
>      int dirty;
>
> @@ -365,7 +365,7 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
>               ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
>              ((mstatus & MSTATUS_XS) == MSTATUS_XS);
>      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
> -    env->mstatus = mstatus;
> +    *env->mstatus = mstatus;
>
>      return 0;
>  }
> @@ -614,7 +614,7 @@ static int read_sstatus(CPURISCVState *env, int csrno, target_ulong *val)
>  {
>      target_ulong mask = ((env->priv_ver >= PRIV_VERSION_1_10_0) ?
>                           sstatus_v1_10_mask : sstatus_v1_9_mask);
> -    *val = env->mstatus & mask;
> +    *val = *env->mstatus & mask;
>      return 0;
>  }
>
> @@ -622,7 +622,7 @@ static int write_sstatus(CPURISCVState *env, int csrno, target_ulong val)
>  {
>      target_ulong mask = ((env->priv_ver >= PRIV_VERSION_1_10_0) ?
>                           sstatus_v1_10_mask : sstatus_v1_9_mask);
> -    target_ulong newval = (env->mstatus & ~mask) | (val & mask);
> +    target_ulong newval = (*env->mstatus & ~mask) | (val & mask);
>      return write_mstatus(env, CSR_MSTATUS, newval);
>  }
>
> @@ -737,7 +737,7 @@ static int read_satp(CPURISCVState *env, int csrno, target_ulong *val)
>      if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
>          *val = 0;
>      } else if (env->priv_ver >= PRIV_VERSION_1_10_0) {
> -        if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
> +        if (env->priv == PRV_S && get_field(*env->mstatus, MSTATUS_TVM)) {
>              return -1;
>          } else {
>              *val = env->satp;
> @@ -762,7 +762,7 @@ static int write_satp(CPURISCVState *env, int csrno, target_ulong val)
>          validate_vm(env, get_field(val, SATP_MODE)) &&
>          ((val ^ env->satp) & (SATP_MODE | SATP_ASID | SATP_PPN)))
>      {
> -        if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
> +        if (env->priv == PRV_S && get_field(*env->mstatus, MSTATUS_TVM)) {
>              return -1;
>          } else {
>              if((val ^ env->satp) & SATP_ASID) {
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 331cc36232..d150551bc9 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -83,11 +83,11 @@ target_ulong helper_sret(CPURISCVState *env, target_ulong cpu_pc_deb)
>      }
>
>      if (env->priv_ver >= PRIV_VERSION_1_10_0 &&
> -        get_field(env->mstatus, MSTATUS_TSR)) {
> +        get_field(*env->mstatus, MSTATUS_TSR)) {
>          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
>      }
>
> -    target_ulong mstatus = env->mstatus;
> +    target_ulong mstatus = *env->mstatus;
>      target_ulong prev_priv = get_field(mstatus, MSTATUS_SPP);
>      mstatus = set_field(mstatus,
>          env->priv_ver >= PRIV_VERSION_1_10_0 ?
> @@ -96,7 +96,7 @@ target_ulong helper_sret(CPURISCVState *env, target_ulong cpu_pc_deb)
>      mstatus = set_field(mstatus, MSTATUS_SPIE, 0);
>      mstatus = set_field(mstatus, MSTATUS_SPP, PRV_U);
>      riscv_cpu_set_mode(env, prev_priv);
> -    env->mstatus = mstatus;
> +    *env->mstatus = mstatus;
>
>      return retpc;
>  }
> @@ -112,7 +112,7 @@ target_ulong helper_mret(CPURISCVState *env, target_ulong cpu_pc_deb)
>          riscv_raise_exception(env, RISCV_EXCP_INST_ADDR_MIS, GETPC());
>      }
>
> -    target_ulong mstatus = env->mstatus;
> +    target_ulong mstatus = *env->mstatus;
>      target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP);
>      mstatus = set_field(mstatus,
>          env->priv_ver >= PRIV_VERSION_1_10_0 ?
> @@ -121,7 +121,7 @@ target_ulong helper_mret(CPURISCVState *env, target_ulong cpu_pc_deb)
>      mstatus = set_field(mstatus, MSTATUS_MPIE, 0);
>      mstatus = set_field(mstatus, MSTATUS_MPP, PRV_U);
>      riscv_cpu_set_mode(env, prev_priv);
> -    env->mstatus = mstatus;
> +    *env->mstatus = mstatus;
>
>      return retpc;
>  }
> @@ -132,7 +132,7 @@ void helper_wfi(CPURISCVState *env)
>
>      if (env->priv == PRV_S &&
>          env->priv_ver >= PRIV_VERSION_1_10_0 &&
> -        get_field(env->mstatus, MSTATUS_TW)) {
> +        get_field(*env->mstatus, MSTATUS_TW)) {
>          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
>      } else {
>          cs->halted = 1;
> @@ -147,7 +147,7 @@ void helper_tlb_flush(CPURISCVState *env)
>      if (!(env->priv >= PRV_S) ||
>          (env->priv == PRV_S &&
>           env->priv_ver >= PRIV_VERSION_1_10_0 &&
> -         get_field(env->mstatus, MSTATUS_TVM))) {
> +         get_field(*env->mstatus, MSTATUS_TVM))) {
>          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
>      } else {
>          tlb_flush(cs);
Alistair Francis Jan. 21, 2020, 11:02 a.m. UTC | #2
On Wed, Jan 8, 2020 at 11:30 AM Palmer Dabbelt <palmerdabbelt@google.com> wrote:
>
> On Mon, 09 Dec 2019 10:11:19 PST (-0800), Alistair Francis wrote:
> > To handle the new Hypervisor CSR register aliasing let's use pointers.
>
> For some reason I thought we were making this explicit?  In other words,
> requiring that all callers provide which privilege mode they're using when
> accessing these CSRs, as opposed to swapping around pointers.  I don't actually
> care that much, but IIRC when we were talking with the ARM guys at Plumbers
> they were pretty adament that would end up being a much cleaner implementation
> as they'd tried this way and later changed over.

I think their implementation is different so it doesn't apply the same here.

My main concern is that due to the modularity of RISC-V I don't expect
all future developers to keep track of the Hypervisor extensions. This
way we always have the correct state in the registers.

There is only one pointer variable left, so we could drop the pointer
swapping part, but for now it's still here.

Alistair

>
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  target/riscv/cpu.c        | 11 +++++++++--
> >  target/riscv/cpu.h        |  9 ++++++++-
> >  target/riscv/cpu_helper.c | 30 +++++++++++++++---------------
> >  target/riscv/csr.c        | 20 ++++++++++----------
> >  target/riscv/op_helper.c  | 14 +++++++-------
> >  5 files changed, 49 insertions(+), 35 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index a07c5689b3..e61cf46a73 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -236,7 +236,7 @@ static void riscv_cpu_dump_state(CPUState *cs, FILE *f, int flags)
> >      qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "pc      ", env->pc);
> >  #ifndef CONFIG_USER_ONLY
> >      qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mhartid ", env->mhartid);
> > -    qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ", env->mstatus);
> > +    qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ", *env->mstatus);
> >      if (riscv_has_ext(env, RVH)) {
> >          qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "hstatus ", env->hstatus);
> >          qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "vsstatus ", env->vsstatus);
> > @@ -336,7 +336,7 @@ static void riscv_cpu_reset(CPUState *cs)
> >      mcc->parent_reset(cs);
> >  #ifndef CONFIG_USER_ONLY
> >      env->priv = PRV_M;
> > -    env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
> > +    *env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
> >      env->mcause = 0;
> >      env->pc = env->resetvec;
> >  #endif
> > @@ -465,8 +465,15 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> >  static void riscv_cpu_init(Object *obj)
> >  {
> >      RISCVCPU *cpu = RISCV_CPU(obj);
> > +#ifndef CONFIG_USER_ONLY
> > +    CPURISCVState *env = &cpu->env;
> > +#endif
> >
> >      cpu_set_cpustate_pointers(cpu);
> > +
> > +#ifndef CONFIG_USER_ONLY
> > +    env->mstatus = &env->mstatus_novirt;
> > +#endif
> >  }
> >
> >  static const VMStateDescription vmstate_riscv_cpu = {
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 21ae5a8b19..9dc8303c62 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -122,7 +122,7 @@ struct CPURISCVState {
> >      target_ulong resetvec;
> >
> >      target_ulong mhartid;
> > -    target_ulong mstatus;
> > +    target_ulong *mstatus;
> >
> >      target_ulong mip;
> >      uint32_t miclaim;
> > @@ -145,6 +145,13 @@ struct CPURISCVState {
> >      target_ulong mcause;
> >      target_ulong mtval;  /* since: priv-1.10.0 */
> >
> > +    /* The following registers are the "real" versions that the pointer
> > +     * versions point to. These should never be used unless you know what you
> > +     * are doing. To access these use the pointer versions instead. This is
> > +     * required to handle the Hypervisor register swapping.
> > +     */
> > +    target_ulong mstatus_novirt;
> > +
> >      /* Hypervisor CSRs */
> >      target_ulong hstatus;
> >      target_ulong hedeleg;
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index b00f66824a..9684da7f7d 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -37,8 +37,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
> >  #ifndef CONFIG_USER_ONLY
> >  static int riscv_cpu_local_irq_pending(CPURISCVState *env)
> >  {
> > -    target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE);
> > -    target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE);
> > +    target_ulong mstatus_mie = get_field(*env->mstatus, MSTATUS_MIE);
> > +    target_ulong mstatus_sie = get_field(*env->mstatus, MSTATUS_SIE);
> >      target_ulong pending = env->mip & env->mie;
> >      target_ulong mie = env->priv < PRV_M || (env->priv == PRV_M && mstatus_mie);
> >      target_ulong sie = env->priv < PRV_S || (env->priv == PRV_S && mstatus_sie);
> > @@ -75,7 +75,7 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> >  /* Return true is floating point support is currently enabled */
> >  bool riscv_cpu_fp_enabled(CPURISCVState *env)
> >  {
> > -    if (env->mstatus & MSTATUS_FS) {
> > +    if (*env->mstatus & MSTATUS_FS) {
> >          return true;
> >      }
> >
> > @@ -198,8 +198,8 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
> >      int mode = mmu_idx;
> >
> >      if (mode == PRV_M && access_type != MMU_INST_FETCH) {
> > -        if (get_field(env->mstatus, MSTATUS_MPRV)) {
> > -            mode = get_field(env->mstatus, MSTATUS_MPP);
> > +        if (get_field(*env->mstatus, MSTATUS_MPRV)) {
> > +            mode = get_field(*env->mstatus, MSTATUS_MPP);
> >          }
> >      }
> >
> > @@ -213,11 +213,11 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
> >
> >      hwaddr base;
> >      int levels, ptidxbits, ptesize, vm, sum;
> > -    int mxr = get_field(env->mstatus, MSTATUS_MXR);
> > +    int mxr = get_field(*env->mstatus, MSTATUS_MXR);
> >
> >      if (env->priv_ver >= PRIV_VERSION_1_10_0) {
> >          base = (hwaddr)get_field(env->satp, SATP_PPN) << PGSHIFT;
> > -        sum = get_field(env->mstatus, MSTATUS_SUM);
> > +        sum = get_field(*env->mstatus, MSTATUS_SUM);
> >          vm = get_field(env->satp, SATP_MODE);
> >          switch (vm) {
> >          case VM_1_10_SV32:
> > @@ -237,8 +237,8 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
> >          }
> >      } else {
> >          base = (hwaddr)(env->sptbr) << PGSHIFT;
> > -        sum = !get_field(env->mstatus, MSTATUS_PUM);
> > -        vm = get_field(env->mstatus, MSTATUS_VM);
> > +        sum = !get_field(*env->mstatus, MSTATUS_PUM);
> > +        vm = get_field(*env->mstatus, MSTATUS_VM);
> >          switch (vm) {
> >          case VM_1_09_SV32:
> >            levels = 2; ptidxbits = 10; ptesize = 4; break;
> > @@ -492,8 +492,8 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> >      ret = get_physical_address(env, &pa, &prot, address, access_type, mmu_idx);
> >
> >      if (mode == PRV_M && access_type != MMU_INST_FETCH) {
> > -        if (get_field(env->mstatus, MSTATUS_MPRV)) {
> > -            mode = get_field(env->mstatus, MSTATUS_MPP);
> > +        if (get_field(*env->mstatus, MSTATUS_MPRV)) {
> > +            mode = get_field(*env->mstatus, MSTATUS_MPP);
> >          }
> >      }
> >
> > @@ -599,12 +599,12 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> >      if (env->priv <= PRV_S &&
> >              cause < TARGET_LONG_BITS && ((deleg >> cause) & 1)) {
> >          /* handle the trap in S-mode */
> > -        target_ulong s = env->mstatus;
> > +        target_ulong s = *env->mstatus;
> >          s = set_field(s, MSTATUS_SPIE, env->priv_ver >= PRIV_VERSION_1_10_0 ?
> >              get_field(s, MSTATUS_SIE) : get_field(s, MSTATUS_UIE << env->priv));
> >          s = set_field(s, MSTATUS_SPP, env->priv);
> >          s = set_field(s, MSTATUS_SIE, 0);
> > -        env->mstatus = s;
> > +        *env->mstatus = s;
> >          env->scause = cause | ((target_ulong)async << (TARGET_LONG_BITS - 1));
> >          env->sepc = env->pc;
> >          env->sbadaddr = tval;
> > @@ -613,12 +613,12 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> >          riscv_cpu_set_mode(env, PRV_S);
> >      } else {
> >          /* handle the trap in M-mode */
> > -        target_ulong s = env->mstatus;
> > +        target_ulong s = *env->mstatus;
> >          s = set_field(s, MSTATUS_MPIE, env->priv_ver >= PRIV_VERSION_1_10_0 ?
> >              get_field(s, MSTATUS_MIE) : get_field(s, MSTATUS_UIE << env->priv));
> >          s = set_field(s, MSTATUS_MPP, env->priv);
> >          s = set_field(s, MSTATUS_MIE, 0);
> > -        env->mstatus = s;
> > +        *env->mstatus = s;
> >          env->mcause = cause | ~(((target_ulong)-1) >> async);
> >          env->mepc = env->pc;
> >          env->mbadaddr = tval;
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 74e911af08..a4b598d49a 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -136,7 +136,7 @@ static int write_fflags(CPURISCVState *env, int csrno, target_ulong val)
> >      if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
> >          return -1;
> >      }
> > -    env->mstatus |= MSTATUS_FS;
> > +    *env->mstatus |= MSTATUS_FS;
> >  #endif
> >      riscv_cpu_set_fflags(env, val & (FSR_AEXC >> FSR_AEXC_SHIFT));
> >      return 0;
> > @@ -159,7 +159,7 @@ static int write_frm(CPURISCVState *env, int csrno, target_ulong val)
> >      if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
> >          return -1;
> >      }
> > -    env->mstatus |= MSTATUS_FS;
> > +    *env->mstatus |= MSTATUS_FS;
> >  #endif
> >      env->frm = val & (FSR_RD >> FSR_RD_SHIFT);
> >      return 0;
> > @@ -183,7 +183,7 @@ static int write_fcsr(CPURISCVState *env, int csrno, target_ulong val)
> >      if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
> >          return -1;
> >      }
> > -    env->mstatus |= MSTATUS_FS;
> > +    *env->mstatus |= MSTATUS_FS;
> >  #endif
> >      env->frm = (val & FSR_RD) >> FSR_RD_SHIFT;
> >      riscv_cpu_set_fflags(env, (val & FSR_AEXC) >> FSR_AEXC_SHIFT);
> > @@ -313,7 +313,7 @@ static int read_mhartid(CPURISCVState *env, int csrno, target_ulong *val)
> >  /* Machine Trap Setup */
> >  static int read_mstatus(CPURISCVState *env, int csrno, target_ulong *val)
> >  {
> > -    *val = env->mstatus;
> > +    *val = *env->mstatus;
> >      return 0;
> >  }
> >
> > @@ -325,7 +325,7 @@ static int validate_vm(CPURISCVState *env, target_ulong vm)
> >
> >  static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> >  {
> > -    target_ulong mstatus = env->mstatus;
> > +    target_ulong mstatus = *env->mstatus;
> >      target_ulong mask = 0;
> >      int dirty;
> >
> > @@ -365,7 +365,7 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> >               ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
> >              ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> >      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
> > -    env->mstatus = mstatus;
> > +    *env->mstatus = mstatus;
> >
> >      return 0;
> >  }
> > @@ -614,7 +614,7 @@ static int read_sstatus(CPURISCVState *env, int csrno, target_ulong *val)
> >  {
> >      target_ulong mask = ((env->priv_ver >= PRIV_VERSION_1_10_0) ?
> >                           sstatus_v1_10_mask : sstatus_v1_9_mask);
> > -    *val = env->mstatus & mask;
> > +    *val = *env->mstatus & mask;
> >      return 0;
> >  }
> >
> > @@ -622,7 +622,7 @@ static int write_sstatus(CPURISCVState *env, int csrno, target_ulong val)
> >  {
> >      target_ulong mask = ((env->priv_ver >= PRIV_VERSION_1_10_0) ?
> >                           sstatus_v1_10_mask : sstatus_v1_9_mask);
> > -    target_ulong newval = (env->mstatus & ~mask) | (val & mask);
> > +    target_ulong newval = (*env->mstatus & ~mask) | (val & mask);
> >      return write_mstatus(env, CSR_MSTATUS, newval);
> >  }
> >
> > @@ -737,7 +737,7 @@ static int read_satp(CPURISCVState *env, int csrno, target_ulong *val)
> >      if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
> >          *val = 0;
> >      } else if (env->priv_ver >= PRIV_VERSION_1_10_0) {
> > -        if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
> > +        if (env->priv == PRV_S && get_field(*env->mstatus, MSTATUS_TVM)) {
> >              return -1;
> >          } else {
> >              *val = env->satp;
> > @@ -762,7 +762,7 @@ static int write_satp(CPURISCVState *env, int csrno, target_ulong val)
> >          validate_vm(env, get_field(val, SATP_MODE)) &&
> >          ((val ^ env->satp) & (SATP_MODE | SATP_ASID | SATP_PPN)))
> >      {
> > -        if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
> > +        if (env->priv == PRV_S && get_field(*env->mstatus, MSTATUS_TVM)) {
> >              return -1;
> >          } else {
> >              if((val ^ env->satp) & SATP_ASID) {
> > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> > index 331cc36232..d150551bc9 100644
> > --- a/target/riscv/op_helper.c
> > +++ b/target/riscv/op_helper.c
> > @@ -83,11 +83,11 @@ target_ulong helper_sret(CPURISCVState *env, target_ulong cpu_pc_deb)
> >      }
> >
> >      if (env->priv_ver >= PRIV_VERSION_1_10_0 &&
> > -        get_field(env->mstatus, MSTATUS_TSR)) {
> > +        get_field(*env->mstatus, MSTATUS_TSR)) {
> >          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> >      }
> >
> > -    target_ulong mstatus = env->mstatus;
> > +    target_ulong mstatus = *env->mstatus;
> >      target_ulong prev_priv = get_field(mstatus, MSTATUS_SPP);
> >      mstatus = set_field(mstatus,
> >          env->priv_ver >= PRIV_VERSION_1_10_0 ?
> > @@ -96,7 +96,7 @@ target_ulong helper_sret(CPURISCVState *env, target_ulong cpu_pc_deb)
> >      mstatus = set_field(mstatus, MSTATUS_SPIE, 0);
> >      mstatus = set_field(mstatus, MSTATUS_SPP, PRV_U);
> >      riscv_cpu_set_mode(env, prev_priv);
> > -    env->mstatus = mstatus;
> > +    *env->mstatus = mstatus;
> >
> >      return retpc;
> >  }
> > @@ -112,7 +112,7 @@ target_ulong helper_mret(CPURISCVState *env, target_ulong cpu_pc_deb)
> >          riscv_raise_exception(env, RISCV_EXCP_INST_ADDR_MIS, GETPC());
> >      }
> >
> > -    target_ulong mstatus = env->mstatus;
> > +    target_ulong mstatus = *env->mstatus;
> >      target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP);
> >      mstatus = set_field(mstatus,
> >          env->priv_ver >= PRIV_VERSION_1_10_0 ?
> > @@ -121,7 +121,7 @@ target_ulong helper_mret(CPURISCVState *env, target_ulong cpu_pc_deb)
> >      mstatus = set_field(mstatus, MSTATUS_MPIE, 0);
> >      mstatus = set_field(mstatus, MSTATUS_MPP, PRV_U);
> >      riscv_cpu_set_mode(env, prev_priv);
> > -    env->mstatus = mstatus;
> > +    *env->mstatus = mstatus;
> >
> >      return retpc;
> >  }
> > @@ -132,7 +132,7 @@ void helper_wfi(CPURISCVState *env)
> >
> >      if (env->priv == PRV_S &&
> >          env->priv_ver >= PRIV_VERSION_1_10_0 &&
> > -        get_field(env->mstatus, MSTATUS_TW)) {
> > +        get_field(*env->mstatus, MSTATUS_TW)) {
> >          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> >      } else {
> >          cs->halted = 1;
> > @@ -147,7 +147,7 @@ void helper_tlb_flush(CPURISCVState *env)
> >      if (!(env->priv >= PRV_S) ||
> >          (env->priv == PRV_S &&
> >           env->priv_ver >= PRIV_VERSION_1_10_0 &&
> > -         get_field(env->mstatus, MSTATUS_TVM))) {
> > +         get_field(*env->mstatus, MSTATUS_TVM))) {
> >          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> >      } else {
> >          tlb_flush(cs);
Jonathan Behrens Jan. 21, 2020, 12:56 p.m. UTC | #3
When I looked through the relevant code a few months ago, I couldn't find
anywhere that could actually be agnostic to whether the real or virtual
registers were in effect (other than emulating the actual CSR modification
instructions). For almost all state, the VS behavior is filtered by HS-mode
code. For example, you can grab satp in either mode but to properly do
address translation you also have to factor in hgatp so you need to know
the virtualization state anyway. Similarly, floating point co-proccessor
state is tracked in two places with V=1 so that both the host and the guest
can independently track dirty bits, but of course only the one in the
"real" mstatus applies in non-virtualized mode.

The idea of using an array to track the two versions of registers seemed
really elegant to me. If you know you want the host version you access
element zero. If you know you want the guest version you access element
one. And if you want the version that the running code would see you index
by the virtualization mode. In any case, the choice indicates that you
thought though which was the right option to use in that instance.

Jonathan

On Tue, Jan 21, 2020 at 6:02 AM Alistair Francis <alistair23@gmail.com>
wrote:

> On Wed, Jan 8, 2020 at 11:30 AM Palmer Dabbelt <palmerdabbelt@google.com>
> wrote:
> >
> > On Mon, 09 Dec 2019 10:11:19 PST (-0800), Alistair Francis wrote:
> > > To handle the new Hypervisor CSR register aliasing let's use pointers.
> >
> > For some reason I thought we were making this explicit?  In other words,
> > requiring that all callers provide which privilege mode they're using
> when
> > accessing these CSRs, as opposed to swapping around pointers.  I don't
> actually
> > care that much, but IIRC when we were talking with the ARM guys at
> Plumbers
> > they were pretty adament that would end up being a much cleaner
> implementation
> > as they'd tried this way and later changed over.
>
> I think their implementation is different so it doesn't apply the same
> here.
>
> My main concern is that due to the modularity of RISC-V I don't expect
> all future developers to keep track of the Hypervisor extensions. This
> way we always have the correct state in the registers.
>
> There is only one pointer variable left, so we could drop the pointer
> swapping part, but for now it's still here.
>
> Alistair
>
> >
> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > ---
> > >  target/riscv/cpu.c        | 11 +++++++++--
> > >  target/riscv/cpu.h        |  9 ++++++++-
> > >  target/riscv/cpu_helper.c | 30 +++++++++++++++---------------
> > >  target/riscv/csr.c        | 20 ++++++++++----------
> > >  target/riscv/op_helper.c  | 14 +++++++-------
> > >  5 files changed, 49 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index a07c5689b3..e61cf46a73 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -236,7 +236,7 @@ static void riscv_cpu_dump_state(CPUState *cs,
> FILE *f, int flags)
> > >      qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "pc      ", env->pc);
> > >  #ifndef CONFIG_USER_ONLY
> > >      qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mhartid ",
> env->mhartid);
> > > -    qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ",
> env->mstatus);
> > > +    qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ",
> *env->mstatus);
> > >      if (riscv_has_ext(env, RVH)) {
> > >          qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "hstatus ",
> env->hstatus);
> > >          qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "vsstatus ",
> env->vsstatus);
> > > @@ -336,7 +336,7 @@ static void riscv_cpu_reset(CPUState *cs)
> > >      mcc->parent_reset(cs);
> > >  #ifndef CONFIG_USER_ONLY
> > >      env->priv = PRV_M;
> > > -    env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
> > > +    *env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
> > >      env->mcause = 0;
> > >      env->pc = env->resetvec;
> > >  #endif
> > > @@ -465,8 +465,15 @@ static void riscv_cpu_realize(DeviceState *dev,
> Error **errp)
> > >  static void riscv_cpu_init(Object *obj)
> > >  {
> > >      RISCVCPU *cpu = RISCV_CPU(obj);
> > > +#ifndef CONFIG_USER_ONLY
> > > +    CPURISCVState *env = &cpu->env;
> > > +#endif
> > >
> > >      cpu_set_cpustate_pointers(cpu);
> > > +
> > > +#ifndef CONFIG_USER_ONLY
> > > +    env->mstatus = &env->mstatus_novirt;
> > > +#endif
> > >  }
> > >
> > >  static const VMStateDescription vmstate_riscv_cpu = {
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index 21ae5a8b19..9dc8303c62 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -122,7 +122,7 @@ struct CPURISCVState {
> > >      target_ulong resetvec;
> > >
> > >      target_ulong mhartid;
> > > -    target_ulong mstatus;
> > > +    target_ulong *mstatus;
> > >
> > >      target_ulong mip;
> > >      uint32_t miclaim;
> > > @@ -145,6 +145,13 @@ struct CPURISCVState {
> > >      target_ulong mcause;
> > >      target_ulong mtval;  /* since: priv-1.10.0 */
> > >
> > > +    /* The following registers are the "real" versions that the
> pointer
> > > +     * versions point to. These should never be used unless you know
> what you
> > > +     * are doing. To access these use the pointer versions instead.
> This is
> > > +     * required to handle the Hypervisor register swapping.
> > > +     */
> > > +    target_ulong mstatus_novirt;
> > > +
> > >      /* Hypervisor CSRs */
> > >      target_ulong hstatus;
> > >      target_ulong hedeleg;
> > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > index b00f66824a..9684da7f7d 100644
> > > --- a/target/riscv/cpu_helper.c
> > > +++ b/target/riscv/cpu_helper.c
> > > @@ -37,8 +37,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool
> ifetch)
> > >  #ifndef CONFIG_USER_ONLY
> > >  static int riscv_cpu_local_irq_pending(CPURISCVState *env)
> > >  {
> > > -    target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE);
> > > -    target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE);
> > > +    target_ulong mstatus_mie = get_field(*env->mstatus, MSTATUS_MIE);
> > > +    target_ulong mstatus_sie = get_field(*env->mstatus, MSTATUS_SIE);
> > >      target_ulong pending = env->mip & env->mie;
> > >      target_ulong mie = env->priv < PRV_M || (env->priv == PRV_M &&
> mstatus_mie);
> > >      target_ulong sie = env->priv < PRV_S || (env->priv == PRV_S &&
> mstatus_sie);
> > > @@ -75,7 +75,7 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int
> interrupt_request)
> > >  /* Return true is floating point support is currently enabled */
> > >  bool riscv_cpu_fp_enabled(CPURISCVState *env)
> > >  {
> > > -    if (env->mstatus & MSTATUS_FS) {
> > > +    if (*env->mstatus & MSTATUS_FS) {
> > >          return true;
> > >      }
> > >
> > > @@ -198,8 +198,8 @@ static int get_physical_address(CPURISCVState
> *env, hwaddr *physical,
> > >      int mode = mmu_idx;
> > >
> > >      if (mode == PRV_M && access_type != MMU_INST_FETCH) {
> > > -        if (get_field(env->mstatus, MSTATUS_MPRV)) {
> > > -            mode = get_field(env->mstatus, MSTATUS_MPP);
> > > +        if (get_field(*env->mstatus, MSTATUS_MPRV)) {
> > > +            mode = get_field(*env->mstatus, MSTATUS_MPP);
> > >          }
> > >      }
> > >
> > > @@ -213,11 +213,11 @@ static int get_physical_address(CPURISCVState
> *env, hwaddr *physical,
> > >
> > >      hwaddr base;
> > >      int levels, ptidxbits, ptesize, vm, sum;
> > > -    int mxr = get_field(env->mstatus, MSTATUS_MXR);
> > > +    int mxr = get_field(*env->mstatus, MSTATUS_MXR);
> > >
> > >      if (env->priv_ver >= PRIV_VERSION_1_10_0) {
> > >          base = (hwaddr)get_field(env->satp, SATP_PPN) << PGSHIFT;
> > > -        sum = get_field(env->mstatus, MSTATUS_SUM);
> > > +        sum = get_field(*env->mstatus, MSTATUS_SUM);
> > >          vm = get_field(env->satp, SATP_MODE);
> > >          switch (vm) {
> > >          case VM_1_10_SV32:
> > > @@ -237,8 +237,8 @@ static int get_physical_address(CPURISCVState
> *env, hwaddr *physical,
> > >          }
> > >      } else {
> > >          base = (hwaddr)(env->sptbr) << PGSHIFT;
> > > -        sum = !get_field(env->mstatus, MSTATUS_PUM);
> > > -        vm = get_field(env->mstatus, MSTATUS_VM);
> > > +        sum = !get_field(*env->mstatus, MSTATUS_PUM);
> > > +        vm = get_field(*env->mstatus, MSTATUS_VM);
> > >          switch (vm) {
> > >          case VM_1_09_SV32:
> > >            levels = 2; ptidxbits = 10; ptesize = 4; break;
> > > @@ -492,8 +492,8 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr
> address, int size,
> > >      ret = get_physical_address(env, &pa, &prot, address, access_type,
> mmu_idx);
> > >
> > >      if (mode == PRV_M && access_type != MMU_INST_FETCH) {
> > > -        if (get_field(env->mstatus, MSTATUS_MPRV)) {
> > > -            mode = get_field(env->mstatus, MSTATUS_MPP);
> > > +        if (get_field(*env->mstatus, MSTATUS_MPRV)) {
> > > +            mode = get_field(*env->mstatus, MSTATUS_MPP);
> > >          }
> > >      }
> > >
> > > @@ -599,12 +599,12 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> > >      if (env->priv <= PRV_S &&
> > >              cause < TARGET_LONG_BITS && ((deleg >> cause) & 1)) {
> > >          /* handle the trap in S-mode */
> > > -        target_ulong s = env->mstatus;
> > > +        target_ulong s = *env->mstatus;
> > >          s = set_field(s, MSTATUS_SPIE, env->priv_ver >=
> PRIV_VERSION_1_10_0 ?
> > >              get_field(s, MSTATUS_SIE) : get_field(s, MSTATUS_UIE <<
> env->priv));
> > >          s = set_field(s, MSTATUS_SPP, env->priv);
> > >          s = set_field(s, MSTATUS_SIE, 0);
> > > -        env->mstatus = s;
> > > +        *env->mstatus = s;
> > >          env->scause = cause | ((target_ulong)async <<
> (TARGET_LONG_BITS - 1));
> > >          env->sepc = env->pc;
> > >          env->sbadaddr = tval;
> > > @@ -613,12 +613,12 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> > >          riscv_cpu_set_mode(env, PRV_S);
> > >      } else {
> > >          /* handle the trap in M-mode */
> > > -        target_ulong s = env->mstatus;
> > > +        target_ulong s = *env->mstatus;
> > >          s = set_field(s, MSTATUS_MPIE, env->priv_ver >=
> PRIV_VERSION_1_10_0 ?
> > >              get_field(s, MSTATUS_MIE) : get_field(s, MSTATUS_UIE <<
> env->priv));
> > >          s = set_field(s, MSTATUS_MPP, env->priv);
> > >          s = set_field(s, MSTATUS_MIE, 0);
> > > -        env->mstatus = s;
> > > +        *env->mstatus = s;
> > >          env->mcause = cause | ~(((target_ulong)-1) >> async);
> > >          env->mepc = env->pc;
> > >          env->mbadaddr = tval;
> > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > index 74e911af08..a4b598d49a 100644
> > > --- a/target/riscv/csr.c
> > > +++ b/target/riscv/csr.c
> > > @@ -136,7 +136,7 @@ static int write_fflags(CPURISCVState *env, int
> csrno, target_ulong val)
> > >      if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
> > >          return -1;
> > >      }
> > > -    env->mstatus |= MSTATUS_FS;
> > > +    *env->mstatus |= MSTATUS_FS;
> > >  #endif
> > >      riscv_cpu_set_fflags(env, val & (FSR_AEXC >> FSR_AEXC_SHIFT));
> > >      return 0;
> > > @@ -159,7 +159,7 @@ static int write_frm(CPURISCVState *env, int
> csrno, target_ulong val)
> > >      if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
> > >          return -1;
> > >      }
> > > -    env->mstatus |= MSTATUS_FS;
> > > +    *env->mstatus |= MSTATUS_FS;
> > >  #endif
> > >      env->frm = val & (FSR_RD >> FSR_RD_SHIFT);
> > >      return 0;
> > > @@ -183,7 +183,7 @@ static int write_fcsr(CPURISCVState *env, int
> csrno, target_ulong val)
> > >      if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
> > >          return -1;
> > >      }
> > > -    env->mstatus |= MSTATUS_FS;
> > > +    *env->mstatus |= MSTATUS_FS;
> > >  #endif
> > >      env->frm = (val & FSR_RD) >> FSR_RD_SHIFT;
> > >      riscv_cpu_set_fflags(env, (val & FSR_AEXC) >> FSR_AEXC_SHIFT);
> > > @@ -313,7 +313,7 @@ static int read_mhartid(CPURISCVState *env, int
> csrno, target_ulong *val)
> > >  /* Machine Trap Setup */
> > >  static int read_mstatus(CPURISCVState *env, int csrno, target_ulong
> *val)
> > >  {
> > > -    *val = env->mstatus;
> > > +    *val = *env->mstatus;
> > >      return 0;
> > >  }
> > >
> > > @@ -325,7 +325,7 @@ static int validate_vm(CPURISCVState *env,
> target_ulong vm)
> > >
> > >  static int write_mstatus(CPURISCVState *env, int csrno, target_ulong
> val)
> > >  {
> > > -    target_ulong mstatus = env->mstatus;
> > > +    target_ulong mstatus = *env->mstatus;
> > >      target_ulong mask = 0;
> > >      int dirty;
> > >
> > > @@ -365,7 +365,7 @@ static int write_mstatus(CPURISCVState *env, int
> csrno, target_ulong val)
> > >               ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
> > >              ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> > >      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
> > > -    env->mstatus = mstatus;
> > > +    *env->mstatus = mstatus;
> > >
> > >      return 0;
> > >  }
> > > @@ -614,7 +614,7 @@ static int read_sstatus(CPURISCVState *env, int
> csrno, target_ulong *val)
> > >  {
> > >      target_ulong mask = ((env->priv_ver >= PRIV_VERSION_1_10_0) ?
> > >                           sstatus_v1_10_mask : sstatus_v1_9_mask);
> > > -    *val = env->mstatus & mask;
> > > +    *val = *env->mstatus & mask;
> > >      return 0;
> > >  }
> > >
> > > @@ -622,7 +622,7 @@ static int write_sstatus(CPURISCVState *env, int
> csrno, target_ulong val)
> > >  {
> > >      target_ulong mask = ((env->priv_ver >= PRIV_VERSION_1_10_0) ?
> > >                           sstatus_v1_10_mask : sstatus_v1_9_mask);
> > > -    target_ulong newval = (env->mstatus & ~mask) | (val & mask);
> > > +    target_ulong newval = (*env->mstatus & ~mask) | (val & mask);
> > >      return write_mstatus(env, CSR_MSTATUS, newval);
> > >  }
> > >
> > > @@ -737,7 +737,7 @@ static int read_satp(CPURISCVState *env, int
> csrno, target_ulong *val)
> > >      if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
> > >          *val = 0;
> > >      } else if (env->priv_ver >= PRIV_VERSION_1_10_0) {
> > > -        if (env->priv == PRV_S && get_field(env->mstatus,
> MSTATUS_TVM)) {
> > > +        if (env->priv == PRV_S && get_field(*env->mstatus,
> MSTATUS_TVM)) {
> > >              return -1;
> > >          } else {
> > >              *val = env->satp;
> > > @@ -762,7 +762,7 @@ static int write_satp(CPURISCVState *env, int
> csrno, target_ulong val)
> > >          validate_vm(env, get_field(val, SATP_MODE)) &&
> > >          ((val ^ env->satp) & (SATP_MODE | SATP_ASID | SATP_PPN)))
> > >      {
> > > -        if (env->priv == PRV_S && get_field(env->mstatus,
> MSTATUS_TVM)) {
> > > +        if (env->priv == PRV_S && get_field(*env->mstatus,
> MSTATUS_TVM)) {
> > >              return -1;
> > >          } else {
> > >              if((val ^ env->satp) & SATP_ASID) {
> > > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> > > index 331cc36232..d150551bc9 100644
> > > --- a/target/riscv/op_helper.c
> > > +++ b/target/riscv/op_helper.c
> > > @@ -83,11 +83,11 @@ target_ulong helper_sret(CPURISCVState *env,
> target_ulong cpu_pc_deb)
> > >      }
> > >
> > >      if (env->priv_ver >= PRIV_VERSION_1_10_0 &&
> > > -        get_field(env->mstatus, MSTATUS_TSR)) {
> > > +        get_field(*env->mstatus, MSTATUS_TSR)) {
> > >          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> > >      }
> > >
> > > -    target_ulong mstatus = env->mstatus;
> > > +    target_ulong mstatus = *env->mstatus;
> > >      target_ulong prev_priv = get_field(mstatus, MSTATUS_SPP);
> > >      mstatus = set_field(mstatus,
> > >          env->priv_ver >= PRIV_VERSION_1_10_0 ?
> > > @@ -96,7 +96,7 @@ target_ulong helper_sret(CPURISCVState *env,
> target_ulong cpu_pc_deb)
> > >      mstatus = set_field(mstatus, MSTATUS_SPIE, 0);
> > >      mstatus = set_field(mstatus, MSTATUS_SPP, PRV_U);
> > >      riscv_cpu_set_mode(env, prev_priv);
> > > -    env->mstatus = mstatus;
> > > +    *env->mstatus = mstatus;
> > >
> > >      return retpc;
> > >  }
> > > @@ -112,7 +112,7 @@ target_ulong helper_mret(CPURISCVState *env,
> target_ulong cpu_pc_deb)
> > >          riscv_raise_exception(env, RISCV_EXCP_INST_ADDR_MIS, GETPC());
> > >      }
> > >
> > > -    target_ulong mstatus = env->mstatus;
> > > +    target_ulong mstatus = *env->mstatus;
> > >      target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP);
> > >      mstatus = set_field(mstatus,
> > >          env->priv_ver >= PRIV_VERSION_1_10_0 ?
> > > @@ -121,7 +121,7 @@ target_ulong helper_mret(CPURISCVState *env,
> target_ulong cpu_pc_deb)
> > >      mstatus = set_field(mstatus, MSTATUS_MPIE, 0);
> > >      mstatus = set_field(mstatus, MSTATUS_MPP, PRV_U);
> > >      riscv_cpu_set_mode(env, prev_priv);
> > > -    env->mstatus = mstatus;
> > > +    *env->mstatus = mstatus;
> > >
> > >      return retpc;
> > >  }
> > > @@ -132,7 +132,7 @@ void helper_wfi(CPURISCVState *env)
> > >
> > >      if (env->priv == PRV_S &&
> > >          env->priv_ver >= PRIV_VERSION_1_10_0 &&
> > > -        get_field(env->mstatus, MSTATUS_TW)) {
> > > +        get_field(*env->mstatus, MSTATUS_TW)) {
> > >          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> > >      } else {
> > >          cs->halted = 1;
> > > @@ -147,7 +147,7 @@ void helper_tlb_flush(CPURISCVState *env)
> > >      if (!(env->priv >= PRV_S) ||
> > >          (env->priv == PRV_S &&
> > >           env->priv_ver >= PRIV_VERSION_1_10_0 &&
> > > -         get_field(env->mstatus, MSTATUS_TVM))) {
> > > +         get_field(*env->mstatus, MSTATUS_TVM))) {
> > >          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> > >      } else {
> > >          tlb_flush(cs);
>
>
Alistair Francis Jan. 22, 2020, midnight UTC | #4
On Tue, Jan 21, 2020 at 10:56 PM Jonathan Behrens <fintelia@gmail.com> wrote:
>
> When I looked through the relevant code a few months ago, I couldn't find anywhere that could actually be agnostic to whether the real or virtual registers were in effect (other than emulating the actual CSR modification instructions). For almost all state, the VS behavior is filtered by HS-mode code. For example, you can grab satp in either mode but to properly do address translation you also have to factor in hgatp so you need to know the virtualization state anyway. Similarly, floating point co-proccessor state is tracked in two places with V=1 so that both the host and the guest can independently track dirty bits, but of course only the one in the "real" mstatus applies in non-virtualized mode.

So the idea is that if you aren't interested in the Hypervisor
extension you can just access the CSRs as usual (inside QEMU's source
code). That is you don't need to know anything about the Hypervisor
extension to add support for other extensions or to work on the RISC-V
target in QEMU.

I was trying to avoid forcing everyone to understand the Hypervisor
extension to develop for RISC-V. For example we don't have to check
virtulasition status to dump the registers, we can just dump them and
know they are in the correct state (we do dump more is we have the
extension though). Image if someone adds a new way to dump registers,
I would like them not to care about if we are in V=1 or V=0 and they
can just dump them and know that the CSRs are correct.

>
> The idea of using an array to track the two versions of registers seemed really elegant to me. If you know you want the host version you access element zero. If you know you want the guest version you access element one. And if you want the version that the running code would see you index by the virtualization mode. In any case, the choice indicates that you thought though which was the right option to use in that instance.

mstatus really is the only one that has any complications. The others
have a vs* version, which could be converted to an array but it
doesn't really matter as we just swap them.

Alistair

>
> Jonathan
>
> On Tue, Jan 21, 2020 at 6:02 AM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Wed, Jan 8, 2020 at 11:30 AM Palmer Dabbelt <palmerdabbelt@google.com> wrote:
>> >
>> > On Mon, 09 Dec 2019 10:11:19 PST (-0800), Alistair Francis wrote:
>> > > To handle the new Hypervisor CSR register aliasing let's use pointers.
>> >
>> > For some reason I thought we were making this explicit?  In other words,
>> > requiring that all callers provide which privilege mode they're using when
>> > accessing these CSRs, as opposed to swapping around pointers.  I don't actually
>> > care that much, but IIRC when we were talking with the ARM guys at Plumbers
>> > they were pretty adament that would end up being a much cleaner implementation
>> > as they'd tried this way and later changed over.
>>
>> I think their implementation is different so it doesn't apply the same here.
>>
>> My main concern is that due to the modularity of RISC-V I don't expect
>> all future developers to keep track of the Hypervisor extensions. This
>> way we always have the correct state in the registers.
>>
>> There is only one pointer variable left, so we could drop the pointer
>> swapping part, but for now it's still here.
>>
>> Alistair
>>
>> >
>> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> > > ---
>> > >  target/riscv/cpu.c        | 11 +++++++++--
>> > >  target/riscv/cpu.h        |  9 ++++++++-
>> > >  target/riscv/cpu_helper.c | 30 +++++++++++++++---------------
>> > >  target/riscv/csr.c        | 20 ++++++++++----------
>> > >  target/riscv/op_helper.c  | 14 +++++++-------
>> > >  5 files changed, 49 insertions(+), 35 deletions(-)
>> > >
>> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> > > index a07c5689b3..e61cf46a73 100644
>> > > --- a/target/riscv/cpu.c
>> > > +++ b/target/riscv/cpu.c
>> > > @@ -236,7 +236,7 @@ static void riscv_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>> > >      qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "pc      ", env->pc);
>> > >  #ifndef CONFIG_USER_ONLY
>> > >      qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mhartid ", env->mhartid);
>> > > -    qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ", env->mstatus);
>> > > +    qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ", *env->mstatus);
>> > >      if (riscv_has_ext(env, RVH)) {
>> > >          qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "hstatus ", env->hstatus);
>> > >          qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "vsstatus ", env->vsstatus);
>> > > @@ -336,7 +336,7 @@ static void riscv_cpu_reset(CPUState *cs)
>> > >      mcc->parent_reset(cs);
>> > >  #ifndef CONFIG_USER_ONLY
>> > >      env->priv = PRV_M;
>> > > -    env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
>> > > +    *env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
>> > >      env->mcause = 0;
>> > >      env->pc = env->resetvec;
>> > >  #endif
>> > > @@ -465,8 +465,15 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>> > >  static void riscv_cpu_init(Object *obj)
>> > >  {
>> > >      RISCVCPU *cpu = RISCV_CPU(obj);
>> > > +#ifndef CONFIG_USER_ONLY
>> > > +    CPURISCVState *env = &cpu->env;
>> > > +#endif
>> > >
>> > >      cpu_set_cpustate_pointers(cpu);
>> > > +
>> > > +#ifndef CONFIG_USER_ONLY
>> > > +    env->mstatus = &env->mstatus_novirt;
>> > > +#endif
>> > >  }
>> > >
>> > >  static const VMStateDescription vmstate_riscv_cpu = {
>> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> > > index 21ae5a8b19..9dc8303c62 100644
>> > > --- a/target/riscv/cpu.h
>> > > +++ b/target/riscv/cpu.h
>> > > @@ -122,7 +122,7 @@ struct CPURISCVState {
>> > >      target_ulong resetvec;
>> > >
>> > >      target_ulong mhartid;
>> > > -    target_ulong mstatus;
>> > > +    target_ulong *mstatus;
>> > >
>> > >      target_ulong mip;
>> > >      uint32_t miclaim;
>> > > @@ -145,6 +145,13 @@ struct CPURISCVState {
>> > >      target_ulong mcause;
>> > >      target_ulong mtval;  /* since: priv-1.10.0 */
>> > >
>> > > +    /* The following registers are the "real" versions that the pointer
>> > > +     * versions point to. These should never be used unless you know what you
>> > > +     * are doing. To access these use the pointer versions instead. This is
>> > > +     * required to handle the Hypervisor register swapping.
>> > > +     */
>> > > +    target_ulong mstatus_novirt;
>> > > +
>> > >      /* Hypervisor CSRs */
>> > >      target_ulong hstatus;
>> > >      target_ulong hedeleg;
>> > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> > > index b00f66824a..9684da7f7d 100644
>> > > --- a/target/riscv/cpu_helper.c
>> > > +++ b/target/riscv/cpu_helper.c
>> > > @@ -37,8 +37,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
>> > >  #ifndef CONFIG_USER_ONLY
>> > >  static int riscv_cpu_local_irq_pending(CPURISCVState *env)
>> > >  {
>> > > -    target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE);
>> > > -    target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE);
>> > > +    target_ulong mstatus_mie = get_field(*env->mstatus, MSTATUS_MIE);
>> > > +    target_ulong mstatus_sie = get_field(*env->mstatus, MSTATUS_SIE);
>> > >      target_ulong pending = env->mip & env->mie;
>> > >      target_ulong mie = env->priv < PRV_M || (env->priv == PRV_M && mstatus_mie);
>> > >      target_ulong sie = env->priv < PRV_S || (env->priv == PRV_S && mstatus_sie);
>> > > @@ -75,7 +75,7 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>> > >  /* Return true is floating point support is currently enabled */
>> > >  bool riscv_cpu_fp_enabled(CPURISCVState *env)
>> > >  {
>> > > -    if (env->mstatus & MSTATUS_FS) {
>> > > +    if (*env->mstatus & MSTATUS_FS) {
>> > >          return true;
>> > >      }
>> > >
>> > > @@ -198,8 +198,8 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>> > >      int mode = mmu_idx;
>> > >
>> > >      if (mode == PRV_M && access_type != MMU_INST_FETCH) {
>> > > -        if (get_field(env->mstatus, MSTATUS_MPRV)) {
>> > > -            mode = get_field(env->mstatus, MSTATUS_MPP);
>> > > +        if (get_field(*env->mstatus, MSTATUS_MPRV)) {
>> > > +            mode = get_field(*env->mstatus, MSTATUS_MPP);
>> > >          }
>> > >      }
>> > >
>> > > @@ -213,11 +213,11 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>> > >
>> > >      hwaddr base;
>> > >      int levels, ptidxbits, ptesize, vm, sum;
>> > > -    int mxr = get_field(env->mstatus, MSTATUS_MXR);
>> > > +    int mxr = get_field(*env->mstatus, MSTATUS_MXR);
>> > >
>> > >      if (env->priv_ver >= PRIV_VERSION_1_10_0) {
>> > >          base = (hwaddr)get_field(env->satp, SATP_PPN) << PGSHIFT;
>> > > -        sum = get_field(env->mstatus, MSTATUS_SUM);
>> > > +        sum = get_field(*env->mstatus, MSTATUS_SUM);
>> > >          vm = get_field(env->satp, SATP_MODE);
>> > >          switch (vm) {
>> > >          case VM_1_10_SV32:
>> > > @@ -237,8 +237,8 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>> > >          }
>> > >      } else {
>> > >          base = (hwaddr)(env->sptbr) << PGSHIFT;
>> > > -        sum = !get_field(env->mstatus, MSTATUS_PUM);
>> > > -        vm = get_field(env->mstatus, MSTATUS_VM);
>> > > +        sum = !get_field(*env->mstatus, MSTATUS_PUM);
>> > > +        vm = get_field(*env->mstatus, MSTATUS_VM);
>> > >          switch (vm) {
>> > >          case VM_1_09_SV32:
>> > >            levels = 2; ptidxbits = 10; ptesize = 4; break;
>> > > @@ -492,8 +492,8 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>> > >      ret = get_physical_address(env, &pa, &prot, address, access_type, mmu_idx);
>> > >
>> > >      if (mode == PRV_M && access_type != MMU_INST_FETCH) {
>> > > -        if (get_field(env->mstatus, MSTATUS_MPRV)) {
>> > > -            mode = get_field(env->mstatus, MSTATUS_MPP);
>> > > +        if (get_field(*env->mstatus, MSTATUS_MPRV)) {
>> > > +            mode = get_field(*env->mstatus, MSTATUS_MPP);
>> > >          }
>> > >      }
>> > >
>> > > @@ -599,12 +599,12 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>> > >      if (env->priv <= PRV_S &&
>> > >              cause < TARGET_LONG_BITS && ((deleg >> cause) & 1)) {
>> > >          /* handle the trap in S-mode */
>> > > -        target_ulong s = env->mstatus;
>> > > +        target_ulong s = *env->mstatus;
>> > >          s = set_field(s, MSTATUS_SPIE, env->priv_ver >= PRIV_VERSION_1_10_0 ?
>> > >              get_field(s, MSTATUS_SIE) : get_field(s, MSTATUS_UIE << env->priv));
>> > >          s = set_field(s, MSTATUS_SPP, env->priv);
>> > >          s = set_field(s, MSTATUS_SIE, 0);
>> > > -        env->mstatus = s;
>> > > +        *env->mstatus = s;
>> > >          env->scause = cause | ((target_ulong)async << (TARGET_LONG_BITS - 1));
>> > >          env->sepc = env->pc;
>> > >          env->sbadaddr = tval;
>> > > @@ -613,12 +613,12 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>> > >          riscv_cpu_set_mode(env, PRV_S);
>> > >      } else {
>> > >          /* handle the trap in M-mode */
>> > > -        target_ulong s = env->mstatus;
>> > > +        target_ulong s = *env->mstatus;
>> > >          s = set_field(s, MSTATUS_MPIE, env->priv_ver >= PRIV_VERSION_1_10_0 ?
>> > >              get_field(s, MSTATUS_MIE) : get_field(s, MSTATUS_UIE << env->priv));
>> > >          s = set_field(s, MSTATUS_MPP, env->priv);
>> > >          s = set_field(s, MSTATUS_MIE, 0);
>> > > -        env->mstatus = s;
>> > > +        *env->mstatus = s;
>> > >          env->mcause = cause | ~(((target_ulong)-1) >> async);
>> > >          env->mepc = env->pc;
>> > >          env->mbadaddr = tval;
>> > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> > > index 74e911af08..a4b598d49a 100644
>> > > --- a/target/riscv/csr.c
>> > > +++ b/target/riscv/csr.c
>> > > @@ -136,7 +136,7 @@ static int write_fflags(CPURISCVState *env, int csrno, target_ulong val)
>> > >      if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
>> > >          return -1;
>> > >      }
>> > > -    env->mstatus |= MSTATUS_FS;
>> > > +    *env->mstatus |= MSTATUS_FS;
>> > >  #endif
>> > >      riscv_cpu_set_fflags(env, val & (FSR_AEXC >> FSR_AEXC_SHIFT));
>> > >      return 0;
>> > > @@ -159,7 +159,7 @@ static int write_frm(CPURISCVState *env, int csrno, target_ulong val)
>> > >      if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
>> > >          return -1;
>> > >      }
>> > > -    env->mstatus |= MSTATUS_FS;
>> > > +    *env->mstatus |= MSTATUS_FS;
>> > >  #endif
>> > >      env->frm = val & (FSR_RD >> FSR_RD_SHIFT);
>> > >      return 0;
>> > > @@ -183,7 +183,7 @@ static int write_fcsr(CPURISCVState *env, int csrno, target_ulong val)
>> > >      if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
>> > >          return -1;
>> > >      }
>> > > -    env->mstatus |= MSTATUS_FS;
>> > > +    *env->mstatus |= MSTATUS_FS;
>> > >  #endif
>> > >      env->frm = (val & FSR_RD) >> FSR_RD_SHIFT;
>> > >      riscv_cpu_set_fflags(env, (val & FSR_AEXC) >> FSR_AEXC_SHIFT);
>> > > @@ -313,7 +313,7 @@ static int read_mhartid(CPURISCVState *env, int csrno, target_ulong *val)
>> > >  /* Machine Trap Setup */
>> > >  static int read_mstatus(CPURISCVState *env, int csrno, target_ulong *val)
>> > >  {
>> > > -    *val = env->mstatus;
>> > > +    *val = *env->mstatus;
>> > >      return 0;
>> > >  }
>> > >
>> > > @@ -325,7 +325,7 @@ static int validate_vm(CPURISCVState *env, target_ulong vm)
>> > >
>> > >  static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
>> > >  {
>> > > -    target_ulong mstatus = env->mstatus;
>> > > +    target_ulong mstatus = *env->mstatus;
>> > >      target_ulong mask = 0;
>> > >      int dirty;
>> > >
>> > > @@ -365,7 +365,7 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
>> > >               ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
>> > >              ((mstatus & MSTATUS_XS) == MSTATUS_XS);
>> > >      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
>> > > -    env->mstatus = mstatus;
>> > > +    *env->mstatus = mstatus;
>> > >
>> > >      return 0;
>> > >  }
>> > > @@ -614,7 +614,7 @@ static int read_sstatus(CPURISCVState *env, int csrno, target_ulong *val)
>> > >  {
>> > >      target_ulong mask = ((env->priv_ver >= PRIV_VERSION_1_10_0) ?
>> > >                           sstatus_v1_10_mask : sstatus_v1_9_mask);
>> > > -    *val = env->mstatus & mask;
>> > > +    *val = *env->mstatus & mask;
>> > >      return 0;
>> > >  }
>> > >
>> > > @@ -622,7 +622,7 @@ static int write_sstatus(CPURISCVState *env, int csrno, target_ulong val)
>> > >  {
>> > >      target_ulong mask = ((env->priv_ver >= PRIV_VERSION_1_10_0) ?
>> > >                           sstatus_v1_10_mask : sstatus_v1_9_mask);
>> > > -    target_ulong newval = (env->mstatus & ~mask) | (val & mask);
>> > > +    target_ulong newval = (*env->mstatus & ~mask) | (val & mask);
>> > >      return write_mstatus(env, CSR_MSTATUS, newval);
>> > >  }
>> > >
>> > > @@ -737,7 +737,7 @@ static int read_satp(CPURISCVState *env, int csrno, target_ulong *val)
>> > >      if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
>> > >          *val = 0;
>> > >      } else if (env->priv_ver >= PRIV_VERSION_1_10_0) {
>> > > -        if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
>> > > +        if (env->priv == PRV_S && get_field(*env->mstatus, MSTATUS_TVM)) {
>> > >              return -1;
>> > >          } else {
>> > >              *val = env->satp;
>> > > @@ -762,7 +762,7 @@ static int write_satp(CPURISCVState *env, int csrno, target_ulong val)
>> > >          validate_vm(env, get_field(val, SATP_MODE)) &&
>> > >          ((val ^ env->satp) & (SATP_MODE | SATP_ASID | SATP_PPN)))
>> > >      {
>> > > -        if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
>> > > +        if (env->priv == PRV_S && get_field(*env->mstatus, MSTATUS_TVM)) {
>> > >              return -1;
>> > >          } else {
>> > >              if((val ^ env->satp) & SATP_ASID) {
>> > > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
>> > > index 331cc36232..d150551bc9 100644
>> > > --- a/target/riscv/op_helper.c
>> > > +++ b/target/riscv/op_helper.c
>> > > @@ -83,11 +83,11 @@ target_ulong helper_sret(CPURISCVState *env, target_ulong cpu_pc_deb)
>> > >      }
>> > >
>> > >      if (env->priv_ver >= PRIV_VERSION_1_10_0 &&
>> > > -        get_field(env->mstatus, MSTATUS_TSR)) {
>> > > +        get_field(*env->mstatus, MSTATUS_TSR)) {
>> > >          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
>> > >      }
>> > >
>> > > -    target_ulong mstatus = env->mstatus;
>> > > +    target_ulong mstatus = *env->mstatus;
>> > >      target_ulong prev_priv = get_field(mstatus, MSTATUS_SPP);
>> > >      mstatus = set_field(mstatus,
>> > >          env->priv_ver >= PRIV_VERSION_1_10_0 ?
>> > > @@ -96,7 +96,7 @@ target_ulong helper_sret(CPURISCVState *env, target_ulong cpu_pc_deb)
>> > >      mstatus = set_field(mstatus, MSTATUS_SPIE, 0);
>> > >      mstatus = set_field(mstatus, MSTATUS_SPP, PRV_U);
>> > >      riscv_cpu_set_mode(env, prev_priv);
>> > > -    env->mstatus = mstatus;
>> > > +    *env->mstatus = mstatus;
>> > >
>> > >      return retpc;
>> > >  }
>> > > @@ -112,7 +112,7 @@ target_ulong helper_mret(CPURISCVState *env, target_ulong cpu_pc_deb)
>> > >          riscv_raise_exception(env, RISCV_EXCP_INST_ADDR_MIS, GETPC());
>> > >      }
>> > >
>> > > -    target_ulong mstatus = env->mstatus;
>> > > +    target_ulong mstatus = *env->mstatus;
>> > >      target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP);
>> > >      mstatus = set_field(mstatus,
>> > >          env->priv_ver >= PRIV_VERSION_1_10_0 ?
>> > > @@ -121,7 +121,7 @@ target_ulong helper_mret(CPURISCVState *env, target_ulong cpu_pc_deb)
>> > >      mstatus = set_field(mstatus, MSTATUS_MPIE, 0);
>> > >      mstatus = set_field(mstatus, MSTATUS_MPP, PRV_U);
>> > >      riscv_cpu_set_mode(env, prev_priv);
>> > > -    env->mstatus = mstatus;
>> > > +    *env->mstatus = mstatus;
>> > >
>> > >      return retpc;
>> > >  }
>> > > @@ -132,7 +132,7 @@ void helper_wfi(CPURISCVState *env)
>> > >
>> > >      if (env->priv == PRV_S &&
>> > >          env->priv_ver >= PRIV_VERSION_1_10_0 &&
>> > > -        get_field(env->mstatus, MSTATUS_TW)) {
>> > > +        get_field(*env->mstatus, MSTATUS_TW)) {
>> > >          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
>> > >      } else {
>> > >          cs->halted = 1;
>> > > @@ -147,7 +147,7 @@ void helper_tlb_flush(CPURISCVState *env)
>> > >      if (!(env->priv >= PRV_S) ||
>> > >          (env->priv == PRV_S &&
>> > >           env->priv_ver >= PRIV_VERSION_1_10_0 &&
>> > > -         get_field(env->mstatus, MSTATUS_TVM))) {
>> > > +         get_field(*env->mstatus, MSTATUS_TVM))) {
>> > >          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
>> > >      } else {
>> > >          tlb_flush(cs);
>>
Jonathan Behrens Jan. 22, 2020, 10:13 p.m. UTC | #5
>
> I was trying to avoid forcing everyone to understand the Hypervisor
> extension to develop for RISC-V.


I agree a full understanding of the hypervisor extension shouldn't be
required. But if code is going to be dealing with supervisor mode
registers, then whether the machine is in HS or VS mode is going to matter
in a lot of cases. The cases where it doesn't matter are particularly
troubling: it wouldn't be obvious from reading the code whether the author
forgot about the possibility of register swapping or if the code actually
works then too. Plus, for custom forks we can always add a one line comment
above the register declarations saying you can just s*xxx*[NOVIRT] to
access the normal supervisor registers unless you want to make sure your
code works with the H-extension.

For example we don't have to check
> virtualization status to dump the registers, we can just dump them and
> know they are in the correct state (we do dump more is we have the
> extension though). Image if someone adds a new way to dump registers,
> I would like them not to care about if we are in V=1 or V=0 and they
> can just dump them and know that the CSRs are correct.
>

This is almost a best case for being orthogonal to other features because
you don't have to understand anything about how the registers impact
machine execution in order to dump them.

But even here it is somewhat debatable whether a naive implementation would
dump the "correct" versions of these registers. One view is that what you
really want would be the values that M-mode code would see since it has the
most objective view of the system. Another is that they are the values that
would be returned if the the current operating mode tried to access them,
or undefined if that access would trap. Maybe you even want to query the
value for a specific mode independent of the current privilege level. If
the behavior you want happens to match up to the chosen design, then yes a
naive implementation that is oblivious the hypervisor extension will work
without having to understand anything. However, I'd attribute that more to
luck than elegance of a particular option.

Jonathan

On Tue, Jan 21, 2020 at 7:01 PM Alistair Francis <alistair23@gmail.com>
wrote:

> On Tue, Jan 21, 2020 at 10:56 PM Jonathan Behrens <fintelia@gmail.com>
> wrote:
> >
> > When I looked through the relevant code a few months ago, I couldn't
> find anywhere that could actually be agnostic to whether the real or
> virtual registers were in effect (other than emulating the actual CSR
> modification instructions). For almost all state, the VS behavior is
> filtered by HS-mode code. For example, you can grab satp in either mode but
> to properly do address translation you also have to factor in hgatp so you
> need to know the virtualization state anyway. Similarly, floating point
> co-proccessor state is tracked in two places with V=1 so that both the host
> and the guest can independently track dirty bits, but of course only the
> one in the "real" mstatus applies in non-virtualized mode.
>
> So the idea is that if you aren't interested in the Hypervisor
> extension you can just access the CSRs as usual (inside QEMU's source
> code). That is you don't need to know anything about the Hypervisor
> extension to add support for other extensions or to work on the RISC-V
> target in QEMU.
>
> I was trying to avoid forcing everyone to understand the Hypervisor
> extension to develop for RISC-V. For example we don't have to check
> virtulasition status to dump the registers, we can just dump them and
> know they are in the correct state (we do dump more is we have the
> extension though). Image if someone adds a new way to dump registers,
> I would like them not to care about if we are in V=1 or V=0 and they
> can just dump them and know that the CSRs are correct.
>
> >
> > The idea of using an array to track the two versions of registers seemed
> really elegant to me. If you know you want the host version you access
> element zero. If you know you want the guest version you access element
> one. And if you want the version that the running code would see you index
> by the virtualization mode. In any case, the choice indicates that you
> thought though which was the right option to use in that instance.
>
> mstatus really is the only one that has any complications. The others
> have a vs* version, which could be converted to an array but it
> doesn't really matter as we just swap them.
>
> Alistair
>
> >
> > Jonathan
> >
> > On Tue, Jan 21, 2020 at 6:02 AM Alistair Francis <alistair23@gmail.com>
> wrote:
> >>
> >> On Wed, Jan 8, 2020 at 11:30 AM Palmer Dabbelt <
> palmerdabbelt@google.com> wrote:
> >> >
> >> > On Mon, 09 Dec 2019 10:11:19 PST (-0800), Alistair Francis wrote:
> >> > > To handle the new Hypervisor CSR register aliasing let's use
> pointers.
> >> >
> >> > For some reason I thought we were making this explicit?  In other
> words,
> >> > requiring that all callers provide which privilege mode they're using
> when
> >> > accessing these CSRs, as opposed to swapping around pointers.  I
> don't actually
> >> > care that much, but IIRC when we were talking with the ARM guys at
> Plumbers
> >> > they were pretty adament that would end up being a much cleaner
> implementation
> >> > as they'd tried this way and later changed over.
> >>
> >> I think their implementation is different so it doesn't apply the same
> here.
> >>
> >> My main concern is that due to the modularity of RISC-V I don't expect
> >> all future developers to keep track of the Hypervisor extensions. This
> >> way we always have the correct state in the registers.
> >>
> >> There is only one pointer variable left, so we could drop the pointer
> >> swapping part, but for now it's still here.
> >>
> >> Alistair
> >>
> >> >
> >> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >> > > ---
> >> > >  target/riscv/cpu.c        | 11 +++++++++--
> >> > >  target/riscv/cpu.h        |  9 ++++++++-
> >> > >  target/riscv/cpu_helper.c | 30 +++++++++++++++---------------
> >> > >  target/riscv/csr.c        | 20 ++++++++++----------
> >> > >  target/riscv/op_helper.c  | 14 +++++++-------
> >> > >  5 files changed, 49 insertions(+), 35 deletions(-)
> >> > >
> >> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> > > index a07c5689b3..e61cf46a73 100644
> >> > > --- a/target/riscv/cpu.c
> >> > > +++ b/target/riscv/cpu.c
> >> > > @@ -236,7 +236,7 @@ static void riscv_cpu_dump_state(CPUState *cs,
> FILE *f, int flags)
> >> > >      qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "pc      ",
> env->pc);
> >> > >  #ifndef CONFIG_USER_ONLY
> >> > >      qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mhartid ",
> env->mhartid);
> >> > > -    qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ",
> env->mstatus);
> >> > > +    qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ",
> *env->mstatus);
> >> > >      if (riscv_has_ext(env, RVH)) {
> >> > >          qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "hstatus ",
> env->hstatus);
> >> > >          qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "vsstatus ",
> env->vsstatus);
> >> > > @@ -336,7 +336,7 @@ static void riscv_cpu_reset(CPUState *cs)
> >> > >      mcc->parent_reset(cs);
> >> > >  #ifndef CONFIG_USER_ONLY
> >> > >      env->priv = PRV_M;
> >> > > -    env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
> >> > > +    *env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
> >> > >      env->mcause = 0;
> >> > >      env->pc = env->resetvec;
> >> > >  #endif
> >> > > @@ -465,8 +465,15 @@ static void riscv_cpu_realize(DeviceState
> *dev, Error **errp)
> >> > >  static void riscv_cpu_init(Object *obj)
> >> > >  {
> >> > >      RISCVCPU *cpu = RISCV_CPU(obj);
> >> > > +#ifndef CONFIG_USER_ONLY
> >> > > +    CPURISCVState *env = &cpu->env;
> >> > > +#endif
> >> > >
> >> > >      cpu_set_cpustate_pointers(cpu);
> >> > > +
> >> > > +#ifndef CONFIG_USER_ONLY
> >> > > +    env->mstatus = &env->mstatus_novirt;
> >> > > +#endif
> >> > >  }
> >> > >
> >> > >  static const VMStateDescription vmstate_riscv_cpu = {
> >> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >> > > index 21ae5a8b19..9dc8303c62 100644
> >> > > --- a/target/riscv/cpu.h
> >> > > +++ b/target/riscv/cpu.h
> >> > > @@ -122,7 +122,7 @@ struct CPURISCVState {
> >> > >      target_ulong resetvec;
> >> > >
> >> > >      target_ulong mhartid;
> >> > > -    target_ulong mstatus;
> >> > > +    target_ulong *mstatus;
> >> > >
> >> > >      target_ulong mip;
> >> > >      uint32_t miclaim;
> >> > > @@ -145,6 +145,13 @@ struct CPURISCVState {
> >> > >      target_ulong mcause;
> >> > >      target_ulong mtval;  /* since: priv-1.10.0 */
> >> > >
> >> > > +    /* The following registers are the "real" versions that the
> pointer
> >> > > +     * versions point to. These should never be used unless you
> know what you
> >> > > +     * are doing. To access these use the pointer versions
> instead. This is
> >> > > +     * required to handle the Hypervisor register swapping.
> >> > > +     */
> >> > > +    target_ulong mstatus_novirt;
> >> > > +
> >> > >      /* Hypervisor CSRs */
> >> > >      target_ulong hstatus;
> >> > >      target_ulong hedeleg;
> >> > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> >> > > index b00f66824a..9684da7f7d 100644
> >> > > --- a/target/riscv/cpu_helper.c
> >> > > +++ b/target/riscv/cpu_helper.c
> >> > > @@ -37,8 +37,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool
> ifetch)
> >> > >  #ifndef CONFIG_USER_ONLY
> >> > >  static int riscv_cpu_local_irq_pending(CPURISCVState *env)
> >> > >  {
> >> > > -    target_ulong mstatus_mie = get_field(env->mstatus,
> MSTATUS_MIE);
> >> > > -    target_ulong mstatus_sie = get_field(env->mstatus,
> MSTATUS_SIE);
> >> > > +    target_ulong mstatus_mie = get_field(*env->mstatus,
> MSTATUS_MIE);
> >> > > +    target_ulong mstatus_sie = get_field(*env->mstatus,
> MSTATUS_SIE);
> >> > >      target_ulong pending = env->mip & env->mie;
> >> > >      target_ulong mie = env->priv < PRV_M || (env->priv == PRV_M &&
> mstatus_mie);
> >> > >      target_ulong sie = env->priv < PRV_S || (env->priv == PRV_S &&
> mstatus_sie);
> >> > > @@ -75,7 +75,7 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int
> interrupt_request)
> >> > >  /* Return true is floating point support is currently enabled */
> >> > >  bool riscv_cpu_fp_enabled(CPURISCVState *env)
> >> > >  {
> >> > > -    if (env->mstatus & MSTATUS_FS) {
> >> > > +    if (*env->mstatus & MSTATUS_FS) {
> >> > >          return true;
> >> > >      }
> >> > >
> >> > > @@ -198,8 +198,8 @@ static int get_physical_address(CPURISCVState
> *env, hwaddr *physical,
> >> > >      int mode = mmu_idx;
> >> > >
> >> > >      if (mode == PRV_M && access_type != MMU_INST_FETCH) {
> >> > > -        if (get_field(env->mstatus, MSTATUS_MPRV)) {
> >> > > -            mode = get_field(env->mstatus, MSTATUS_MPP);
> >> > > +        if (get_field(*env->mstatus, MSTATUS_MPRV)) {
> >> > > +            mode = get_field(*env->mstatus, MSTATUS_MPP);
> >> > >          }
> >> > >      }
> >> > >
> >> > > @@ -213,11 +213,11 @@ static int get_physical_address(CPURISCVState
> *env, hwaddr *physical,
> >> > >
> >> > >      hwaddr base;
> >> > >      int levels, ptidxbits, ptesize, vm, sum;
> >> > > -    int mxr = get_field(env->mstatus, MSTATUS_MXR);
> >> > > +    int mxr = get_field(*env->mstatus, MSTATUS_MXR);
> >> > >
> >> > >      if (env->priv_ver >= PRIV_VERSION_1_10_0) {
> >> > >          base = (hwaddr)get_field(env->satp, SATP_PPN) << PGSHIFT;
> >> > > -        sum = get_field(env->mstatus, MSTATUS_SUM);
> >> > > +        sum = get_field(*env->mstatus, MSTATUS_SUM);
> >> > >          vm = get_field(env->satp, SATP_MODE);
> >> > >          switch (vm) {
> >> > >          case VM_1_10_SV32:
> >> > > @@ -237,8 +237,8 @@ static int get_physical_address(CPURISCVState
> *env, hwaddr *physical,
> >> > >          }
> >> > >      } else {
> >> > >          base = (hwaddr)(env->sptbr) << PGSHIFT;
> >> > > -        sum = !get_field(env->mstatus, MSTATUS_PUM);
> >> > > -        vm = get_field(env->mstatus, MSTATUS_VM);
> >> > > +        sum = !get_field(*env->mstatus, MSTATUS_PUM);
> >> > > +        vm = get_field(*env->mstatus, MSTATUS_VM);
> >> > >          switch (vm) {
> >> > >          case VM_1_09_SV32:
> >> > >            levels = 2; ptidxbits = 10; ptesize = 4; break;
> >> > > @@ -492,8 +492,8 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr
> address, int size,
> >> > >      ret = get_physical_address(env, &pa, &prot, address,
> access_type, mmu_idx);
> >> > >
> >> > >      if (mode == PRV_M && access_type != MMU_INST_FETCH) {
> >> > > -        if (get_field(env->mstatus, MSTATUS_MPRV)) {
> >> > > -            mode = get_field(env->mstatus, MSTATUS_MPP);
> >> > > +        if (get_field(*env->mstatus, MSTATUS_MPRV)) {
> >> > > +            mode = get_field(*env->mstatus, MSTATUS_MPP);
> >> > >          }
> >> > >      }
> >> > >
> >> > > @@ -599,12 +599,12 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> >> > >      if (env->priv <= PRV_S &&
> >> > >              cause < TARGET_LONG_BITS && ((deleg >> cause) & 1)) {
> >> > >          /* handle the trap in S-mode */
> >> > > -        target_ulong s = env->mstatus;
> >> > > +        target_ulong s = *env->mstatus;
> >> > >          s = set_field(s, MSTATUS_SPIE, env->priv_ver >=
> PRIV_VERSION_1_10_0 ?
> >> > >              get_field(s, MSTATUS_SIE) : get_field(s, MSTATUS_UIE
> << env->priv));
> >> > >          s = set_field(s, MSTATUS_SPP, env->priv);
> >> > >          s = set_field(s, MSTATUS_SIE, 0);
> >> > > -        env->mstatus = s;
> >> > > +        *env->mstatus = s;
> >> > >          env->scause = cause | ((target_ulong)async <<
> (TARGET_LONG_BITS - 1));
> >> > >          env->sepc = env->pc;
> >> > >          env->sbadaddr = tval;
> >> > > @@ -613,12 +613,12 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> >> > >          riscv_cpu_set_mode(env, PRV_S);
> >> > >      } else {
> >> > >          /* handle the trap in M-mode */
> >> > > -        target_ulong s = env->mstatus;
> >> > > +        target_ulong s = *env->mstatus;
> >> > >          s = set_field(s, MSTATUS_MPIE, env->priv_ver >=
> PRIV_VERSION_1_10_0 ?
> >> > >              get_field(s, MSTATUS_MIE) : get_field(s, MSTATUS_UIE
> << env->priv));
> >> > >          s = set_field(s, MSTATUS_MPP, env->priv);
> >> > >          s = set_field(s, MSTATUS_MIE, 0);
> >> > > -        env->mstatus = s;
> >> > > +        *env->mstatus = s;
> >> > >          env->mcause = cause | ~(((target_ulong)-1) >> async);
> >> > >          env->mepc = env->pc;
> >> > >          env->mbadaddr = tval;
> >> > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> > > index 74e911af08..a4b598d49a 100644
> >> > > --- a/target/riscv/csr.c
> >> > > +++ b/target/riscv/csr.c
> >> > > @@ -136,7 +136,7 @@ static int write_fflags(CPURISCVState *env, int
> csrno, target_ulong val)
> >> > >      if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
> >> > >          return -1;
> >> > >      }
> >> > > -    env->mstatus |= MSTATUS_FS;
> >> > > +    *env->mstatus |= MSTATUS_FS;
> >> > >  #endif
> >> > >      riscv_cpu_set_fflags(env, val & (FSR_AEXC >> FSR_AEXC_SHIFT));
> >> > >      return 0;
> >> > > @@ -159,7 +159,7 @@ static int write_frm(CPURISCVState *env, int
> csrno, target_ulong val)
> >> > >      if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
> >> > >          return -1;
> >> > >      }
> >> > > -    env->mstatus |= MSTATUS_FS;
> >> > > +    *env->mstatus |= MSTATUS_FS;
> >> > >  #endif
> >> > >      env->frm = val & (FSR_RD >> FSR_RD_SHIFT);
> >> > >      return 0;
> >> > > @@ -183,7 +183,7 @@ static int write_fcsr(CPURISCVState *env, int
> csrno, target_ulong val)
> >> > >      if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
> >> > >          return -1;
> >> > >      }
> >> > > -    env->mstatus |= MSTATUS_FS;
> >> > > +    *env->mstatus |= MSTATUS_FS;
> >> > >  #endif
> >> > >      env->frm = (val & FSR_RD) >> FSR_RD_SHIFT;
> >> > >      riscv_cpu_set_fflags(env, (val & FSR_AEXC) >> FSR_AEXC_SHIFT);
> >> > > @@ -313,7 +313,7 @@ static int read_mhartid(CPURISCVState *env, int
> csrno, target_ulong *val)
> >> > >  /* Machine Trap Setup */
> >> > >  static int read_mstatus(CPURISCVState *env, int csrno,
> target_ulong *val)
> >> > >  {
> >> > > -    *val = env->mstatus;
> >> > > +    *val = *env->mstatus;
> >> > >      return 0;
> >> > >  }
> >> > >
> >> > > @@ -325,7 +325,7 @@ static int validate_vm(CPURISCVState *env,
> target_ulong vm)
> >> > >
> >> > >  static int write_mstatus(CPURISCVState *env, int csrno,
> target_ulong val)
> >> > >  {
> >> > > -    target_ulong mstatus = env->mstatus;
> >> > > +    target_ulong mstatus = *env->mstatus;
> >> > >      target_ulong mask = 0;
> >> > >      int dirty;
> >> > >
> >> > > @@ -365,7 +365,7 @@ static int write_mstatus(CPURISCVState *env,
> int csrno, target_ulong val)
> >> > >               ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
> >> > >              ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> >> > >      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
> >> > > -    env->mstatus = mstatus;
> >> > > +    *env->mstatus = mstatus;
> >> > >
> >> > >      return 0;
> >> > >  }
> >> > > @@ -614,7 +614,7 @@ static int read_sstatus(CPURISCVState *env, int
> csrno, target_ulong *val)
> >> > >  {
> >> > >      target_ulong mask = ((env->priv_ver >= PRIV_VERSION_1_10_0) ?
> >> > >                           sstatus_v1_10_mask : sstatus_v1_9_mask);
> >> > > -    *val = env->mstatus & mask;
> >> > > +    *val = *env->mstatus & mask;
> >> > >      return 0;
> >> > >  }
> >> > >
> >> > > @@ -622,7 +622,7 @@ static int write_sstatus(CPURISCVState *env,
> int csrno, target_ulong val)
> >> > >  {
> >> > >      target_ulong mask = ((env->priv_ver >= PRIV_VERSION_1_10_0) ?
> >> > >                           sstatus_v1_10_mask : sstatus_v1_9_mask);
> >> > > -    target_ulong newval = (env->mstatus & ~mask) | (val & mask);
> >> > > +    target_ulong newval = (*env->mstatus & ~mask) | (val & mask);
> >> > >      return write_mstatus(env, CSR_MSTATUS, newval);
> >> > >  }
> >> > >
> >> > > @@ -737,7 +737,7 @@ static int read_satp(CPURISCVState *env, int
> csrno, target_ulong *val)
> >> > >      if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
> >> > >          *val = 0;
> >> > >      } else if (env->priv_ver >= PRIV_VERSION_1_10_0) {
> >> > > -        if (env->priv == PRV_S && get_field(env->mstatus,
> MSTATUS_TVM)) {
> >> > > +        if (env->priv == PRV_S && get_field(*env->mstatus,
> MSTATUS_TVM)) {
> >> > >              return -1;
> >> > >          } else {
> >> > >              *val = env->satp;
> >> > > @@ -762,7 +762,7 @@ static int write_satp(CPURISCVState *env, int
> csrno, target_ulong val)
> >> > >          validate_vm(env, get_field(val, SATP_MODE)) &&
> >> > >          ((val ^ env->satp) & (SATP_MODE | SATP_ASID | SATP_PPN)))
> >> > >      {
> >> > > -        if (env->priv == PRV_S && get_field(env->mstatus,
> MSTATUS_TVM)) {
> >> > > +        if (env->priv == PRV_S && get_field(*env->mstatus,
> MSTATUS_TVM)) {
> >> > >              return -1;
> >> > >          } else {
> >> > >              if((val ^ env->satp) & SATP_ASID) {
> >> > > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> >> > > index 331cc36232..d150551bc9 100644
> >> > > --- a/target/riscv/op_helper.c
> >> > > +++ b/target/riscv/op_helper.c
> >> > > @@ -83,11 +83,11 @@ target_ulong helper_sret(CPURISCVState *env,
> target_ulong cpu_pc_deb)
> >> > >      }
> >> > >
> >> > >      if (env->priv_ver >= PRIV_VERSION_1_10_0 &&
> >> > > -        get_field(env->mstatus, MSTATUS_TSR)) {
> >> > > +        get_field(*env->mstatus, MSTATUS_TSR)) {
> >> > >          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST,
> GETPC());
> >> > >      }
> >> > >
> >> > > -    target_ulong mstatus = env->mstatus;
> >> > > +    target_ulong mstatus = *env->mstatus;
> >> > >      target_ulong prev_priv = get_field(mstatus, MSTATUS_SPP);
> >> > >      mstatus = set_field(mstatus,
> >> > >          env->priv_ver >= PRIV_VERSION_1_10_0 ?
> >> > > @@ -96,7 +96,7 @@ target_ulong helper_sret(CPURISCVState *env,
> target_ulong cpu_pc_deb)
> >> > >      mstatus = set_field(mstatus, MSTATUS_SPIE, 0);
> >> > >      mstatus = set_field(mstatus, MSTATUS_SPP, PRV_U);
> >> > >      riscv_cpu_set_mode(env, prev_priv);
> >> > > -    env->mstatus = mstatus;
> >> > > +    *env->mstatus = mstatus;
> >> > >
> >> > >      return retpc;
> >> > >  }
> >> > > @@ -112,7 +112,7 @@ target_ulong helper_mret(CPURISCVState *env,
> target_ulong cpu_pc_deb)
> >> > >          riscv_raise_exception(env, RISCV_EXCP_INST_ADDR_MIS,
> GETPC());
> >> > >      }
> >> > >
> >> > > -    target_ulong mstatus = env->mstatus;
> >> > > +    target_ulong mstatus = *env->mstatus;
> >> > >      target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP);
> >> > >      mstatus = set_field(mstatus,
> >> > >          env->priv_ver >= PRIV_VERSION_1_10_0 ?
> >> > > @@ -121,7 +121,7 @@ target_ulong helper_mret(CPURISCVState *env,
> target_ulong cpu_pc_deb)
> >> > >      mstatus = set_field(mstatus, MSTATUS_MPIE, 0);
> >> > >      mstatus = set_field(mstatus, MSTATUS_MPP, PRV_U);
> >> > >      riscv_cpu_set_mode(env, prev_priv);
> >> > > -    env->mstatus = mstatus;
> >> > > +    *env->mstatus = mstatus;
> >> > >
> >> > >      return retpc;
> >> > >  }
> >> > > @@ -132,7 +132,7 @@ void helper_wfi(CPURISCVState *env)
> >> > >
> >> > >      if (env->priv == PRV_S &&
> >> > >          env->priv_ver >= PRIV_VERSION_1_10_0 &&
> >> > > -        get_field(env->mstatus, MSTATUS_TW)) {
> >> > > +        get_field(*env->mstatus, MSTATUS_TW)) {
> >> > >          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST,
> GETPC());
> >> > >      } else {
> >> > >          cs->halted = 1;
> >> > > @@ -147,7 +147,7 @@ void helper_tlb_flush(CPURISCVState *env)
> >> > >      if (!(env->priv >= PRV_S) ||
> >> > >          (env->priv == PRV_S &&
> >> > >           env->priv_ver >= PRIV_VERSION_1_10_0 &&
> >> > > -         get_field(env->mstatus, MSTATUS_TVM))) {
> >> > > +         get_field(*env->mstatus, MSTATUS_TVM))) {
> >> > >          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST,
> GETPC());
> >> > >      } else {
> >> > >          tlb_flush(cs);
> >>
>

Patch
diff mbox series

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index a07c5689b3..e61cf46a73 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -236,7 +236,7 @@  static void riscv_cpu_dump_state(CPUState *cs, FILE *f, int flags)
     qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "pc      ", env->pc);
 #ifndef CONFIG_USER_ONLY
     qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mhartid ", env->mhartid);
-    qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ", env->mstatus);
+    qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ", *env->mstatus);
     if (riscv_has_ext(env, RVH)) {
         qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "hstatus ", env->hstatus);
         qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "vsstatus ", env->vsstatus);
@@ -336,7 +336,7 @@  static void riscv_cpu_reset(CPUState *cs)
     mcc->parent_reset(cs);
 #ifndef CONFIG_USER_ONLY
     env->priv = PRV_M;
-    env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
+    *env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
     env->mcause = 0;
     env->pc = env->resetvec;
 #endif
@@ -465,8 +465,15 @@  static void riscv_cpu_realize(DeviceState *dev, Error **errp)
 static void riscv_cpu_init(Object *obj)
 {
     RISCVCPU *cpu = RISCV_CPU(obj);
+#ifndef CONFIG_USER_ONLY
+    CPURISCVState *env = &cpu->env;
+#endif
 
     cpu_set_cpustate_pointers(cpu);
+
+#ifndef CONFIG_USER_ONLY
+    env->mstatus = &env->mstatus_novirt;
+#endif
 }
 
 static const VMStateDescription vmstate_riscv_cpu = {
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 21ae5a8b19..9dc8303c62 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -122,7 +122,7 @@  struct CPURISCVState {
     target_ulong resetvec;
 
     target_ulong mhartid;
-    target_ulong mstatus;
+    target_ulong *mstatus;
 
     target_ulong mip;
     uint32_t miclaim;
@@ -145,6 +145,13 @@  struct CPURISCVState {
     target_ulong mcause;
     target_ulong mtval;  /* since: priv-1.10.0 */
 
+    /* The following registers are the "real" versions that the pointer
+     * versions point to. These should never be used unless you know what you
+     * are doing. To access these use the pointer versions instead. This is
+     * required to handle the Hypervisor register swapping.
+     */
+    target_ulong mstatus_novirt;
+
     /* Hypervisor CSRs */
     target_ulong hstatus;
     target_ulong hedeleg;
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index b00f66824a..9684da7f7d 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -37,8 +37,8 @@  int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
 #ifndef CONFIG_USER_ONLY
 static int riscv_cpu_local_irq_pending(CPURISCVState *env)
 {
-    target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE);
-    target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE);
+    target_ulong mstatus_mie = get_field(*env->mstatus, MSTATUS_MIE);
+    target_ulong mstatus_sie = get_field(*env->mstatus, MSTATUS_SIE);
     target_ulong pending = env->mip & env->mie;
     target_ulong mie = env->priv < PRV_M || (env->priv == PRV_M && mstatus_mie);
     target_ulong sie = env->priv < PRV_S || (env->priv == PRV_S && mstatus_sie);
@@ -75,7 +75,7 @@  bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 /* Return true is floating point support is currently enabled */
 bool riscv_cpu_fp_enabled(CPURISCVState *env)
 {
-    if (env->mstatus & MSTATUS_FS) {
+    if (*env->mstatus & MSTATUS_FS) {
         return true;
     }
 
@@ -198,8 +198,8 @@  static int get_physical_address(CPURISCVState *env, hwaddr *physical,
     int mode = mmu_idx;
 
     if (mode == PRV_M && access_type != MMU_INST_FETCH) {
-        if (get_field(env->mstatus, MSTATUS_MPRV)) {
-            mode = get_field(env->mstatus, MSTATUS_MPP);
+        if (get_field(*env->mstatus, MSTATUS_MPRV)) {
+            mode = get_field(*env->mstatus, MSTATUS_MPP);
         }
     }
 
@@ -213,11 +213,11 @@  static int get_physical_address(CPURISCVState *env, hwaddr *physical,
 
     hwaddr base;
     int levels, ptidxbits, ptesize, vm, sum;
-    int mxr = get_field(env->mstatus, MSTATUS_MXR);
+    int mxr = get_field(*env->mstatus, MSTATUS_MXR);
 
     if (env->priv_ver >= PRIV_VERSION_1_10_0) {
         base = (hwaddr)get_field(env->satp, SATP_PPN) << PGSHIFT;
-        sum = get_field(env->mstatus, MSTATUS_SUM);
+        sum = get_field(*env->mstatus, MSTATUS_SUM);
         vm = get_field(env->satp, SATP_MODE);
         switch (vm) {
         case VM_1_10_SV32:
@@ -237,8 +237,8 @@  static int get_physical_address(CPURISCVState *env, hwaddr *physical,
         }
     } else {
         base = (hwaddr)(env->sptbr) << PGSHIFT;
-        sum = !get_field(env->mstatus, MSTATUS_PUM);
-        vm = get_field(env->mstatus, MSTATUS_VM);
+        sum = !get_field(*env->mstatus, MSTATUS_PUM);
+        vm = get_field(*env->mstatus, MSTATUS_VM);
         switch (vm) {
         case VM_1_09_SV32:
           levels = 2; ptidxbits = 10; ptesize = 4; break;
@@ -492,8 +492,8 @@  bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     ret = get_physical_address(env, &pa, &prot, address, access_type, mmu_idx);
 
     if (mode == PRV_M && access_type != MMU_INST_FETCH) {
-        if (get_field(env->mstatus, MSTATUS_MPRV)) {
-            mode = get_field(env->mstatus, MSTATUS_MPP);
+        if (get_field(*env->mstatus, MSTATUS_MPRV)) {
+            mode = get_field(*env->mstatus, MSTATUS_MPP);
         }
     }
 
@@ -599,12 +599,12 @@  void riscv_cpu_do_interrupt(CPUState *cs)
     if (env->priv <= PRV_S &&
             cause < TARGET_LONG_BITS && ((deleg >> cause) & 1)) {
         /* handle the trap in S-mode */
-        target_ulong s = env->mstatus;
+        target_ulong s = *env->mstatus;
         s = set_field(s, MSTATUS_SPIE, env->priv_ver >= PRIV_VERSION_1_10_0 ?
             get_field(s, MSTATUS_SIE) : get_field(s, MSTATUS_UIE << env->priv));
         s = set_field(s, MSTATUS_SPP, env->priv);
         s = set_field(s, MSTATUS_SIE, 0);
-        env->mstatus = s;
+        *env->mstatus = s;
         env->scause = cause | ((target_ulong)async << (TARGET_LONG_BITS - 1));
         env->sepc = env->pc;
         env->sbadaddr = tval;
@@ -613,12 +613,12 @@  void riscv_cpu_do_interrupt(CPUState *cs)
         riscv_cpu_set_mode(env, PRV_S);
     } else {
         /* handle the trap in M-mode */
-        target_ulong s = env->mstatus;
+        target_ulong s = *env->mstatus;
         s = set_field(s, MSTATUS_MPIE, env->priv_ver >= PRIV_VERSION_1_10_0 ?
             get_field(s, MSTATUS_MIE) : get_field(s, MSTATUS_UIE << env->priv));
         s = set_field(s, MSTATUS_MPP, env->priv);
         s = set_field(s, MSTATUS_MIE, 0);
-        env->mstatus = s;
+        *env->mstatus = s;
         env->mcause = cause | ~(((target_ulong)-1) >> async);
         env->mepc = env->pc;
         env->mbadaddr = tval;
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 74e911af08..a4b598d49a 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -136,7 +136,7 @@  static int write_fflags(CPURISCVState *env, int csrno, target_ulong val)
     if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
         return -1;
     }
-    env->mstatus |= MSTATUS_FS;
+    *env->mstatus |= MSTATUS_FS;
 #endif
     riscv_cpu_set_fflags(env, val & (FSR_AEXC >> FSR_AEXC_SHIFT));
     return 0;
@@ -159,7 +159,7 @@  static int write_frm(CPURISCVState *env, int csrno, target_ulong val)
     if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
         return -1;
     }
-    env->mstatus |= MSTATUS_FS;
+    *env->mstatus |= MSTATUS_FS;
 #endif
     env->frm = val & (FSR_RD >> FSR_RD_SHIFT);
     return 0;
@@ -183,7 +183,7 @@  static int write_fcsr(CPURISCVState *env, int csrno, target_ulong val)
     if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
         return -1;
     }
-    env->mstatus |= MSTATUS_FS;
+    *env->mstatus |= MSTATUS_FS;
 #endif
     env->frm = (val & FSR_RD) >> FSR_RD_SHIFT;
     riscv_cpu_set_fflags(env, (val & FSR_AEXC) >> FSR_AEXC_SHIFT);
@@ -313,7 +313,7 @@  static int read_mhartid(CPURISCVState *env, int csrno, target_ulong *val)
 /* Machine Trap Setup */
 static int read_mstatus(CPURISCVState *env, int csrno, target_ulong *val)
 {
-    *val = env->mstatus;
+    *val = *env->mstatus;
     return 0;
 }
 
@@ -325,7 +325,7 @@  static int validate_vm(CPURISCVState *env, target_ulong vm)
 
 static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
 {
-    target_ulong mstatus = env->mstatus;
+    target_ulong mstatus = *env->mstatus;
     target_ulong mask = 0;
     int dirty;
 
@@ -365,7 +365,7 @@  static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
              ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
             ((mstatus & MSTATUS_XS) == MSTATUS_XS);
     mstatus = set_field(mstatus, MSTATUS_SD, dirty);
-    env->mstatus = mstatus;
+    *env->mstatus = mstatus;
 
     return 0;
 }
@@ -614,7 +614,7 @@  static int read_sstatus(CPURISCVState *env, int csrno, target_ulong *val)
 {
     target_ulong mask = ((env->priv_ver >= PRIV_VERSION_1_10_0) ?
                          sstatus_v1_10_mask : sstatus_v1_9_mask);
-    *val = env->mstatus & mask;
+    *val = *env->mstatus & mask;
     return 0;
 }
 
@@ -622,7 +622,7 @@  static int write_sstatus(CPURISCVState *env, int csrno, target_ulong val)
 {
     target_ulong mask = ((env->priv_ver >= PRIV_VERSION_1_10_0) ?
                          sstatus_v1_10_mask : sstatus_v1_9_mask);
-    target_ulong newval = (env->mstatus & ~mask) | (val & mask);
+    target_ulong newval = (*env->mstatus & ~mask) | (val & mask);
     return write_mstatus(env, CSR_MSTATUS, newval);
 }
 
@@ -737,7 +737,7 @@  static int read_satp(CPURISCVState *env, int csrno, target_ulong *val)
     if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
         *val = 0;
     } else if (env->priv_ver >= PRIV_VERSION_1_10_0) {
-        if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
+        if (env->priv == PRV_S && get_field(*env->mstatus, MSTATUS_TVM)) {
             return -1;
         } else {
             *val = env->satp;
@@ -762,7 +762,7 @@  static int write_satp(CPURISCVState *env, int csrno, target_ulong val)
         validate_vm(env, get_field(val, SATP_MODE)) &&
         ((val ^ env->satp) & (SATP_MODE | SATP_ASID | SATP_PPN)))
     {
-        if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
+        if (env->priv == PRV_S && get_field(*env->mstatus, MSTATUS_TVM)) {
             return -1;
         } else {
             if((val ^ env->satp) & SATP_ASID) {
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 331cc36232..d150551bc9 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -83,11 +83,11 @@  target_ulong helper_sret(CPURISCVState *env, target_ulong cpu_pc_deb)
     }
 
     if (env->priv_ver >= PRIV_VERSION_1_10_0 &&
-        get_field(env->mstatus, MSTATUS_TSR)) {
+        get_field(*env->mstatus, MSTATUS_TSR)) {
         riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
     }
 
-    target_ulong mstatus = env->mstatus;
+    target_ulong mstatus = *env->mstatus;
     target_ulong prev_priv = get_field(mstatus, MSTATUS_SPP);
     mstatus = set_field(mstatus,
         env->priv_ver >= PRIV_VERSION_1_10_0 ?
@@ -96,7 +96,7 @@  target_ulong helper_sret(CPURISCVState *env, target_ulong cpu_pc_deb)
     mstatus = set_field(mstatus, MSTATUS_SPIE, 0);
     mstatus = set_field(mstatus, MSTATUS_SPP, PRV_U);
     riscv_cpu_set_mode(env, prev_priv);
-    env->mstatus = mstatus;
+    *env->mstatus = mstatus;
 
     return retpc;
 }
@@ -112,7 +112,7 @@  target_ulong helper_mret(CPURISCVState *env, target_ulong cpu_pc_deb)
         riscv_raise_exception(env, RISCV_EXCP_INST_ADDR_MIS, GETPC());
     }
 
-    target_ulong mstatus = env->mstatus;
+    target_ulong mstatus = *env->mstatus;
     target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP);
     mstatus = set_field(mstatus,
         env->priv_ver >= PRIV_VERSION_1_10_0 ?
@@ -121,7 +121,7 @@  target_ulong helper_mret(CPURISCVState *env, target_ulong cpu_pc_deb)
     mstatus = set_field(mstatus, MSTATUS_MPIE, 0);
     mstatus = set_field(mstatus, MSTATUS_MPP, PRV_U);
     riscv_cpu_set_mode(env, prev_priv);
-    env->mstatus = mstatus;
+    *env->mstatus = mstatus;
 
     return retpc;
 }
@@ -132,7 +132,7 @@  void helper_wfi(CPURISCVState *env)
 
     if (env->priv == PRV_S &&
         env->priv_ver >= PRIV_VERSION_1_10_0 &&
-        get_field(env->mstatus, MSTATUS_TW)) {
+        get_field(*env->mstatus, MSTATUS_TW)) {
         riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
     } else {
         cs->halted = 1;
@@ -147,7 +147,7 @@  void helper_tlb_flush(CPURISCVState *env)
     if (!(env->priv >= PRV_S) ||
         (env->priv == PRV_S &&
          env->priv_ver >= PRIV_VERSION_1_10_0 &&
-         get_field(env->mstatus, MSTATUS_TVM))) {
+         get_field(*env->mstatus, MSTATUS_TVM))) {
         riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
     } else {
         tlb_flush(cs);