Re: [PATCH v6 2/5] target/riscv: smstateen check for h/senvcfg

[Mayuresh Chitale]

On Fri, 2022-07-22 at 08:45 +0800, Weiwei Li wrote:
> 在 2022/7/21 下午11:31, Mayuresh Chitale 写道:
> > Accesses to henvcfg, henvcfgh and senvcfg are allowed only if
> > corresponding bit in mstateen0/hstateen0 is enabled. Otherwise an
> > illegal instruction trap is generated.
> > 
> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > ---
> >   target/riscv/csr.c | 100
> > +++++++++++++++++++++++++++++++++++++++++----
> >   1 file changed, 93 insertions(+), 7 deletions(-)
> > 
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 27032a416c..ab06b117f9 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -40,6 +40,55 @@ void riscv_set_csr_ops(int csrno,
> > riscv_csr_operations *ops)
> >   }
> >   
> >   /* Predicates */
> > +#if !defined(CONFIG_USER_ONLY)
> > +static RISCVException smstateen_acc_ok(CPURISCVState *env, int
> > index,
> > +                                       uint64_t bit)
> > +{
> > +    bool virt = riscv_cpu_virt_enabled(env);
> > +    CPUState *cs = env_cpu(env);
> > +    RISCVCPU *cpu = RISCV_CPU(cs);
> > +    uint64_t hstateen = env->hstateen[index];
> > +    uint64_t sstateen = env->sstateen[index];
> > +
> > +    if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
> > +        return RISCV_EXCP_NONE;
> > +    }
> > +
> > +    if (!(env->mstateen[index] & bit)) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    /*
> > +     * Treat hstateen and sstateen as read-only zero if
> > mstateen0.staten
> > +     * is clear.
> > +     */
> > +    if (!(env->mstateen[index] & SMSTATEEN_STATEN)) {
> > +        hstateen = 0;
> > +        sstateen = 0;
> > +    }
> > +
> > +    if (virt) {
> > +        if (!(hstateen & bit)) {
> > +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > +        }
> > +        /*
> > +         * Treat sstateen as read-only zero if hstateen0.staten is
> > clear.
> > +         */
> > +        if (!(hstateen & SMSTATEEN_STATEN)) {
> > +            sstateen = 0;
> > +        }
> > +    }
> > +
> > +    if (env->priv == PRV_U && riscv_has_ext(env, RVS)) {
> > +        if (!(sstateen & bit)) {
> > +            return RISCV_EXCP_ILLEGAL_INST;
> > +        }
> > +    }
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +#endif
> > +
> >   static RISCVException fs(CPURISCVState *env, int csrno)
> >   {
> >   #if !defined(CONFIG_USER_ONLY)
> > @@ -1708,6 +1757,13 @@ static RISCVException
> > write_menvcfgh(CPURISCVState *env, int csrno,
> >   static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
> >                                    target_ulong *val)
> >   {
> > +    RISCVException ret;
> > +
> > +    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
> 
> I think it's better to add this check into the predicate.
> 
> By the way, sharing the same function for all related csrs  is
> easily 
> misunderstood. However, It seems correct.

We use the default global predicates ie hmode/smode etc for the envcfg
registers and the global predicates cant be modified to include
additional checks for envcfg registers. We could implement new
predicates for envcfg but I think the current approach is simpler.
> 
> Regards,
> 
> Weiwei Li
> 
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> > +
> >       *val = env->senvcfg;
> >       return RISCV_EXCP_NONE;
> >   }
> > @@ -1716,15 +1772,27 @@ static RISCVException
> > write_senvcfg(CPURISCVState *env, int csrno,
> >                                     target_ulong val)
> >   {
> >       uint64_t mask = SENVCFG_FIOM | SENVCFG_CBIE | SENVCFG_CBCFE |
> > SENVCFG_CBZE;
> > +    RISCVException ret;
> >   
> > -    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
> > +    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> >   
> > +    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
> >       return RISCV_EXCP_NONE;
> >   }
> >   
> >   static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
> >                                    target_ulong *val)
> >   {
> > +    RISCVException ret;
> > +
> > +    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> > +
> >       *val = env->henvcfg;
> >       return RISCV_EXCP_NONE;
> >   }
> > @@ -1733,6 +1801,12 @@ static RISCVException
> > write_henvcfg(CPURISCVState *env, int csrno,
> >                                     target_ulong val)
> >   {
> >       uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE |
> > HENVCFG_CBZE;
> > +    RISCVException ret;
> > +
> > +    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> >   
> >       if (riscv_cpu_mxl(env) == MXL_RV64) {
> >           mask |= HENVCFG_PBMTE | HENVCFG_STCE;
> > @@ -1746,6 +1820,13 @@ static RISCVException
> > write_henvcfg(CPURISCVState *env, int csrno,
> >   static RISCVException read_henvcfgh(CPURISCVState *env, int
> > csrno,
> >                                    target_ulong *val)
> >   {
> > +    RISCVException ret;
> > +
> > +    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> > +
> >       *val = env->henvcfg >> 32;
> >       return RISCV_EXCP_NONE;
> >   }
> > @@ -1755,9 +1836,14 @@ static RISCVException
> > write_henvcfgh(CPURISCVState *env, int csrno,
> >   {
> >       uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE;
> >       uint64_t valh = (uint64_t)val << 32;
> > +    RISCVException ret;
> >   
> > -    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
> > +    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> >   
> > +    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
> >       return RISCV_EXCP_NONE;
> >   }
> >   
> > @@ -1789,7 +1875,7 @@ static RISCVException
> > write_mstateen(CPURISCVState *env, int csrno,
> >   static RISCVException write_mstateen0(CPURISCVState *env, int
> > csrno,
> >                                         target_ulong new_val)
> >   {
> > -    uint64_t wr_mask = SMSTATEEN_STATEN;
> > +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
> >   
> >       return write_mstateen(env, csrno, wr_mask, new_val);
> >   }
> > @@ -1836,7 +1922,7 @@ static RISCVException
> > write_mstateenh(CPURISCVState *env, int csrno,
> >   static RISCVException write_mstateen0h(CPURISCVState *env, int
> > csrno,
> >                                         target_ulong new_val)
> >   {
> > -    uint64_t wr_mask = SMSTATEEN_STATEN;
> > +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
> >   
> >       return write_mstateenh(env, csrno, wr_mask, new_val);
> >   }
> > @@ -1885,7 +1971,7 @@ static RISCVException
> > write_hstateen(CPURISCVState *env, int csrno,
> >   static RISCVException write_hstateen0(CPURISCVState *env, int
> > csrno,
> >                                         target_ulong new_val)
> >   {
> > -    uint64_t wr_mask = SMSTATEEN_STATEN;
> > +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
> >   
> >       return write_hstateen(env, csrno, wr_mask, new_val);
> >   }
> > @@ -1936,7 +2022,7 @@ static RISCVException
> > write_hstateenh(CPURISCVState *env, int csrno,
> >   static RISCVException write_hstateen0h(CPURISCVState *env, int
> > csrno,
> >                                          target_ulong new_val)
> >   {
> > -    uint64_t wr_mask = SMSTATEEN_STATEN;
> > +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
> >   
> >       return write_hstateenh(env, csrno, wr_mask, new_val);
> >   }
> > @@ -1995,7 +2081,7 @@ static RISCVException
> > write_sstateen(CPURISCVState *env, int csrno,
> >   static RISCVException write_sstateen0(CPURISCVState *env, int
> > csrno,
> >                                         target_ulong new_val)
> >   {
> > -    uint64_t wr_mask = SMSTATEEN_STATEN;
> > +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
> >   
> >       return write_sstateen(env, csrno, wr_mask, new_val);
> >   }