| Message ID | 20220523235057.123882-10-atishp@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Improve PMU support | expand |
On Tue, May 24, 2022 at 8:02 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. > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > Acked-by: Alistair Francis <alistair.francis@wdc.com> > 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 723b52d836d3..e229f53c674d 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -74,6 +74,7 @@ static RISCVException ctr(CPURISCVState *env, int csrno) > CPUState *cs = env_cpu(env); > RISCVCPU *cpu = RISCV_CPU(cs); > int ctr_index; > + target_ulong ctr_mask; > int base_csrno = CSR_CYCLE; > bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false; > > @@ -82,122 +83,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))) { > Should we also check PRV_U against env->scounteren here? Regards, Frank Chang > + 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 > -- > 2.25.1 > > >
On Wed, May 25, 2022 at 3:24 AM Frank Chang <frank.chang@sifive.com> wrote: > > On Tue, May 24, 2022 at 8:02 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. >> >> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> >> Acked-by: Alistair Francis <alistair.francis@wdc.com> >> 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 723b52d836d3..e229f53c674d 100644 >> --- a/target/riscv/csr.c >> +++ b/target/riscv/csr.c >> @@ -74,6 +74,7 @@ static RISCVException ctr(CPURISCVState *env, int csrno) >> CPUState *cs = env_cpu(env); >> RISCVCPU *cpu = RISCV_CPU(cs); >> int ctr_index; >> + target_ulong ctr_mask; >> int base_csrno = CSR_CYCLE; >> bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false; >> >> @@ -82,122 +83,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))) { > > > Should we also check PRV_U against env->scounteren here? > Ahh yes. That's required while hpmcounter is being accessed from the userspace. Good catch. Thanks. Will fix it. > Regards, > Frank Chang > >> >> + 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 >> -- >> 2.25.1 >> >>
diff --git a/target/riscv/csr.c b/target/riscv/csr.c index 723b52d836d3..e229f53c674d 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -74,6 +74,7 @@ static RISCVException ctr(CPURISCVState *env, int csrno) CPUState *cs = env_cpu(env); RISCVCPU *cpu = RISCV_CPU(cs); int ctr_index; + target_ulong ctr_mask; int base_csrno = CSR_CYCLE; bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false; @@ -82,122 +83,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