[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [ RFC v2 1/9] target/riscv: Fix PMU CSR predicate function
From: |
Atish Patra |
Subject: |
Re: [ RFC v2 1/9] target/riscv: Fix PMU CSR predicate function |
Date: |
Thu, 16 Sep 2021 11:39:11 -0700 |
On Wed, Sep 15, 2021 at 7:51 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Sep 10, 2021 at 4:27 AM Atish Patra <atish.patra@wdc.com> wrote:
> >
> > Currently, the predicate function for PMU related CSRs only works if
> > virtualization is enabled. Ideally, they should check the mcountern
> > bits before cycle/minstret/hpmcounterx access. The predicate function
> > also calculates the counter index incorrectly for hpmcounterx.
> >
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > ---
> > target/riscv/csr.c | 62 +++++++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 58 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 9a4ed18ac597..0515d851b948 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, HCOUNTEREN_CY)) {
> > + return RISCV_EXCP_ILLEGAL_INST;
> > + }
> > + break;
> > + case CSR_TIME:
> > + if (!get_field(env->mcounteren, HCOUNTEREN_TM)) {
> > + return RISCV_EXCP_ILLEGAL_INST;
> > + }
> > + break;
> > + case CSR_INSTRET:
> > + if (!get_field(env->mcounteren, HCOUNTEREN_IR)) {
> > + return RISCV_EXCP_ILLEGAL_INST;
> > + }
> > + break;
> > + case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> > + ctr_index = csrno - CSR_HPMCOUNTER3 + 3;
> > + if (!get_field(env->mcounteren, 1 << ctr_index)) {
> > + return RISCV_EXCP_ILLEGAL_INST;
> > + }
> > + break;
> > + }
> > + if (riscv_cpu_is_32bit(env)) {
> > + switch (csrno) {
> > + case CSR_CYCLEH:
> > + if (!get_field(env->mcounteren, HCOUNTEREN_CY)) {
> > + return RISCV_EXCP_ILLEGAL_INST;
> > + }
> > + break;
> > + case CSR_TIMEH:
> > + if (!get_field(env->mcounteren, HCOUNTEREN_TM)) {
> > + return RISCV_EXCP_ILLEGAL_INST;
> > + }
> > + break;
> > + case CSR_INSTRETH:
> > + if (!get_field(env->mcounteren, HCOUNTEREN_IR)) {
> > + return RISCV_EXCP_ILLEGAL_INST;
> > + }
> > + break;
> > + case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> > + ctr_index = csrno - CSR_HPMCOUNTER3H + 3;
> > + if (!get_field(env->mcounteren, 1 << ctr_index)) {
> > + return RISCV_EXCP_ILLEGAL_INST;
> > + }
> > + break;
> > + }
> > + }
> > + }
> > +
> > if (riscv_cpu_virt_enabled(env)) {
> > switch (csrno) {
> > case CSR_CYCLE:
> > @@ -89,8 +141,9 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > }
> > break;
> > case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> > - if (!get_field(env->hcounteren, 1 << (csrno -
> > CSR_HPMCOUNTER3)) &&
> > - get_field(env->mcounteren, 1 << (csrno -
> > CSR_HPMCOUNTER3))) {
> > + ctr_index = csrno - CSR_HPMCOUNTER3 + 3;
>
> ctr_index = csrno - CSR_CYCLE;
Will update it.
>
> > + if (!get_field(env->hcounteren, 1 << ctr_index) &&
> > + get_field(env->mcounteren, 1 << ctr_index)) {
>
> This fix (along with the H part below) should be put in a separate patch.
Sure.
>
> > return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > }
> > break;
> > @@ -116,8 +169,9 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > }
> > break;
> > case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> > - if (!get_field(env->hcounteren, 1 << (csrno -
> > CSR_HPMCOUNTER3H)) &&
> > - get_field(env->mcounteren, 1 << (csrno -
> > CSR_HPMCOUNTER3H))) {
> > + ctr_index = csrno - CSR_HPMCOUNTER3H + 3;
>
> ctr_index = csrno - CSR_CYCLEH;
>
> > + if (!get_field(env->hcounteren, 1 << ctr_index) &&
> > + get_field(env->mcounteren, 1 << ctr_index)) {
> > return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > }
> > break;
> > --
>
> You may need to rebase the patch on:
> 20210915084601.24304-1-bmeng.cn@gmail.com/">http://patchwork.ozlabs.org/project/qemu-devel/patch/20210915084601.24304-1-bmeng.cn@gmail.com/
>
Sure.
> Regards,
> Bin
>
--
Regards,
Atish
- [ RFC v2 0/9] Improve PMU support, Atish Patra, 2021/09/09
- [ RFC v2 1/9] target/riscv: Fix PMU CSR predicate function, Atish Patra, 2021/09/09
- [ RFC v2 8/9] target/riscv: Add few cache related PMU events, Atish Patra, 2021/09/09
- [ RFC v2 2/9] target/riscv: pmu: Rename the counters extension to pmu, Atish Patra, 2021/09/09
- [ RFC v2 6/9] target/riscv: Support mcycle/minstret write operation, Atish Patra, 2021/09/09
- [ RFC v2 9/9] hw/riscv: virt: Add PMU DT node to the device tree, Atish Patra, 2021/09/09
- [ RFC v2 3/9] target/riscv: pmu: Make number of counters configurable, Atish Patra, 2021/09/09
- [ RFC v2 4/9] target/riscv: Implement mcountinhibit CSR, Atish Patra, 2021/09/09