Re: [PATCH v2 3/3] target/riscv: Drop support for ISA spec version 1.09.

qemu-riscv

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 3/3] target/riscv: Drop support for ISA spec version 1.09.

From:	Alistair Francis
Subject:	Re: [PATCH v2 3/3] target/riscv: Drop support for ISA spec version 1.09.1
Date:	Thu, 21 May 2020 08:47:48 -0700

On Wed, May 20, 2020 at 6:18 PM Bin Meng <address@hidden> wrote:
>
> On Fri, May 8, 2020 at 3:22 AM Alistair Francis
> <address@hidden> wrote:
> >
> > The RISC-V ISA spec version 1.09.1 has been deprecated in QEMU since
> > 4.1. It's not commonly used so let's remove support for it.
> >
> > Signed-off-by: Alistair Francis <address@hidden>
> > ---
> >  target/riscv/cpu.c                            |  2 -
> >  target/riscv/cpu.h                            |  1 -
> >  target/riscv/csr.c                            | 82 ++++---------------
> >  .../riscv/insn_trans/trans_privileged.inc.c   |  6 --
> >  4 files changed, 17 insertions(+), 74 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 112f2e3a2f..eeb91f8513 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -368,8 +368,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> > **errp)
> >              priv_version = PRIV_VERSION_1_11_0;
> >          } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.10.0")) {
> >              priv_version = PRIV_VERSION_1_10_0;
> > -        } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.9.1")) {
> > -            priv_version = PRIV_VERSION_1_09_1;
> >          } else {
> >              error_setg(errp,
> >                         "Unsupported privilege spec version '%s'",
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 76b98d7a33..c022539012 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -73,7 +73,6 @@ enum {
> >      RISCV_FEATURE_MISA
> >  };
> >
> > -#define PRIV_VERSION_1_09_1 0x00010901
> >  #define PRIV_VERSION_1_10_0 0x00011000
> >  #define PRIV_VERSION_1_11_0 0x00011100
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 11d184cd16..df3498b24f 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -58,31 +58,11 @@ static int ctr(CPURISCVState *env, int csrno)
> >  #if !defined(CONFIG_USER_ONLY)
> >      CPUState *cs = env_cpu(env);
> >      RISCVCPU *cpu = RISCV_CPU(cs);
> > -    uint32_t ctr_en = ~0u;
> >
> >      if (!cpu->cfg.ext_counters) {
> >          /* The Counters extensions is not enabled */
> >          return -1;
> >      }
> > -
> > -    /*
> > -     * The counters are always enabled at run time on newer priv specs, as 
> > the
> > -     * CSR has changed from controlling that the counters can be read to
> > -     * controlling that the counters increment.
> > -     */
> > -    if (env->priv_ver > PRIV_VERSION_1_09_1) {
> > -        return 0;
> > -    }
> > -
> > -    if (env->priv < PRV_M) {
> > -        ctr_en &= env->mcounteren;
> > -    }
> > -    if (env->priv < PRV_S) {
> > -        ctr_en &= env->scounteren;
> > -    }
> > -    if (!(ctr_en & (1u << (csrno & 31)))) {
> > -        return -1;
> > -    }
> >  #endif
> >      return 0;
> >  }
> > @@ -358,34 +338,21 @@ static int write_mstatus(CPURISCVState *env, int 
> > csrno, target_ulong val)
> >      int dirty;
> >
> >      /* flush tlb on mstatus fields that affect VM */
> > -    if (env->priv_ver <= PRIV_VERSION_1_09_1) {
> > -        if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP |
> > -                MSTATUS_MPRV | MSTATUS_SUM | MSTATUS_VM)) {
> > -            tlb_flush(env_cpu(env));
> > -        }
> > -        mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE |
> > -            MSTATUS_SPP | MSTATUS_FS | MSTATUS_MPRV | MSTATUS_SUM |
> > -            MSTATUS_MPP | MSTATUS_MXR |
> > -            (validate_vm(env, get_field(val, MSTATUS_VM)) ?
> > -                MSTATUS_VM : 0);
> > +    if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
> > +            MSTATUS_MPRV | MSTATUS_SUM)) {
> > +        tlb_flush(env_cpu(env));
> >      }
> > -    if (env->priv_ver >= PRIV_VERSION_1_10_0) {
> > -        if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
> > -                MSTATUS_MPRV | MSTATUS_SUM)) {
> > -            tlb_flush(env_cpu(env));
> > -        }
> > -        mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE |
> > -            MSTATUS_SPP | MSTATUS_FS | MSTATUS_MPRV | MSTATUS_SUM |
> > -            MSTATUS_MPP | MSTATUS_MXR | MSTATUS_TVM | MSTATUS_TSR |
> > -            MSTATUS_TW;
> > +    mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE |
> > +        MSTATUS_SPP | MSTATUS_FS | MSTATUS_MPRV | MSTATUS_SUM |
> > +        MSTATUS_MPP | MSTATUS_MXR | MSTATUS_TVM | MSTATUS_TSR |
> > +        MSTATUS_TW;
> >  #if defined(TARGET_RISCV64)
> > -            /*
> > -             * RV32: MPV and MTL are not in mstatus. The current plan is to
> > -             * add them to mstatush. For now, we just don't support it.
> > -             */
> > -            mask |= MSTATUS_MTL | MSTATUS_MPV;
> > +        /*
> > +         * RV32: MPV and MTL are not in mstatus. The current plan is to
> > +         * add them to mstatush. For now, we just don't support it.
> > +         */
> > +        mask |= MSTATUS_MTL | MSTATUS_MPV;
>
> The indentation level is wrong

Good catch.

>
> >  #endif
> > -    }
> >
> >      mstatus = (mstatus & ~mask) | (val & mask);
> >
> > @@ -553,8 +520,7 @@ static int write_mcounteren(CPURISCVState *env, int 
> > csrno, target_ulong val)
> >  /* This regiser is replaced with CSR_MCOUNTINHIBIT in 1.11.0 */
> >  static int read_mscounteren(CPURISCVState *env, int csrno, target_ulong 
> > *val)
> >  {
> > -    if (env->priv_ver > PRIV_VERSION_1_09_1
> > -        && env->priv_ver < PRIV_VERSION_1_11_0) {
> > +    if (env->priv_ver < PRIV_VERSION_1_11_0) {
> >          return -1;
> >      }
> >      *val = env->mcounteren;
> > @@ -564,8 +530,7 @@ static int read_mscounteren(CPURISCVState *env, int 
> > csrno, target_ulong *val)
> >  /* This regiser is replaced with CSR_MCOUNTINHIBIT in 1.11.0 */
> >  static int write_mscounteren(CPURISCVState *env, int csrno, target_ulong 
> > val)
> >  {
> > -    if (env->priv_ver > PRIV_VERSION_1_09_1
> > -        && env->priv_ver < PRIV_VERSION_1_11_0) {
> > +    if (env->priv_ver < PRIV_VERSION_1_11_0) {
> >          return -1;
> >      }
> >      env->mcounteren = val;
> > @@ -574,20 +539,13 @@ static int write_mscounteren(CPURISCVState *env, int 
> > csrno, target_ulong val)
> >
> >  static int read_mucounteren(CPURISCVState *env, int csrno, target_ulong 
> > *val)
> >  {
> > -    if (env->priv_ver > PRIV_VERSION_1_09_1) {
> > -        return -1;
> > -    }
> > -    *val = env->scounteren;
> > +    return -1;
> >      return 0;
> >  }
> >
> >  static int write_mucounteren(CPURISCVState *env, int csrno, target_ulong 
> > val)
> >  {
> > -    if (env->priv_ver > PRIV_VERSION_1_09_1) {
> > -        return -1;
> > -    }
> > -    env->scounteren = val;
> > -    return 0;
> > +    return -1;
> >  }
> >
> >  /* Machine Trap Handling */
> > @@ -829,13 +787,7 @@ static int write_satp(CPURISCVState *env, int csrno, 
> > target_ulong val)
> >      if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
> >          return 0;
> >      }
> > -    if (env->priv_ver <= PRIV_VERSION_1_09_1 && (val ^ env->sptbr)) {
> > -        tlb_flush(env_cpu(env));
> > -        env->sptbr = val & (((target_ulong)
> > -            1 << (TARGET_PHYS_ADDR_SPACE_BITS - PGSHIFT)) - 1);
> > -    }
> > -    if (env->priv_ver >= PRIV_VERSION_1_10_0 &&
> > -        validate_vm(env, get_field(val, SATP_MODE)) &&
> > +    if (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)) {
> > diff --git a/target/riscv/insn_trans/trans_privileged.inc.c 
> > b/target/riscv/insn_trans/trans_privileged.inc.c
> > index 76c2fad71c..1af9fa7df8 100644
> > --- a/target/riscv/insn_trans/trans_privileged.inc.c
> > +++ b/target/riscv/insn_trans/trans_privileged.inc.c
> > @@ -95,12 +95,6 @@ static bool trans_sfence_vma(DisasContext *ctx, 
> > arg_sfence_vma *a)
> >
> >  static bool trans_sfence_vm(DisasContext *ctx, arg_sfence_vm *a)
> >  {
> > -#ifndef CONFIG_USER_ONLY
> > -    if (ctx->priv_ver <= PRIV_VERSION_1_09_1) {
> > -        gen_helper_tlb_flush(cpu_env);
> > -        return true;
> > -    }
> > -#endif
> >      return false;
> >  }
>
> There are 3 more places in this file that should be cleaned up:
>
> ./target/riscv/insn_trans/trans_privileged.inc.c:88:    if
> (ctx->priv_ver >= PRIV_VERSION_1_10_0) {
> ./target/riscv/insn_trans/trans_privileged.inc.c:110:    if
> (ctx->priv_ver >= PRIV_VERSION_1_10_0 &&
> ./target/riscv/insn_trans/trans_privileged.inc.c:130:    if
> (ctx->priv_ver >= PRIV_VERSION_1_10_0 &&
>
> And the following place in monitor.c should be cleaned up too:
>
> ./target/riscv/monitor.c:218:    if (env->priv_ver < PRIV_VERSION_1_10_0) {
>
> And several other places in
>
> ./target/riscv/op_helper.c:87:    if (env->priv_ver >= PRIV_VERSION_1_10_0 &&
> ./target/riscv/op_helper.c:123:            env->priv_ver >=
> PRIV_VERSION_1_10_0 ?
> ./target/riscv/op_helper.c:151:        env->priv_ver >= PRIV_VERSION_1_10_0 ?
> ./target/riscv/op_helper.c:180:        env->priv_ver >= PRIV_VERSION_1_10_0 &&
> ./target/riscv/op_helper.c:196:         env->priv_ver >= PRIV_VERSION_1_10_0 
> &&

Yeah you are right. I have fixed all of these.

Alistair

>
> Regards,
> Bin

[Prev in Thread]

Current Thread

[Next in Thread]

[PATCH v2 0/3] RTISC-V: Remove deprecated ISA, CPUs and machines, Alistair Francis, 2020/05/07
- [PATCH v2 1/3] hw/riscv: spike: Remove deprecated ISA specific machines, Alistair Francis, 2020/05/07
  - Re: [PATCH v2 1/3] hw/riscv: spike: Remove deprecated ISA specific machines, Bin Meng, 2020/05/20
  - Re: [PATCH v2 1/3] hw/riscv: spike: Remove deprecated ISA specific machines, Bin Meng, 2020/05/21
- [PATCH v2 2/3] target/riscv: Remove the deprecated CPUs, Alistair Francis, 2020/05/07
  - Re: [PATCH v2 2/3] target/riscv: Remove the deprecated CPUs, Bin Meng, 2020/05/20
- [PATCH v2 3/3] target/riscv: Drop support for ISA spec version 1.09.1, Alistair Francis, 2020/05/07
  - Re: [PATCH v2 3/3] target/riscv: Drop support for ISA spec version 1.09.1, Bin Meng, 2020/05/20
    - Re: [PATCH v2 3/3] target/riscv: Drop support for ISA spec version 1.09.1, Alistair Francis <=
- Re: [PATCH v2 0/3] RTISC-V: Remove deprecated ISA, CPUs and machines, Alistair Francis, 2020/05/19

Prev by Date: Re: [PATCH v2 02/10] MAINTAINERS: Add an 'overall' entry for accelerators
Next by Date: Re: [PATCH] riscv: Change the default behavior if no -bios option is specified
Previous by thread: Re: [PATCH v2 3/3] target/riscv: Drop support for ISA spec version 1.09.1
Next by thread: Re: [PATCH v2 0/3] RTISC-V: Remove deprecated ISA, CPUs and machines
Index(es):
- Date
- Thread