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

[Anup Patel]

On Thu, Jan 30, 2020 at 8:14 PM Palmer Dabbelt <address@hidden> wrote:
>
> On Wed, 22 Jan 2020 11:30:31 GMT (+0000), Anup Patel 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>
> > Reviewed-by: Alistair Francis <address@hidden>
> > ---
> >  target/riscv/cpu.h        |  5 +++
> >  target/riscv/cpu_helper.c |  5 +++
> >  target/riscv/csr.c        | 86 +++++++++++++++++++++++++++++++++++++--
> >  3 files changed, 92 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..44ff1d80ec 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -238,6 +238,32 @@ 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;
> > +    }
> > +
> > +    *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 +957,56 @@ 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;
> > +}
>
> Oh, I guess I missed this when reading Alistair's H extension patches, but it
> looks like htimedelta is mandatory.  In other words, these writes should
> succeed regardless of whether or not rdtime is implemented.  I opened a
> question on the spec to make sure, but if that's the case then it should 
> always
> be implemented: https://github.com/riscv/riscv-isa-manual/issues/481

The HTIMEDELTA CSR is tide with TIME CSR because it controls
TIME CSR value read from VS-mode and VU-mode.

TIME (VS-mode or VU-mode) = TIME (HS-mode or M-mode) + HTIMEDELTA

If HW does not implement TIME CSR then HW should not implement
HTIMEDELTA as well. In this case, OpenSBI will trap-n-emulate both
TIME CSR and HTIMEDELTA CSR. If HW implements TIME CSR then HW
has to implement HTIMEDELTA as well.

To summarize, TIME CSR is optional for HW hence HTIMEDELTA is
optional for HW as well. Also, HW designers should either implement
both CSRs or skip both.

Regards,
Anup

>
> > +
> > +#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 +1279,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 +1350,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