Re: [PATCH v1 RFC Zisslpcfi 3/9] target/riscv: implements CSRs and new bits in existing CSRs in zisslpcfi

[Deepak Gupta]

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;
> >>   }