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

[Weiwei Li]

在 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