|
From: | Jason Chien |
Subject: | Re: [PATCH 3/6] target/riscv: Add support for Control Transfer Records extension CSRs. |
Date: | Tue, 4 Jun 2024 18:14:24 +0800 |
User-agent: | Mozilla Thunderbird |
missing sctrctl?This commit adds support for [m|s|vs]ctrcontrol, sctrstatus and sctrdepth CSRs handling. Signed-off-by: Rajnesh Kanwal <rkanwal@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) {
+ 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:
If smstateen is not implemented:
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;
}
In riscv_csrrw_check(), an virtual-instruction exception is always reported no matter what. Do we need this check?+ +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);
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.+ } + + 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;
The "depth" on the right side may exceed SCTRDEPTH_MAX.+ } + + 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;
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 & (~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;
When V=1, vsctrctl substitutes for sctrctl.+ } + + 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);
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.+ 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;
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?+ } + + env->vsctrctl = (env->vsctrctl & ~mask) | (new_val & mask); + + return RISCV_EXCP_NONE; +}
I think this can be one line.+ 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 },
same here+ [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 },
/* Performance Counters */ [CSR_HPMCOUNTER3] = { "hpmcounter3", ctr, read_hpmcounter }, [CSR_HPMCOUNTER4] = { "hpmcounter4", ctr, read_hpmcounter },
[Prev in Thread] | Current Thread | [Next in Thread] |