[PATCH v2 0/2] RISC-V TIME CSR for privileged mode

qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed

* [PATCH v2 0/2] RISC-V TIME CSR for privileged mode
@ 2020-01-22 11:30 Anup Patel
  2020-01-22 11:30 ` [PATCH v2 1/2] target/riscv: Emulate TIME CSRs " Anup Patel
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Anup Patel @ 2020-01-22 11:30 UTC (permalink / raw)
  To: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar
  Cc: Atish Patra, Anup Patel, qemu-riscv@nongnu.org,
	qemu-devel@nongnu.org, Anup Patel

This series adds emulation of TIME CSRs for privileged mode. With
this series, we see approximately 25+% improvement in hackbench
numbers for non-virtualized (or Host) Linux and 40+% improvement
in hackbench numbers for Guest/VM Linux.

These patches are based on mainline/alistair/riscv-hyp-ext-v0.5.1
branch of https://github.com/kvm-riscv/qemu.git and can be found
in riscv_time_csr_v2 branch of same repo.

Changes since v1:
 - Use braces for single-line if-statements

Anup Patel (2):
  target/riscv: Emulate TIME CSRs for privileged mode
  hw/riscv: Provide rdtime callback for TCG in CLINT emulation

 hw/riscv/sifive_clint.c   |  1 +
 target/riscv/cpu.h        |  5 +++
 target/riscv/cpu_helper.c |  5 +++
 target/riscv/csr.c        | 86 +++++++++++++++++++++++++++++++++++++--
 4 files changed, 93 insertions(+), 4 deletions(-)

-- 
2.17.1



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 1/2] target/riscv: Emulate TIME CSRs for privileged mode
  2020-01-22 11:30 [PATCH v2 0/2] RISC-V TIME CSR for privileged mode Anup Patel
@ 2020-01-22 11:30 ` Anup Patel
  2020-01-22 11:30 ` [PATCH v2 2/2] hw/riscv: Provide rdtime callback for TCG in CLINT emulation Anup Patel
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: Anup Patel @ 2020-01-22 11:30 UTC (permalink / raw)
  To: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar
  Cc: Atish Patra, Anup Patel, qemu-riscv@nongnu.org,
	qemu-devel@nongnu.org, Anup Patel

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 <anup.patel@wdc.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 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;
+}
+
+#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



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/2] hw/riscv: Provide rdtime callback for TCG in CLINT emulation
  2020-01-22 11:30 [PATCH v2 0/2] RISC-V TIME CSR for privileged mode Anup Patel
  2020-01-22 11:30 ` [PATCH v2 1/2] target/riscv: Emulate TIME CSRs " Anup Patel
@ 2020-01-22 11:30 ` Anup Patel
  2020-01-30 14:44 ` [PATCH v2 1/2] target/riscv: Emulate TIME CSRs for privileged mode Palmer Dabbelt
  2020-01-30 14:49 ` [PATCH v2 2/2] hw/riscv: Provide rdtime callback for TCG in CLINT emulation Palmer Dabbelt
  3 siblings, 0 replies; 7+ messages in thread
From: Anup Patel @ 2020-01-22 11:30 UTC (permalink / raw)
  To: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar
  Cc: Atish Patra, Anup Patel, qemu-riscv@nongnu.org,
	qemu-devel@nongnu.org, Anup Patel

This patch extends CLINT emulation to provide rdtime callback for
TCG. This rdtime callback will be called wheneven TIME CSRs are
read in privileged modes.

Signed-off-by: Anup Patel <anup.patel@wdc.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/sifive_clint.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
index e5a8f75cee..805503dc27 100644
--- a/hw/riscv/sifive_clint.c
+++ b/hw/riscv/sifive_clint.c
@@ -236,6 +236,7 @@ DeviceState *sifive_clint_create(hwaddr addr, hwaddr size, uint32_t num_harts,
         if (!env) {
             continue;
         }
+        riscv_cpu_set_rdtime_fn(env, cpu_riscv_read_rtc);
         env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                   &sifive_clint_timer_cb, cpu);
         env->timecmp = 0;
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] target/riscv: Emulate TIME CSRs for privileged mode
  2020-01-22 11:30 [PATCH v2 0/2] RISC-V TIME CSR for privileged mode Anup Patel
  2020-01-22 11:30 ` [PATCH v2 1/2] target/riscv: Emulate TIME CSRs " Anup Patel
  2020-01-22 11:30 ` [PATCH v2 2/2] hw/riscv: Provide rdtime callback for TCG in CLINT emulation Anup Patel
@ 2020-01-30 14:44 ` Palmer Dabbelt
  2020-01-30 15:22   ` Anup Patel
  2020-01-30 14:49 ` [PATCH v2 2/2] hw/riscv: Provide rdtime callback for TCG in CLINT emulation Palmer Dabbelt
  3 siblings, 1 reply; 7+ messages in thread
From: Palmer Dabbelt @ 2020-01-30 14:44 UTC (permalink / raw)
  To: Anup Patel
  Cc: Peter Maydell, Alistair Francis, sagark, Atish Patra, anup,
	qemu-riscv, qemu-devel, Anup Patel

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 <anup.patel@wdc.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  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

> +
> +#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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] target/riscv: Emulate TIME CSRs for privileged mode
  2020-01-30 14:44 ` [PATCH v2 1/2] target/riscv: Emulate TIME CSRs for privileged mode Palmer Dabbelt
@ 2020-01-30 15:22   ` Anup Patel
  0 siblings, 0 replies; 7+ messages in thread
From: Anup Patel @ 2020-01-30 15:22 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Peter Maydell, open list:RISC-V, Sagar Karandikar, Anup Patel,
	QEMU Developers, Atish Patra, Alistair Francis

On Thu, Jan 30, 2020 at 8:14 PM Palmer Dabbelt <palmerdabbelt@google.com> 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 <anup.patel@wdc.com>
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 2/2] hw/riscv: Provide rdtime callback for TCG in CLINT emulation
  2020-01-22 11:30 [PATCH v2 0/2] RISC-V TIME CSR for privileged mode Anup Patel
                   ` (2 preceding siblings ...)
  2020-01-30 14:44 ` [PATCH v2 1/2] target/riscv: Emulate TIME CSRs for privileged mode Palmer Dabbelt
@ 2020-01-30 14:49 ` Palmer Dabbelt
  2020-01-30 15:27   ` Anup Patel
  3 siblings, 1 reply; 7+ messages in thread
From: Palmer Dabbelt @ 2020-01-30 14:49 UTC (permalink / raw)
  To: Anup Patel
  Cc: Peter Maydell, Alistair Francis, sagark, Atish Patra, anup,
	qemu-riscv, qemu-devel, Anup Patel

On Wed, 22 Jan 2020 11:30:36 GMT (+0000), Anup Patel wrote:
> This patch extends CLINT emulation to provide rdtime callback for
> TCG. This rdtime callback will be called wheneven TIME CSRs are
> read in privileged modes.
>
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/sifive_clint.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
> index e5a8f75cee..805503dc27 100644
> --- a/hw/riscv/sifive_clint.c
> +++ b/hw/riscv/sifive_clint.c
> @@ -236,6 +236,7 @@ DeviceState *sifive_clint_create(hwaddr addr, hwaddr size, uint32_t num_harts,
>          if (!env) {
>              continue;
>          }
> +        riscv_cpu_set_rdtime_fn(env, cpu_riscv_read_rtc);
>          env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>                                    &sifive_clint_timer_cb, cpu);
>          env->timecmp = 0;
> -- 
> 2.17.1

Can you make this optional?  Ideally via a command-line argument, but at a
minimum as via the board configuration files.  As it stands this will enable
the direct rdtime implemnetation everywhere, and while that's sensible for the
virt board I'd prefer to avoid changing the behavior of the sifive_u board in
ways that differ from the hardware when that's easy.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 2/2] hw/riscv: Provide rdtime callback for TCG in CLINT emulation
  2020-01-30 14:49 ` [PATCH v2 2/2] hw/riscv: Provide rdtime callback for TCG in CLINT emulation Palmer Dabbelt
@ 2020-01-30 15:27   ` Anup Patel
  0 siblings, 0 replies; 7+ messages in thread
From: Anup Patel @ 2020-01-30 15:27 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Peter Maydell, open list:RISC-V, Sagar Karandikar, Anup Patel,
	QEMU Developers, Atish Patra, Alistair Francis

On Thu, Jan 30, 2020 at 8:19 PM Palmer Dabbelt <palmerdabbelt@google.com> wrote:
>
> On Wed, 22 Jan 2020 11:30:36 GMT (+0000), Anup Patel wrote:
> > This patch extends CLINT emulation to provide rdtime callback for
> > TCG. This rdtime callback will be called wheneven TIME CSRs are
> > read in privileged modes.
> >
> > Signed-off-by: Anup Patel <anup.patel@wdc.com>
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  hw/riscv/sifive_clint.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
> > index e5a8f75cee..805503dc27 100644
> > --- a/hw/riscv/sifive_clint.c
> > +++ b/hw/riscv/sifive_clint.c
> > @@ -236,6 +236,7 @@ DeviceState *sifive_clint_create(hwaddr addr, hwaddr size, uint32_t num_harts,
> >          if (!env) {
> >              continue;
> >          }
> > +        riscv_cpu_set_rdtime_fn(env, cpu_riscv_read_rtc);
> >          env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> >                                    &sifive_clint_timer_cb, cpu);
> >          env->timecmp = 0;
> > --
> > 2.17.1
>
> Can you make this optional?  Ideally via a command-line argument, but at a
> minimum as via the board configuration files.  As it stands this will enable
> the direct rdtime implemnetation everywhere, and while that's sensible for the
> virt board I'd prefer to avoid changing the behavior of the sifive_u board in
> ways that differ from the hardware when that's easy.

Command-line will unnecessary make things complicated for users.

I think the better option is to make it board specific so that we can
emulate exact HW behaviour in QEMU. This way since real-world
SiFive unleashed board does not have TIME CSR even QEMU will
not emulate TIME CSR for "sifive_u" machine. For now, we should
definitely emulate TIME CSR for virt machine because of the
performance improvement.

Regards,
Anup


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-01-30 15:28 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-01-22 11:30 [PATCH v2 0/2] RISC-V TIME CSR for privileged mode Anup Patel
2020-01-22 11:30 ` [PATCH v2 1/2] target/riscv: Emulate TIME CSRs " Anup Patel
2020-01-22 11:30 ` [PATCH v2 2/2] hw/riscv: Provide rdtime callback for TCG in CLINT emulation Anup Patel
2020-01-30 14:44 ` [PATCH v2 1/2] target/riscv: Emulate TIME CSRs for privileged mode Palmer Dabbelt
2020-01-30 15:22   ` Anup Patel
2020-01-30 14:49 ` [PATCH v2 2/2] hw/riscv: Provide rdtime callback for TCG in CLINT emulation Palmer Dabbelt
2020-01-30 15:27   ` Anup Patel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).