Re: [Qemu-devel] [PATCH v1 3/9] target/riscv: Comment in the mcountinhibit CSR

[Alistair Francis <alistair23@gmail.com>]

On Mon, Jun 24, 2019 at 2:31 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Mon, 17 Jun 2019 18:31:08 PDT (-0700), Alistair Francis wrote:
> > Add a comment for the new mcountinhibit which conflicts with the
> > CSR_MUCOUNTEREN from version 1.09.1. This can be updated when we remove
> > 1.09.1.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  target/riscv/cpu_bits.h | 1 +
> >  target/riscv/csr.c      | 6 ++++--
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index 47450a3cdb..11f971ad5d 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -136,6 +136,7 @@
> >  #define CSR_MCOUNTEREN      0x306
> >
> >  /* Legacy Counter Setup (priv v1.9.1) */
> > +/* Update to #define CSR_MCOUNTINHIBIT 0x320 for 1.11.0 */
> >  #define CSR_MUCOUNTEREN     0x320
> >  #define CSR_MSCOUNTEREN     0x321
> >  #define CSR_MHCOUNTEREN     0x322
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index c67d29e206..437387fd28 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -461,18 +461,20 @@ static int write_mcounteren(CPURISCVState *env, int csrno, target_ulong val)
> >      return 0;
> >  }
> >
> > +/* 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) {
> > +    if (env->priv_ver > PRIV_VERSION_1_09_1 && env->priv_ver < PRIV_VERSION_1_11_0) {
> >          return -1;
> >      }
> >      *val = env->mcounteren;
> >      return 0;
> >  }
> >
> > +/* 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) {
> > +    if (env->priv_ver > PRIV_VERSION_1_09_1 && env->priv_ver < PRIV_VERSION_1_11_0) {
> >          return -1;
> >      }
> >      env->mcounteren = val;
>
> I don't think this one is right: this should be unsupported on 1.11, as the
> semantics of this bit are slightly different.  It shouldn't be that hard to
> just emulate it fully for both 1.09.1 and 1.11: for 1.09 this disables access
> to the counters (which still tick), while for 1.11 it disables ticking the
> counters (which can still be accessed).  Since we don't do anything with the
> counters in QEMU, I think this should do it
>
> LMK if you're OK with me replacing the patch with this
>
> commit e9169ccd5ca97a036de41dad23f37f6724712b90
> Author: Alistair Francis <alistair.francis@wdc.com>
> Date:   Mon Jun 17 18:31:08 2019 -0700
>
>     target/riscv: Add the mcountinhibit CSR
>
>     1.11 defines mcountinhibit, which has the same numeric CSR value as
>     mucounteren from 1.09.1 but has different semantics.  This patch enables
>     the CSR for 1.11-based targets, which is trivial to implement because
>     the counters in QEMU never tick (legal according to the spec).
>
>     Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>     [Palmer: Fix counter access semantics, change commit message to indicate
>     the behavior is fully emulated.]
>     Reviewed-by: Palmer Dabbelt <palmer@sifive.com>
>     Signed-off-by: Palmer Dabbelt <palmer@sifive.com>

Yep, looks good.

Alistair

>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 47450a3cdb75..11f971ad5df0 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -136,6 +136,7 @@
>  #define CSR_MCOUNTEREN      0x306
>
>  /* Legacy Counter Setup (priv v1.9.1) */
> +/* Update to #define CSR_MCOUNTINHIBIT 0x320 for 1.11.0 */
>  #define CSR_MUCOUNTEREN     0x320
>  #define CSR_MSCOUNTEREN     0x321
>  #define CSR_MHCOUNTEREN     0x322
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index c67d29e20618..2622b2e05474 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -56,6 +56,14 @@ static int fs(CPURISCVState *env, int csrno)
>  static int ctr(CPURISCVState *env, int csrno)
>  {
>  #if !defined(CONFIG_USER_ONLY)
> +    /*
> +     * The counters are always enabled 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;
> +
>      uint32_t ctr_en = ~0u;
>
>      if (env->priv < PRV_M) {
> @@ -461,18 +469,20 @@ static int write_mcounteren(CPURISCVState *env, int csrno, target_ulong val)
>      return 0;
>  }
>
> +/* 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) {
> +    if (env->priv_ver > PRIV_VERSION_1_09_1 && env->priv_ver < PRIV_VERSION_1_11_0) {
>          return -1;
>      }
>      *val = env->mcounteren;
>      return 0;
>  }
>
> +/* 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) {
> +    if (env->priv_ver > PRIV_VERSION_1_09_1 && env->priv_ver < PRIV_VERSION_1_11_0) {
>          return -1;
>      }
>      env->mcounteren = val;