On Wed, Jul 27, 2022 at 5:56 PM Weiwei Li <liwei...@iscas.ac.cn> wrote:
>
> 在 2022/7/28 上午5:40, Atish Kumar Patra 写道:
>
>
>
> On Wed, Jul 27, 2022 at 1:35 AM Weiwei Li <liwei...@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...@gmail.com>
>> > Acked-by: Alistair Francis <alistair.fran...@wdc.com>
>> > Signed-off-by: Atish Patra <ati...@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
>>
>>
