diff mbox series

[v10,04/12] target/riscv: pmu: Make number of counters configurable

Message ID 20220620231603.2547260-5-atishp@rivosinc.com (mailing list archive)
State New, archived
Headers show
Series Improve PMU support | expand

Commit Message

Atish Kumar Patra June 20, 2022, 11:15 p.m. UTC 
The RISC-V privilege specification provides flexibility to implement
any number of counters from 29 programmable counters. However, the QEMU
implements all the counters.

Make it configurable through pmu config parameter which now will indicate
how many programmable counters should be implemented by the cpu.

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Atish Patra <atish.patra@wdc.com>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 target/riscv/cpu.c |  3 +-
 target/riscv/cpu.h |  2 +-
 target/riscv/csr.c | 94 ++++++++++++++++++++++++++++++----------------
 3 files changed, 63 insertions(+), 36 deletions(-)

Comments

Weiwei Li July 4, 2022, 3:26 p.m. UTC | #1 
在 2022/6/21 上午7:15, Atish Patra 写道:
> The RISC-V privilege specification provides flexibility to implement
> any number of counters from 29 programmable counters. However, the QEMU
> implements all the counters.
>
> Make it configurable through pmu config parameter which now will indicate
> how many programmable counters should be implemented by the cpu.
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>   target/riscv/cpu.c |  3 +-
>   target/riscv/cpu.h |  2 +-
>   target/riscv/csr.c | 94 ++++++++++++++++++++++++++++++----------------
>   3 files changed, 63 insertions(+), 36 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 1b57b3c43980..d12c6dc630ca 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -851,7 +851,6 @@ static void riscv_cpu_init(Object *obj)
>   {
>       RISCVCPU *cpu = RISCV_CPU(obj);
>   
> -    cpu->cfg.ext_pmu = true;
>       cpu->cfg.ext_ifencei = true;
>       cpu->cfg.ext_icsr = true;
>       cpu->cfg.mmu = true;
> @@ -879,7 +878,7 @@ static Property riscv_cpu_extensions[] = {
>       DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
>       DEFINE_PROP_BOOL("v", RISCVCPU, cfg.ext_v, false),
>       DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, true),
> -    DEFINE_PROP_BOOL("pmu", RISCVCPU, cfg.ext_pmu, true),
> +    DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),

I think It's better to add  a check on cfg.pmu_num to  <= 29.

>       DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
>       DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
>       DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false),
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 252c30a55d78..ffee54ea5c27 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -397,7 +397,6 @@ struct RISCVCPUConfig {
>       bool ext_zksed;
>       bool ext_zksh;
>       bool ext_zkt;
> -    bool ext_pmu;
>       bool ext_ifencei;
>       bool ext_icsr;
>       bool ext_svinval;
> @@ -421,6 +420,7 @@ struct RISCVCPUConfig {
>       /* Vendor-specific custom extensions */
>       bool ext_XVentanaCondOps;
>   
> +    uint8_t pmu_num;
>       char *priv_spec;
>       char *user_spec;
>       char *bext_spec;
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 0ca05c77883c..b4a8e15f498f 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -73,9 +73,17 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
>       CPUState *cs = env_cpu(env);
>       RISCVCPU *cpu = RISCV_CPU(cs);
>       int ctr_index;
> +    int base_csrno = CSR_HPMCOUNTER3;
> +    bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false;
>   
> -    if (!cpu->cfg.ext_pmu) {
> -        /* The PMU extension is not enabled */
> +    if (rv32 && csrno >= CSR_CYCLEH) {
> +        /* Offset for RV32 hpmcounternh counters */
> +        base_csrno += 0x80;
> +    }
> +    ctr_index = csrno - base_csrno;
> +
> +    if (!cpu->cfg.pmu_num || ctr_index >= (cpu->cfg.pmu_num)) {
> +        /* No counter is enabled in PMU or the counter is out of range */

I seems unnecessary to add '!cpu->cfg.pmu_num ' here,   'ctr_index >= 
(cpu->cfg.pmu_num)' is true

when cpu->cfg.pmu_num is zero if the problem for base_csrno is fixed.

Ragards,

Weiwei Li

>           return RISCV_EXCP_ILLEGAL_INST;
>       }
>   
> @@ -103,7 +111,7 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
>               }
>               break;
>           }
> -        if (riscv_cpu_mxl(env) == MXL_RV32) {
> +        if (rv32) {
>               switch (csrno) {
>               case CSR_CYCLEH:
>                   if (!get_field(env->mcounteren, COUNTEREN_CY)) {
> @@ -158,7 +166,7 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
>               }
>               break;
>           }
> -        if (riscv_cpu_mxl(env) == MXL_RV32) {
> +        if (rv32) {
>               switch (csrno) {
>               case CSR_CYCLEH:
>                   if (!get_field(env->hcounteren, COUNTEREN_CY) &&
> @@ -202,6 +210,26 @@ static RISCVException ctr32(CPURISCVState *env, int csrno)
>   }
>   
>   #if !defined(CONFIG_USER_ONLY)
> +static RISCVException mctr(CPURISCVState *env, int csrno)
> +{
> +    CPUState *cs = env_cpu(env);
> +    RISCVCPU *cpu = RISCV_CPU(cs);
> +    int ctr_index;
> +    int base_csrno = CSR_MHPMCOUNTER3;
> +
> +    if ((riscv_cpu_mxl(env) == MXL_RV32) && csrno >= CSR_MCYCLEH) {
> +        /* Offset for RV32 mhpmcounternh counters */
> +        base_csrno += 0x80;
> +    }
> +    ctr_index = csrno - base_csrno;
> +    if (!cpu->cfg.pmu_num || ctr_index >= cpu->cfg.pmu_num) {
> +        /* The PMU is not enabled or counter is out of range*/
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
>   static RISCVException any(CPURISCVState *env, int csrno)
>   {
>       return RISCV_EXCP_NONE;
> @@ -3687,35 +3715,35 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>       [CSR_HPMCOUNTER30]   = { "hpmcounter30",   ctr,    read_zero },
>       [CSR_HPMCOUNTER31]   = { "hpmcounter31",   ctr,    read_zero },
>   
> -    [CSR_MHPMCOUNTER3]   = { "mhpmcounter3",   any,    read_zero },
> -    [CSR_MHPMCOUNTER4]   = { "mhpmcounter4",   any,    read_zero },
> -    [CSR_MHPMCOUNTER5]   = { "mhpmcounter5",   any,    read_zero },
> -    [CSR_MHPMCOUNTER6]   = { "mhpmcounter6",   any,    read_zero },
> -    [CSR_MHPMCOUNTER7]   = { "mhpmcounter7",   any,    read_zero },
> -    [CSR_MHPMCOUNTER8]   = { "mhpmcounter8",   any,    read_zero },
> -    [CSR_MHPMCOUNTER9]   = { "mhpmcounter9",   any,    read_zero },
> -    [CSR_MHPMCOUNTER10]  = { "mhpmcounter10",  any,    read_zero },
> -    [CSR_MHPMCOUNTER11]  = { "mhpmcounter11",  any,    read_zero },
> -    [CSR_MHPMCOUNTER12]  = { "mhpmcounter12",  any,    read_zero },
> -    [CSR_MHPMCOUNTER13]  = { "mhpmcounter13",  any,    read_zero },
> -    [CSR_MHPMCOUNTER14]  = { "mhpmcounter14",  any,    read_zero },
> -    [CSR_MHPMCOUNTER15]  = { "mhpmcounter15",  any,    read_zero },
> -    [CSR_MHPMCOUNTER16]  = { "mhpmcounter16",  any,    read_zero },
> -    [CSR_MHPMCOUNTER17]  = { "mhpmcounter17",  any,    read_zero },
> -    [CSR_MHPMCOUNTER18]  = { "mhpmcounter18",  any,    read_zero },
> -    [CSR_MHPMCOUNTER19]  = { "mhpmcounter19",  any,    read_zero },
> -    [CSR_MHPMCOUNTER20]  = { "mhpmcounter20",  any,    read_zero },
> -    [CSR_MHPMCOUNTER21]  = { "mhpmcounter21",  any,    read_zero },
> -    [CSR_MHPMCOUNTER22]  = { "mhpmcounter22",  any,    read_zero },
> -    [CSR_MHPMCOUNTER23]  = { "mhpmcounter23",  any,    read_zero },
> -    [CSR_MHPMCOUNTER24]  = { "mhpmcounter24",  any,    read_zero },
> -    [CSR_MHPMCOUNTER25]  = { "mhpmcounter25",  any,    read_zero },
> -    [CSR_MHPMCOUNTER26]  = { "mhpmcounter26",  any,    read_zero },
> -    [CSR_MHPMCOUNTER27]  = { "mhpmcounter27",  any,    read_zero },
> -    [CSR_MHPMCOUNTER28]  = { "mhpmcounter28",  any,    read_zero },
> -    [CSR_MHPMCOUNTER29]  = { "mhpmcounter29",  any,    read_zero },
> -    [CSR_MHPMCOUNTER30]  = { "mhpmcounter30",  any,    read_zero },
> -    [CSR_MHPMCOUNTER31]  = { "mhpmcounter31",  any,    read_zero },
> +    [CSR_MHPMCOUNTER3]   = { "mhpmcounter3",   mctr,   read_zero },
> +    [CSR_MHPMCOUNTER4]   = { "mhpmcounter4",   mctr,   read_zero },
> +    [CSR_MHPMCOUNTER5]   = { "mhpmcounter5",   mctr,   read_zero },
> +    [CSR_MHPMCOUNTER6]   = { "mhpmcounter6",   mctr,   read_zero },
> +    [CSR_MHPMCOUNTER7]   = { "mhpmcounter7",   mctr,   read_zero },
> +    [CSR_MHPMCOUNTER8]   = { "mhpmcounter8",   mctr,   read_zero },
> +    [CSR_MHPMCOUNTER9]   = { "mhpmcounter9",   mctr,   read_zero },
> +    [CSR_MHPMCOUNTER10]  = { "mhpmcounter10",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER11]  = { "mhpmcounter11",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER12]  = { "mhpmcounter12",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER13]  = { "mhpmcounter13",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER14]  = { "mhpmcounter14",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER15]  = { "mhpmcounter15",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER16]  = { "mhpmcounter16",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER17]  = { "mhpmcounter17",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER18]  = { "mhpmcounter18",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER19]  = { "mhpmcounter19",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER20]  = { "mhpmcounter20",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER21]  = { "mhpmcounter21",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER22]  = { "mhpmcounter22",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER23]  = { "mhpmcounter23",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER24]  = { "mhpmcounter24",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER25]  = { "mhpmcounter25",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER26]  = { "mhpmcounter26",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER27]  = { "mhpmcounter27",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER28]  = { "mhpmcounter28",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER29]  = { "mhpmcounter29",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER30]  = { "mhpmcounter30",  mctr,   read_zero },
> +    [CSR_MHPMCOUNTER31]  = { "mhpmcounter31",  mctr,   read_zero },
>   
>       [CSR_MHPMEVENT3]     = { "mhpmevent3",     any,    read_zero },
>       [CSR_MHPMEVENT4]     = { "mhpmevent4",     any,    read_zero },
Weiwei Li July 5, 2022, 12:38 a.m. UTC | #2 
在 2022/7/4 下午11:26, Weiwei Li 写道:
>
> 在 2022/6/21 上午7:15, Atish Patra 写道:
>> The RISC-V privilege specification provides flexibility to implement
>> any number of counters from 29 programmable counters. However, the QEMU
>> implements all the counters.
>>
>> Make it configurable through pmu config parameter which now will 
>> indicate
>> how many programmable counters should be implemented by the cpu.
>>
>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>> ---
>>   target/riscv/cpu.c |  3 +-
>>   target/riscv/cpu.h |  2 +-
>>   target/riscv/csr.c | 94 ++++++++++++++++++++++++++++++----------------
>>   3 files changed, 63 insertions(+), 36 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 1b57b3c43980..d12c6dc630ca 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -851,7 +851,6 @@ static void riscv_cpu_init(Object *obj)
>>   {
>>       RISCVCPU *cpu = RISCV_CPU(obj);
>>   -    cpu->cfg.ext_pmu = true;
>>       cpu->cfg.ext_ifencei = true;
>>       cpu->cfg.ext_icsr = true;
>>       cpu->cfg.mmu = true;
>> @@ -879,7 +878,7 @@ static Property riscv_cpu_extensions[] = {
>>       DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
>>       DEFINE_PROP_BOOL("v", RISCVCPU, cfg.ext_v, false),
>>       DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, true),
>> -    DEFINE_PROP_BOOL("pmu", RISCVCPU, cfg.ext_pmu, true),
>> +    DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
>
> I think It's better to add  a check on cfg.pmu_num to  <= 29.
>
OK, I find this check in the following patch.
>>       DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
>>       DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
>>       DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false),
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 252c30a55d78..ffee54ea5c27 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -397,7 +397,6 @@ struct RISCVCPUConfig {
>>       bool ext_zksed;
>>       bool ext_zksh;
>>       bool ext_zkt;
>> -    bool ext_pmu;
>>       bool ext_ifencei;
>>       bool ext_icsr;
>>       bool ext_svinval;
>> @@ -421,6 +420,7 @@ struct RISCVCPUConfig {
>>       /* Vendor-specific custom extensions */
>>       bool ext_XVentanaCondOps;
>>   +    uint8_t pmu_num;
>>       char *priv_spec;
>>       char *user_spec;
>>       char *bext_spec;
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index 0ca05c77883c..b4a8e15f498f 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -73,9 +73,17 @@ static RISCVException ctr(CPURISCVState *env, int 
>> csrno)
>>       CPUState *cs = env_cpu(env);
>>       RISCVCPU *cpu = RISCV_CPU(cs);
>>       int ctr_index;
>> +    int base_csrno = CSR_HPMCOUNTER3;
>> +    bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false;
>>   -    if (!cpu->cfg.ext_pmu) {
>> -        /* The PMU extension is not enabled */
>> +    if (rv32 && csrno >= CSR_CYCLEH) {
>> +        /* Offset for RV32 hpmcounternh counters */
>> +        base_csrno += 0x80;
>> +    }
>> +    ctr_index = csrno - base_csrno;
>> +
>> +    if (!cpu->cfg.pmu_num || ctr_index >= (cpu->cfg.pmu_num)) {
>> +        /* No counter is enabled in PMU or the counter is out of 
>> range */
>
> I seems unnecessary to add '!cpu->cfg.pmu_num ' here, 'ctr_index >= 
> (cpu->cfg.pmu_num)' is true
Typo.  I -> It
>
> when cpu->cfg.pmu_num is zero if the problem for base_csrno is fixed.
>
> Ragards,
>
> Weiwei Li
>
>>           return RISCV_EXCP_ILLEGAL_INST;
>>       }
>>   @@ -103,7 +111,7 @@ static RISCVException ctr(CPURISCVState *env, 
>> int csrno)
>>               }
>>               break;
>>           }
>> -        if (riscv_cpu_mxl(env) == MXL_RV32) {
>> +        if (rv32) {
>>               switch (csrno) {
>>               case CSR_CYCLEH:
>>                   if (!get_field(env->mcounteren, COUNTEREN_CY)) {
>> @@ -158,7 +166,7 @@ static RISCVException ctr(CPURISCVState *env, int 
>> csrno)
>>               }
>>               break;
>>           }
>> -        if (riscv_cpu_mxl(env) == MXL_RV32) {
>> +        if (rv32) {
>>               switch (csrno) {
>>               case CSR_CYCLEH:
>>                   if (!get_field(env->hcounteren, COUNTEREN_CY) &&
>> @@ -202,6 +210,26 @@ static RISCVException ctr32(CPURISCVState *env, 
>> int csrno)
>>   }
>>     #if !defined(CONFIG_USER_ONLY)
>> +static RISCVException mctr(CPURISCVState *env, int csrno)
>> +{
>> +    CPUState *cs = env_cpu(env);
>> +    RISCVCPU *cpu = RISCV_CPU(cs);
>> +    int ctr_index;
>> +    int base_csrno = CSR_MHPMCOUNTER3;
>> +
>> +    if ((riscv_cpu_mxl(env) == MXL_RV32) && csrno >= CSR_MCYCLEH) {
>> +        /* Offset for RV32 mhpmcounternh counters */
>> +        base_csrno += 0x80;
>> +    }
>> +    ctr_index = csrno - base_csrno;
>> +    if (!cpu->cfg.pmu_num || ctr_index >= cpu->cfg.pmu_num) {
>> +        /* The PMU is not enabled or counter is out of range*/
>> +        return RISCV_EXCP_ILLEGAL_INST;
>> +    }
>> +
>> +    return RISCV_EXCP_NONE;
>> +}
>> +
>>   static RISCVException any(CPURISCVState *env, int csrno)
>>   {
>>       return RISCV_EXCP_NONE;
>> @@ -3687,35 +3715,35 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>>       [CSR_HPMCOUNTER30]   = { "hpmcounter30",   ctr, read_zero },
>>       [CSR_HPMCOUNTER31]   = { "hpmcounter31",   ctr, read_zero },
>>   -    [CSR_MHPMCOUNTER3]   = { "mhpmcounter3",   any, read_zero },
>> -    [CSR_MHPMCOUNTER4]   = { "mhpmcounter4",   any, read_zero },
>> -    [CSR_MHPMCOUNTER5]   = { "mhpmcounter5",   any, read_zero },
>> -    [CSR_MHPMCOUNTER6]   = { "mhpmcounter6",   any, read_zero },
>> -    [CSR_MHPMCOUNTER7]   = { "mhpmcounter7",   any, read_zero },
>> -    [CSR_MHPMCOUNTER8]   = { "mhpmcounter8",   any, read_zero },
>> -    [CSR_MHPMCOUNTER9]   = { "mhpmcounter9",   any, read_zero },
>> -    [CSR_MHPMCOUNTER10]  = { "mhpmcounter10",  any, read_zero },
>> -    [CSR_MHPMCOUNTER11]  = { "mhpmcounter11",  any, read_zero },
>> -    [CSR_MHPMCOUNTER12]  = { "mhpmcounter12",  any, read_zero },
>> -    [CSR_MHPMCOUNTER13]  = { "mhpmcounter13",  any, read_zero },
>> -    [CSR_MHPMCOUNTER14]  = { "mhpmcounter14",  any, read_zero },
>> -    [CSR_MHPMCOUNTER15]  = { "mhpmcounter15",  any, read_zero },
>> -    [CSR_MHPMCOUNTER16]  = { "mhpmcounter16",  any, read_zero },
>> -    [CSR_MHPMCOUNTER17]  = { "mhpmcounter17",  any, read_zero },
>> -    [CSR_MHPMCOUNTER18]  = { "mhpmcounter18",  any, read_zero },
>> -    [CSR_MHPMCOUNTER19]  = { "mhpmcounter19",  any, read_zero },
>> -    [CSR_MHPMCOUNTER20]  = { "mhpmcounter20",  any, read_zero },
>> -    [CSR_MHPMCOUNTER21]  = { "mhpmcounter21",  any, read_zero },
>> -    [CSR_MHPMCOUNTER22]  = { "mhpmcounter22",  any, read_zero },
>> -    [CSR_MHPMCOUNTER23]  = { "mhpmcounter23",  any, read_zero },
>> -    [CSR_MHPMCOUNTER24]  = { "mhpmcounter24",  any, read_zero },
>> -    [CSR_MHPMCOUNTER25]  = { "mhpmcounter25",  any, read_zero },
>> -    [CSR_MHPMCOUNTER26]  = { "mhpmcounter26",  any, read_zero },
>> -    [CSR_MHPMCOUNTER27]  = { "mhpmcounter27",  any, read_zero },
>> -    [CSR_MHPMCOUNTER28]  = { "mhpmcounter28",  any, read_zero },
>> -    [CSR_MHPMCOUNTER29]  = { "mhpmcounter29",  any, read_zero },
>> -    [CSR_MHPMCOUNTER30]  = { "mhpmcounter30",  any, read_zero },
>> -    [CSR_MHPMCOUNTER31]  = { "mhpmcounter31",  any, read_zero },
>> +    [CSR_MHPMCOUNTER3]   = { "mhpmcounter3",   mctr, read_zero },
>> +    [CSR_MHPMCOUNTER4]   = { "mhpmcounter4",   mctr, read_zero },
>> +    [CSR_MHPMCOUNTER5]   = { "mhpmcounter5",   mctr, read_zero },
>> +    [CSR_MHPMCOUNTER6]   = { "mhpmcounter6",   mctr, read_zero },
>> +    [CSR_MHPMCOUNTER7]   = { "mhpmcounter7",   mctr, read_zero },
>> +    [CSR_MHPMCOUNTER8]   = { "mhpmcounter8",   mctr, read_zero },
>> +    [CSR_MHPMCOUNTER9]   = { "mhpmcounter9",   mctr, read_zero },
>> +    [CSR_MHPMCOUNTER10]  = { "mhpmcounter10",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER11]  = { "mhpmcounter11",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER12]  = { "mhpmcounter12",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER13]  = { "mhpmcounter13",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER14]  = { "mhpmcounter14",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER15]  = { "mhpmcounter15",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER16]  = { "mhpmcounter16",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER17]  = { "mhpmcounter17",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER18]  = { "mhpmcounter18",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER19]  = { "mhpmcounter19",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER20]  = { "mhpmcounter20",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER21]  = { "mhpmcounter21",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER22]  = { "mhpmcounter22",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER23]  = { "mhpmcounter23",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER24]  = { "mhpmcounter24",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER25]  = { "mhpmcounter25",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER26]  = { "mhpmcounter26",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER27]  = { "mhpmcounter27",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER28]  = { "mhpmcounter28",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER29]  = { "mhpmcounter29",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER30]  = { "mhpmcounter30",  mctr, read_zero },
>> +    [CSR_MHPMCOUNTER31]  = { "mhpmcounter31",  mctr, read_zero },
>>         [CSR_MHPMEVENT3]     = { "mhpmevent3",     any, read_zero },
>>       [CSR_MHPMEVENT4]     = { "mhpmevent4",     any, read_zero },
>
Atish Kumar Patra July 5, 2022, 7:51 a.m. UTC | #3 
On Mon, Jul 4, 2022 at 5:38 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
>
> 在 2022/7/4 下午11:26, Weiwei Li 写道:
> >
> > 在 2022/6/21 上午7:15, Atish Patra 写道:
> >> The RISC-V privilege specification provides flexibility to implement
> >> any number of counters from 29 programmable counters. However, the QEMU
> >> implements all the counters.
> >>
> >> Make it configurable through pmu config parameter which now will
> >> indicate
> >> how many programmable counters should be implemented by the cpu.
> >>
> >> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> >> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> >> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> >> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> >> ---
> >>   target/riscv/cpu.c |  3 +-
> >>   target/riscv/cpu.h |  2 +-
> >>   target/riscv/csr.c | 94 ++++++++++++++++++++++++++++++----------------
> >>   3 files changed, 63 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index 1b57b3c43980..d12c6dc630ca 100644
> >> --- a/target/riscv/cpu.c
> >> +++ b/target/riscv/cpu.c
> >> @@ -851,7 +851,6 @@ static void riscv_cpu_init(Object *obj)
> >>   {
> >>       RISCVCPU *cpu = RISCV_CPU(obj);
> >>   -    cpu->cfg.ext_pmu = true;
> >>       cpu->cfg.ext_ifencei = true;
> >>       cpu->cfg.ext_icsr = true;
> >>       cpu->cfg.mmu = true;
> >> @@ -879,7 +878,7 @@ static Property riscv_cpu_extensions[] = {
> >>       DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
> >>       DEFINE_PROP_BOOL("v", RISCVCPU, cfg.ext_v, false),
> >>       DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, true),
> >> -    DEFINE_PROP_BOOL("pmu", RISCVCPU, cfg.ext_pmu, true),
> >> +    DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> >
> > I think It's better to add  a check on cfg.pmu_num to  <= 29.
> >
> OK, I find this check in the following patch.
> >>       DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
> >>       DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> >>       DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false),
> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >> index 252c30a55d78..ffee54ea5c27 100644
> >> --- a/target/riscv/cpu.h
> >> +++ b/target/riscv/cpu.h
> >> @@ -397,7 +397,6 @@ struct RISCVCPUConfig {
> >>       bool ext_zksed;
> >>       bool ext_zksh;
> >>       bool ext_zkt;
> >> -    bool ext_pmu;
> >>       bool ext_ifencei;
> >>       bool ext_icsr;
> >>       bool ext_svinval;
> >> @@ -421,6 +420,7 @@ struct RISCVCPUConfig {
> >>       /* Vendor-specific custom extensions */
> >>       bool ext_XVentanaCondOps;
> >>   +    uint8_t pmu_num;
> >>       char *priv_spec;
> >>       char *user_spec;
> >>       char *bext_spec;
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index 0ca05c77883c..b4a8e15f498f 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -73,9 +73,17 @@ static RISCVException ctr(CPURISCVState *env, int
> >> csrno)
> >>       CPUState *cs = env_cpu(env);
> >>       RISCVCPU *cpu = RISCV_CPU(cs);
> >>       int ctr_index;
> >> +    int base_csrno = CSR_HPMCOUNTER3;
> >> +    bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false;
> >>   -    if (!cpu->cfg.ext_pmu) {
> >> -        /* The PMU extension is not enabled */
> >> +    if (rv32 && csrno >= CSR_CYCLEH) {
> >> +        /* Offset for RV32 hpmcounternh counters */
> >> +        base_csrno += 0x80;
> >> +    }
> >> +    ctr_index = csrno - base_csrno;
> >> +
> >> +    if (!cpu->cfg.pmu_num || ctr_index >= (cpu->cfg.pmu_num)) {
> >> +        /* No counter is enabled in PMU or the counter is out of
> >> range */
> >
> > I seems unnecessary to add '!cpu->cfg.pmu_num ' here, 'ctr_index >=
> > (cpu->cfg.pmu_num)' is true

The check is improved in the following patches as well.

> Typo.  I -> It
> >
> > when cpu->cfg.pmu_num is zero if the problem for base_csrno is fixed.
> >
> > Ragards,
> >
> > Weiwei Li
> >
> >>           return RISCV_EXCP_ILLEGAL_INST;
> >>       }
> >>   @@ -103,7 +111,7 @@ static RISCVException ctr(CPURISCVState *env,
> >> int csrno)
> >>               }
> >>               break;
> >>           }
> >> -        if (riscv_cpu_mxl(env) == MXL_RV32) {
> >> +        if (rv32) {
> >>               switch (csrno) {
> >>               case CSR_CYCLEH:
> >>                   if (!get_field(env->mcounteren, COUNTEREN_CY)) {
> >> @@ -158,7 +166,7 @@ static RISCVException ctr(CPURISCVState *env, int
> >> csrno)
> >>               }
> >>               break;
> >>           }
> >> -        if (riscv_cpu_mxl(env) == MXL_RV32) {
> >> +        if (rv32) {
> >>               switch (csrno) {
> >>               case CSR_CYCLEH:
> >>                   if (!get_field(env->hcounteren, COUNTEREN_CY) &&
> >> @@ -202,6 +210,26 @@ static RISCVException ctr32(CPURISCVState *env,
> >> int csrno)
> >>   }
> >>     #if !defined(CONFIG_USER_ONLY)
> >> +static RISCVException mctr(CPURISCVState *env, int csrno)
> >> +{
> >> +    CPUState *cs = env_cpu(env);
> >> +    RISCVCPU *cpu = RISCV_CPU(cs);
> >> +    int ctr_index;
> >> +    int base_csrno = CSR_MHPMCOUNTER3;
> >> +
> >> +    if ((riscv_cpu_mxl(env) == MXL_RV32) && csrno >= CSR_MCYCLEH) {
> >> +        /* Offset for RV32 mhpmcounternh counters */
> >> +        base_csrno += 0x80;
> >> +    }
> >> +    ctr_index = csrno - base_csrno;
> >> +    if (!cpu->cfg.pmu_num || ctr_index >= cpu->cfg.pmu_num) {
> >> +        /* The PMU is not enabled or counter is out of range*/
> >> +        return RISCV_EXCP_ILLEGAL_INST;
> >> +    }
> >> +
> >> +    return RISCV_EXCP_NONE;
> >> +}
> >> +
> >>   static RISCVException any(CPURISCVState *env, int csrno)
> >>   {
> >>       return RISCV_EXCP_NONE;
> >> @@ -3687,35 +3715,35 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> >>       [CSR_HPMCOUNTER30]   = { "hpmcounter30",   ctr, read_zero },
> >>       [CSR_HPMCOUNTER31]   = { "hpmcounter31",   ctr, read_zero },
> >>   -    [CSR_MHPMCOUNTER3]   = { "mhpmcounter3",   any, read_zero },
> >> -    [CSR_MHPMCOUNTER4]   = { "mhpmcounter4",   any, read_zero },
> >> -    [CSR_MHPMCOUNTER5]   = { "mhpmcounter5",   any, read_zero },
> >> -    [CSR_MHPMCOUNTER6]   = { "mhpmcounter6",   any, read_zero },
> >> -    [CSR_MHPMCOUNTER7]   = { "mhpmcounter7",   any, read_zero },
> >> -    [CSR_MHPMCOUNTER8]   = { "mhpmcounter8",   any, read_zero },
> >> -    [CSR_MHPMCOUNTER9]   = { "mhpmcounter9",   any, read_zero },
> >> -    [CSR_MHPMCOUNTER10]  = { "mhpmcounter10",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER11]  = { "mhpmcounter11",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER12]  = { "mhpmcounter12",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER13]  = { "mhpmcounter13",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER14]  = { "mhpmcounter14",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER15]  = { "mhpmcounter15",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER16]  = { "mhpmcounter16",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER17]  = { "mhpmcounter17",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER18]  = { "mhpmcounter18",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER19]  = { "mhpmcounter19",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER20]  = { "mhpmcounter20",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER21]  = { "mhpmcounter21",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER22]  = { "mhpmcounter22",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER23]  = { "mhpmcounter23",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER24]  = { "mhpmcounter24",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER25]  = { "mhpmcounter25",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER26]  = { "mhpmcounter26",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER27]  = { "mhpmcounter27",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER28]  = { "mhpmcounter28",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER29]  = { "mhpmcounter29",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER30]  = { "mhpmcounter30",  any, read_zero },
> >> -    [CSR_MHPMCOUNTER31]  = { "mhpmcounter31",  any, read_zero },
> >> +    [CSR_MHPMCOUNTER3]   = { "mhpmcounter3",   mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER4]   = { "mhpmcounter4",   mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER5]   = { "mhpmcounter5",   mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER6]   = { "mhpmcounter6",   mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER7]   = { "mhpmcounter7",   mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER8]   = { "mhpmcounter8",   mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER9]   = { "mhpmcounter9",   mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER10]  = { "mhpmcounter10",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER11]  = { "mhpmcounter11",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER12]  = { "mhpmcounter12",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER13]  = { "mhpmcounter13",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER14]  = { "mhpmcounter14",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER15]  = { "mhpmcounter15",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER16]  = { "mhpmcounter16",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER17]  = { "mhpmcounter17",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER18]  = { "mhpmcounter18",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER19]  = { "mhpmcounter19",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER20]  = { "mhpmcounter20",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER21]  = { "mhpmcounter21",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER22]  = { "mhpmcounter22",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER23]  = { "mhpmcounter23",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER24]  = { "mhpmcounter24",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER25]  = { "mhpmcounter25",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER26]  = { "mhpmcounter26",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER27]  = { "mhpmcounter27",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER28]  = { "mhpmcounter28",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER29]  = { "mhpmcounter29",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER30]  = { "mhpmcounter30",  mctr, read_zero },
> >> +    [CSR_MHPMCOUNTER31]  = { "mhpmcounter31",  mctr, read_zero },
> >>         [CSR_MHPMEVENT3]     = { "mhpmevent3",     any, read_zero },
> >>       [CSR_MHPMEVENT4]     = { "mhpmevent4",     any, read_zero },
> >
>
Weiwei Li July 5, 2022, 8:16 a.m. UTC | #4 
在 2022/7/5 下午3:51, Atish Kumar Patra 写道:
> On Mon, Jul 4, 2022 at 5:38 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>>
>> 在 2022/7/4 下午11:26, Weiwei Li 写道:
>>> 在 2022/6/21 上午7:15, Atish Patra 写道:
>>>> The RISC-V privilege specification provides flexibility to implement
>>>> any number of counters from 29 programmable counters. However, the QEMU
>>>> implements all the counters.
>>>>
>>>> Make it configurable through pmu config parameter which now will
>>>> indicate
>>>> how many programmable counters should be implemented by the cpu.
>>>>
>>>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>>>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>>> ---
>>>>    target/riscv/cpu.c |  3 +-
>>>>    target/riscv/cpu.h |  2 +-
>>>>    target/riscv/csr.c | 94 ++++++++++++++++++++++++++++++----------------
>>>>    3 files changed, 63 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>>> index 1b57b3c43980..d12c6dc630ca 100644
>>>> --- a/target/riscv/cpu.c
>>>> +++ b/target/riscv/cpu.c
>>>> @@ -851,7 +851,6 @@ static void riscv_cpu_init(Object *obj)
>>>>    {
>>>>        RISCVCPU *cpu = RISCV_CPU(obj);
>>>>    -    cpu->cfg.ext_pmu = true;
>>>>        cpu->cfg.ext_ifencei = true;
>>>>        cpu->cfg.ext_icsr = true;
>>>>        cpu->cfg.mmu = true;
>>>> @@ -879,7 +878,7 @@ static Property riscv_cpu_extensions[] = {
>>>>        DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
>>>>        DEFINE_PROP_BOOL("v", RISCVCPU, cfg.ext_v, false),
>>>>        DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, true),
>>>> -    DEFINE_PROP_BOOL("pmu", RISCVCPU, cfg.ext_pmu, true),
>>>> +    DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
>>> I think It's better to add  a check on cfg.pmu_num to  <= 29.
>>>
>> OK, I find this check in the following patch.
>>>>        DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
>>>>        DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
>>>>        DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false),
>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>>> index 252c30a55d78..ffee54ea5c27 100644
>>>> --- a/target/riscv/cpu.h
>>>> +++ b/target/riscv/cpu.h
>>>> @@ -397,7 +397,6 @@ struct RISCVCPUConfig {
>>>>        bool ext_zksed;
>>>>        bool ext_zksh;
>>>>        bool ext_zkt;
>>>> -    bool ext_pmu;
>>>>        bool ext_ifencei;
>>>>        bool ext_icsr;
>>>>        bool ext_svinval;
>>>> @@ -421,6 +420,7 @@ struct RISCVCPUConfig {
>>>>        /* Vendor-specific custom extensions */
>>>>        bool ext_XVentanaCondOps;
>>>>    +    uint8_t pmu_num;
>>>>        char *priv_spec;
>>>>        char *user_spec;
>>>>        char *bext_spec;
>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>>> index 0ca05c77883c..b4a8e15f498f 100644
>>>> --- a/target/riscv/csr.c
>>>> +++ b/target/riscv/csr.c
>>>> @@ -73,9 +73,17 @@ static RISCVException ctr(CPURISCVState *env, int
>>>> csrno)
>>>>        CPUState *cs = env_cpu(env);
>>>>        RISCVCPU *cpu = RISCV_CPU(cs);
>>>>        int ctr_index;
>>>> +    int base_csrno = CSR_HPMCOUNTER3;
>>>> +    bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false;
>>>>    -    if (!cpu->cfg.ext_pmu) {
>>>> -        /* The PMU extension is not enabled */
>>>> +    if (rv32 && csrno >= CSR_CYCLEH) {
>>>> +        /* Offset for RV32 hpmcounternh counters */
>>>> +        base_csrno += 0x80;
>>>> +    }
>>>> +    ctr_index = csrno - base_csrno;
>>>> +
>>>> +    if (!cpu->cfg.pmu_num || ctr_index >= (cpu->cfg.pmu_num)) {
>>>> +        /* No counter is enabled in PMU or the counter is out of
>>>> range */
>>> I seems unnecessary to add '!cpu->cfg.pmu_num ' here, 'ctr_index >=
>>> (cpu->cfg.pmu_num)' is true
> The check is improved in the following patches as well.
>
Do you mean 'if ((!cpu->cfg.pmu_num || !(cpu->pmu_avail_ctrs & 
ctr_mask)))'  in patch 9 ?

In this condition, '!cpu->cfg.pmu_num' seems unnecessary too.

Regards,

Weiwei Li

>> Typo.  I -> It
>>> when cpu->cfg.pmu_num is zero if the problem for base_csrno is fixed.
>>>
>>> Ragards,
>>>
>>> Weiwei Li
>>>
>>>>            return RISCV_EXCP_ILLEGAL_INST;
>>>>        }
>>>>    @@ -103,7 +111,7 @@ static RISCVException ctr(CPURISCVState *env,
>>>> int csrno)
>>>>                }
>>>>                break;
>>>>            }
>>>> -        if (riscv_cpu_mxl(env) == MXL_RV32) {
>>>> +        if (rv32) {
>>>>                switch (csrno) {
>>>>                case CSR_CYCLEH:
>>>>                    if (!get_field(env->mcounteren, COUNTEREN_CY)) {
>>>> @@ -158,7 +166,7 @@ static RISCVException ctr(CPURISCVState *env, int
>>>> csrno)
>>>>                }
>>>>                break;
>>>>            }
>>>> -        if (riscv_cpu_mxl(env) == MXL_RV32) {
>>>> +        if (rv32) {
>>>>                switch (csrno) {
>>>>                case CSR_CYCLEH:
>>>>                    if (!get_field(env->hcounteren, COUNTEREN_CY) &&
>>>> @@ -202,6 +210,26 @@ static RISCVException ctr32(CPURISCVState *env,
>>>> int csrno)
>>>>    }
>>>>      #if !defined(CONFIG_USER_ONLY)
>>>> +static RISCVException mctr(CPURISCVState *env, int csrno)
>>>> +{
>>>> +    CPUState *cs = env_cpu(env);
>>>> +    RISCVCPU *cpu = RISCV_CPU(cs);
>>>> +    int ctr_index;
>>>> +    int base_csrno = CSR_MHPMCOUNTER3;
>>>> +
>>>> +    if ((riscv_cpu_mxl(env) == MXL_RV32) && csrno >= CSR_MCYCLEH) {
>>>> +        /* Offset for RV32 mhpmcounternh counters */
>>>> +        base_csrno += 0x80;
>>>> +    }
>>>> +    ctr_index = csrno - base_csrno;
>>>> +    if (!cpu->cfg.pmu_num || ctr_index >= cpu->cfg.pmu_num) {
>>>> +        /* The PMU is not enabled or counter is out of range*/
>>>> +        return RISCV_EXCP_ILLEGAL_INST;
>>>> +    }
>>>> +
>>>> +    return RISCV_EXCP_NONE;
>>>> +}
>>>> +
>>>>    static RISCVException any(CPURISCVState *env, int csrno)
>>>>    {
>>>>        return RISCV_EXCP_NONE;
>>>> @@ -3687,35 +3715,35 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>>>>        [CSR_HPMCOUNTER30]   = { "hpmcounter30",   ctr, read_zero },
>>>>        [CSR_HPMCOUNTER31]   = { "hpmcounter31",   ctr, read_zero },
>>>>    -    [CSR_MHPMCOUNTER3]   = { "mhpmcounter3",   any, read_zero },
>>>> -    [CSR_MHPMCOUNTER4]   = { "mhpmcounter4",   any, read_zero },
>>>> -    [CSR_MHPMCOUNTER5]   = { "mhpmcounter5",   any, read_zero },
>>>> -    [CSR_MHPMCOUNTER6]   = { "mhpmcounter6",   any, read_zero },
>>>> -    [CSR_MHPMCOUNTER7]   = { "mhpmcounter7",   any, read_zero },
>>>> -    [CSR_MHPMCOUNTER8]   = { "mhpmcounter8",   any, read_zero },
>>>> -    [CSR_MHPMCOUNTER9]   = { "mhpmcounter9",   any, read_zero },
>>>> -    [CSR_MHPMCOUNTER10]  = { "mhpmcounter10",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER11]  = { "mhpmcounter11",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER12]  = { "mhpmcounter12",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER13]  = { "mhpmcounter13",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER14]  = { "mhpmcounter14",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER15]  = { "mhpmcounter15",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER16]  = { "mhpmcounter16",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER17]  = { "mhpmcounter17",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER18]  = { "mhpmcounter18",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER19]  = { "mhpmcounter19",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER20]  = { "mhpmcounter20",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER21]  = { "mhpmcounter21",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER22]  = { "mhpmcounter22",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER23]  = { "mhpmcounter23",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER24]  = { "mhpmcounter24",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER25]  = { "mhpmcounter25",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER26]  = { "mhpmcounter26",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER27]  = { "mhpmcounter27",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER28]  = { "mhpmcounter28",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER29]  = { "mhpmcounter29",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER30]  = { "mhpmcounter30",  any, read_zero },
>>>> -    [CSR_MHPMCOUNTER31]  = { "mhpmcounter31",  any, read_zero },
>>>> +    [CSR_MHPMCOUNTER3]   = { "mhpmcounter3",   mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER4]   = { "mhpmcounter4",   mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER5]   = { "mhpmcounter5",   mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER6]   = { "mhpmcounter6",   mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER7]   = { "mhpmcounter7",   mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER8]   = { "mhpmcounter8",   mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER9]   = { "mhpmcounter9",   mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER10]  = { "mhpmcounter10",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER11]  = { "mhpmcounter11",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER12]  = { "mhpmcounter12",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER13]  = { "mhpmcounter13",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER14]  = { "mhpmcounter14",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER15]  = { "mhpmcounter15",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER16]  = { "mhpmcounter16",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER17]  = { "mhpmcounter17",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER18]  = { "mhpmcounter18",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER19]  = { "mhpmcounter19",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER20]  = { "mhpmcounter20",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER21]  = { "mhpmcounter21",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER22]  = { "mhpmcounter22",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER23]  = { "mhpmcounter23",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER24]  = { "mhpmcounter24",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER25]  = { "mhpmcounter25",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER26]  = { "mhpmcounter26",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER27]  = { "mhpmcounter27",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER28]  = { "mhpmcounter28",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER29]  = { "mhpmcounter29",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER30]  = { "mhpmcounter30",  mctr, read_zero },
>>>> +    [CSR_MHPMCOUNTER31]  = { "mhpmcounter31",  mctr, read_zero },
>>>>          [CSR_MHPMEVENT3]     = { "mhpmevent3",     any, read_zero },
>>>>        [CSR_MHPMEVENT4]     = { "mhpmevent4",     any, read_zero },
Atish Patra July 26, 2022, 10:19 p.m. UTC | #5 
On Tue, Jul 5, 2022 at 1:20 AM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
>
> 在 2022/7/5 下午3:51, Atish Kumar Patra 写道:
> > On Mon, Jul 4, 2022 at 5:38 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
> >>
> >> 在 2022/7/4 下午11:26, Weiwei Li 写道:
> >>> 在 2022/6/21 上午7:15, Atish Patra 写道:
> >>>> The RISC-V privilege specification provides flexibility to implement
> >>>> any number of counters from 29 programmable counters. However, the QEMU
> >>>> implements all the counters.
> >>>>
> >>>> Make it configurable through pmu config parameter which now will
> >>>> indicate
> >>>> how many programmable counters should be implemented by the cpu.
> >>>>
> >>>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> >>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> >>>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> >>>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> >>>> ---
> >>>>    target/riscv/cpu.c |  3 +-
> >>>>    target/riscv/cpu.h |  2 +-
> >>>>    target/riscv/csr.c | 94 ++++++++++++++++++++++++++++++----------------
> >>>>    3 files changed, 63 insertions(+), 36 deletions(-)
> >>>>
> >>>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >>>> index 1b57b3c43980..d12c6dc630ca 100644
> >>>> --- a/target/riscv/cpu.c
> >>>> +++ b/target/riscv/cpu.c
> >>>> @@ -851,7 +851,6 @@ static void riscv_cpu_init(Object *obj)
> >>>>    {
> >>>>        RISCVCPU *cpu = RISCV_CPU(obj);
> >>>>    -    cpu->cfg.ext_pmu = true;
> >>>>        cpu->cfg.ext_ifencei = true;
> >>>>        cpu->cfg.ext_icsr = true;
> >>>>        cpu->cfg.mmu = true;
> >>>> @@ -879,7 +878,7 @@ static Property riscv_cpu_extensions[] = {
> >>>>        DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
> >>>>        DEFINE_PROP_BOOL("v", RISCVCPU, cfg.ext_v, false),
> >>>>        DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, true),
> >>>> -    DEFINE_PROP_BOOL("pmu", RISCVCPU, cfg.ext_pmu, true),
> >>>> +    DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> >>> I think It's better to add  a check on cfg.pmu_num to  <= 29.
> >>>
> >> OK, I find this check in the following patch.
> >>>>        DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
> >>>>        DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> >>>>        DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false),
> >>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >>>> index 252c30a55d78..ffee54ea5c27 100644
> >>>> --- a/target/riscv/cpu.h
> >>>> +++ b/target/riscv/cpu.h
> >>>> @@ -397,7 +397,6 @@ struct RISCVCPUConfig {
> >>>>        bool ext_zksed;
> >>>>        bool ext_zksh;
> >>>>        bool ext_zkt;
> >>>> -    bool ext_pmu;
> >>>>        bool ext_ifencei;
> >>>>        bool ext_icsr;
> >>>>        bool ext_svinval;
> >>>> @@ -421,6 +420,7 @@ struct RISCVCPUConfig {
> >>>>        /* Vendor-specific custom extensions */
> >>>>        bool ext_XVentanaCondOps;
> >>>>    +    uint8_t pmu_num;
> >>>>        char *priv_spec;
> >>>>        char *user_spec;
> >>>>        char *bext_spec;
> >>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >>>> index 0ca05c77883c..b4a8e15f498f 100644
> >>>> --- a/target/riscv/csr.c
> >>>> +++ b/target/riscv/csr.c
> >>>> @@ -73,9 +73,17 @@ static RISCVException ctr(CPURISCVState *env, int
> >>>> csrno)
> >>>>        CPUState *cs = env_cpu(env);
> >>>>        RISCVCPU *cpu = RISCV_CPU(cs);
> >>>>        int ctr_index;
> >>>> +    int base_csrno = CSR_HPMCOUNTER3;
> >>>> +    bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false;
> >>>>    -    if (!cpu->cfg.ext_pmu) {
> >>>> -        /* The PMU extension is not enabled */
> >>>> +    if (rv32 && csrno >= CSR_CYCLEH) {
> >>>> +        /* Offset for RV32 hpmcounternh counters */
> >>>> +        base_csrno += 0x80;
> >>>> +    }
> >>>> +    ctr_index = csrno - base_csrno;
> >>>> +
> >>>> +    if (!cpu->cfg.pmu_num || ctr_index >= (cpu->cfg.pmu_num)) {
> >>>> +        /* No counter is enabled in PMU or the counter is out of
> >>>> range */
> >>> I seems unnecessary to add '!cpu->cfg.pmu_num ' here, 'ctr_index >=
> >>> (cpu->cfg.pmu_num)' is true
> > The check is improved in the following patches as well.
> >
> Do you mean 'if ((!cpu->cfg.pmu_num || !(cpu->pmu_avail_ctrs &
> ctr_mask)))'  in patch 9 ?
>
> In this condition, '!cpu->cfg.pmu_num' seems unnecessary too.
>

Yes. I will remove it.

> Regards,
>
> Weiwei Li
>
> >> Typo.  I -> It
> >>> when cpu->cfg.pmu_num is zero if the problem for base_csrno is fixed.
> >>>
> >>> Ragards,
> >>>
> >>> Weiwei Li
> >>>
> >>>>            return RISCV_EXCP_ILLEGAL_INST;
> >>>>        }
> >>>>    @@ -103,7 +111,7 @@ static RISCVException ctr(CPURISCVState *env,
> >>>> int csrno)
> >>>>                }
> >>>>                break;
> >>>>            }
> >>>> -        if (riscv_cpu_mxl(env) == MXL_RV32) {
> >>>> +        if (rv32) {
> >>>>                switch (csrno) {
> >>>>                case CSR_CYCLEH:
> >>>>                    if (!get_field(env->mcounteren, COUNTEREN_CY)) {
> >>>> @@ -158,7 +166,7 @@ static RISCVException ctr(CPURISCVState *env, int
> >>>> csrno)
> >>>>                }
> >>>>                break;
> >>>>            }
> >>>> -        if (riscv_cpu_mxl(env) == MXL_RV32) {
> >>>> +        if (rv32) {
> >>>>                switch (csrno) {
> >>>>                case CSR_CYCLEH:
> >>>>                    if (!get_field(env->hcounteren, COUNTEREN_CY) &&
> >>>> @@ -202,6 +210,26 @@ static RISCVException ctr32(CPURISCVState *env,
> >>>> int csrno)
> >>>>    }
> >>>>      #if !defined(CONFIG_USER_ONLY)
> >>>> +static RISCVException mctr(CPURISCVState *env, int csrno)
> >>>> +{
> >>>> +    CPUState *cs = env_cpu(env);
> >>>> +    RISCVCPU *cpu = RISCV_CPU(cs);
> >>>> +    int ctr_index;
> >>>> +    int base_csrno = CSR_MHPMCOUNTER3;
> >>>> +
> >>>> +    if ((riscv_cpu_mxl(env) == MXL_RV32) && csrno >= CSR_MCYCLEH) {
> >>>> +        /* Offset for RV32 mhpmcounternh counters */
> >>>> +        base_csrno += 0x80;
> >>>> +    }
> >>>> +    ctr_index = csrno - base_csrno;
> >>>> +    if (!cpu->cfg.pmu_num || ctr_index >= cpu->cfg.pmu_num) {
> >>>> +        /* The PMU is not enabled or counter is out of range*/
> >>>> +        return RISCV_EXCP_ILLEGAL_INST;
> >>>> +    }
> >>>> +
> >>>> +    return RISCV_EXCP_NONE;
> >>>> +}
> >>>> +
> >>>>    static RISCVException any(CPURISCVState *env, int csrno)
> >>>>    {
> >>>>        return RISCV_EXCP_NONE;
> >>>> @@ -3687,35 +3715,35 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> >>>>        [CSR_HPMCOUNTER30]   = { "hpmcounter30",   ctr, read_zero },
> >>>>        [CSR_HPMCOUNTER31]   = { "hpmcounter31",   ctr, read_zero },
> >>>>    -    [CSR_MHPMCOUNTER3]   = { "mhpmcounter3",   any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER4]   = { "mhpmcounter4",   any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER5]   = { "mhpmcounter5",   any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER6]   = { "mhpmcounter6",   any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER7]   = { "mhpmcounter7",   any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER8]   = { "mhpmcounter8",   any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER9]   = { "mhpmcounter9",   any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER10]  = { "mhpmcounter10",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER11]  = { "mhpmcounter11",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER12]  = { "mhpmcounter12",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER13]  = { "mhpmcounter13",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER14]  = { "mhpmcounter14",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER15]  = { "mhpmcounter15",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER16]  = { "mhpmcounter16",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER17]  = { "mhpmcounter17",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER18]  = { "mhpmcounter18",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER19]  = { "mhpmcounter19",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER20]  = { "mhpmcounter20",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER21]  = { "mhpmcounter21",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER22]  = { "mhpmcounter22",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER23]  = { "mhpmcounter23",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER24]  = { "mhpmcounter24",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER25]  = { "mhpmcounter25",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER26]  = { "mhpmcounter26",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER27]  = { "mhpmcounter27",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER28]  = { "mhpmcounter28",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER29]  = { "mhpmcounter29",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER30]  = { "mhpmcounter30",  any, read_zero },
> >>>> -    [CSR_MHPMCOUNTER31]  = { "mhpmcounter31",  any, read_zero },
> >>>> +    [CSR_MHPMCOUNTER3]   = { "mhpmcounter3",   mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER4]   = { "mhpmcounter4",   mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER5]   = { "mhpmcounter5",   mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER6]   = { "mhpmcounter6",   mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER7]   = { "mhpmcounter7",   mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER8]   = { "mhpmcounter8",   mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER9]   = { "mhpmcounter9",   mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER10]  = { "mhpmcounter10",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER11]  = { "mhpmcounter11",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER12]  = { "mhpmcounter12",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER13]  = { "mhpmcounter13",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER14]  = { "mhpmcounter14",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER15]  = { "mhpmcounter15",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER16]  = { "mhpmcounter16",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER17]  = { "mhpmcounter17",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER18]  = { "mhpmcounter18",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER19]  = { "mhpmcounter19",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER20]  = { "mhpmcounter20",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER21]  = { "mhpmcounter21",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER22]  = { "mhpmcounter22",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER23]  = { "mhpmcounter23",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER24]  = { "mhpmcounter24",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER25]  = { "mhpmcounter25",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER26]  = { "mhpmcounter26",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER27]  = { "mhpmcounter27",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER28]  = { "mhpmcounter28",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER29]  = { "mhpmcounter29",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER30]  = { "mhpmcounter30",  mctr, read_zero },
> >>>> +    [CSR_MHPMCOUNTER31]  = { "mhpmcounter31",  mctr, read_zero },
> >>>>          [CSR_MHPMEVENT3]     = { "mhpmevent3",     any, read_zero },
> >>>>        [CSR_MHPMEVENT4]     = { "mhpmevent4",     any, read_zero },
>
>
diff mbox series

Patch

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 1b57b3c43980..d12c6dc630ca 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -851,7 +851,6 @@  static void riscv_cpu_init(Object *obj)
 {
     RISCVCPU *cpu = RISCV_CPU(obj);
 
-    cpu->cfg.ext_pmu = true;
     cpu->cfg.ext_ifencei = true;
     cpu->cfg.ext_icsr = true;
     cpu->cfg.mmu = true;
@@ -879,7 +878,7 @@  static Property riscv_cpu_extensions[] = {
     DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
     DEFINE_PROP_BOOL("v", RISCVCPU, cfg.ext_v, false),
     DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, true),
-    DEFINE_PROP_BOOL("pmu", RISCVCPU, cfg.ext_pmu, true),
+    DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
     DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
     DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
     DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false),
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 252c30a55d78..ffee54ea5c27 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -397,7 +397,6 @@  struct RISCVCPUConfig {
     bool ext_zksed;
     bool ext_zksh;
     bool ext_zkt;
-    bool ext_pmu;
     bool ext_ifencei;
     bool ext_icsr;
     bool ext_svinval;
@@ -421,6 +420,7 @@  struct RISCVCPUConfig {
     /* Vendor-specific custom extensions */
     bool ext_XVentanaCondOps;
 
+    uint8_t pmu_num;
     char *priv_spec;
     char *user_spec;
     char *bext_spec;
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 0ca05c77883c..b4a8e15f498f 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -73,9 +73,17 @@  static RISCVException ctr(CPURISCVState *env, int csrno)
     CPUState *cs = env_cpu(env);
     RISCVCPU *cpu = RISCV_CPU(cs);
     int ctr_index;
+    int base_csrno = CSR_HPMCOUNTER3;
+    bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false;
 
-    if (!cpu->cfg.ext_pmu) {
-        /* The PMU extension is not enabled */
+    if (rv32 && csrno >= CSR_CYCLEH) {
+        /* Offset for RV32 hpmcounternh counters */
+        base_csrno += 0x80;
+    }
+    ctr_index = csrno - base_csrno;
+
+    if (!cpu->cfg.pmu_num || ctr_index >= (cpu->cfg.pmu_num)) {
+        /* No counter is enabled in PMU or the counter is out of range */
         return RISCV_EXCP_ILLEGAL_INST;
     }
 
@@ -103,7 +111,7 @@  static RISCVException ctr(CPURISCVState *env, int csrno)
             }
             break;
         }
-        if (riscv_cpu_mxl(env) == MXL_RV32) {
+        if (rv32) {
             switch (csrno) {
             case CSR_CYCLEH:
                 if (!get_field(env->mcounteren, COUNTEREN_CY)) {
@@ -158,7 +166,7 @@  static RISCVException ctr(CPURISCVState *env, int csrno)
             }
             break;
         }
-        if (riscv_cpu_mxl(env) == MXL_RV32) {
+        if (rv32) {
             switch (csrno) {
             case CSR_CYCLEH:
                 if (!get_field(env->hcounteren, COUNTEREN_CY) &&
@@ -202,6 +210,26 @@  static RISCVException ctr32(CPURISCVState *env, int csrno)
 }
 
 #if !defined(CONFIG_USER_ONLY)
+static RISCVException mctr(CPURISCVState *env, int csrno)
+{
+    CPUState *cs = env_cpu(env);
+    RISCVCPU *cpu = RISCV_CPU(cs);
+    int ctr_index;
+    int base_csrno = CSR_MHPMCOUNTER3;
+
+    if ((riscv_cpu_mxl(env) == MXL_RV32) && csrno >= CSR_MCYCLEH) {
+        /* Offset for RV32 mhpmcounternh counters */
+        base_csrno += 0x80;
+    }
+    ctr_index = csrno - base_csrno;
+    if (!cpu->cfg.pmu_num || ctr_index >= cpu->cfg.pmu_num) {
+        /* The PMU is not enabled or counter is out of range*/
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    return RISCV_EXCP_NONE;
+}
+
 static RISCVException any(CPURISCVState *env, int csrno)
 {
     return RISCV_EXCP_NONE;
@@ -3687,35 +3715,35 @@  riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
     [CSR_HPMCOUNTER30]   = { "hpmcounter30",   ctr,    read_zero },
     [CSR_HPMCOUNTER31]   = { "hpmcounter31",   ctr,    read_zero },
 
-    [CSR_MHPMCOUNTER3]   = { "mhpmcounter3",   any,    read_zero },
-    [CSR_MHPMCOUNTER4]   = { "mhpmcounter4",   any,    read_zero },
-    [CSR_MHPMCOUNTER5]   = { "mhpmcounter5",   any,    read_zero },
-    [CSR_MHPMCOUNTER6]   = { "mhpmcounter6",   any,    read_zero },
-    [CSR_MHPMCOUNTER7]   = { "mhpmcounter7",   any,    read_zero },
-    [CSR_MHPMCOUNTER8]   = { "mhpmcounter8",   any,    read_zero },
-    [CSR_MHPMCOUNTER9]   = { "mhpmcounter9",   any,    read_zero },
-    [CSR_MHPMCOUNTER10]  = { "mhpmcounter10",  any,    read_zero },
-    [CSR_MHPMCOUNTER11]  = { "mhpmcounter11",  any,    read_zero },
-    [CSR_MHPMCOUNTER12]  = { "mhpmcounter12",  any,    read_zero },
-    [CSR_MHPMCOUNTER13]  = { "mhpmcounter13",  any,    read_zero },
-    [CSR_MHPMCOUNTER14]  = { "mhpmcounter14",  any,    read_zero },
-    [CSR_MHPMCOUNTER15]  = { "mhpmcounter15",  any,    read_zero },
-    [CSR_MHPMCOUNTER16]  = { "mhpmcounter16",  any,    read_zero },
-    [CSR_MHPMCOUNTER17]  = { "mhpmcounter17",  any,    read_zero },
-    [CSR_MHPMCOUNTER18]  = { "mhpmcounter18",  any,    read_zero },
-    [CSR_MHPMCOUNTER19]  = { "mhpmcounter19",  any,    read_zero },
-    [CSR_MHPMCOUNTER20]  = { "mhpmcounter20",  any,    read_zero },
-    [CSR_MHPMCOUNTER21]  = { "mhpmcounter21",  any,    read_zero },
-    [CSR_MHPMCOUNTER22]  = { "mhpmcounter22",  any,    read_zero },
-    [CSR_MHPMCOUNTER23]  = { "mhpmcounter23",  any,    read_zero },
-    [CSR_MHPMCOUNTER24]  = { "mhpmcounter24",  any,    read_zero },
-    [CSR_MHPMCOUNTER25]  = { "mhpmcounter25",  any,    read_zero },
-    [CSR_MHPMCOUNTER26]  = { "mhpmcounter26",  any,    read_zero },
-    [CSR_MHPMCOUNTER27]  = { "mhpmcounter27",  any,    read_zero },
-    [CSR_MHPMCOUNTER28]  = { "mhpmcounter28",  any,    read_zero },
-    [CSR_MHPMCOUNTER29]  = { "mhpmcounter29",  any,    read_zero },
-    [CSR_MHPMCOUNTER30]  = { "mhpmcounter30",  any,    read_zero },
-    [CSR_MHPMCOUNTER31]  = { "mhpmcounter31",  any,    read_zero },
+    [CSR_MHPMCOUNTER3]   = { "mhpmcounter3",   mctr,   read_zero },
+    [CSR_MHPMCOUNTER4]   = { "mhpmcounter4",   mctr,   read_zero },
+    [CSR_MHPMCOUNTER5]   = { "mhpmcounter5",   mctr,   read_zero },
+    [CSR_MHPMCOUNTER6]   = { "mhpmcounter6",   mctr,   read_zero },
+    [CSR_MHPMCOUNTER7]   = { "mhpmcounter7",   mctr,   read_zero },
+    [CSR_MHPMCOUNTER8]   = { "mhpmcounter8",   mctr,   read_zero },
+    [CSR_MHPMCOUNTER9]   = { "mhpmcounter9",   mctr,   read_zero },
+    [CSR_MHPMCOUNTER10]  = { "mhpmcounter10",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER11]  = { "mhpmcounter11",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER12]  = { "mhpmcounter12",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER13]  = { "mhpmcounter13",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER14]  = { "mhpmcounter14",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER15]  = { "mhpmcounter15",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER16]  = { "mhpmcounter16",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER17]  = { "mhpmcounter17",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER18]  = { "mhpmcounter18",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER19]  = { "mhpmcounter19",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER20]  = { "mhpmcounter20",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER21]  = { "mhpmcounter21",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER22]  = { "mhpmcounter22",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER23]  = { "mhpmcounter23",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER24]  = { "mhpmcounter24",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER25]  = { "mhpmcounter25",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER26]  = { "mhpmcounter26",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER27]  = { "mhpmcounter27",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER28]  = { "mhpmcounter28",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER29]  = { "mhpmcounter29",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER30]  = { "mhpmcounter30",  mctr,   read_zero },
+    [CSR_MHPMCOUNTER31]  = { "mhpmcounter31",  mctr,   read_zero },
 
     [CSR_MHPMEVENT3]     = { "mhpmevent3",     any,    read_zero },
     [CSR_MHPMEVENT4]     = { "mhpmevent4",     any,    read_zero },