Re: [PATCH 1/2] target/riscv: Emulate TIME CSRs for privileged mode

[Alistair Francis]

On Tue, Jan 21, 2020 at 7:03 PM Anup Patel <address@hidden> wrote:
>
> Currently, TIME CSRs are emulated only for user-only mode. This
> patch add TIME CSRs emulation for privileged mode.
>
> For privileged mode, the TIME CSRs will return value provided
> by rdtime callback which is registered by QEMU machine/platform
> emulation (i.e. CLINT emulation). If rdtime callback is not
> available then the monitor (i.e. OpenSBI) will trap-n-emulate
> TIME CSRs in software.
>
> We see 25+% performance improvement in hackbench numbers when
> TIME CSRs are not trap-n-emulated.
>
> Signed-off-by: Anup Patel <address@hidden>
> ---
>  target/riscv/cpu.h        |  5 +++
>  target/riscv/cpu_helper.c |  5 +++
>  target/riscv/csr.c        | 80 +++++++++++++++++++++++++++++++++++++--
>  3 files changed, 86 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 53bc6af5f7..473e01da6c 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -169,6 +169,7 @@ struct CPURISCVState {
>      target_ulong htval;
>      target_ulong htinst;
>      target_ulong hgatp;
> +    uint64_t htimedelta;
>
>      /* Virtual CSRs */
>      target_ulong vsstatus;
> @@ -204,6 +205,9 @@ struct CPURISCVState {
>      /* physical memory protection */
>      pmp_table_t pmp_state;
>
> +    /* machine specific rdtime callback */
> +    uint64_t (*rdtime_fn)(void);
> +
>      /* True if in debugger mode.  */
>      bool debugger;
>  #endif
> @@ -325,6 +329,7 @@ void riscv_cpu_swap_hypervisor_regs(CPURISCVState *env);
>  int riscv_cpu_claim_interrupts(RISCVCPU *cpu, uint32_t interrupts);
>  uint32_t riscv_cpu_update_mip(RISCVCPU *cpu, uint32_t mask, uint32_t value);
>  #define BOOL_TO_MASK(x) (-!!(x)) /* helper for riscv_cpu_update_mip value */
> +void riscv_cpu_set_rdtime_fn(CPURISCVState *env, uint64_t (*fn)(void));
>  #endif
>  void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv);
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 7166e6199e..c85f44d933 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -250,6 +250,11 @@ uint32_t riscv_cpu_update_mip(RISCVCPU *cpu, uint32_t 
> mask, uint32_t value)
>      return old;
>  }
>
> +void riscv_cpu_set_rdtime_fn(CPURISCVState *env, uint64_t (*fn)(void))
> +{
> +    env->rdtime_fn = fn;
> +}
> +
>  void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv)
>  {
>      if (newpriv > PRV_M) {
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index b28058f9d5..a55b543735 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -238,6 +238,30 @@ static int read_timeh(CPURISCVState *env, int csrno, 
> target_ulong *val)
>
>  #else /* CONFIG_USER_ONLY */
>
> +static int read_time(CPURISCVState *env, int csrno, target_ulong *val)
> +{
> +    uint64_t delta = riscv_cpu_virt_enabled(env) ? env->htimedelta : 0;
> +
> +    if (!env->rdtime_fn)
> +        return -1;

QEMU uses brackets around single line if statements (like Rust :) ).
Can you add brackets to all of them?

After that:

Reviewed-by: Alistair Francis <address@hidden>

Alistair

> +
> +    *val = env->rdtime_fn() + delta;
> +    return 0;
> +}
> +
> +#if defined(TARGET_RISCV32)
> +static int read_timeh(CPURISCVState *env, int csrno, target_ulong *val)
> +{
> +    uint64_t delta = riscv_cpu_virt_enabled(env) ? env->htimedelta : 0;
> +
> +    if (!env->rdtime_fn)
> +        return -1;
> +
> +    *val = (env->rdtime_fn() + delta) >> 32;
> +    return 0;
> +}
> +#endif
> +
>  /* Machine constants */
>
>  #define M_MODE_INTERRUPTS  (MIP_MSIP | MIP_MTIP | MIP_MEIP)
> @@ -931,6 +955,52 @@ static int write_hgatp(CPURISCVState *env, int csrno, 
> target_ulong val)
>      return 0;
>  }
>
> +static int read_htimedelta(CPURISCVState *env, int csrno, target_ulong *val)
> +{
> +    if (!env->rdtime_fn)
> +        return -1;
> +
> +#if defined(TARGET_RISCV32)
> +    *val = env->htimedelta & 0xffffffff;
> +#else
> +    *val = env->htimedelta;
> +#endif
> +    return 0;
> +}
> +
> +static int write_htimedelta(CPURISCVState *env, int csrno, target_ulong val)
> +{
> +    if (!env->rdtime_fn)
> +        return -1;
> +
> +#if defined(TARGET_RISCV32)
> +    env->htimedelta = deposit64(env->htimedelta, 0, 32, (uint64_t)val);
> +#else
> +    env->htimedelta = val;
> +#endif
> +    return 0;
> +}
> +
> +#if defined(TARGET_RISCV32)
> +static int read_htimedeltah(CPURISCVState *env, int csrno, target_ulong *val)
> +{
> +    if (!env->rdtime_fn)
> +        return -1;
> +
> +    *val = env->htimedelta >> 32;
> +    return 0;
> +}
> +
> +static int write_htimedeltah(CPURISCVState *env, int csrno, target_ulong val)
> +{
> +    if (!env->rdtime_fn)
> +        return -1;
> +
> +    env->htimedelta = deposit64(env->htimedelta, 32, 32, (uint64_t)val);
> +    return 0;
> +}
> +#endif
> +
>  /* Virtual CSR Registers */
>  static int read_vsstatus(CPURISCVState *env, int csrno, target_ulong *val)
>  {
> @@ -1203,14 +1273,12 @@ static riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = 
> {
>      [CSR_INSTRETH] =            { ctr,  read_instreth                       
> },
>  #endif
>
> -    /* User-level time CSRs are only available in linux-user
> -     * In privileged mode, the monitor emulates these CSRs */
> -#if defined(CONFIG_USER_ONLY)
> +    /* In privileged mode, the monitor will have to emulate TIME CSRs only if
> +     * rdtime callback is not provided by machine/platform emulation */
>      [CSR_TIME] =                { ctr,  read_time                           
> },
>  #if defined(TARGET_RISCV32)
>      [CSR_TIMEH] =               { ctr,  read_timeh                          
> },
>  #endif
> -#endif
>
>  #if !defined(CONFIG_USER_ONLY)
>      /* Machine Timers and Counters */
> @@ -1276,6 +1344,10 @@ static riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>      [CSR_HTVAL] =               { hmode,   read_htval,       write_htval     
>  },
>      [CSR_HTINST] =              { hmode,   read_htinst,      write_htinst    
>  },
>      [CSR_HGATP] =               { hmode,   read_hgatp,       write_hgatp     
>  },
> +    [CSR_HTIMEDELTA] =          { hmode,   read_htimedelta,  
> write_htimedelta },
> +#if defined(TARGET_RISCV32)
> +    [CSR_HTIMEDELTAH] =         { hmode,   read_htimedeltah, 
> write_htimedeltah},
> +#endif
>
>      [CSR_VSSTATUS] =            { hmode,   read_vsstatus,    write_vsstatus  
>  },
>      [CSR_VSIP] =                { hmode,   NULL,     NULL,     rmw_vsip      
>  },
> --
> 2.17.1
>
>