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

Message ID	8c0eebc3868757e9ed312ac35e1f5325d5a18e76.1575914822.git.alistair.francis@wdc.com
State	New
Headers	show Return-Path: <qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org> IronPort-SDR: dBSkXLV23XBS8IOKEIv1q90jXV/9C4PWTvrsbkiHrvD8b9T5hm2qg6fdqHtNFOFB/8Di9wGDxL rIzE6JTFpoKOtGR8pJRvQygIrrfhA2ld8TkKK4x5HIfEMBe9ZoSXjtZLQ+zP6TyiRLgltjCRb9 gEJ1stInmzPIk6zT926191KkOt6zjwMWAAIo5yLVA7Qc4zS0qUgxxCY1WwX2Rn5YAT9J9/jRaH 4fMdZQoDYRBJnRsIMr1+0XKnj3rRlC3uwnHt9/jNTOJgQ4frRO7hKrg2PQMqOgh79Nsn+rKcV6 7jQ= IronPort-SDR: uQyaHjf26Ya3J74ZCISkB1rln+iUiqTOlsrRiC427zRT/z9wtxpb2+phv2DGDrCCwe2i/qAhpL pYdvsWFYpKoZqIxSjH1m6xh/RppgKfRnu23m2+1yNDy7DhB8loWx32Glc+wViVFV1zbg9ZGFQv mB97HPXsOC/p9svi91ap2mWQqabjsFIgnzCapZ4ANdTnDf+XrnHXfMoqQvYsLcV1BGJiBTey0/ k74vF5ldwCE+J3TQi55Bx+ScbJppn8go3h/8yWGFzozc1Kt53mnWItuf9IMyFyElAmtMeTxs18 QVf6ks1ny9QKwh64G6ALa5Wn IronPort-SDR: V+iOzYURZzhtTgnniCVgiXNuK0ex9eEOX8Hy8ZitkIvBp8Btg9QFqj75yge9p08eTBX5c4/p9O CC5JUYv9PzVCh8jICGN2af0bFkQ0bFBO5A9WBCLeA+WKiOYAtHiVC8r1TmHhi7o8s9oanr23NJ tqgW6tjrzuHwwC3jcGuZgRyLMKfTGZ5Sd2u48PYDCAvS1YRXPpLG6knNK3i9EQ80N5ddHRH8GF pJmvktpqBGpyHW6zBpRvW/FIQrlbENb0Ob9vdzGjpzNonwx436Hsy1ZgLl1aIbLz+0zoOSHRJq AJA= WDCIronportException: Internal From: Alistair Francis <alistair.francis@wdc.com> To: qemu-devel@nongnu.org, qemu-riscv@nongnu.org Subject: [PATCH v1 15/36] target/riscv: Convert mstatus to pointers Date: Mon, 9 Dec 2019 10:11:19 -0800 Message-Id: <8c0eebc3868757e9ed312ac35e1f5325d5a18e76.1575914822.git.alistair.francis@wdc.com> In-Reply-To: <cover.1575914822.git.alistair.francis@wdc.com> References: <cover.1575914822.git.alistair.francis@wdc.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: list Cc: alistair.francis@wdc.com, palmer@dabbelt.com, alistair23@gmail.com Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" <qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>
Series	Add RISC-V Hypervisor Extension v0.5 \| expand [v1,00/36] Add RISC-V Hypervisor Extension v0.5 [v1,01/36] target/riscv: Convert MIP CSR to target_ulong [v1,02/36] target/riscv: Don't set write permissions on dirty PTEs [v1,03/36] target/riscv: Add the Hypervisor extension [v1,04/36] target/riscv: Add the Hypervisor CSRs to CPUState [v1,05/36] target/riscv: Add support for the new execption numbers [v1,06/36] target/riscv: Rename the H irqs to VS irqs [v1,07/36] target/riscv: Add the virtulisation mode [v1,08/36] target/riscv: Add the force HS exception mode [v1,09/36] target/riscv: Fix CSR perm checking for HS mode [v1,10/36] target/riscv: Print priv and virt in disas log [v1,11/36] target/riscv: Dump Hypervisor registers if enabled [v1,12/36] target/riscv: Add Hypervisor CSR access functions [v1,13/36] target/riscv: Add Hypervisor virtual CSRs accesses [v1,14/36] target/riscv: Add Hypervisor virtual CSRs accesses [v1,15/36] target/riscv: Convert mstatus to pointers [v1,16/36] target/riscv: Add virtual register swapping function [v1,17/36] target/riscv: Set VS bits in mideleg for Hyp extension [v1,18/36] target/riscv: Extend the MIE CSR to support virtulisation [v1,19/36] target/riscv: Extend the SIP CSR to support virtulisation [v1,20/36] target/riscv: Add support for virtual interrupt setting [v1,21/36] target/ricsv: Flush the TLB on virtulisation mode changes [v1,22/36] target/riscv: Generate illegal instruction on WFI when V=1 [v1,23/36] target/riscv: Add hypvervisor trap support [v1,24/36] target/riscv: Add Hypervisor trap return support [v1,25/36] target/riscv: Add hfence instructions [v1,26/36] target/riscv: Remove the hret instruction [v1,27/36] target/riscv: Disable guest FP support based on virtual status [v1,28/36] target/riscv: Mark both sstatus and vsstatus as dirty [v1,29/36] target/riscv: Respect MPRV and SPRV for floating point ops [v1,30/36] target/riscv: Allow specifying MMU stage [v1,31/36] target/riscv: Implement second stage MMU [v1,32/36] target/riscv: Raise the new execptions when 2nd stage translation fails [v1,33/36] target/riscv: Set htval and mtval2 on execptions [v1,34/36] target/riscv: Add support for the 32-bit MSTATUSH CSR [v1,35/36] target/riscv: Add the MSTATUS_MPV_ISSET helper macro [v1,36/36] target/riscv: Allow enabling the Hypervisor extension

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(-)

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);
> >>
>

Palmer Dabbelt Jan. 30, 2020, 2:48 p.m. UTC | #6

On Tue, 21 Jan 2020 11:02:01 GMT (+0000), 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.

OK, so in the interest of moving things forwards let's just

Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>

so we can merge this -- it's too big of a patch set to wait around on something
so small for.  I think that was the last one missing a review, right?

>
> 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. 31, 2020, 5:31 p.m. UTC | #7

On Thu, Jan 30, 2020 at 6:48 AM Palmer Dabbelt <palmerdabbelt@google.com> wrote:
>
> On Tue, 21 Jan 2020 11:02:01 GMT (+0000), 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.
>
> OK, so in the interest of moving things forwards let's just
>
> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>

Thanks

>
> so we can merge this -- it's too big of a patch set to wait around on something
> so small for.  I think that was the last one missing a review, right?

I have made one small change and dismissed your review from a patch,
it also looks like one patch hasn't been reviewed either.

I'll send a v2 later today that has been rebased on master.

Alistair

>
> >
> > 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 Feb. 1, 2020, 12:09 a.m. UTC | #8

On Fri, Jan 31, 2020 at 9:31 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Jan 30, 2020 at 6:48 AM Palmer Dabbelt <palmerdabbelt@google.com> wrote:
> >
> > On Tue, 21 Jan 2020 11:02:01 GMT (+0000), 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.
> >
> > OK, so in the interest of moving things forwards let's just
> >
> > Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
>
> Thanks
>
> >
> > so we can merge this -- it's too big of a patch set to wait around on something
> > so small for.  I think that was the last one missing a review, right?
>
> I have made one small change and dismissed your review from a patch,
> it also looks like one patch hasn't been reviewed either.
>
> I'll send a v2 later today that has been rebased on master.

After rebasing everything stopped working. While attempting to fix the
problem I remeoved this patch and managed to get it working again. So
now in v2 there are no CSR pointers, just value swapping for
everything.

I fixed a few bugs that I noticed related to FP, I have cleared your
review Palmer from a few patches. I'll send the v2 for review soon.
It's pretty similar to the v1 so should be easy to review.

Alistair

>
> Alistair
>
> >
> > >
> > > 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);

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

Commit Message

Comments

Patch