Message ID | 20220727064913.1041427-3-atishp@rivosinc.com |
---|---|
State | New |
Headers | show |
Series | Improve PMU support | expand |
在 2022/7/27 下午2:49, Atish Patra 写道: > 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 | 112 +++++---------------------------------------- > 1 file changed, 11 insertions(+), 101 deletions(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index 1233bfa0a726..57dbbf9b09a0 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,31 @@ 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->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))) || > + ((env->priv == PRV_U) && (!get_field(env->scounteren, 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; > } The logic is changed here. In original logic, RISCV_EXCP_VIRT_INSTRUCTION_FAULT is triggered when !get_field(env->hcounteren, 1 << ctr_index) && get_field(env->mcounteren, 1 << ctr_index) The new logic is RISCV_EXCP_VIRT_INSTRUCTION_FAULT is triggered when !get_field(env->mcounteren, ctr_mask) or !get_field(env->hcounteren, 1 << ctr_index) && get_field(env->mcounteren, 1 << ctr_index) Regards, Weiwei Li > } > #endif
On Wed, Jul 27, 2022 at 1:35 AM Weiwei Li <liweiwei@iscas.ac.cn> wrote: > > 在 2022/7/27 下午2:49, Atish Patra 写道: > > 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 | 112 +++++---------------------------------------- > > 1 file changed, 11 insertions(+), 101 deletions(-) > > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > > index 1233bfa0a726..57dbbf9b09a0 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,31 @@ 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->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))) || > > + ((env->priv == PRV_U) && (!get_field(env->scounteren, > 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; > > } > > The logic is changed here. In original logic, > RISCV_EXCP_VIRT_INSTRUCTION_FAULT is triggered when > > !get_field(env->hcounteren, 1 << ctr_index) && get_field(env->mcounteren, > 1 << ctr_index) > > The new logic is RISCV_EXCP_VIRT_INSTRUCTION_FAULT is triggered when > !get_field(env->mcounteren, ctr_mask) > > or !get_field(env->hcounteren, 1 << ctr_index) && > get_field(env->mcounteren, 1 << ctr_index) > > Yes. It's just an optimization where we can break early just by checking mcountern. Do you see any issue with it ? > Regards, > > Weiwei Li > > > } > > #endif > >
在 2022/7/28 上午5:40, Atish Kumar Patra 写道: > > > On Wed, Jul 27, 2022 at 1:35 AM Weiwei Li <liweiwei@iscas.ac.cn > <mailto:liweiwei@iscas.ac.cn>> wrote: > > > 在 2022/7/27 下午2:49, Atish Patra 写道: > > 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 > <mailto:bmeng.cn@gmail.com>> > > Acked-by: Alistair Francis <alistair.francis@wdc.com > <mailto:alistair.francis@wdc.com>> > > Signed-off-by: Atish Patra <atishp@rivosinc.com > <mailto:atishp@rivosinc.com>> > > --- > > target/riscv/csr.c | 112 > +++++---------------------------------------- > > 1 file changed, 11 insertions(+), 101 deletions(-) > > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > > index 1233bfa0a726..57dbbf9b09a0 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,31 @@ 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->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))) || > > + ((env->priv == PRV_U) && (!get_field(env->scounteren, > 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; > > } > > The logic is changed here. In original logic, > RISCV_EXCP_VIRT_INSTRUCTION_FAULT is triggered when > > !get_field(env->hcounteren, 1 << ctr_index) && > get_field(env->mcounteren, 1 << ctr_index) > > The new logic is RISCV_EXCP_VIRT_INSTRUCTION_FAULT is triggered > when !get_field(env->mcounteren, ctr_mask) > > or !get_field(env->hcounteren, 1 << ctr_index) && > get_field(env->mcounteren, 1 << ctr_index) > > > Yes. It's just an optimization where we can break early just by > checking mcountern. Do you see any issue with it ? The section 8.6.1 of riscv- privileged spec lists the cases (including the Xcounten ralated cases) which will raise a virtual instruction exception. However all the the Xcounten ralated cases have a common condition "the same bit in mcounteren is 1". So this optimization seems not correct. Regards, Weiwei Li > Regards, > > Weiwei Li > > > } > > #endif >
On Wed, Jul 27, 2022 at 5:56 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote: > > 在 2022/7/28 上午5:40, Atish Kumar Patra 写道: > > > > On Wed, Jul 27, 2022 at 1:35 AM Weiwei Li <liweiwei@iscas.ac.cn> wrote: > >> >> 在 2022/7/27 下午2:49, Atish Patra 写道: >> > 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 | 112 +++++---------------------------------------- >> > 1 file changed, 11 insertions(+), 101 deletions(-) >> > >> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c >> > index 1233bfa0a726..57dbbf9b09a0 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,31 @@ 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->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))) || >> > + ((env->priv == PRV_U) && (!get_field(env->scounteren, >> 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; >> > } >> >> The logic is changed here. In original logic, >> RISCV_EXCP_VIRT_INSTRUCTION_FAULT is triggered when >> >> !get_field(env->hcounteren, 1 << ctr_index) && get_field(env->mcounteren, >> 1 << ctr_index) >> >> The new logic is RISCV_EXCP_VIRT_INSTRUCTION_FAULT is triggered when >> !get_field(env->mcounteren, ctr_mask) >> >> or !get_field(env->hcounteren, 1 << ctr_index) && >> get_field(env->mcounteren, 1 << ctr_index) >> >> > Yes. It's just an optimization where we can break early just by checking > mcountern. Do you see any issue with it ? > > The section 8.6.1 of riscv- privileged spec lists the cases (including > the Xcounten ralated cases) which will raise a > > virtual instruction exception. However all the the Xcounten ralated cases > have a common condition > > "the same bit in mcounteren is 1". > Ahh yes. Got it. I will revert it to the original logic in the next version. > So this optimization seems not correct. > > Regards, > > Weiwei Li > > > >> Regards, >> >> Weiwei Li >> >> > } >> > #endif >> >>
diff --git a/target/riscv/csr.c b/target/riscv/csr.c index 1233bfa0a726..57dbbf9b09a0 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,31 @@ 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->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))) || + ((env->priv == PRV_U) && (!get_field(env->scounteren, 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