It makes sense. Thank you for the explanation.
Rajnesh Kanwal <rkan...@rivosinc.com> 於 2024年6月10日 週一 下午10:12寫道：
>
> Thanks Jason for your review.
>
> On Tue, Jun 4, 2024 at 11:14 AM Jason Chien <jason.ch...@sifive.com>
> wrote:
> >
> >
> > Rajnesh Kanwal 於 2024/5/30 上午 12:09 寫道:
> >
> > This commit adds support for [m|s|vs]ctrcontrol, sctrstatus and
> > sctrdepth CSRs handling.
> >
> > Signed-off-by: Rajnesh Kanwal <rkan...@rivosinc.com>
> > ---
> > target/riscv/cpu.h | 5 ++
> > target/riscv/cpu_cfg.h | 2 +
> > target/riscv/csr.c | 159 +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 166 insertions(+)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index a185e2d494..3d4d5172b8 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -263,6 +263,11 @@ struct CPUArchState {
> > target_ulong mcause;
> > target_ulong mtval; /* since: priv-1.10.0 */
> >
> > + uint64_t mctrctl;
> > + uint32_t sctrdepth;
> > + uint32_t sctrstatus;
> > + uint64_t vsctrctl;
> > +
> > /* Machine and Supervisor interrupt priorities */
> > uint8_t miprio[64];
> > uint8_t siprio[64];
> > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> > index d9354dc80a..d329a65811 100644
> > --- a/target/riscv/cpu_cfg.h
> > +++ b/target/riscv/cpu_cfg.h
> > @@ -123,6 +123,8 @@ struct RISCVCPUConfig {
> > bool ext_zvfhmin;
> > bool ext_smaia;
> > bool ext_ssaia;
> > + bool ext_smctr;
> > + bool ext_ssctr;
> > bool ext_sscofpmf;
> > bool ext_smepmp;
> > bool rvv_ta_all_1s;
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 2f92e4b717..888084d8e5 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -621,6 +621,61 @@ static RISCVException pointer_masking(CPURISCVState
> *env, int csrno)
> > return RISCV_EXCP_ILLEGAL_INST;
> > }
> >
> > +/*
> > + * M-mode:
> > + * Without ext_smctr raise illegal inst excep.
> > + * Otherwise everything is accessible to m-mode.
> > + *
> > + * S-mode:
> > + * Without ext_ssctr or mstateen.ctr raise illegal inst excep.
> > + * Otherwise everything other than mctrctl is accessible.
> > + *
> > + * VS-mode:
> > + * Without ext_ssctr or mstateen.ctr raise illegal inst excep.
> > + * Without hstateen.ctr raise virtual illegal inst excep.
> > + * Otherwise allow vsctrctl, sctrstatus, 0x200-0x2ff entry range.
> > + * Always raise illegal instruction exception for sctrdepth.
> > + */
> > +static RISCVException ctr_mmode(CPURISCVState *env, int csrno)
> > +{
> > + /* Check if smctr-ext is present */
> > + if (riscv_cpu_cfg(env)->ext_smctr) {
> > + return RISCV_EXCP_NONE;
> > + }
> > +
> > + return RISCV_EXCP_ILLEGAL_INST;
> > +}
> > +
> > +static RISCVException ctr_smode(CPURISCVState *env, int csrno)
> > +{
> > + if ((env->priv == PRV_M && riscv_cpu_cfg(env)->ext_smctr) ||
> > + (env->priv == PRV_S && !env->virt_enabled &&
> > + riscv_cpu_cfg(env)->ext_ssctr)) {
> > + return smstateen_acc_ok(env, 0, SMSTATEEN0_CTR);
> > + }
> > +
> > + if (env->priv == PRV_S && env->virt_enabled &&
> > + riscv_cpu_cfg(env)->ext_ssctr) {
> > + if (csrno == CSR_SCTRSTATUS) {
> >
> > missing sctrctl?
> >
> > + return smstateen_acc_ok(env, 0, SMSTATEEN0_CTR);
> > + }
> > +
> > + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > + }
> > +
> > + return RISCV_EXCP_ILLEGAL_INST;
> > +}
> >
> > I think there is no need to bind M-mode with ext_smctr, S-mode with
> ext_ssctr and VS-mode with ext_ssctr, since this predicate function is for
> S-mode CSRs, which are defined in both smctr and ssctr, we just need to
> check at least one of ext_ssctr or ext_smctr is true.
> >
> > The spec states that:
> > Attempts to access sctrdepth from VS-mode or VU-mode raise a
> virtual-instruction exception, unless CTR state enable access restrictions
> apply.
> >
> > In my understanding, we should check the presence of smstateen extension
> first, and
> >
> > if smstateen is implemented:
> >
> > for sctrctl and sctrstatus, call smstateen_acc_ok()
> > for sctrdepth, call smstateen_acc_ok(), and if there is any exception
> returned, always report virtual-instruction exception.
>
> For sctrdepth, we are supposed to always return a virt-inst exception in
> case of
> VS-VU mode unless CTR state enable access restrictions apply.
>
> So for sctrdepth, call smstateen_acc_ok(), and if there is no exception
> returned
> (mstateen.CTR=1 and hstateen.CTR=1 for virt mode), check if we are in
> virtual
> mode and return virtual-instruction exception otherwise return
> RISCV_EXCP_NONE.
> Note that if hstateen.CTR=0, smstateen_acc_ok() will return
> virtual-instruction
> exception which means regardless of the hstateen.CTR state, we will always
> return virtual-instruction exception for VS/VU mode access to sctrdepth.
>
> Basically this covers following rules for sctrdepth:
>
> if mstateen.ctr == 0
> return RISCV_EXCP_ILLEGAL_INST; // For all modes lower than M-mode.
> else if in virt-mode // regardless of the state of hstateen.CTR
> return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> else
> return RISCV_EXCP_NONE
>
> >
> > If smstateen is not implemented:
> >
> > for sctrctl and sctrstatus, there is no check.
> > for sctrdepth, I think the spec is ambiguous. What does "CTR state
> enable access restrictions apply" mean when smstateen is not implemented?
>
> As per my understanding, this means if mstateen.CTR=0 then we return an
> illegal instruction exception regardless if it's virtual mode or not. This
> is
> the only effect of CTR state enable on sctrdepth CSR. If mstateen.CTR=1,
> sctrdepth access from VS-mode results in virtual-instruction exception
> regardless of hstateen.CTR.
>
> Based on this, we have following model for predicate checks:
>
> if smstateen is implemented:
>
> for sctrctl and sctrstatus, call smstateen_acc_ok()
> for sctrdepth, call smstateen_acc_ok(), and if there is no exception,
> check if we are in virtual mode and return virtual-instruction
> exception
> otherwise return RISCV_EXCP_NONE.
>
> If smstateen is not implemented:
>
> for sctrctl and sctrstatus, there is no check.
> for sctrdepth, if in VS/VU mode return virtual-instruction exception
> otherwise
> no check.
>
> Here is the code to better understand this.
>
> static RISCVException ctr_smode(CPURISCVState *env, int csrno)
> {
> const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
>
> if (!cfg->ext_ssctr && !cfg->ext_smctr) {
> return RISCV_EXCP_ILLEGAL_INST;
> }
>
> if (riscv_cpu_cfg(env)->ext_smstateen) {
> RISCVException ret = smstateen_acc_ok(env, 0, SMSTATEEN0_CTR);
> if (ret == RISCV_EXCP_NONE && csrno == CSR_SCTRDEPTH &&
> env->virt_enabled) {
> return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> }
> return ret;
> } else {
> if (csrno == CSR_SCTRDEPTH && env->virt_enabled) {
> return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> }
> }
>
> return RISCV_EXCP_NONE;
> }
>
> Given smstateen_acc_ok() returns RISCV_EXCP_NONE in case if ext_smstateen
> is not
> implemented, this can be further simplified to:
>
> static RISCVException ctr_smode(CPURISCVState *env, int csrno)
> {
> const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
>
> if (!cfg->ext_ssctr && !cfg->ext_smctr) {
> return RISCV_EXCP_ILLEGAL_INST;
> }
>
> RISCVException ret = smstateen_acc_ok(env, 0, SMSTATEEN0_CTR);
> if (ret == RISCV_EXCP_NONE && csrno == CSR_SCTRDEPTH &&
> env->virt_enabled) {
> return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> }
> return ret;
> }
>
> >
> > Here is the code to better understand my description.
> >
> > static RISCVException ctr_smode(CPURISCVState *env, int csrno)
> > {
> > const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
> >
> > if (!cfg->ext_ssctr && !cfg->ext_smctr) {
> > return RISCV_EXCP_ILLEGAL_INST;
> > }
> >
> > if (riscv_cpu_cfg(env)->ext_smstateen) {
> > RISCVException ret = smstateen_acc_ok(env, 0, SMSTATEEN0_CTR);
> > if (ret != RISCV_EXCP_NONE) {
> > if (csrno == CSR_SCTRDEPTH && env->virt_enabled) {
> > return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > }
> >
> > return ret;
> > }
> > } else {
> > /* The spec is ambiguous. */
> > if (csrno == CSR_SCTRDEPTH && env->virt_enabled) {
> > return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > }
> > }
> >
> > return RISCV_EXCP_NONE;
> > }
> >
> > +
> > +static RISCVException ctr_vsmode(CPURISCVState *env, int csrno)
> > +{
> > + if (env->priv == PRV_S && env->virt_enabled &&
> > + riscv_cpu_cfg(env)->ext_ssctr) {
> > + return smstateen_acc_ok(env, 0, SMSTATEEN0_CTR);
> >
> > In riscv_csrrw_check(), an virtual-instruction exception is always
> reported no matter what. Do we need this check?
> >
> > + }
> > +
> > + return ctr_smode(env, csrno);
> > +}
> > +
> > static RISCVException aia_hmode(CPURISCVState *env, int csrno)
> > {
> > int ret;
> > @@ -3835,6 +3890,100 @@ static RISCVException write_satp(CPURISCVState
> *env, int csrno,
> > return RISCV_EXCP_NONE;
> > }
> >
> > +static RISCVException rmw_sctrdepth(CPURISCVState *env, int csrno,
> > + target_ulong *ret_val,
> > + target_ulong new_val, target_ulong
> wr_mask)
> > +{
> > + uint64_t mask = wr_mask & SCTRDEPTH_MASK;
> > +
> > + if (ret_val) {
> > + *ret_val = env->sctrdepth & SCTRDEPTH_MASK;
> >
> > We don't need to do bitwise and with SCTRDEPTH_MASK on read accesses
> when we always do bitwise and with SCTRDEPTH_MASK on write accesses.
> >
> > + }
> > +
> > + env->sctrdepth = (env->sctrdepth & ~mask) | (new_val & mask);
> > +
> > + /* Correct depth. */
> > + if (wr_mask & SCTRDEPTH_MASK) {
> > + uint64_t depth = get_field(env->sctrdepth, SCTRDEPTH_MASK);
> > +
> > + if (depth > SCTRDEPTH_MAX) {
> > + env->sctrdepth =
> > + set_field(env->sctrdepth, SCTRDEPTH_MASK,
> SCTRDEPTH_MAX);
> > + }
> > +
> > + /* Update sctrstatus.WRPTR with a legal value */
> > + depth = 16 << depth;
> >
> > The "depth" on the right side may exceed SCTRDEPTH_MAX.
> >
> > + env->sctrstatus =
> > + env->sctrstatus & (~SCTRSTATUS_WRPTR_MASK | (depth - 1));
> > + }
> > +
> > + return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException rmw_mctrctl(CPURISCVState *env, int csrno,
> > + target_ulong *ret_val,
> > + target_ulong new_val, target_ulong
> wr_mask)
> > +{
> > + uint64_t mask = wr_mask & MCTRCTL_MASK;
> > +
> > + if (ret_val) {
> > + *ret_val = env->mctrctl & MCTRCTL_MASK;
> >
> > There is no need to do bitwise and with the mask on read accesses when
> we always do bitwise and with the mask on write accesses.
> >
> > + }
> > +
> > + env->mctrctl = (env->mctrctl & ~mask) | (new_val & mask);
> > +
> > + return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException rmw_sctrctl(CPURISCVState *env, int csrno,
> > + target_ulong *ret_val,
> > + target_ulong new_val, target_ulong
> wr_mask)
> > +{
> > + uint64_t mask = wr_mask & SCTRCTL_MASK;
> > + RISCVException ret;
> > +
> > + ret = rmw_mctrctl(env, csrno, ret_val, new_val, mask);
> >
> > When V=1, vsctrctl substitutes for sctrctl.
> >
> > + if (ret_val) {
> > + *ret_val &= SCTRCTL_MASK;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static RISCVException rmw_sctrstatus(CPURISCVState *env, int csrno,
> > + target_ulong *ret_val,
> > + target_ulong new_val, target_ulong
> wr_mask)
> > +{
> > + uint32_t depth = 16 << get_field(env->sctrdepth, SCTRDEPTH_MASK);
> > + uint32_t mask = wr_mask & SCTRSTATUS_MASK;
> > +
> > + if (ret_val) {
> > + *ret_val = env->sctrstatus & SCTRSTATUS_MASK;
> >
> > There is no need to do bitwise and with the mask on read accesses when
> we always do bitwise and with the mask on write accesses.
> >
> > + }
> > +
> > + env->sctrstatus = (env->sctrstatus & ~mask) | (new_val & mask);
> > +
> > + /* Update sctrstatus.WRPTR with a legal value */
> > + env->sctrstatus = env->sctrstatus & (~SCTRSTATUS_WRPTR_MASK |
> (depth - 1));
> > +
> > + return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException rmw_vsctrctl(CPURISCVState *env, int csrno,
> > + target_ulong *ret_val,
> > + target_ulong new_val, target_ulong
> wr_mask)
> > +{
> > + uint64_t mask = wr_mask & VSCTRCTL_MASK;
> > +
> > + if (ret_val) {
> > + *ret_val = env->vsctrctl & VSCTRCTL_MASK;
> >
> > There is no need to do bitwise and with the mask on read accesses when
> we always do bitwise and with the mask on write accesses.
> >
> > + }
> > +
> > + env->vsctrctl = (env->vsctrctl & ~mask) | (new_val & mask);
> > +
> > + return RISCV_EXCP_NONE;
> > +}
> >
> > Is it possible to define rmw_xctrctl() instead of three individual rmw
> functions and use a switch case to select the mask and the CSR for the
> purpose of reducing code size?
> >
> > +
> > static RISCVException read_vstopi(CPURISCVState *env, int csrno,
> > target_ulong *val)
> > {
> > @@ -5771,6 +5920,16 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> > [CSR_SPMBASE] = { "spmbase", pointer_masking, read_spmbase,
> > write_spmbase
> },
> >
> > + [CSR_MCTRCTL] = { "mctrctl", ctr_mmode, NULL, NULL,
> > + rmw_mctrctl },
> >
> > I think this can be one line.
> >
> > + [CSR_SCTRCTL] = { "sctrctl", ctr_smode, NULL, NULL,
> > + rmw_sctrctl },
> >
> > same here
> >
> > + [CSR_SCTRDEPTH] = { "sctrdepth", ctr_smode, NULL, NULL,
> > + rmw_sctrdepth },
> >
> > same here
> >
> > + [CSR_SCTRSTATUS] = { "sctrstatus", ctr_smode, NULL,
> NULL,
> > + rmw_sctrstatus },
> >
> > same here
> >
> > + [CSR_VSCTRCTL] = { "vsctrctl", ctr_vsmode, NULL, NULL,
> > + rmw_vsctrctl },
> >
> > same here
> >
> > /* Performance Counters */
> > [CSR_HPMCOUNTER3] = { "hpmcounter3", ctr, read_hpmcounter
> },
> > [CSR_HPMCOUNTER4] = { "hpmcounter4", ctr, read_hpmcounter
> },
>
