[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 5/5] target/riscv: Implement privilege mode filtering for
|
From: |
Alistair Francis |
|
Subject: |
Re: [PATCH v5 5/5] target/riscv: Implement privilege mode filtering for cycle/instret |
|
Date: |
Wed, 20 Mar 2024 14:54:11 +1000 |
On Thu, Mar 7, 2024 at 7:26 PM Atish Patra <atishp@rivosinc.com> wrote:
>
>
> On 3/4/24 22:47, LIU Zhiwei wrote:
> >
> > On 2024/2/29 2:51, Atish Patra wrote:
> >> Privilege mode filtering can also be emulated for cycle/instret by
> >> tracking host_ticks/icount during each privilege mode switch. This
> >> patch implements that for both cycle/instret and mhpmcounters. The
> >> first one requires Smcntrpmf while the other one requires Sscofpmf
> >> to be enabled.
> >>
> >> The cycle/instret are still computed using host ticks when icount
> >> is not enabled. Otherwise, they are computed using raw icount which
> >> is more accurate in icount mode.
> >>
> >> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> >> ---
> >> target/riscv/cpu.h | 11 +++++
> >> target/riscv/cpu_bits.h | 5 ++
> >> target/riscv/cpu_helper.c | 17 ++++++-
> >> target/riscv/csr.c | 96 ++++++++++++++++++++++++++++++---------
> >> target/riscv/pmu.c | 64 ++++++++++++++++++++++++++
> >> target/riscv/pmu.h | 2 +
> >> 6 files changed, 171 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >> index 174e8ba8e847..9e21d7f7d635 100644
> >> --- a/target/riscv/cpu.h
> >> +++ b/target/riscv/cpu.h
> >> @@ -157,6 +157,15 @@ typedef struct PMUCTRState {
> >> target_ulong irq_overflow_left;
> >> } PMUCTRState;
> >> +typedef struct PMUFixedCtrState {
> >> + /* Track cycle and icount for each privilege mode */
> >> + uint64_t counter[4];
> >> + uint64_t counter_prev[4];
> >> + /* Track cycle and icount for each privilege mode when V = 1*/
> >> + uint64_t counter_virt[2];
> >> + uint64_t counter_virt_prev[2];
> >> +} PMUFixedCtrState;
> >> +
> >> struct CPUArchState {
> >> target_ulong gpr[32];
> >> target_ulong gprh[32]; /* 64 top bits of the 128-bit registers */
> >> @@ -353,6 +362,8 @@ struct CPUArchState {
> >> /* PMU event selector configured values for RV32 */
> >> target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
> >> + PMUFixedCtrState pmu_fixed_ctrs[2];
> >> +
> >> target_ulong sscratch;
> >> target_ulong mscratch;
> >> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> >> index e866c60a400c..5fe349e313dc 100644
> >> --- a/target/riscv/cpu_bits.h
> >> +++ b/target/riscv/cpu_bits.h
> >> @@ -920,6 +920,11 @@ typedef enum RISCVException {
> >> #define MHPMEVENT_BIT_VUINH BIT_ULL(58)
> >> #define MHPMEVENTH_BIT_VUINH BIT(26)
> >> +#define MHPMEVENT_FILTER_MASK (MHPMEVENT_BIT_MINH | \
> >> + MHPMEVENT_BIT_SINH | \
> >> + MHPMEVENT_BIT_UINH | \
> >> + MHPMEVENT_BIT_VSINH | \
> >> + MHPMEVENT_BIT_VUINH)
> >> #define MHPMEVENT_SSCOF_MASK _ULL(0xFFFF000000000000)
> >> #define MHPMEVENT_IDX_MASK 0xFFFFF
> >> #define MHPMEVENT_SSCOF_RESVD 16
> >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> >> index d462d95ee165..33965d843d46 100644
> >> --- a/target/riscv/cpu_helper.c
> >> +++ b/target/riscv/cpu_helper.c
> >> @@ -718,8 +718,21 @@ void riscv_cpu_set_mode(CPURISCVState *env,
> >> target_ulong newpriv)
> >> {
> >> g_assert(newpriv <= PRV_M && newpriv != PRV_RESERVED);
> >> - if (icount_enabled() && newpriv != env->priv) {
> >> - riscv_itrigger_update_priv(env);
> >> + /*
> >> + * Invoke cycle/instret update between priv mode changes or
> >> + * VS->HS mode transition is SPV bit must be set
> >> + * HS->VS mode transition where virt_enabled must be set
> >> + * In both cases, priv will S mode only.
> >> + */
> >> + if (newpriv != env->priv ||
> >> + (env->priv == PRV_S && newpriv == PRV_S &&
> >> + (env->virt_enabled || get_field(env->hstatus, HSTATUS_SPV)))) {
> >> + if (icount_enabled()) {
> >> + riscv_itrigger_update_priv(env);
> >> + riscv_pmu_icount_update_priv(env, newpriv);
> >> + } else {
> >> + riscv_pmu_cycle_update_priv(env, newpriv);
> >> + }
> >> }
> >> /* tlb_flush is unnecessary as mode is contained in mmu_idx */
> >> env->priv = newpriv;
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index ff9bac537593..482e212c5f74 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -788,32 +788,16 @@ static RISCVException write_vcsr(CPURISCVState
> >> *env, int csrno,
> >> return RISCV_EXCP_NONE;
> >> }
> >> +#if defined(CONFIG_USER_ONLY)
> >> /* User Timers and Counters */
> >> static target_ulong get_ticks(bool shift)
> >> {
> >> - int64_t val;
> >> - target_ulong result;
> >> -
> >> -#if !defined(CONFIG_USER_ONLY)
> >> - if (icount_enabled()) {
> >> - val = icount_get();
> >> - } else {
> >> - val = cpu_get_host_ticks();
> >> - }
> >> -#else
> >> - val = cpu_get_host_ticks();
> >> -#endif
> >> -
> >> - if (shift) {
> >> - result = val >> 32;
> >> - } else {
> >> - result = val;
> >> - }
> >> + int64_t val = cpu_get_host_ticks();
> >> + target_ulong result = shift ? val >> 32 : val;
> >> return result;
> >> }
> >> -#if defined(CONFIG_USER_ONLY)
> >> static RISCVException read_time(CPURISCVState *env, int csrno,
> >> target_ulong *val)
> >> {
> >> @@ -952,6 +936,71 @@ static RISCVException
> >> write_mhpmeventh(CPURISCVState *env, int csrno,
> >> return RISCV_EXCP_NONE;
> >> }
> >> +static target_ulong
> >> riscv_pmu_ctr_get_fixed_counters_val(CPURISCVState *env,
> >> + int
> >> counter_idx,
> >> + bool
> >> upper_half)
> >> +{
> >> + uint64_t curr_val = 0;
> >> + target_ulong result = 0;
> >> + uint64_t *counter_arr = icount_enabled() ?
> >> env->pmu_fixed_ctrs[1].counter :
> >> + env->pmu_fixed_ctrs[0].counter;
> >> + uint64_t *counter_arr_virt = icount_enabled() ?
> >> + env->pmu_fixed_ctrs[1].counter_virt :
> >> + env->pmu_fixed_ctrs[0].counter_virt;
> >> + uint64_t cfg_val = 0;
> >> +
> >> + if (counter_idx == 0) {
> >> + cfg_val = upper_half ? ((uint64_t)env->mcyclecfgh << 32) :
> >> + env->mcyclecfg;
> >> + } else if (counter_idx == 2) {
> >> + cfg_val = upper_half ? ((uint64_t)env->minstretcfgh << 32) :
> >> + env->minstretcfg;
> >> + } else {
> >> + cfg_val = upper_half ?
> >> + ((uint64_t)env->mhpmeventh_val[counter_idx] << 32) :
> >> + env->mhpmevent_val[counter_idx];
> >> + cfg_val &= MHPMEVENT_FILTER_MASK;
> >> + }
> >> +
> >> + if (!cfg_val) {
> >> + if (icount_enabled()) {
> >> + curr_val = icount_get_raw();
> >> + } else {
> >> + curr_val = cpu_get_host_ticks();
> >> + }
> >> + goto done;
> >> + }
> >> +
> >> + if (!(cfg_val & MCYCLECFG_BIT_MINH)) {
> >> + curr_val += counter_arr[PRV_M];
> >> + }
> >> +
> >> + if (!(cfg_val & MCYCLECFG_BIT_SINH)) {
> >> + curr_val += counter_arr[PRV_S];
> >> + }
> >> +
> >> + if (!(cfg_val & MCYCLECFG_BIT_UINH)) {
> >> + curr_val += counter_arr[PRV_U];
> >> + }
> >> +
> >> + if (!(cfg_val & MCYCLECFG_BIT_VSINH)) {
> >> + curr_val += counter_arr_virt[PRV_S];
> >> + }
> >> +
> >> + if (!(cfg_val & MCYCLECFG_BIT_VUINH)) {
> >> + curr_val += counter_arr_virt[PRV_U];
> >> + }
> >> +
> >> +done:
> >> + if (riscv_cpu_mxl(env) == MXL_RV32) {
> >> + result = upper_half ? curr_val >> 32 : curr_val;
> >> + } else {
> >> + result = curr_val;
> >> + }
> >> +
> >> + return result;
> >> +}
> >> +
> >> static RISCVException write_mhpmcounter(CPURISCVState *env, int csrno,
> >> target_ulong val)
> >> {
> >> @@ -962,7 +1011,8 @@ static RISCVException
> >> write_mhpmcounter(CPURISCVState *env, int csrno,
> >> counter->mhpmcounter_val = val;
> >> if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
> >> riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
> >> - counter->mhpmcounter_prev = get_ticks(false);
> >> + counter->mhpmcounter_prev =
> >> riscv_pmu_ctr_get_fixed_counters_val(env,
> >> + ctr_idx, false);
> >> if (ctr_idx > 2) {
> >> if (riscv_cpu_mxl(env) == MXL_RV32) {
> >> mhpmctr_val = mhpmctr_val |
> >> @@ -990,7 +1040,8 @@ static RISCVException
> >> write_mhpmcounterh(CPURISCVState *env, int csrno,
> >> mhpmctr_val = mhpmctr_val | (mhpmctrh_val << 32);
> >> if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
> >> riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
> >> - counter->mhpmcounterh_prev = get_ticks(true);
> >> + counter->mhpmcounterh_prev =
> >> riscv_pmu_ctr_get_fixed_counters_val(env,
> >> + ctr_idx, true);
> >> if (ctr_idx > 2) {
> >> riscv_pmu_setup_timer(env, mhpmctr_val, ctr_idx);
> >> }
> >> @@ -1031,7 +1082,8 @@ static RISCVException
> >> riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
> >> */
> >> if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
> >> riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
> >> - *val = get_ticks(upper_half) - ctr_prev + ctr_val;
> >> + *val = riscv_pmu_ctr_get_fixed_counters_val(env, ctr_idx,
> >> upper_half) -
> >> + ctr_prev + ctr_val;
> >> } else {
> >> *val = ctr_val;
> >> }
> >> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> >> index 0e7d58b8a5c2..37309ff64cb6 100644
> >> --- a/target/riscv/pmu.c
> >> +++ b/target/riscv/pmu.c
> >> @@ -19,6 +19,7 @@
> >> #include "qemu/osdep.h"
> >> #include "qemu/log.h"
> >> #include "qemu/error-report.h"
> >> +#include "qemu/timer.h"
> >> #include "cpu.h"
> >> #include "pmu.h"
> >> #include "sysemu/cpu-timers.h"
> >> @@ -176,6 +177,69 @@ static int riscv_pmu_incr_ctr_rv64(RISCVCPU
> >> *cpu, uint32_t ctr_idx)
> >> return 0;
> >> }
> >> +void riscv_pmu_icount_update_priv(CPURISCVState *env, target_ulong
> >> newpriv)
> >> +{
> >> + uint64_t delta;
> >> + uint64_t *counter_arr;
> >> + uint64_t *counter_arr_prev;
> >> + uint64_t current_icount = icount_get_raw();
> >> +
> >> + if (env->virt_enabled) {
> >> + counter_arr = env->pmu_fixed_ctrs[1].counter_virt;
> >> + counter_arr_prev = env->pmu_fixed_ctrs[1].counter_virt_prev;
> >> + } else {
> >> + counter_arr = env->pmu_fixed_ctrs[1].counter;
> >> + counter_arr_prev = env->pmu_fixed_ctrs[1].counter_prev;
> >> + }
> >> +
> >> + if (newpriv != env->priv) {
> >> + delta = current_icount - counter_arr_prev[env->priv];
> >> + counter_arr_prev[newpriv] = current_icount;
> >> + } else {
> >> + delta = current_icount - counter_arr_prev[env->priv];
> >> + if (env->virt_enabled) {
> >> + /* HS->VS transition.The previous value should
> >> correspond to HS. */
> >
> > Hi Atish,
> >
> > Dose env->virt_enabled ensure HS->VS transition?
> >
> As per my understanding, riscv_cpu_set_virt_enabled always invoked
> before riscv_cpu_set_mode.
In helper_mret() we call riscv_cpu_set_mode() before
riscv_cpu_set_virt_enabled().
I don't think there is any requirement on which order we call the functions
>
> That means env->virt_enabled must be enabled before we enter into this
> function. Let me know if I missed
env->virt_enabled will be true if we are in HS or VS mode, but you
don't know the transition from just env->virt_enabled being set.
Alistair
>
> an scenario.
>
>
> > I think VS->VS also keeps env->virt_enabled to be true, where the
> > previous value should correspond to VS.
> >
> yeah. good point. We should check HSTATUS_SPV here as well.
>
>
> > Thanks,
> > Zhiwei
> >
> >> + env->pmu_fixed_ctrs[1].counter_prev[PRV_S] = current_icount;
> >> + } else if (get_field(env->hstatus, HSTATUS_SPV)) {
> >> + /* VS->HS transition.The previous value should
> >> correspond to VS. */
> >> + env->pmu_fixed_ctrs[1].counter_virt_prev[PRV_S] =
> >> current_icount;
> >> + }
> >> + }
> >> +
> >> + counter_arr[env->priv] += delta;
> >> +}
> >> +
> >> +void riscv_pmu_cycle_update_priv(CPURISCVState *env, target_ulong
> >> newpriv)
> >> +{
> >> + uint64_t delta;
> >> + uint64_t *counter_arr;
> >> + uint64_t *counter_arr_prev;
> >> + uint64_t current_host_ticks = cpu_get_host_ticks();
> >> +
> >> + if (env->virt_enabled) {
> >> + counter_arr = env->pmu_fixed_ctrs[0].counter_virt;
> >> + counter_arr_prev = env->pmu_fixed_ctrs[0].counter_virt_prev;
> >> + } else {
> >> + counter_arr = env->pmu_fixed_ctrs[0].counter;
> >> + counter_arr_prev = env->pmu_fixed_ctrs[0].counter_prev;
> >> + }
> >> +
> >> + if (newpriv != env->priv) {
> >> + delta = current_host_ticks - counter_arr_prev[env->priv];
> >> + counter_arr_prev[newpriv] = current_host_ticks;
> >> + } else {
> >> + delta = current_host_ticks - counter_arr_prev[env->priv];
> >> + if (env->virt_enabled) {
> >> + env->pmu_fixed_ctrs[0].counter_prev[PRV_S] =
> >> current_host_ticks;
> >> + } else if (get_field(env->hstatus, HSTATUS_SPV)) {
> >> + env->pmu_fixed_ctrs[0].counter_virt_prev[PRV_S] =
> >> + current_host_ticks;
> >> + }
> >> + }
> >> +
> >> + counter_arr[env->priv] += delta;
> >> +}
> >> +
> >> int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx
> >> event_idx)
> >> {
> >> uint32_t ctr_idx;
> >> diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
> >> index 505fc850d38e..50de6031a730 100644
> >> --- a/target/riscv/pmu.h
> >> +++ b/target/riscv/pmu.h
> >> @@ -31,3 +31,5 @@ int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum
> >> riscv_pmu_event_idx event_idx);
> >> void riscv_pmu_generate_fdt_node(void *fdt, uint32_t cmask, char
> >> *pmu_name);
> >> int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value,
> >> uint32_t ctr_idx);
> >> +void riscv_pmu_icount_update_priv(CPURISCVState *env, target_ulong
> >> newpriv);
> >> +void riscv_pmu_cycle_update_priv(CPURISCVState *env, target_ulong
> >> newpriv);
>