Re: [RFC PATCH v5 2/4] target/riscv: smstateen check for h/senvcfg

[Alistair Francis]

On Thu, Jun 16, 2022 at 4:54 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Sat, Jun 4, 2022 at 2:16 AM Mayuresh Chitale
> <mchitale@ventanamicro.com> wrote:
> >
> > 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 | 84 ++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 78 insertions(+), 6 deletions(-)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 324fefce59..ae91ae1f7e 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -39,6 +39,37 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations 
> > *ops)
> >  }
> >
> >  /* Predicates */
> > +static RISCVException smstateen_acc_ok(CPURISCVState *env, int mode, int 
> > bit)
> > +{
> > +    CPUState *cs = env_cpu(env);
> > +    RISCVCPU *cpu = RISCV_CPU(cs);
> > +    bool virt = riscv_cpu_virt_enabled(env);
> > +
> > +    if (!cpu->cfg.ext_smstateen) {
> > +        return RISCV_EXCP_NONE;
> > +    }
> > +
> > +#if !defined(CONFIG_USER_ONLY)
> > +    if (!(env->mstateen[0] & 1UL << bit)) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    if (virt) {
> > +        if (!(env->hstateen[0] & 1UL << bit)) {
> > +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > +        }
> > +    }
> > +
> > +    if (mode == PRV_U) {
> > +        if (!(env->sstateen[0] & 1UL << bit)) {
> > +            return RISCV_EXCP_ILLEGAL_INST;
> > +        }
> > +    }
> > +#endif
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> >  static RISCVException fs(CPURISCVState *env, int csrno)
> >  {
> >  #if !defined(CONFIG_USER_ONLY)
> > @@ -1557,6 +1588,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, PRV_S, SMSTATEEN0_HSENVCFG);
>
> Couldn't this be part of the original permission check so we don't
> need a second check?

Whoops, misread the function. Ignore that

Alistair

>
> Alistair
>
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> > +
> >      *val = env->senvcfg;
> >      return RISCV_EXCP_NONE;
> >  }
> > @@ -1565,15 +1603,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, PRV_S, 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, PRV_S, SMSTATEEN0_HSENVCFG);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> > +
> >      *val = env->henvcfg;
> >      return RISCV_EXCP_NONE;
> >  }
> > @@ -1582,6 +1632,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, PRV_S, SMSTATEEN0_HSENVCFG);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> >
> >      if (riscv_cpu_mxl(env) == MXL_RV64) {
> >          mask |= HENVCFG_PBMTE | HENVCFG_STCE;
> > @@ -1595,6 +1651,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, PRV_S, SMSTATEEN0_HSENVCFG);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> > +
> >      *val = env->henvcfg >> 32;
> >      return RISCV_EXCP_NONE;
> >  }
> > @@ -1604,9 +1667,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, PRV_S, SMSTATEEN0_HSENVCFG);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> >
> > +    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
> >      return RISCV_EXCP_NONE;
> >  }
> >
> > @@ -1628,7 +1696,8 @@ static RISCVException write_mstateen(CPURISCVState 
> > *env, int csrno,
> >                                       target_ulong new_val)
> >  {
> >      uint64_t *reg;
> > -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> > +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> > +                       (1UL << SMSTATEEN0_HSENVCFG);
> >
> >      reg = &env->mstateen[csrno - CSR_MSTATEEN0];
> >      write_smstateen(env, reg, wr_mask, new_val);
> > @@ -1649,7 +1718,8 @@ static RISCVException write_mstateenh(CPURISCVState 
> > *env, int csrno,
> >  {
> >      uint64_t *reg;
> >      uint64_t val;
> > -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> > +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> > +                       (1UL << SMSTATEEN0_HSENVCFG);
> >
> >      reg = &env->mstateen[csrno - CSR_MSTATEEN0H];
> >      val = (uint64_t)new_val << 32;
> > @@ -1671,7 +1741,8 @@ static RISCVException write_hstateen(CPURISCVState 
> > *env, int csrno,
> >                                       target_ulong new_val)
> >  {
> >      uint64_t *reg;
> > -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> > +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> > +                       (1UL << SMSTATEEN0_HSENVCFG);
> >      int index = csrno - CSR_HSTATEEN0;
> >
> >      reg = &env->hstateen[index];
> > @@ -1694,8 +1765,9 @@ static RISCVException write_hstateenh(CPURISCVState 
> > *env, int csrno,
> >  {
> >      uint64_t *reg;
> >      uint64_t val;
> > -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> >      int index = csrno - CSR_HSTATEEN0H;
> > +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> > +                       (1UL << SMSTATEEN0_HSENVCFG);
> >
> >      reg = &env->hstateen[index];
> >      val = (uint64_t)new_val << 32;
> > --
> > 2.25.1
> >
> >