| Message ID | 20230209062404.3582018-4-debug@rivosinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,RFC,Zisslpcfi,1/9] target/riscv: adding zimops and zisslpcfi extension to RISCV cpu config | expand |
On 2023/2/9 14:23, Deepak Gupta wrote: > CSR_SSP and CSR_LPLR are new CSR additions to cpu/hart. This patch allows > access to these CSRs. A predicate routine handles access to these CSR as > per specification. > > This patch also implments new bit definitions in menvcfg/henvcfg/mstatus/ > sstatus CSRs to master enabled cfi and enable forward cfi in S and M mode. > mstatus CSR holds forward and backward cfi enabling for U mode. > > There is no enabling bit for backward cfi in S and M mode. It is always > enabled if extension is implemented by CPU. > > Signed-off-by: Deepak Gupta <debug@rivosinc.com> > Signed-off-by: Kip Walker <kip@rivosinc.com> > --- > target/riscv/csr.c | 137 ++++++++++++++++++++++++++++++++++++++++++++- > target/riscv/pmp.c | 9 +++ > 2 files changed, 145 insertions(+), 1 deletion(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index 0db2c233e5..24e208ebed 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -163,6 +163,50 @@ static RISCVException ctr32(CPURISCVState *env, int csrno) > return ctr(env, csrno); > } > > +static RISCVException cfi(CPURISCVState *env, int csrno) > +{ > + /* no cfi extension */ > + if (!env_archcpu(env)->cfg.ext_cfi) { > + return RISCV_EXCP_ILLEGAL_INST; > + } > + /* > + * CONFIG_USER_MODE always allow access for now. Better for user mode only > + * functionality > + */ > +#if !defined(CONFIG_USER_ONLY) > + /* current priv not M */ > + if (env->priv != PRV_M) { > + /* menvcfg says no CFI */ > + if (!get_field(env->menvcfg, MENVCFG_CFI)) { > + return RISCV_EXCP_ILLEGAL_INST; > + } > + > + /* V = 1 and henvcfg says no CFI. raise virtual instr fault */ > + if (riscv_cpu_virt_enabled(env) && > + !get_field(env->henvcfg, HENVCFG_CFI)) { > + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > + } > + > + /* > + * LPLR and SSP are not accessible to U mode if disabled via status > + * CSR > + */ > + if (env->priv == PRV_U) { > + if (csrno == CSR_LPLR && > + !get_field(env->mstatus, MSTATUS_UFCFIEN)) { > + return RISCV_EXCP_ILLEGAL_INST; > + } > + if (csrno == CSR_SSP && > + !get_field(env->mstatus, MSTATUS_UBCFIEN)) { > + return RISCV_EXCP_ILLEGAL_INST; > + } > + } > + } > +#endif > + > + return RISCV_EXCP_NONE; > +} > + > #if !defined(CONFIG_USER_ONLY) > static RISCVException mctr(CPURISCVState *env, int csrno) > { > @@ -485,6 +529,32 @@ static RISCVException seed(CPURISCVState *env, int csrno) > #endif > } > > +/* Zisslpcfi CSR_LPLR read/write */ > +static int read_lplr(CPURISCVState *env, int csrno, target_ulong *val) > +{ > + *val = env->lplr; > + return RISCV_EXCP_NONE; > +} > + > +static int write_lplr(CPURISCVState *env, int csrno, target_ulong val) > +{ > + env->lplr = val & (LPLR_UL | LPLR_ML | LPLR_LL); > + return RISCV_EXCP_NONE; > +} > + > +/* Zisslpcfi CSR_SSP read/write */ > +static int read_ssp(CPURISCVState *env, int csrno, target_ulong *val) > +{ > + *val = env->ssp; > + return RISCV_EXCP_NONE; > +} > + > +static int write_ssp(CPURISCVState *env, int csrno, target_ulong val) > +{ > + env->ssp = val; > + return RISCV_EXCP_NONE; > +} > + > /* User Floating-Point CSRs */ > static RISCVException read_fflags(CPURISCVState *env, int csrno, > target_ulong *val) > @@ -1227,7 +1297,7 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, > > /* flush tlb on mstatus fields that affect VM */ > if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV | > - MSTATUS_MPRV | MSTATUS_SUM)) { > + MSTATUS_MPRV | MSTATUS_SUM | MSTATUS_UFCFIEN | MSTATUS_UBCFIEN)) { These two fields should be guarded by the check of ext_cfi. And MSTATUS_UBCFIEN field change don't need flush tlb. I didn't get why we should flush tlb for forward cfi. For background, there are some enhancement for the PTE and PMP, we may need do some memory adjustments. But forward cfi just adds some instructions. Why we should flush tlb? Does the tlb can't be used any more? > tlb_flush(env_cpu(env)); > } > mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE | > @@ -1250,6 +1320,11 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, > } > } > > + /* If cfi extension is available, then apply cfi status mask */ > + if (env_archcpu(env)->cfg.ext_cfi) { > + mask |= CFISTATUS_M_MASK; > + } > + > mstatus = (mstatus & ~mask) | (val & mask); > > if (xl > MXL_RV32) { > @@ -1880,9 +1955,17 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno, > target_ulong val) > { > uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE | MENVCFG_CBZE; > + uint64_t cfi_mask = MENVCFG_CFI | MENVCFG_SFCFIEN; > > if (riscv_cpu_mxl(env) == MXL_RV64) { > mask |= MENVCFG_PBMTE | MENVCFG_STCE; > + if (env_archcpu(env)->cfg.ext_cfi) { > + mask |= cfi_mask; > + /* If any cfi enabling bit changes in menvcfg, flush tlb */ > + if ((val ^ env->menvcfg) & cfi_mask) { > + tlb_flush(env_cpu(env)); Don't flush tlb for MENVCFG_SFCFIE field changes. > + } > + } > } > env->menvcfg = (env->menvcfg & ~mask) | (val & mask); > > @@ -1900,8 +1983,17 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno, > target_ulong val) > { > uint64_t mask = MENVCFG_PBMTE | MENVCFG_STCE; > + uint64_t cfi_mask = MENVCFG_CFI | MENVCFG_SFCFIEN; MENVCFG_SFCFIE > uint64_t valh = (uint64_t)val << 32; > > + if (env_archcpu(env)->cfg.ext_cfi) { > + mask |= cfi_mask; > + /* If any cfi enabling bit changes in menvcfg, flush tlb */ > + if ((val ^ env->menvcfg) & cfi_mask) { > + tlb_flush(env_cpu(env)); > + } If SFCFIE field change, we should not flush the tlb. > + } > + > env->menvcfg = (env->menvcfg & ~mask) | (valh & mask); > > return RISCV_EXCP_NONE; > @@ -1954,6 +2046,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno, > target_ulong val) > { > uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE; > + uint64_t cfi_mask = HENVCFG_CFI | HENVCFG_SFCFIEN; HENVCFG_SFCFIE > RISCVException ret; > > ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG); > @@ -1963,6 +2056,18 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno, > > if (riscv_cpu_mxl(env) == MXL_RV64) { > mask |= HENVCFG_PBMTE | HENVCFG_STCE; > + /* > + * If cfi available and menvcfg.CFI = 1, then apply cfi mask for > + * henvcfg > + */ > + if (env_archcpu(env)->cfg.ext_cfi && > + get_field(env->menvcfg, MENVCFG_CFI)) { > + mask |= cfi_mask; > + /* If any cfi enabling bit changes in henvcfg, flush tlb */ > + if ((val ^ env->henvcfg) & cfi_mask) { > + tlb_flush(env_cpu(env)); > + } If SFCFIE field change, we should not flush the tlb. > + } > } > > env->henvcfg = (env->henvcfg & ~mask) | (val & mask); > @@ -1988,9 +2093,19 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno, > target_ulong val) > { > uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE; > + uint64_t cfi_mask = HENVCFG_CFI | HENVCFG_SFCFIEN; > uint64_t valh = (uint64_t)val << 32; > RISCVException ret; > > + if (env_archcpu(env)->cfg.ext_cfi && > + get_field(env->menvcfg, MENVCFG_CFI)) { > + mask |= cfi_mask; > + /* If any cfi enabling bit changes in henvcfg, flush tlb */ > + if ((val ^ env->henvcfg) & cfi_mask) { > + tlb_flush(env_cpu(env)); > + } > + } > + > ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG); > if (ret != RISCV_EXCP_NONE) { > return ret; > @@ -2270,6 +2385,11 @@ static RISCVException read_sstatus_i128(CPURISCVState *env, int csrno, > mask |= SSTATUS64_UXL; > } > > + if ((env_archcpu(env)->cfg.ext_cfi) && > + get_field(env->menvcfg, MENVCFG_CFI)) { > + mask |= CFISTATUS_S_MASK; > + } > + > *val = int128_make128(sstatus, add_status_sd(MXL_RV128, sstatus)); > return RISCV_EXCP_NONE; > } > @@ -2281,6 +2401,11 @@ static RISCVException read_sstatus(CPURISCVState *env, int csrno, > if (env->xl != MXL_RV32 || env->debugger) { > mask |= SSTATUS64_UXL; > } > + > + if ((env_archcpu(env)->cfg.ext_cfi) && > + get_field(env->menvcfg, MENVCFG_CFI)) { > + mask |= CFISTATUS_S_MASK; > + } > /* TODO: Use SXL not MXL. */ > *val = add_status_sd(riscv_cpu_mxl(env), env->mstatus & mask); > return RISCV_EXCP_NONE; > @@ -2296,6 +2421,12 @@ static RISCVException write_sstatus(CPURISCVState *env, int csrno, > mask |= SSTATUS64_UXL; > } > } > + > + /* If cfi available and menvcfg.CFI = 1, apply CFI mask for sstatus */ > + if ((env_archcpu(env)->cfg.ext_cfi) && > + get_field(env->menvcfg, MENVCFG_CFI)) { > + mask |= CFISTATUS_S_MASK; > + } > target_ulong newval = (env->mstatus & ~mask) | (val & mask); > return write_mstatus(env, CSR_MSTATUS, newval); > } > @@ -4001,6 +4132,10 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = { > /* Crypto Extension */ > [CSR_SEED] = { "seed", seed, NULL, NULL, rmw_seed }, > > + /* User mode CFI CSR */ > + [CSR_LPLR] = { "lplr", cfi, read_lplr, write_lplr }, > + [CSR_SSP] = { "ssp", cfi, read_ssp, write_ssp }, > + > #if !defined(CONFIG_USER_ONLY) > /* Machine Timers and Counters */ > [CSR_MCYCLE] = { "mcycle", any, read_hpmcounter, > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c > index d1126a6066..89745d46cd 100644 > --- a/target/riscv/pmp.c > +++ b/target/riscv/pmp.c > @@ -579,6 +579,15 @@ void mseccfg_csr_write(CPURISCVState *env, target_ulong val) > /* Sticky bits */ > val |= (env->mseccfg & (MSECCFG_MMWP | MSECCFG_MML)); > > + /* M-mode forward cfi to be enabled if cfi extension is implemented */ > + if (env_archcpu(env)->cfg.ext_cfi) { > + val |= (val & MSECCFG_MFCFIEN); This statement does nothing. Is it a typo? > + /* If forward cfi in mseccfg is being toggled, flush tlb */ > + if ((env->mseccfg ^ val) & MSECCFG_MFCFIEN) { > + tlb_flush(env_cpu(env)); > + } No need flush tlb. Zhiwei > + } > + > env->mseccfg = val; > } >
I don't find the modification for read_mstatus. Zhiwei On 2023/2/15 13:47, LIU Zhiwei wrote: > > On 2023/2/9 14:23, Deepak Gupta wrote: >> CSR_SSP and CSR_LPLR are new CSR additions to cpu/hart. This patch >> allows >> access to these CSRs. A predicate routine handles access to these CSR as >> per specification. >> >> This patch also implments new bit definitions in >> menvcfg/henvcfg/mstatus/ >> sstatus CSRs to master enabled cfi and enable forward cfi in S and M >> mode. >> mstatus CSR holds forward and backward cfi enabling for U mode. >> >> There is no enabling bit for backward cfi in S and M mode. It is always >> enabled if extension is implemented by CPU. >> >> Signed-off-by: Deepak Gupta <debug@rivosinc.com> >> Signed-off-by: Kip Walker <kip@rivosinc.com> >> --- >> target/riscv/csr.c | 137 ++++++++++++++++++++++++++++++++++++++++++++- >> target/riscv/pmp.c | 9 +++ >> 2 files changed, 145 insertions(+), 1 deletion(-) >> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >> index 0db2c233e5..24e208ebed 100644 >> --- a/target/riscv/csr.c >> +++ b/target/riscv/csr.c >> @@ -163,6 +163,50 @@ static RISCVException ctr32(CPURISCVState *env, >> int csrno) >> return ctr(env, csrno); >> } >> +static RISCVException cfi(CPURISCVState *env, int csrno) >> +{ >> + /* no cfi extension */ >> + if (!env_archcpu(env)->cfg.ext_cfi) { >> + return RISCV_EXCP_ILLEGAL_INST; >> + } >> + /* >> + * CONFIG_USER_MODE always allow access for now. Better for user >> mode only >> + * functionality >> + */ >> +#if !defined(CONFIG_USER_ONLY) >> + /* current priv not M */ >> + if (env->priv != PRV_M) { >> + /* menvcfg says no CFI */ >> + if (!get_field(env->menvcfg, MENVCFG_CFI)) { >> + return RISCV_EXCP_ILLEGAL_INST; >> + } >> + >> + /* V = 1 and henvcfg says no CFI. raise virtual instr fault */ >> + if (riscv_cpu_virt_enabled(env) && >> + !get_field(env->henvcfg, HENVCFG_CFI)) { >> + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; >> + } >> + >> + /* >> + * LPLR and SSP are not accessible to U mode if disabled via >> status >> + * CSR >> + */ >> + if (env->priv == PRV_U) { >> + if (csrno == CSR_LPLR && >> + !get_field(env->mstatus, MSTATUS_UFCFIEN)) { >> + return RISCV_EXCP_ILLEGAL_INST; >> + } >> + if (csrno == CSR_SSP && >> + !get_field(env->mstatus, MSTATUS_UBCFIEN)) { >> + return RISCV_EXCP_ILLEGAL_INST; >> + } >> + } >> + } >> +#endif >> + >> + return RISCV_EXCP_NONE; >> +} >> + >> #if !defined(CONFIG_USER_ONLY) >> static RISCVException mctr(CPURISCVState *env, int csrno) >> { >> @@ -485,6 +529,32 @@ static RISCVException seed(CPURISCVState *env, >> int csrno) >> #endif >> } >> +/* Zisslpcfi CSR_LPLR read/write */ >> +static int read_lplr(CPURISCVState *env, int csrno, target_ulong *val) >> +{ >> + *val = env->lplr; >> + return RISCV_EXCP_NONE; >> +} >> + >> +static int write_lplr(CPURISCVState *env, int csrno, target_ulong val) >> +{ >> + env->lplr = val & (LPLR_UL | LPLR_ML | LPLR_LL); >> + return RISCV_EXCP_NONE; >> +} >> + >> +/* Zisslpcfi CSR_SSP read/write */ >> +static int read_ssp(CPURISCVState *env, int csrno, target_ulong *val) >> +{ >> + *val = env->ssp; >> + return RISCV_EXCP_NONE; >> +} >> + >> +static int write_ssp(CPURISCVState *env, int csrno, target_ulong val) >> +{ >> + env->ssp = val; >> + return RISCV_EXCP_NONE; >> +} >> + >> /* User Floating-Point CSRs */ >> static RISCVException read_fflags(CPURISCVState *env, int csrno, >> target_ulong *val) >> @@ -1227,7 +1297,7 @@ static RISCVException >> write_mstatus(CPURISCVState *env, int csrno, >> /* flush tlb on mstatus fields that affect VM */ >> if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV | >> - MSTATUS_MPRV | MSTATUS_SUM)) { >> + MSTATUS_MPRV | MSTATUS_SUM | MSTATUS_UFCFIEN | >> MSTATUS_UBCFIEN)) { > > These two fields should be guarded by the check of ext_cfi. > > And MSTATUS_UBCFIEN field change don't need flush tlb. > > I didn't get why we should flush tlb for forward cfi. For background, > there are some enhancement for the PTE and PMP, we may need do some > memory adjustments. But forward cfi just adds some instructions. Why > we should flush tlb? Does the tlb can't be used any more? > >> tlb_flush(env_cpu(env)); >> } >> mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE | >> @@ -1250,6 +1320,11 @@ static RISCVException >> write_mstatus(CPURISCVState *env, int csrno, >> } >> } >> + /* If cfi extension is available, then apply cfi status mask */ >> + if (env_archcpu(env)->cfg.ext_cfi) { >> + mask |= CFISTATUS_M_MASK; >> + } >> + >> mstatus = (mstatus & ~mask) | (val & mask); >> if (xl > MXL_RV32) { >> @@ -1880,9 +1955,17 @@ static RISCVException >> write_menvcfg(CPURISCVState *env, int csrno, >> target_ulong val) >> { >> uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE | >> MENVCFG_CBZE; >> + uint64_t cfi_mask = MENVCFG_CFI | MENVCFG_SFCFIEN; >> if (riscv_cpu_mxl(env) == MXL_RV64) { >> mask |= MENVCFG_PBMTE | MENVCFG_STCE; >> + if (env_archcpu(env)->cfg.ext_cfi) { >> + mask |= cfi_mask; >> + /* If any cfi enabling bit changes in menvcfg, flush tlb */ >> + if ((val ^ env->menvcfg) & cfi_mask) { >> + tlb_flush(env_cpu(env)); > Don't flush tlb for MENVCFG_SFCFIE field changes. >> + } >> + } >> } >> env->menvcfg = (env->menvcfg & ~mask) | (val & mask); >> @@ -1900,8 +1983,17 @@ static RISCVException >> write_menvcfgh(CPURISCVState *env, int csrno, >> target_ulong val) >> { >> uint64_t mask = MENVCFG_PBMTE | MENVCFG_STCE; >> + uint64_t cfi_mask = MENVCFG_CFI | MENVCFG_SFCFIEN; > MENVCFG_SFCFIE >> uint64_t valh = (uint64_t)val << 32; >> + if (env_archcpu(env)->cfg.ext_cfi) { >> + mask |= cfi_mask; >> + /* If any cfi enabling bit changes in menvcfg, flush tlb */ >> + if ((val ^ env->menvcfg) & cfi_mask) { >> + tlb_flush(env_cpu(env)); >> + } > If SFCFIE field change, we should not flush the tlb. >> + } >> + >> env->menvcfg = (env->menvcfg & ~mask) | (valh & mask); >> return RISCV_EXCP_NONE; >> @@ -1954,6 +2046,7 @@ static RISCVException >> write_henvcfg(CPURISCVState *env, int csrno, >> target_ulong val) >> { >> uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | >> HENVCFG_CBZE; >> + uint64_t cfi_mask = HENVCFG_CFI | HENVCFG_SFCFIEN; > > HENVCFG_SFCFIE > >> RISCVException ret; >> ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG); >> @@ -1963,6 +2056,18 @@ static RISCVException >> write_henvcfg(CPURISCVState *env, int csrno, >> if (riscv_cpu_mxl(env) == MXL_RV64) { >> mask |= HENVCFG_PBMTE | HENVCFG_STCE; >> + /* >> + * If cfi available and menvcfg.CFI = 1, then apply cfi mask >> for >> + * henvcfg >> + */ >> + if (env_archcpu(env)->cfg.ext_cfi && >> + get_field(env->menvcfg, MENVCFG_CFI)) { >> + mask |= cfi_mask; >> + /* If any cfi enabling bit changes in henvcfg, flush tlb */ >> + if ((val ^ env->henvcfg) & cfi_mask) { >> + tlb_flush(env_cpu(env)); >> + } > If SFCFIE field change, we should not flush the tlb. >> + } >> } >> env->henvcfg = (env->henvcfg & ~mask) | (val & mask); >> @@ -1988,9 +2093,19 @@ static RISCVException >> write_henvcfgh(CPURISCVState *env, int csrno, >> target_ulong val) >> { >> uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE; >> + uint64_t cfi_mask = HENVCFG_CFI | HENVCFG_SFCFIEN; >> uint64_t valh = (uint64_t)val << 32; >> RISCVException ret; >> + if (env_archcpu(env)->cfg.ext_cfi && >> + get_field(env->menvcfg, MENVCFG_CFI)) { >> + mask |= cfi_mask; >> + /* If any cfi enabling bit changes in henvcfg, flush tlb */ >> + if ((val ^ env->henvcfg) & cfi_mask) { >> + tlb_flush(env_cpu(env)); >> + } >> + } >> + >> ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG); >> if (ret != RISCV_EXCP_NONE) { >> return ret; >> @@ -2270,6 +2385,11 @@ static RISCVException >> read_sstatus_i128(CPURISCVState *env, int csrno, >> mask |= SSTATUS64_UXL; >> } >> + if ((env_archcpu(env)->cfg.ext_cfi) && >> + get_field(env->menvcfg, MENVCFG_CFI)) { >> + mask |= CFISTATUS_S_MASK; >> + } >> + >> *val = int128_make128(sstatus, add_status_sd(MXL_RV128, sstatus)); >> return RISCV_EXCP_NONE; >> } >> @@ -2281,6 +2401,11 @@ static RISCVException >> read_sstatus(CPURISCVState *env, int csrno, >> if (env->xl != MXL_RV32 || env->debugger) { >> mask |= SSTATUS64_UXL; >> } >> + >> + if ((env_archcpu(env)->cfg.ext_cfi) && >> + get_field(env->menvcfg, MENVCFG_CFI)) { >> + mask |= CFISTATUS_S_MASK; >> + } >> /* TODO: Use SXL not MXL. */ >> *val = add_status_sd(riscv_cpu_mxl(env), env->mstatus & mask); >> return RISCV_EXCP_NONE; >> @@ -2296,6 +2421,12 @@ static RISCVException >> write_sstatus(CPURISCVState *env, int csrno, >> mask |= SSTATUS64_UXL; >> } >> } >> + >> + /* If cfi available and menvcfg.CFI = 1, apply CFI mask for >> sstatus */ >> + if ((env_archcpu(env)->cfg.ext_cfi) && >> + get_field(env->menvcfg, MENVCFG_CFI)) { >> + mask |= CFISTATUS_S_MASK; >> + } >> target_ulong newval = (env->mstatus & ~mask) | (val & mask); >> return write_mstatus(env, CSR_MSTATUS, newval); >> } >> @@ -4001,6 +4132,10 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = { >> /* Crypto Extension */ >> [CSR_SEED] = { "seed", seed, NULL, NULL, rmw_seed }, >> + /* User mode CFI CSR */ >> + [CSR_LPLR] = { "lplr", cfi, read_lplr, write_lplr }, >> + [CSR_SSP] = { "ssp", cfi, read_ssp, write_ssp }, >> + >> #if !defined(CONFIG_USER_ONLY) >> /* Machine Timers and Counters */ >> [CSR_MCYCLE] = { "mcycle", any, read_hpmcounter, >> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c >> index d1126a6066..89745d46cd 100644 >> --- a/target/riscv/pmp.c >> +++ b/target/riscv/pmp.c >> @@ -579,6 +579,15 @@ void mseccfg_csr_write(CPURISCVState *env, >> target_ulong val) >> /* Sticky bits */ >> val |= (env->mseccfg & (MSECCFG_MMWP | MSECCFG_MML)); >> + /* M-mode forward cfi to be enabled if cfi extension is >> implemented */ >> + if (env_archcpu(env)->cfg.ext_cfi) { >> + val |= (val & MSECCFG_MFCFIEN); > This statement does nothing. Is it a typo? >> + /* If forward cfi in mseccfg is being toggled, flush tlb */ >> + if ((env->mseccfg ^ val) & MSECCFG_MFCFIEN) { >> + tlb_flush(env_cpu(env)); >> + } > > No need flush tlb. > > Zhiwei > >> + } >> + >> env->mseccfg = val; >> }
On Tue, Feb 14, 2023 at 9:47 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote: > > > On 2023/2/9 14:23, Deepak Gupta wrote: > > CSR_SSP and CSR_LPLR are new CSR additions to cpu/hart. This patch allows > > access to these CSRs. A predicate routine handles access to these CSR as > > per specification. > > > > This patch also implments new bit definitions in menvcfg/henvcfg/mstatus/ > > sstatus CSRs to master enabled cfi and enable forward cfi in S and M mode. > > mstatus CSR holds forward and backward cfi enabling for U mode. > > > > There is no enabling bit for backward cfi in S and M mode. It is always > > enabled if extension is implemented by CPU. > > > > Signed-off-by: Deepak Gupta <debug@rivosinc.com> > > Signed-off-by: Kip Walker <kip@rivosinc.com> > > --- > > target/riscv/csr.c | 137 ++++++++++++++++++++++++++++++++++++++++++++- > > target/riscv/pmp.c | 9 +++ > > 2 files changed, 145 insertions(+), 1 deletion(-) > > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > > index 0db2c233e5..24e208ebed 100644 > > --- a/target/riscv/csr.c > > +++ b/target/riscv/csr.c > > @@ -163,6 +163,50 @@ static RISCVException ctr32(CPURISCVState *env, int csrno) > > return ctr(env, csrno); > > } > > > > +static RISCVException cfi(CPURISCVState *env, int csrno) > > +{ > > + /* no cfi extension */ > > + if (!env_archcpu(env)->cfg.ext_cfi) { > > + return RISCV_EXCP_ILLEGAL_INST; > > + } > > + /* > > + * CONFIG_USER_MODE always allow access for now. Better for user mode only > > + * functionality > > + */ > > +#if !defined(CONFIG_USER_ONLY) > > + /* current priv not M */ > > + if (env->priv != PRV_M) { > > + /* menvcfg says no CFI */ > > + if (!get_field(env->menvcfg, MENVCFG_CFI)) { > > + return RISCV_EXCP_ILLEGAL_INST; > > + } > > + > > + /* V = 1 and henvcfg says no CFI. raise virtual instr fault */ > > + if (riscv_cpu_virt_enabled(env) && > > + !get_field(env->henvcfg, HENVCFG_CFI)) { > > + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > > + } > > + > > + /* > > + * LPLR and SSP are not accessible to U mode if disabled via status > > + * CSR > > + */ > > + if (env->priv == PRV_U) { > > + if (csrno == CSR_LPLR && > > + !get_field(env->mstatus, MSTATUS_UFCFIEN)) { > > + return RISCV_EXCP_ILLEGAL_INST; > > + } > > + if (csrno == CSR_SSP && > > + !get_field(env->mstatus, MSTATUS_UBCFIEN)) { > > + return RISCV_EXCP_ILLEGAL_INST; > > + } > > + } > > + } > > +#endif > > + > > + return RISCV_EXCP_NONE; > > +} > > + > > #if !defined(CONFIG_USER_ONLY) > > static RISCVException mctr(CPURISCVState *env, int csrno) > > { > > @@ -485,6 +529,32 @@ static RISCVException seed(CPURISCVState *env, int csrno) > > #endif > > } > > > > +/* Zisslpcfi CSR_LPLR read/write */ > > +static int read_lplr(CPURISCVState *env, int csrno, target_ulong *val) > > +{ > > + *val = env->lplr; > > + return RISCV_EXCP_NONE; > > +} > > + > > +static int write_lplr(CPURISCVState *env, int csrno, target_ulong val) > > +{ > > + env->lplr = val & (LPLR_UL | LPLR_ML | LPLR_LL); > > + return RISCV_EXCP_NONE; > > +} > > + > > +/* Zisslpcfi CSR_SSP read/write */ > > +static int read_ssp(CPURISCVState *env, int csrno, target_ulong *val) > > +{ > > + *val = env->ssp; > > + return RISCV_EXCP_NONE; > > +} > > + > > +static int write_ssp(CPURISCVState *env, int csrno, target_ulong val) > > +{ > > + env->ssp = val; > > + return RISCV_EXCP_NONE; > > +} > > + > > /* User Floating-Point CSRs */ > > static RISCVException read_fflags(CPURISCVState *env, int csrno, > > target_ulong *val) > > @@ -1227,7 +1297,7 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, > > > > /* flush tlb on mstatus fields that affect VM */ > > if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV | > > - MSTATUS_MPRV | MSTATUS_SUM)) { > > + MSTATUS_MPRV | MSTATUS_SUM | MSTATUS_UFCFIEN | MSTATUS_UBCFIEN)) { > > These two fields should be guarded by the check of ext_cfi. Yes this makes sense. Will fix it. > > And MSTATUS_UBCFIEN field change don't need flush tlb. > TCG code-gen would be different depending on whether ubcfi is enabled or not. As an example a TB might have code generated when bcfi was enabled. But if someone disables it, translation needs to happen again and zimops implementation should be generated this time. > I didn't get why we should flush tlb for forward cfi. For background, > there are some enhancement for the PTE and PMP, we may need do some > memory adjustments. But forward cfi just adds some instructions. Why we > should flush tlb? Does the tlb can't be used any more? Pretty much same reasoning as back cfi. commit message in patch #7 goes in more detail on that. TLB flush is mainly to invalidate TB. > > > tlb_flush(env_cpu(env)); > > } > > mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE | > > @@ -1250,6 +1320,11 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, > > } > > } > > > > + /* If cfi extension is available, then apply cfi status mask */ > > + if (env_archcpu(env)->cfg.ext_cfi) { > > + mask |= CFISTATUS_M_MASK; > > + } > > + > > mstatus = (mstatus & ~mask) | (val & mask); > > > > if (xl > MXL_RV32) { > > @@ -1880,9 +1955,17 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno, > > target_ulong val) > > { > > uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE | MENVCFG_CBZE; > > + uint64_t cfi_mask = MENVCFG_CFI | MENVCFG_SFCFIEN; > > > > if (riscv_cpu_mxl(env) == MXL_RV64) { > > mask |= MENVCFG_PBMTE | MENVCFG_STCE; > > + if (env_archcpu(env)->cfg.ext_cfi) { > > + mask |= cfi_mask; > > + /* If any cfi enabling bit changes in menvcfg, flush tlb */ > > + if ((val ^ env->menvcfg) & cfi_mask) { > > + tlb_flush(env_cpu(env)); > Don't flush tlb for MENVCFG_SFCFIE field changes. We need to because we have invalidate earlier generated TB code. > > + } > > + } > > } > > env->menvcfg = (env->menvcfg & ~mask) | (val & mask); > > > > @@ -1900,8 +1983,17 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno, > > target_ulong val) > > { > > uint64_t mask = MENVCFG_PBMTE | MENVCFG_STCE; > > + uint64_t cfi_mask = MENVCFG_CFI | MENVCFG_SFCFIEN; > MENVCFG_SFCFIE > > uint64_t valh = (uint64_t)val << 32; > > > > + if (env_archcpu(env)->cfg.ext_cfi) { > > + mask |= cfi_mask; > > + /* If any cfi enabling bit changes in menvcfg, flush tlb */ > > + if ((val ^ env->menvcfg) & cfi_mask) { > > + tlb_flush(env_cpu(env)); > > + } > If SFCFIE field change, we should not flush the tlb. It's same reasoning that generated TB code is invalid now. > > + } > > + > > env->menvcfg = (env->menvcfg & ~mask) | (valh & mask); > > > > return RISCV_EXCP_NONE; > > @@ -1954,6 +2046,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno, > > target_ulong val) > > { > > uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE; > > + uint64_t cfi_mask = HENVCFG_CFI | HENVCFG_SFCFIEN; > > HENVCFG_SFCFIE > > > RISCVException ret; > > > > ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG); > > @@ -1963,6 +2056,18 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno, > > > > if (riscv_cpu_mxl(env) == MXL_RV64) { > > mask |= HENVCFG_PBMTE | HENVCFG_STCE; > > + /* > > + * If cfi available and menvcfg.CFI = 1, then apply cfi mask for > > + * henvcfg > > + */ > > + if (env_archcpu(env)->cfg.ext_cfi && > > + get_field(env->menvcfg, MENVCFG_CFI)) { > > + mask |= cfi_mask; > > + /* If any cfi enabling bit changes in henvcfg, flush tlb */ > > + if ((val ^ env->henvcfg) & cfi_mask) { > > + tlb_flush(env_cpu(env)); > > + } > If SFCFIE field change, we should not flush the tlb. It's same reasoning that generated TB code is invalid now. > > + } > > } > > > > env->henvcfg = (env->henvcfg & ~mask) | (val & mask); > > @@ -1988,9 +2093,19 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno, > > target_ulong val) > > { > > uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE; > > + uint64_t cfi_mask = HENVCFG_CFI | HENVCFG_SFCFIEN; > > uint64_t valh = (uint64_t)val << 32; > > RISCVException ret; > > > > + if (env_archcpu(env)->cfg.ext_cfi && > > + get_field(env->menvcfg, MENVCFG_CFI)) { > > + mask |= cfi_mask; > > + /* If any cfi enabling bit changes in henvcfg, flush tlb */ > > + if ((val ^ env->henvcfg) & cfi_mask) { > > + tlb_flush(env_cpu(env)); > > + } > > + } > > + > > ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG); > > if (ret != RISCV_EXCP_NONE) { > > return ret; > > @@ -2270,6 +2385,11 @@ static RISCVException read_sstatus_i128(CPURISCVState *env, int csrno, > > mask |= SSTATUS64_UXL; > > } > > > > + if ((env_archcpu(env)->cfg.ext_cfi) && > > + get_field(env->menvcfg, MENVCFG_CFI)) { > > + mask |= CFISTATUS_S_MASK; > > + } > > + > > *val = int128_make128(sstatus, add_status_sd(MXL_RV128, sstatus)); > > return RISCV_EXCP_NONE; > > } > > @@ -2281,6 +2401,11 @@ static RISCVException read_sstatus(CPURISCVState *env, int csrno, > > if (env->xl != MXL_RV32 || env->debugger) { > > mask |= SSTATUS64_UXL; > > } > > + > > + if ((env_archcpu(env)->cfg.ext_cfi) && > > + get_field(env->menvcfg, MENVCFG_CFI)) { > > + mask |= CFISTATUS_S_MASK; > > + } > > /* TODO: Use SXL not MXL. */ > > *val = add_status_sd(riscv_cpu_mxl(env), env->mstatus & mask); > > return RISCV_EXCP_NONE; > > @@ -2296,6 +2421,12 @@ static RISCVException write_sstatus(CPURISCVState *env, int csrno, > > mask |= SSTATUS64_UXL; > > } > > } > > + > > + /* If cfi available and menvcfg.CFI = 1, apply CFI mask for sstatus */ > > + if ((env_archcpu(env)->cfg.ext_cfi) && > > + get_field(env->menvcfg, MENVCFG_CFI)) { > > + mask |= CFISTATUS_S_MASK; > > + } > > target_ulong newval = (env->mstatus & ~mask) | (val & mask); > > return write_mstatus(env, CSR_MSTATUS, newval); > > } > > @@ -4001,6 +4132,10 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = { > > /* Crypto Extension */ > > [CSR_SEED] = { "seed", seed, NULL, NULL, rmw_seed }, > > > > + /* User mode CFI CSR */ > > + [CSR_LPLR] = { "lplr", cfi, read_lplr, write_lplr }, > > + [CSR_SSP] = { "ssp", cfi, read_ssp, write_ssp }, > > + > > #if !defined(CONFIG_USER_ONLY) > > /* Machine Timers and Counters */ > > [CSR_MCYCLE] = { "mcycle", any, read_hpmcounter, > > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c > > index d1126a6066..89745d46cd 100644 > > --- a/target/riscv/pmp.c > > +++ b/target/riscv/pmp.c > > @@ -579,6 +579,15 @@ void mseccfg_csr_write(CPURISCVState *env, target_ulong val) > > /* Sticky bits */ > > val |= (env->mseccfg & (MSECCFG_MMWP | MSECCFG_MML)); > > > > + /* M-mode forward cfi to be enabled if cfi extension is implemented */ > > + if (env_archcpu(env)->cfg.ext_cfi) { > > + val |= (val & MSECCFG_MFCFIEN); > This statement does nothing. Is it a typo? > > + /* If forward cfi in mseccfg is being toggled, flush tlb */ > > + if ((env->mseccfg ^ val) & MSECCFG_MFCFIEN) { > > + tlb_flush(env_cpu(env)); > > + } > > No need flush tlb. > > Zhiwei > > > + } > > + > > env->mseccfg = val; > > } > >
On Tue, Feb 14, 2023 at 10:24 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote: > > I don't find the modification for read_mstatus. Doesn't need any modification in read_mstatus. It just returns whatever is in the mstatus. > > Zhiwei > > On 2023/2/15 13:47, LIU Zhiwei wrote: > > > > On 2023/2/9 14:23, Deepak Gupta wrote: > >> CSR_SSP and CSR_LPLR are new CSR additions to cpu/hart. This patch > >> allows > >> access to these CSRs. A predicate routine handles access to these CSR as > >> per specification. > >> > >> This patch also implments new bit definitions in > >> menvcfg/henvcfg/mstatus/ > >> sstatus CSRs to master enabled cfi and enable forward cfi in S and M > >> mode. > >> mstatus CSR holds forward and backward cfi enabling for U mode. > >> > >> There is no enabling bit for backward cfi in S and M mode. It is always > >> enabled if extension is implemented by CPU. > >> > >> Signed-off-by: Deepak Gupta <debug@rivosinc.com> > >> Signed-off-by: Kip Walker <kip@rivosinc.com> > >> --- > >> target/riscv/csr.c | 137 ++++++++++++++++++++++++++++++++++++++++++++- > >> target/riscv/pmp.c | 9 +++ > >> 2 files changed, 145 insertions(+), 1 deletion(-) > >> > >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c > >> index 0db2c233e5..24e208ebed 100644 > >> --- a/target/riscv/csr.c > >> +++ b/target/riscv/csr.c > >> @@ -163,6 +163,50 @@ static RISCVException ctr32(CPURISCVState *env, > >> int csrno) > >> return ctr(env, csrno); > >> } > >> +static RISCVException cfi(CPURISCVState *env, int csrno) > >> +{ > >> + /* no cfi extension */ > >> + if (!env_archcpu(env)->cfg.ext_cfi) { > >> + return RISCV_EXCP_ILLEGAL_INST; > >> + } > >> + /* > >> + * CONFIG_USER_MODE always allow access for now. Better for user > >> mode only > >> + * functionality > >> + */ > >> +#if !defined(CONFIG_USER_ONLY) > >> + /* current priv not M */ > >> + if (env->priv != PRV_M) { > >> + /* menvcfg says no CFI */ > >> + if (!get_field(env->menvcfg, MENVCFG_CFI)) { > >> + return RISCV_EXCP_ILLEGAL_INST; > >> + } > >> + > >> + /* V = 1 and henvcfg says no CFI. raise virtual instr fault */ > >> + if (riscv_cpu_virt_enabled(env) && > >> + !get_field(env->henvcfg, HENVCFG_CFI)) { > >> + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; > >> + } > >> + > >> + /* > >> + * LPLR and SSP are not accessible to U mode if disabled via > >> status > >> + * CSR > >> + */ > >> + if (env->priv == PRV_U) { > >> + if (csrno == CSR_LPLR && > >> + !get_field(env->mstatus, MSTATUS_UFCFIEN)) { > >> + return RISCV_EXCP_ILLEGAL_INST; > >> + } > >> + if (csrno == CSR_SSP && > >> + !get_field(env->mstatus, MSTATUS_UBCFIEN)) { > >> + return RISCV_EXCP_ILLEGAL_INST; > >> + } > >> + } > >> + } > >> +#endif > >> + > >> + return RISCV_EXCP_NONE; > >> +} > >> + > >> #if !defined(CONFIG_USER_ONLY) > >> static RISCVException mctr(CPURISCVState *env, int csrno) > >> { > >> @@ -485,6 +529,32 @@ static RISCVException seed(CPURISCVState *env, > >> int csrno) > >> #endif > >> } > >> +/* Zisslpcfi CSR_LPLR read/write */ > >> +static int read_lplr(CPURISCVState *env, int csrno, target_ulong *val) > >> +{ > >> + *val = env->lplr; > >> + return RISCV_EXCP_NONE; > >> +} > >> + > >> +static int write_lplr(CPURISCVState *env, int csrno, target_ulong val) > >> +{ > >> + env->lplr = val & (LPLR_UL | LPLR_ML | LPLR_LL); > >> + return RISCV_EXCP_NONE; > >> +} > >> + > >> +/* Zisslpcfi CSR_SSP read/write */ > >> +static int read_ssp(CPURISCVState *env, int csrno, target_ulong *val) > >> +{ > >> + *val = env->ssp; > >> + return RISCV_EXCP_NONE; > >> +} > >> + > >> +static int write_ssp(CPURISCVState *env, int csrno, target_ulong val) > >> +{ > >> + env->ssp = val; > >> + return RISCV_EXCP_NONE; > >> +} > >> + > >> /* User Floating-Point CSRs */ > >> static RISCVException read_fflags(CPURISCVState *env, int csrno, > >> target_ulong *val) > >> @@ -1227,7 +1297,7 @@ static RISCVException > >> write_mstatus(CPURISCVState *env, int csrno, > >> /* flush tlb on mstatus fields that affect VM */ > >> if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV | > >> - MSTATUS_MPRV | MSTATUS_SUM)) { > >> + MSTATUS_MPRV | MSTATUS_SUM | MSTATUS_UFCFIEN | > >> MSTATUS_UBCFIEN)) { > > > > These two fields should be guarded by the check of ext_cfi. > > > > And MSTATUS_UBCFIEN field change don't need flush tlb. > > > > I didn't get why we should flush tlb for forward cfi. For background, > > there are some enhancement for the PTE and PMP, we may need do some > > memory adjustments. But forward cfi just adds some instructions. Why > > we should flush tlb? Does the tlb can't be used any more? > > > >> tlb_flush(env_cpu(env)); > >> } > >> mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE | > >> @@ -1250,6 +1320,11 @@ static RISCVException > >> write_mstatus(CPURISCVState *env, int csrno, > >> } > >> } > >> + /* If cfi extension is available, then apply cfi status mask */ > >> + if (env_archcpu(env)->cfg.ext_cfi) { > >> + mask |= CFISTATUS_M_MASK; > >> + } > >> + > >> mstatus = (mstatus & ~mask) | (val & mask); > >> if (xl > MXL_RV32) { > >> @@ -1880,9 +1955,17 @@ static RISCVException > >> write_menvcfg(CPURISCVState *env, int csrno, > >> target_ulong val) > >> { > >> uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE | > >> MENVCFG_CBZE; > >> + uint64_t cfi_mask = MENVCFG_CFI | MENVCFG_SFCFIEN; > >> if (riscv_cpu_mxl(env) == MXL_RV64) { > >> mask |= MENVCFG_PBMTE | MENVCFG_STCE; > >> + if (env_archcpu(env)->cfg.ext_cfi) { > >> + mask |= cfi_mask; > >> + /* If any cfi enabling bit changes in menvcfg, flush tlb */ > >> + if ((val ^ env->menvcfg) & cfi_mask) { > >> + tlb_flush(env_cpu(env)); > > Don't flush tlb for MENVCFG_SFCFIE field changes. > >> + } > >> + } > >> } > >> env->menvcfg = (env->menvcfg & ~mask) | (val & mask); > >> @@ -1900,8 +1983,17 @@ static RISCVException > >> write_menvcfgh(CPURISCVState *env, int csrno, > >> target_ulong val) > >> { > >> uint64_t mask = MENVCFG_PBMTE | MENVCFG_STCE; > >> + uint64_t cfi_mask = MENVCFG_CFI | MENVCFG_SFCFIEN; > > MENVCFG_SFCFIE > >> uint64_t valh = (uint64_t)val << 32; > >> + if (env_archcpu(env)->cfg.ext_cfi) { > >> + mask |= cfi_mask; > >> + /* If any cfi enabling bit changes in menvcfg, flush tlb */ > >> + if ((val ^ env->menvcfg) & cfi_mask) { > >> + tlb_flush(env_cpu(env)); > >> + } > > If SFCFIE field change, we should not flush the tlb. > >> + } > >> + > >> env->menvcfg = (env->menvcfg & ~mask) | (valh & mask); > >> return RISCV_EXCP_NONE; > >> @@ -1954,6 +2046,7 @@ static RISCVException > >> write_henvcfg(CPURISCVState *env, int csrno, > >> target_ulong val) > >> { > >> uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | > >> HENVCFG_CBZE; > >> + uint64_t cfi_mask = HENVCFG_CFI | HENVCFG_SFCFIEN; > > > > HENVCFG_SFCFIE > > > >> RISCVException ret; > >> ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG); > >> @@ -1963,6 +2056,18 @@ static RISCVException > >> write_henvcfg(CPURISCVState *env, int csrno, > >> if (riscv_cpu_mxl(env) == MXL_RV64) { > >> mask |= HENVCFG_PBMTE | HENVCFG_STCE; > >> + /* > >> + * If cfi available and menvcfg.CFI = 1, then apply cfi mask > >> for > >> + * henvcfg > >> + */ > >> + if (env_archcpu(env)->cfg.ext_cfi && > >> + get_field(env->menvcfg, MENVCFG_CFI)) { > >> + mask |= cfi_mask; > >> + /* If any cfi enabling bit changes in henvcfg, flush tlb */ > >> + if ((val ^ env->henvcfg) & cfi_mask) { > >> + tlb_flush(env_cpu(env)); > >> + } > > If SFCFIE field change, we should not flush the tlb. > >> + } > >> } > >> env->henvcfg = (env->henvcfg & ~mask) | (val & mask); > >> @@ -1988,9 +2093,19 @@ static RISCVException > >> write_henvcfgh(CPURISCVState *env, int csrno, > >> target_ulong val) > >> { > >> uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE; > >> + uint64_t cfi_mask = HENVCFG_CFI | HENVCFG_SFCFIEN; > >> uint64_t valh = (uint64_t)val << 32; > >> RISCVException ret; > >> + if (env_archcpu(env)->cfg.ext_cfi && > >> + get_field(env->menvcfg, MENVCFG_CFI)) { > >> + mask |= cfi_mask; > >> + /* If any cfi enabling bit changes in henvcfg, flush tlb */ > >> + if ((val ^ env->henvcfg) & cfi_mask) { > >> + tlb_flush(env_cpu(env)); > >> + } > >> + } > >> + > >> ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG); > >> if (ret != RISCV_EXCP_NONE) { > >> return ret; > >> @@ -2270,6 +2385,11 @@ static RISCVException > >> read_sstatus_i128(CPURISCVState *env, int csrno, > >> mask |= SSTATUS64_UXL; > >> } > >> + if ((env_archcpu(env)->cfg.ext_cfi) && > >> + get_field(env->menvcfg, MENVCFG_CFI)) { > >> + mask |= CFISTATUS_S_MASK; > >> + } > >> + > >> *val = int128_make128(sstatus, add_status_sd(MXL_RV128, sstatus)); > >> return RISCV_EXCP_NONE; > >> } > >> @@ -2281,6 +2401,11 @@ static RISCVException > >> read_sstatus(CPURISCVState *env, int csrno, > >> if (env->xl != MXL_RV32 || env->debugger) { > >> mask |= SSTATUS64_UXL; > >> } > >> + > >> + if ((env_archcpu(env)->cfg.ext_cfi) && > >> + get_field(env->menvcfg, MENVCFG_CFI)) { > >> + mask |= CFISTATUS_S_MASK; > >> + } > >> /* TODO: Use SXL not MXL. */ > >> *val = add_status_sd(riscv_cpu_mxl(env), env->mstatus & mask); > >> return RISCV_EXCP_NONE; > >> @@ -2296,6 +2421,12 @@ static RISCVException > >> write_sstatus(CPURISCVState *env, int csrno, > >> mask |= SSTATUS64_UXL; > >> } > >> } > >> + > >> + /* If cfi available and menvcfg.CFI = 1, apply CFI mask for > >> sstatus */ > >> + if ((env_archcpu(env)->cfg.ext_cfi) && > >> + get_field(env->menvcfg, MENVCFG_CFI)) { > >> + mask |= CFISTATUS_S_MASK; > >> + } > >> target_ulong newval = (env->mstatus & ~mask) | (val & mask); > >> return write_mstatus(env, CSR_MSTATUS, newval); > >> } > >> @@ -4001,6 +4132,10 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = { > >> /* Crypto Extension */ > >> [CSR_SEED] = { "seed", seed, NULL, NULL, rmw_seed }, > >> + /* User mode CFI CSR */ > >> + [CSR_LPLR] = { "lplr", cfi, read_lplr, write_lplr }, > >> + [CSR_SSP] = { "ssp", cfi, read_ssp, write_ssp }, > >> + > >> #if !defined(CONFIG_USER_ONLY) > >> /* Machine Timers and Counters */ > >> [CSR_MCYCLE] = { "mcycle", any, read_hpmcounter, > >> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c > >> index d1126a6066..89745d46cd 100644 > >> --- a/target/riscv/pmp.c > >> +++ b/target/riscv/pmp.c > >> @@ -579,6 +579,15 @@ void mseccfg_csr_write(CPURISCVState *env, > >> target_ulong val) > >> /* Sticky bits */ > >> val |= (env->mseccfg & (MSECCFG_MMWP | MSECCFG_MML)); > >> + /* M-mode forward cfi to be enabled if cfi extension is > >> implemented */ > >> + if (env_archcpu(env)->cfg.ext_cfi) { > >> + val |= (val & MSECCFG_MFCFIEN); > > This statement does nothing. Is it a typo? > >> + /* If forward cfi in mseccfg is being toggled, flush tlb */ > >> + if ((env->mseccfg ^ val) & MSECCFG_MFCFIEN) { > >> + tlb_flush(env_cpu(env)); > >> + } > > > > No need flush tlb. > > > > Zhiwei > > > >> + } > >> + > >> env->mseccfg = val; > >> }
On 2/15/23 13:33, Deepak Gupta wrote: > On Tue, Feb 14, 2023 at 9:47 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote: >> And MSTATUS_UBCFIEN field change don't need flush tlb. >> > > TCG code-gen would be different depending on whether ubcfi is enabled or not. > As an example a TB might have code generated when bcfi was enabled. > But if someone disables it, > translation needs to happen again and zimops implementation should be > generated this time. tlb_flush does not affect translation. TB are tied to physical addresses and are only flushed by writes or tb_flush(). The correct fix is to allocate a bit from FIELD(TB_FLAGS, X, Y, 1), and adjust cpu_get_tb_cpu_state to indicate when CFI is active in the current cpu mode. r~
On Wed, Feb 15, 2023 at 4:02 PM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 2/15/23 13:33, Deepak Gupta wrote: > > On Tue, Feb 14, 2023 at 9:47 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote: > >> And MSTATUS_UBCFIEN field change don't need flush tlb. > >> > > > > TCG code-gen would be different depending on whether ubcfi is enabled or not. > > As an example a TB might have code generated when bcfi was enabled. > > But if someone disables it, > > translation needs to happen again and zimops implementation should be > > generated this time. > > tlb_flush does not affect translation. TB are tied to physical addresses and are only > flushed by writes or tb_flush(). > > The correct fix is to allocate a bit from FIELD(TB_FLAGS, X, Y, 1), and adjust > cpu_get_tb_cpu_state to indicate when CFI is active in the current cpu mode. > > Hmm... So this looks like a major oversight on my side. I had been under the impression that tlb flush does TB flushes too. I was trying to save on TB_FLAGS. I saw `tb_jmp_cache` was being cleared, didn't dig deep and assumed that tlb flush clears up TB as well. Now that you've pointed it out, it looks like that's a different optimization. So looks like this definitely needs a fix. Question: I'll basically need two bits (one for forward cfi and one for backward cfi). But I need to throw away the TB if cfi enabling bits mismatch at the time TB was generated and the current state of enabling bits. Reason being, this needs to get translated again and zimops need to be generated. What's the best way to throw away a single TB? > r~
On 2/15/23 15:38, Deepak Gupta wrote: > Question: > I'll basically need two bits (one for forward cfi and one for backward cfi). Are they separately enabled? It may also be possible to use a single bit and then perform a runtime check. I guess I should read the spec... > But I need to throw away the TB if cfi enabling bits mismatch at the > time TB was generated and the current state of enabling bits. > Reason being, this needs to get translated again and zimops need to be > generated. > > What's the best way to throw away a single TB? You don't throw TBs away at all. The current cpu state is produced by cpu_get_tb_cpu_state. This is included into the hash table lookup and will only match a TB which has been generated with the same state. Which means that you can have multiple live TBs, those with CFI enabled and those without, and the correct one will be selected at runtime. r~
On Wed, Feb 15, 2023 at 6:44 PM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 2/15/23 15:38, Deepak Gupta wrote: > > Question: > > I'll basically need two bits (one for forward cfi and one for backward cfi). > > Are they separately enabled? It may also be possible to use a single bit and then perform > a runtime check. I guess I should read the spec... There is a master enable but there're controls to individually enable & disable. Thus the CPU could be in a state where one is enabled and other is not. > > > But I need to throw away the TB if cfi enabling bits mismatch at the > > time TB was generated and the current state of enabling bits. > > Reason being, this needs to get translated again and zimops need to be > > generated. > > > > What's the best way to throw away a single TB? > > You don't throw TBs away at all. > > The current cpu state is produced by cpu_get_tb_cpu_state. This is included into the hash > table lookup and will only match a TB which has been generated with the same state. Which > means that you can have multiple live TBs, those with CFI enabled and those without, and > the correct one will be selected at runtime. > Thanks a lot. I see `tb_lookup` & `tb_htable_lookup`. I'll revise and add some bits in TB_FLAGS for both of them. And probably ask some questions on the list if anything is not clear to me. > > r~
diff --git a/target/riscv/csr.c b/target/riscv/csr.c index 0db2c233e5..24e208ebed 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -163,6 +163,50 @@ static RISCVException ctr32(CPURISCVState *env, int csrno) return ctr(env, csrno); } +static RISCVException cfi(CPURISCVState *env, int csrno) +{ + /* no cfi extension */ + if (!env_archcpu(env)->cfg.ext_cfi) { + return RISCV_EXCP_ILLEGAL_INST; + } + /* + * CONFIG_USER_MODE always allow access for now. Better for user mode only + * functionality + */ +#if !defined(CONFIG_USER_ONLY) + /* current priv not M */ + if (env->priv != PRV_M) { + /* menvcfg says no CFI */ + if (!get_field(env->menvcfg, MENVCFG_CFI)) { + return RISCV_EXCP_ILLEGAL_INST; + } + + /* V = 1 and henvcfg says no CFI. raise virtual instr fault */ + if (riscv_cpu_virt_enabled(env) && + !get_field(env->henvcfg, HENVCFG_CFI)) { + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; + } + + /* + * LPLR and SSP are not accessible to U mode if disabled via status + * CSR + */ + if (env->priv == PRV_U) { + if (csrno == CSR_LPLR && + !get_field(env->mstatus, MSTATUS_UFCFIEN)) { + return RISCV_EXCP_ILLEGAL_INST; + } + if (csrno == CSR_SSP && + !get_field(env->mstatus, MSTATUS_UBCFIEN)) { + return RISCV_EXCP_ILLEGAL_INST; + } + } + } +#endif + + return RISCV_EXCP_NONE; +} + #if !defined(CONFIG_USER_ONLY) static RISCVException mctr(CPURISCVState *env, int csrno) { @@ -485,6 +529,32 @@ static RISCVException seed(CPURISCVState *env, int csrno) #endif } +/* Zisslpcfi CSR_LPLR read/write */ +static int read_lplr(CPURISCVState *env, int csrno, target_ulong *val) +{ + *val = env->lplr; + return RISCV_EXCP_NONE; +} + +static int write_lplr(CPURISCVState *env, int csrno, target_ulong val) +{ + env->lplr = val & (LPLR_UL | LPLR_ML | LPLR_LL); + return RISCV_EXCP_NONE; +} + +/* Zisslpcfi CSR_SSP read/write */ +static int read_ssp(CPURISCVState *env, int csrno, target_ulong *val) +{ + *val = env->ssp; + return RISCV_EXCP_NONE; +} + +static int write_ssp(CPURISCVState *env, int csrno, target_ulong val) +{ + env->ssp = val; + return RISCV_EXCP_NONE; +} + /* User Floating-Point CSRs */ static RISCVException read_fflags(CPURISCVState *env, int csrno, target_ulong *val) @@ -1227,7 +1297,7 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, /* flush tlb on mstatus fields that affect VM */ if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV | - MSTATUS_MPRV | MSTATUS_SUM)) { + MSTATUS_MPRV | MSTATUS_SUM | MSTATUS_UFCFIEN | MSTATUS_UBCFIEN)) { tlb_flush(env_cpu(env)); } mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE | @@ -1250,6 +1320,11 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, } } + /* If cfi extension is available, then apply cfi status mask */ + if (env_archcpu(env)->cfg.ext_cfi) { + mask |= CFISTATUS_M_MASK; + } + mstatus = (mstatus & ~mask) | (val & mask); if (xl > MXL_RV32) { @@ -1880,9 +1955,17 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno, target_ulong val) { uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE | MENVCFG_CBZE; + uint64_t cfi_mask = MENVCFG_CFI | MENVCFG_SFCFIEN; if (riscv_cpu_mxl(env) == MXL_RV64) { mask |= MENVCFG_PBMTE | MENVCFG_STCE; + if (env_archcpu(env)->cfg.ext_cfi) { + mask |= cfi_mask; + /* If any cfi enabling bit changes in menvcfg, flush tlb */ + if ((val ^ env->menvcfg) & cfi_mask) { + tlb_flush(env_cpu(env)); + } + } } env->menvcfg = (env->menvcfg & ~mask) | (val & mask); @@ -1900,8 +1983,17 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno, target_ulong val) { uint64_t mask = MENVCFG_PBMTE | MENVCFG_STCE; + uint64_t cfi_mask = MENVCFG_CFI | MENVCFG_SFCFIEN; uint64_t valh = (uint64_t)val << 32; + if (env_archcpu(env)->cfg.ext_cfi) { + mask |= cfi_mask; + /* If any cfi enabling bit changes in menvcfg, flush tlb */ + if ((val ^ env->menvcfg) & cfi_mask) { + tlb_flush(env_cpu(env)); + } + } + env->menvcfg = (env->menvcfg & ~mask) | (valh & mask); return RISCV_EXCP_NONE; @@ -1954,6 +2046,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno, target_ulong val) { uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE; + uint64_t cfi_mask = HENVCFG_CFI | HENVCFG_SFCFIEN; RISCVException ret; ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG); @@ -1963,6 +2056,18 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno, if (riscv_cpu_mxl(env) == MXL_RV64) { mask |= HENVCFG_PBMTE | HENVCFG_STCE; + /* + * If cfi available and menvcfg.CFI = 1, then apply cfi mask for + * henvcfg + */ + if (env_archcpu(env)->cfg.ext_cfi && + get_field(env->menvcfg, MENVCFG_CFI)) { + mask |= cfi_mask; + /* If any cfi enabling bit changes in henvcfg, flush tlb */ + if ((val ^ env->henvcfg) & cfi_mask) { + tlb_flush(env_cpu(env)); + } + } } env->henvcfg = (env->henvcfg & ~mask) | (val & mask); @@ -1988,9 +2093,19 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno, target_ulong val) { uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE; + uint64_t cfi_mask = HENVCFG_CFI | HENVCFG_SFCFIEN; uint64_t valh = (uint64_t)val << 32; RISCVException ret; + if (env_archcpu(env)->cfg.ext_cfi && + get_field(env->menvcfg, MENVCFG_CFI)) { + mask |= cfi_mask; + /* If any cfi enabling bit changes in henvcfg, flush tlb */ + if ((val ^ env->henvcfg) & cfi_mask) { + tlb_flush(env_cpu(env)); + } + } + ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG); if (ret != RISCV_EXCP_NONE) { return ret; @@ -2270,6 +2385,11 @@ static RISCVException read_sstatus_i128(CPURISCVState *env, int csrno, mask |= SSTATUS64_UXL; } + if ((env_archcpu(env)->cfg.ext_cfi) && + get_field(env->menvcfg, MENVCFG_CFI)) { + mask |= CFISTATUS_S_MASK; + } + *val = int128_make128(sstatus, add_status_sd(MXL_RV128, sstatus)); return RISCV_EXCP_NONE; } @@ -2281,6 +2401,11 @@ static RISCVException read_sstatus(CPURISCVState *env, int csrno, if (env->xl != MXL_RV32 || env->debugger) { mask |= SSTATUS64_UXL; } + + if ((env_archcpu(env)->cfg.ext_cfi) && + get_field(env->menvcfg, MENVCFG_CFI)) { + mask |= CFISTATUS_S_MASK; + } /* TODO: Use SXL not MXL. */ *val = add_status_sd(riscv_cpu_mxl(env), env->mstatus & mask); return RISCV_EXCP_NONE; @@ -2296,6 +2421,12 @@ static RISCVException write_sstatus(CPURISCVState *env, int csrno, mask |= SSTATUS64_UXL; } } + + /* If cfi available and menvcfg.CFI = 1, apply CFI mask for sstatus */ + if ((env_archcpu(env)->cfg.ext_cfi) && + get_field(env->menvcfg, MENVCFG_CFI)) { + mask |= CFISTATUS_S_MASK; + } target_ulong newval = (env->mstatus & ~mask) | (val & mask); return write_mstatus(env, CSR_MSTATUS, newval); } @@ -4001,6 +4132,10 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = { /* Crypto Extension */ [CSR_SEED] = { "seed", seed, NULL, NULL, rmw_seed }, + /* User mode CFI CSR */ + [CSR_LPLR] = { "lplr", cfi, read_lplr, write_lplr }, + [CSR_SSP] = { "ssp", cfi, read_ssp, write_ssp }, + #if !defined(CONFIG_USER_ONLY) /* Machine Timers and Counters */ [CSR_MCYCLE] = { "mcycle", any, read_hpmcounter, diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c index d1126a6066..89745d46cd 100644 --- a/target/riscv/pmp.c +++ b/target/riscv/pmp.c @@ -579,6 +579,15 @@ void mseccfg_csr_write(CPURISCVState *env, target_ulong val) /* Sticky bits */ val |= (env->mseccfg & (MSECCFG_MMWP | MSECCFG_MML)); + /* M-mode forward cfi to be enabled if cfi extension is implemented */ + if (env_archcpu(env)->cfg.ext_cfi) { + val |= (val & MSECCFG_MFCFIEN); + /* If forward cfi in mseccfg is being toggled, flush tlb */ + if ((env->mseccfg ^ val) & MSECCFG_MFCFIEN) { + tlb_flush(env_cpu(env)); + } + } + env->mseccfg = val; }