Re: [ PATCH v3 02/10] target/riscv: Implement PMU CSR predicate function for

[Bin Meng]

On Tue, Oct 26, 2021 at 3:56 AM Atish Patra <atish.patra@wdc.com> wrote:
>

The commit title is incomplete


> Currently, the predicate function for PMU related CSRs only works if
> virtualization is enabled. It also does not check mcounteren bits before
> before cycle/minstret/hpmcounterx access.
>
> Support supervisor mode access in the predicate function as well.
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> ---
>  target/riscv/csr.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 1ec776013435..de484c74d3b4 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -62,12 +62,64 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
>  #if !defined(CONFIG_USER_ONLY)
>      CPUState *cs = env_cpu(env);
>      RISCVCPU *cpu = RISCV_CPU(cs);
> +    int ctr_index;
>
>      if (!cpu->cfg.ext_counters) {
>          /* The Counters extensions is not enabled */
>          return RISCV_EXCP_ILLEGAL_INST;
>      }
>
> +    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:
> +            ctr_index = csrno - CSR_CYCLE;
> +            if (!get_field(env->mcounteren, 1 << ctr_index)) {
> +                return RISCV_EXCP_ILLEGAL_INST;
> +            }
> +            break;

The above switch..case can be simplified with the logic of last case
as the same one applies to all cases.

> +        }
> +        if (riscv_cpu_is_32bit(env)) {
> +            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:
> +                ctr_index = csrno - CSR_CYCLEH;
> +                if (!get_field(env->mcounteren, 1 << ctr_index)) {
> +                    return RISCV_EXCP_ILLEGAL_INST;
> +                }

ditto

> +                break;
> +            }
> +        }
> +    }
> +
>      if (riscv_cpu_virt_enabled(env)) {
>          switch (csrno) {
>          case CSR_CYCLE:
>

Regards,
Bin