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

[Alistair Francis <alistair23@gmail.com>]

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?

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