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

[Weiwei Li]

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

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