Re: [Qemu-riscv] [Qemu-devel] [PATCH] target/riscv: Hardwire mcounter.TM and upper bits of [m|s]counteren

[Alistair Francis]

On Fri, Jun 28, 2019 at 1:12 PM <address@hidden> wrote:
>
> From: Jonathan Behrens <address@hidden>
>
> QEMU currently always triggers an illegal instruction exception when code
> attempts to read the time CSR. This is valid behavor, but only if the TM bit 
> in
> mcounteren is hardwired to zero. This change also corrects mcounteren and 
> scounteren CSRs to be 32-bits on both
> 32-bit and 64-bit targets.

Thanks for the patch.

Can you wrap your commit message at ~70 lines?

>
> Signed-off-by: Jonathan Behrens <address@hidden>
> ---
>  target/riscv/cpu.h | 4 ++--
>  target/riscv/csr.c | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 0adb307f32..2d0cbe9c78 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -151,8 +151,8 @@ struct CPURISCVState {
>      target_ulong mcause;
>      target_ulong mtval;  /* since: priv-1.10.0 */
>
> -    target_ulong scounteren;
> -    target_ulong mcounteren;
> +    uint32_t scounteren;
> +    uint32_t mcounteren;
>
>      target_ulong sscratch;
>      target_ulong mscratch;
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index e0d4586760..89cf9734c3 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -473,7 +473,8 @@ static int write_mcounteren(CPURISCVState *env, int 
> csrno, target_ulong val)
>      if (env->priv_ver < PRIV_VERSION_1_10_0) {
>          return -1;
>      }
> -    env->mcounteren = val;
> +    /* mcounteren.TM is hardwired to zero, all other bits are writable */
> +    env->mcounteren = val & ~(1 << (CSR_TIME & 31));

Isn't CSR_TIME & 31 just 0? Can this just be changed 1 << 1 or even
better add a macro?

Otherwise this patch looks good :)

Alistair

>      return 0;
>  }
>
> --
> 2.22.0
>