On Tue, Jan 21, 2020 at 7:03 PM Anup Patel <anup.pa...@wdc.com> 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.pa...@wdc.com>
> ---
> 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 <alistair.fran...@wdc.com>
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
>
>
