diff mbox series

[v10,09/12] target/riscv: Simplify counter predicate function

Message ID 20220620231603.2547260-10-atishp@rivosinc.com
State New
Headers show
Series Improve PMU support | expand

Commit Message

Atish Kumar Patra June 20, 2022, 11:15 p.m. UTC 
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(-)

Comments

Weiwei Li July 4, 2022, 3:19 p.m. UTC | #1 
在 2022/6/21 上午7:15, 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 2664ce265784..9367e2af9b90 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->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))) ||
> +       ((env->priv == PRV_U) && (!get_field(env->scounteren, ctr_mask)))) {
> +        return RISCV_EXCP_ILLEGAL_INST;
>       }

Sorry. I didn't realize this simplification and sent a similar patch to 
fix the problems in Xcounteren

related check I found when I tried to learn the patchset for state 
enable extension two days ago.

I think there are several difference between our understanding, 
following is my modifications:

+    if (csrno <= CSR_HPMCOUNTER31 && csrno >= CSR_CYCLE) {
+        field = 1 << (csrno - CSR_CYCLE);
+    } else if (riscv_cpu_mxl(env) == MXL_RV32 && csrno <= CSR_HPMCOUNTER31H &&
+               csrno >= CSR_CYCLEH) {
+        field = 1 << (csrno - CSR_CYCLEH);
+    }
+
+    if (env->priv < PRV_M && !get_field(env->mcounteren, field)) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    if (riscv_cpu_virt_enabled(env) && !get_field(env->hcounteren, field)) {
+        return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+    }
+
+    if (riscv_has_ext(env, RVS) && env->priv == PRV_U &&
+        !get_field(env->scounteren, field)) {
+        if (riscv_cpu_virt_enabled(env)) {
+            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+        } else {
+            return RISCV_EXCP_ILLEGAL_INST;
          }
      }


1) For any less-privileged mode under M, illegal exception is raised if matching
bit in mcounteren is zero.

2) For VS/VU mode('H' extension is supported implicitly), virtual instruction
exception is raised if matching bit in hcounteren is zero.

3) scounteren csr only works in U/VU mode when 'S' extension is supported:
    For U mode, illegal exception is raised if matching bit in scounteren is zero.
    For VU mode, virtual instruction exception exception is raised if matching bit
in scounteren is zero.

Regards,
Weiwei Li

>   
>       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
Atish Kumar Patra July 5, 2022, 8 a.m. UTC | #2 
On Mon, Jul 4, 2022 at 8:19 AM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
>
> 在 2022/6/21 上午7:15, 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 2664ce265784..9367e2af9b90 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->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))) ||
> +       ((env->priv == PRV_U) && (!get_field(env->scounteren, ctr_mask)))) {
> +        return RISCV_EXCP_ILLEGAL_INST;
>      }
>
> Sorry. I didn't realize this simplification and sent a similar patch to fix the problems in Xcounteren
>
> related check I found when I tried to learn the patchset for state enable extension two days ago.
>
> I think there are several difference between our understanding, following is my modifications:
>
> +    if (csrno <= CSR_HPMCOUNTER31 && csrno >= CSR_CYCLE) {
> +        field = 1 << (csrno - CSR_CYCLE);
> +    } else if (riscv_cpu_mxl(env) == MXL_RV32 && csrno <= CSR_HPMCOUNTER31H &&
> +               csrno >= CSR_CYCLEH) {
> +        field = 1 << (csrno - CSR_CYCLEH);
> +    }
> +
> +    if (env->priv < PRV_M && !get_field(env->mcounteren, field)) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    if (riscv_cpu_virt_enabled(env) && !get_field(env->hcounteren, field)) {
> +        return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> +    }
> +
> +    if (riscv_has_ext(env, RVS) && env->priv == PRV_U &&
> +        !get_field(env->scounteren, field)) {
> +        if (riscv_cpu_virt_enabled(env)) {
> +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> +        } else {
> +            return RISCV_EXCP_ILLEGAL_INST;
>          }
>      }
>
>
> 1) For any less-privileged mode under M, illegal exception is raised if matching
> bit in mcounteren is zero.
>

As per the priv spec, in the section 3.1.11
"When one of these bits is set, access to the corresponding register
is permitted in the next implemented privilege mode (S-mode if
implemented, otherwise U-mode)."

mcounteren controls the access for U-mode only if the next implemented
mode is U (riscv_has_ext(env, RVS) must be false).
I did not add the additional check as the ctr is defined only for
!CONFIG_USER_ONLY.

> 2) For VS/VU mode('H' extension is supported implicitly), virtual instruction
> exception is raised if matching bit in hcounteren is zero.
>
> 3) scounteren csr only works in U/VU mode when 'S' extension is supported:

Yes. But we don't need additional check for 'S' extension as it will
be done by the predicate function "smode"

>    For U mode, illegal exception is raised if matching bit in scounteren is zero.
>    For VU mode, virtual instruction exception exception is raised if matching bit
> in scounteren is zero.
>
> Regards,
> Weiwei Li
>
>
>      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
Weiwei Li July 5, 2022, 8:41 a.m. UTC | #3 
在 2022/7/5 下午4:00, Atish Kumar Patra 写道:
> On Mon, Jul 4, 2022 at 8:19 AM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>>
>> 在 2022/6/21 上午7:15, 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 2664ce265784..9367e2af9b90 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->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))) ||
>> +       ((env->priv == PRV_U) && (!get_field(env->scounteren, ctr_mask)))) {
>> +        return RISCV_EXCP_ILLEGAL_INST;
>>       }
>>
>> Sorry. I didn't realize this simplification and sent a similar patch to fix the problems in Xcounteren
>>
>> related check I found when I tried to learn the patchset for state enable extension two days ago.
>>
>> I think there are several difference between our understanding, following is my modifications:
>>
>> +    if (csrno <= CSR_HPMCOUNTER31 && csrno >= CSR_CYCLE) {
>> +        field = 1 << (csrno - CSR_CYCLE);
>> +    } else if (riscv_cpu_mxl(env) == MXL_RV32 && csrno <= CSR_HPMCOUNTER31H &&
>> +               csrno >= CSR_CYCLEH) {
>> +        field = 1 << (csrno - CSR_CYCLEH);
>> +    }
>> +
>> +    if (env->priv < PRV_M && !get_field(env->mcounteren, field)) {
>> +        return RISCV_EXCP_ILLEGAL_INST;
>> +    }
>> +
>> +    if (riscv_cpu_virt_enabled(env) && !get_field(env->hcounteren, field)) {
>> +        return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>> +    }
>> +
>> +    if (riscv_has_ext(env, RVS) && env->priv == PRV_U &&
>> +        !get_field(env->scounteren, field)) {
>> +        if (riscv_cpu_virt_enabled(env)) {
>> +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>> +        } else {
>> +            return RISCV_EXCP_ILLEGAL_INST;
>>           }
>>       }
>>
>>
>> 1) For any less-privileged mode under M, illegal exception is raised if matching
>> bit in mcounteren is zero.
>>
> As per the priv spec, in the section 3.1.11
> "When one of these bits is set, access to the corresponding register
> is permitted in the next implemented privilege mode (S-mode if
> implemented, otherwise U-mode)."
>
> mcounteren controls the access for U-mode only if the next implemented
> mode is U (riscv_has_ext(env, RVS) must be false).
> I did not add the additional check as the ctr is defined only for
> !CONFIG_USER_ONLY.

In section 3.1.11, It also have description like this:

"In systems with U-mode, the mcounteren must be implemented, but all 
fields are WARL and may
be read-only zero, indicating reads to the corresponding counter will 
cause an illegal instruction
exception when executing in a less-privileged mode."

And !CONFIG_USER_ONLY is defined for QEMU system emulation,  it doesn't 
means current priv

cannot be  PRV_U mode.

>
>> 2) For VS/VU mode('H' extension is supported implicitly), virtual instruction
>> exception is raised if matching bit in hcounteren is zero.
>>
>> 3) scounteren csr only works in U/VU mode when 'S' extension is supported:
> Yes. But we don't need additional check for 'S' extension as it will
> be done by the predicate function "smode"

This is the question, smode can only guard the read/write of scounteren. 
If 'S' extension is not implemented,

scounteren will be zero. and If check is done as following:

+    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;
      }

any access from PRV_U will trigger illegal instruction exception. From above spec, this kind of
access will be controlled by mcounteren, and will be legal if matching bit in mcounteren is 1.

Regards,
Weiwei Li

>
>>     For U mode, illegal exception is raised if matching bit in scounteren is zero.
>>     For VU mode, virtual instruction exception exception is raised if matching bit
>> in scounteren is zero.
>>
>> Regards,
>> Weiwei Li
>>
>>
>>       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
Heiko Stübner July 14, 2022, 9:54 a.m. UTC | #4 
Am Dienstag, 21. Juni 2022, 01:15:59 CEST schrieb 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>

Tested-by: Heiko Stuebner <heiko@sntech.de>
diff mbox series

Patch

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 2664ce265784..9367e2af9b90 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->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))) ||
+       ((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