Re: [PATCH v1 27/28] target/riscv: Add the MSTATUS_MPV_ISSET helper macro

[Alistair Francis]

On Tue, Oct 8, 2019 at 11:36 AM Palmer Dabbelt <address@hidden> wrote:
>
> On Fri, 23 Aug 2019 16:39:00 PDT (-0700), Alistair Francis wrote:
> > Add a helper macro MSTATUS_MPV_ISSET() which will determine if the
> > MSTATUS_MPV bit is set for both 32-bit and 64-bit RISC-V.
> >
> > Signed-off-by: Alistair Francis <address@hidden>
> > ---
> >  target/riscv/cpu_bits.h   | 11 +++++++++++
> >  target/riscv/cpu_helper.c |  4 ++--
> >  target/riscv/op_helper.c  |  2 +-
> >  target/riscv/translate.c  |  2 +-
> >  4 files changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index 55e20af6d9..7056d9218b 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -365,8 +365,19 @@
> >  #define MSTATUS_TVM         0x00100000 /* since: priv-1.10 */
> >  #define MSTATUS_TW          0x20000000 /* since: priv-1.10 */
> >  #define MSTATUS_TSR         0x40000000 /* since: priv-1.10 */
> > +#if defined(TARGET_RISCV64)
> >  #define MSTATUS_MTL         0x4000000000ULL
> >  #define MSTATUS_MPV         0x8000000000ULL
> > +#elif defined(TARGET_RISCV32)
> > +#define MSTATUS_MTL         0x00000040
> > +#define MSTATUS_MPV         0x00000080
> > +#endif
> > +
> > +#ifdef TARGET_RISCV32
> > +# define MSTATUS_MPV_ISSET(env)  get_field(*env->mstatush, MSTATUS_MPV)
> > +#else
> > +# define MSTATUS_MPV_ISSET(env)  get_field(*env->mstatus, MSTATUS_MPV)
> > +#endif
> >
> >  #define MSTATUS64_UXL       0x0000000300000000ULL
> >  #define MSTATUS64_SXL       0x0000000C00000000ULL
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 8c80486dd0..2b88f756bb 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -351,7 +351,7 @@ static int get_physical_address(CPURISCVState *env, 
> > hwaddr *physical,
> >              mode = get_field(*env->mstatus, MSTATUS_MPP);
> >
> >              if (riscv_has_ext(env, RVH) &&
> > -                get_field(*env->mstatus, MSTATUS_MPV)) {
> > +                MSTATUS_MPV_ISSET(env)) {
> >                  use_background = true;
> >              }
> >          }
> > @@ -730,7 +730,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, 
> > int size,
> >          m_mode_two_stage = env->priv == PRV_M &&
> >                             access_type != MMU_INST_FETCH &&
> >                             get_field(*env->mstatus, MSTATUS_MPRV) &&
> > -                           get_field(*env->mstatus, MSTATUS_MPV);
> > +                           MSTATUS_MPV_ISSET(env);
> >
> >          hs_mode_two_stage = env->priv == PRV_S &&
> >                              !riscv_cpu_virt_enabled(env) &&
> > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> > index 8dec1aee99..6149cd9c15 100644
> > --- a/target/riscv/op_helper.c
> > +++ b/target/riscv/op_helper.c
> > @@ -146,7 +146,7 @@ target_ulong helper_mret(CPURISCVState *env, 
> > target_ulong cpu_pc_deb)
> >
> >      target_ulong mstatus = *env->mstatus;
> >      target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP);
> > -    target_ulong prev_virt = get_field(mstatus, MSTATUS_MPV);
> > +    target_ulong prev_virt = MSTATUS_MPV_ISSET(env);
> >      mstatus = set_field(mstatus,
> >          env->priv_ver >= PRIV_VERSION_1_10_0 ?
> >          MSTATUS_MIE : MSTATUS_UIE << prev_priv,
> > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > index ea19ba9c5d..f0d9860429 100644
> > --- a/target/riscv/translate.c
> > +++ b/target/riscv/translate.c
> > @@ -754,7 +754,7 @@ static void 
> > riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
> >          ctx->virt_enabled = riscv_cpu_virt_enabled(env);
> >          if (env->priv_ver == PRV_M &&
> >              get_field(*env->mstatus, MSTATUS_MPRV) &&
> > -            get_field(*env->mstatus, MSTATUS_MPV)) {
> > +            MSTATUS_MPV_ISSET(env)) {
> >              ctx->virt_enabled = true;
> >          } else if (env->priv == PRV_S &&
> >                     !riscv_cpu_virt_enabled(env) &&
>
> This should be either ordered before or atomic with the patch that allows
> mstatush.mpv to be set, as otherwise there's point at which QEMU doesn't match
> the ISA.

I can't change the order due to dependencies, I can squash them but as
the Hypervisor extension can't be turned on there isn't really a
conflict with the ISA.

Do you still want me to squash them?

Alistair