| Message ID | 20220107004846.378859-10-atishp@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Improve PMU support | expand |
On Fri, Jan 7, 2022 at 10:22 AM Atish Patra <atishp@rivosinc.com> wrote: > > All the hpmcounters and the fixed counters (CY, IR, TM) can be represented > as a unified counter. Thus, the predicate function doesn't need handle each > case separately. > > Simplify the predicate function so that we just handle things differently > between RV32/RV64 and S/HS mode. > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > --- > target/riscv/csr.c | 111 ++++----------------------------------------- > 1 file changed, 10 insertions(+), 101 deletions(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index d3a8bba6a518..feb053eb3f7b 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -109,6 +109,7 @@ static RISCVException ctr(CPURISCVState *env, int csrno) > CPUState *cs = env_cpu(env); > RISCVCPU *cpu = RISCV_CPU(cs); > int ctr_index; > + uint64_t ctr_mask; Use target_ulong should be enough? > int base_csrno = CSR_CYCLE; > bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false; > > @@ -117,122 +118,30 @@ static RISCVException ctr(CPURISCVState *env, int csrno) > base_csrno += 0x80; > } > ctr_index = csrno - base_csrno; > + ctr_mask = BIT(ctr_index); > > if ((csrno >= CSR_CYCLE && csrno <= CSR_INSTRET) || > (csrno >= CSR_CYCLEH && csrno <= CSR_INSTRETH)) { > goto skip_ext_pmu_check; > } > > - if ((!cpu->cfg.pmu_num || !(cpu->pmu_avail_ctrs & BIT(ctr_index)))) { > + if ((!cpu->cfg.pmu_num || !(cpu->pmu_avail_ctrs & ctr_mask))) { > /* No counter is enabled in PMU or the counter is out of range */ > return RISCV_EXCP_ILLEGAL_INST; > } > > skip_ext_pmu_check: > > - if (env->priv == PRV_S) { > - switch (csrno) { > - case CSR_CYCLE: > - if (!get_field(env->mcounteren, COUNTEREN_CY)) { > - return RISCV_EXCP_ILLEGAL_INST; > - } > - break; > - case CSR_TIME: > - if (!get_field(env->mcounteren, COUNTEREN_TM)) { > - return RISCV_EXCP_ILLEGAL_INST; > - } > - break; > - case CSR_INSTRET: > - if (!get_field(env->mcounteren, COUNTEREN_IR)) { > - return RISCV_EXCP_ILLEGAL_INST; > - } > - break; > - case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31: > - if (!get_field(env->mcounteren, 1 << ctr_index)) { > - return RISCV_EXCP_ILLEGAL_INST; > - } > - break; > - } > - if (rv32) { > - switch (csrno) { > - case CSR_CYCLEH: > - if (!get_field(env->mcounteren, COUNTEREN_CY)) { > - return RISCV_EXCP_ILLEGAL_INST; > - } > - break; > - case CSR_TIMEH: > - if (!get_field(env->mcounteren, COUNTEREN_TM)) { > - return RISCV_EXCP_ILLEGAL_INST; > - } > - break; > - case CSR_INSTRETH: > - if (!get_field(env->mcounteren, COUNTEREN_IR)) { > - return RISCV_EXCP_ILLEGAL_INST; > - } > - break; > - case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H: > - if (!get_field(env->mcounteren, 1 << ctr_index)) { > - return RISCV_EXCP_ILLEGAL_INST; > - } > - break; > - } > - } > + if ((env->priv == PRV_S) && (!get_field(env->mcounteren, ctr_mask))) { > + return RISCV_EXCP_ILLEGAL_INST; > } > > if (riscv_cpu_virt_enabled(env)) { > - switch (csrno) { > - case CSR_CYCLE: > - if (!get_field(env->hcounteren, COUNTEREN_CY) && > - get_field(env->mcounteren, COUNTEREN_CY)) { > - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > - } > - break; > - case CSR_TIME: > - if (!get_field(env->hcounteren, COUNTEREN_TM) && > - get_field(env->mcounteren, COUNTEREN_TM)) { > - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > - } > - break; > - case CSR_INSTRET: > - if (!get_field(env->hcounteren, COUNTEREN_IR) && > - get_field(env->mcounteren, COUNTEREN_IR)) { > - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > - } > - break; > - case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31: > - if (!get_field(env->hcounteren, 1 << ctr_index) && > - get_field(env->mcounteren, 1 << ctr_index)) { > - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > - } > - break; > - } > - if (rv32) { > - switch (csrno) { > - case CSR_CYCLEH: > - if (!get_field(env->hcounteren, COUNTEREN_CY) && > - get_field(env->mcounteren, COUNTEREN_CY)) { > - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > - } > - break; > - case CSR_TIMEH: > - if (!get_field(env->hcounteren, COUNTEREN_TM) && > - get_field(env->mcounteren, COUNTEREN_TM)) { > - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > - } > - break; > - case CSR_INSTRETH: > - if (!get_field(env->hcounteren, COUNTEREN_IR) && > - get_field(env->mcounteren, COUNTEREN_IR)) { > - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > - } > - break; > - case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H: > - if (!get_field(env->hcounteren, 1 << ctr_index) && > - get_field(env->mcounteren, 1 << ctr_index)) { > - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > - } > - break; > - } > + if (!get_field(env->mcounteren, ctr_mask)) { > + /* The bit must be set in mcountern for HS mode access */ > + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > + } else if (!get_field(env->hcounteren, ctr_mask)) { > + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > } > } Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
On Mon, Jan 10, 2022 at 12:27 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > On Fri, Jan 7, 2022 at 10:22 AM Atish Patra <atishp@rivosinc.com> wrote: > > > > All the hpmcounters and the fixed counters (CY, IR, TM) can be represented > > as a unified counter. Thus, the predicate function doesn't need handle each > > case separately. > > > > Simplify the predicate function so that we just handle things differently > > between RV32/RV64 and S/HS mode. > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > > --- > > target/riscv/csr.c | 111 ++++----------------------------------------- > > 1 file changed, 10 insertions(+), 101 deletions(-) > > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > > index d3a8bba6a518..feb053eb3f7b 100644 > > --- a/target/riscv/csr.c > > +++ b/target/riscv/csr.c > > @@ -109,6 +109,7 @@ static RISCVException ctr(CPURISCVState *env, int csrno) > > CPUState *cs = env_cpu(env); > > RISCVCPU *cpu = RISCV_CPU(cs); > > int ctr_index; > > + uint64_t ctr_mask; > > Use target_ulong should be enough? > Yeah. Will fix it in the next version. > > int base_csrno = CSR_CYCLE; > > bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false; > > > > @@ -117,122 +118,30 @@ static RISCVException ctr(CPURISCVState *env, int csrno) > > base_csrno += 0x80; > > } > > ctr_index = csrno - base_csrno; > > + ctr_mask = BIT(ctr_index); > > > > if ((csrno >= CSR_CYCLE && csrno <= CSR_INSTRET) || > > (csrno >= CSR_CYCLEH && csrno <= CSR_INSTRETH)) { > > goto skip_ext_pmu_check; > > } > > > > - if ((!cpu->cfg.pmu_num || !(cpu->pmu_avail_ctrs & BIT(ctr_index)))) { > > + if ((!cpu->cfg.pmu_num || !(cpu->pmu_avail_ctrs & ctr_mask))) { > > /* No counter is enabled in PMU or the counter is out of range */ > > return RISCV_EXCP_ILLEGAL_INST; > > } > > > > skip_ext_pmu_check: > > > > - if (env->priv == PRV_S) { > > - switch (csrno) { > > - case CSR_CYCLE: > > - if (!get_field(env->mcounteren, COUNTEREN_CY)) { > > - return RISCV_EXCP_ILLEGAL_INST; > > - } > > - break; > > - case CSR_TIME: > > - if (!get_field(env->mcounteren, COUNTEREN_TM)) { > > - return RISCV_EXCP_ILLEGAL_INST; > > - } > > - break; > > - case CSR_INSTRET: > > - if (!get_field(env->mcounteren, COUNTEREN_IR)) { > > - return RISCV_EXCP_ILLEGAL_INST; > > - } > > - break; > > - case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31: > > - if (!get_field(env->mcounteren, 1 << ctr_index)) { > > - return RISCV_EXCP_ILLEGAL_INST; > > - } > > - break; > > - } > > - if (rv32) { > > - switch (csrno) { > > - case CSR_CYCLEH: > > - if (!get_field(env->mcounteren, COUNTEREN_CY)) { > > - return RISCV_EXCP_ILLEGAL_INST; > > - } > > - break; > > - case CSR_TIMEH: > > - if (!get_field(env->mcounteren, COUNTEREN_TM)) { > > - return RISCV_EXCP_ILLEGAL_INST; > > - } > > - break; > > - case CSR_INSTRETH: > > - if (!get_field(env->mcounteren, COUNTEREN_IR)) { > > - return RISCV_EXCP_ILLEGAL_INST; > > - } > > - break; > > - case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H: > > - if (!get_field(env->mcounteren, 1 << ctr_index)) { > > - return RISCV_EXCP_ILLEGAL_INST; > > - } > > - break; > > - } > > - } > > + if ((env->priv == PRV_S) && (!get_field(env->mcounteren, ctr_mask))) { > > + return RISCV_EXCP_ILLEGAL_INST; > > } > > > > if (riscv_cpu_virt_enabled(env)) { > > - switch (csrno) { > > - case CSR_CYCLE: > > - if (!get_field(env->hcounteren, COUNTEREN_CY) && > > - get_field(env->mcounteren, COUNTEREN_CY)) { > > - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > > - } > > - break; > > - case CSR_TIME: > > - if (!get_field(env->hcounteren, COUNTEREN_TM) && > > - get_field(env->mcounteren, COUNTEREN_TM)) { > > - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > > - } > > - break; > > - case CSR_INSTRET: > > - if (!get_field(env->hcounteren, COUNTEREN_IR) && > > - get_field(env->mcounteren, COUNTEREN_IR)) { > > - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > > - } > > - break; > > - case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31: > > - if (!get_field(env->hcounteren, 1 << ctr_index) && > > - get_field(env->mcounteren, 1 << ctr_index)) { > > - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > > - } > > - break; > > - } > > - if (rv32) { > > - switch (csrno) { > > - case CSR_CYCLEH: > > - if (!get_field(env->hcounteren, COUNTEREN_CY) && > > - get_field(env->mcounteren, COUNTEREN_CY)) { > > - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > > - } > > - break; > > - case CSR_TIMEH: > > - if (!get_field(env->hcounteren, COUNTEREN_TM) && > > - get_field(env->mcounteren, COUNTEREN_TM)) { > > - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > > - } > > - break; > > - case CSR_INSTRETH: > > - if (!get_field(env->hcounteren, COUNTEREN_IR) && > > - get_field(env->mcounteren, COUNTEREN_IR)) { > > - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > > - } > > - break; > > - case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H: > > - if (!get_field(env->hcounteren, 1 << ctr_index) && > > - get_field(env->mcounteren, 1 << ctr_index)) { > > - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > > - } > > - break; > > - } > > + if (!get_field(env->mcounteren, ctr_mask)) { > > + /* The bit must be set in mcountern for HS mode access */ > > + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > > + } else if (!get_field(env->hcounteren, ctr_mask)) { > > + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > > } > > } > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
diff --git a/target/riscv/csr.c b/target/riscv/csr.c index d3a8bba6a518..feb053eb3f7b 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -109,6 +109,7 @@ static RISCVException ctr(CPURISCVState *env, int csrno) CPUState *cs = env_cpu(env); RISCVCPU *cpu = RISCV_CPU(cs); int ctr_index; + uint64_t ctr_mask; int base_csrno = CSR_CYCLE; bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false; @@ -117,122 +118,30 @@ static RISCVException ctr(CPURISCVState *env, int csrno) base_csrno += 0x80; } ctr_index = csrno - base_csrno; + ctr_mask = BIT(ctr_index); if ((csrno >= CSR_CYCLE && csrno <= CSR_INSTRET) || (csrno >= CSR_CYCLEH && csrno <= CSR_INSTRETH)) { goto skip_ext_pmu_check; } - if ((!cpu->cfg.pmu_num || !(cpu->pmu_avail_ctrs & BIT(ctr_index)))) { + if ((!cpu->cfg.pmu_num || !(cpu->pmu_avail_ctrs & ctr_mask))) { /* No counter is enabled in PMU or the counter is out of range */ return RISCV_EXCP_ILLEGAL_INST; } skip_ext_pmu_check: - if (env->priv == PRV_S) { - switch (csrno) { - case CSR_CYCLE: - if (!get_field(env->mcounteren, COUNTEREN_CY)) { - return RISCV_EXCP_ILLEGAL_INST; - } - break; - case CSR_TIME: - if (!get_field(env->mcounteren, COUNTEREN_TM)) { - return RISCV_EXCP_ILLEGAL_INST; - } - break; - case CSR_INSTRET: - if (!get_field(env->mcounteren, COUNTEREN_IR)) { - return RISCV_EXCP_ILLEGAL_INST; - } - break; - case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31: - if (!get_field(env->mcounteren, 1 << ctr_index)) { - return RISCV_EXCP_ILLEGAL_INST; - } - break; - } - if (rv32) { - switch (csrno) { - case CSR_CYCLEH: - if (!get_field(env->mcounteren, COUNTEREN_CY)) { - return RISCV_EXCP_ILLEGAL_INST; - } - break; - case CSR_TIMEH: - if (!get_field(env->mcounteren, COUNTEREN_TM)) { - return RISCV_EXCP_ILLEGAL_INST; - } - break; - case CSR_INSTRETH: - if (!get_field(env->mcounteren, COUNTEREN_IR)) { - return RISCV_EXCP_ILLEGAL_INST; - } - break; - case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H: - if (!get_field(env->mcounteren, 1 << ctr_index)) { - return RISCV_EXCP_ILLEGAL_INST; - } - break; - } - } + if ((env->priv == PRV_S) && (!get_field(env->mcounteren, ctr_mask))) { + return RISCV_EXCP_ILLEGAL_INST; } if (riscv_cpu_virt_enabled(env)) { - switch (csrno) { - case CSR_CYCLE: - if (!get_field(env->hcounteren, COUNTEREN_CY) && - get_field(env->mcounteren, COUNTEREN_CY)) { - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; - } - break; - case CSR_TIME: - if (!get_field(env->hcounteren, COUNTEREN_TM) && - get_field(env->mcounteren, COUNTEREN_TM)) { - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; - } - break; - case CSR_INSTRET: - if (!get_field(env->hcounteren, COUNTEREN_IR) && - get_field(env->mcounteren, COUNTEREN_IR)) { - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; - } - break; - case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31: - if (!get_field(env->hcounteren, 1 << ctr_index) && - get_field(env->mcounteren, 1 << ctr_index)) { - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; - } - break; - } - if (rv32) { - switch (csrno) { - case CSR_CYCLEH: - if (!get_field(env->hcounteren, COUNTEREN_CY) && - get_field(env->mcounteren, COUNTEREN_CY)) { - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; - } - break; - case CSR_TIMEH: - if (!get_field(env->hcounteren, COUNTEREN_TM) && - get_field(env->mcounteren, COUNTEREN_TM)) { - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; - } - break; - case CSR_INSTRETH: - if (!get_field(env->hcounteren, COUNTEREN_IR) && - get_field(env->mcounteren, COUNTEREN_IR)) { - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; - } - break; - case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H: - if (!get_field(env->hcounteren, 1 << ctr_index) && - get_field(env->mcounteren, 1 << ctr_index)) { - return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; - } - break; - } + if (!get_field(env->mcounteren, ctr_mask)) { + /* The bit must be set in mcountern for HS mode access */ + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; + } else if (!get_field(env->hcounteren, ctr_mask)) { + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; } } #endif
All the hpmcounters and the fixed counters (CY, IR, TM) can be represented as a unified counter. Thus, the predicate function doesn't need handle each case separately. Simplify the predicate function so that we just handle things differently between RV32/RV64 and S/HS mode. Signed-off-by: Atish Patra <atishp@rivosinc.com> --- target/riscv/csr.c | 111 ++++----------------------------------------- 1 file changed, 10 insertions(+), 101 deletions(-)