Re: [PATCH 6/6] target/riscv: simplify the check in hmode to resue the check in riscv_csrrw_check

[Alistair Francis]

On Sun, Jul 10, 2022 at 6:30 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> Just add 1 to the effective privledge level when in HS mode, then reuse the 
> check
> 'effective_priv < csr_priv' in riscv_csrrw_check to replace the privilege 
> level
> related check in hmode. Then, hmode will only check whether H extension is 
> supported.
>
> when accessing Hypervior CSRs:
>    1) if access from M privilege level, the check of 'effective_priv < 
> csr_priv'
> passes, returns hmode(...) which will return RISCV_EXCP_ILLEGAL_INST when H
> extension is not supported and return RISCV_EXCP_NONE otherwise.
>    2) if access from HS privilege level, effective_priv will add 1, the check
> passes too, also returns hmode(...) too.
>    3) if access from VS/VU privilege level, the check fails, and returns
> RISCV_EXCP_VIRT_INSTRUCTION_FAULT
>    4) if access from U privilege level, the check fails, and returns
> RISCV_EXCP_ILLEGAL_INST
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/csr.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 975007f1ac..2b3ed94366 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -312,13 +312,7 @@ static int aia_smode32(CPURISCVState *env, int csrno)
>  static RISCVException hmode(CPURISCVState *env, int csrno)
>  {
>      if (riscv_has_ext(env, RVH)) {
> -        /* Hypervisor extension is supported */
> -        if ((env->priv == PRV_S && !riscv_cpu_virt_enabled(env)) ||
> -            env->priv == PRV_M) {
> -            return RISCV_EXCP_NONE;
> -        } else {
> -            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> -        }
> +        return RISCV_EXCP_NONE;
>      }
>
>      return RISCV_EXCP_ILLEGAL_INST;
> @@ -3280,13 +3274,11 @@ static inline RISCVException 
> riscv_csrrw_check(CPURISCVState *env,
>  #if !defined(CONFIG_USER_ONLY)
>      int csr_priv, effective_priv = env->priv;
>
> -    if (riscv_has_ext(env, RVH) && env->priv == PRV_S) {
> +    if (riscv_has_ext(env, RVH) && env->priv == PRV_S &&
> +        !riscv_cpu_virt_enabled(env)) {
>          /*
> -         * We are in either HS or VS mode.
> -         * Add 1 to the effective privledge level to allow us to access the
> -         * Hypervisor CSRs. The `hmode` predicate will determine if access
> -         * should be allowed(HS) or if a virtual instruction exception 
> should be
> -         * raised(VS).
> +         * We are in HS mode. Add 1 to the effective privledge level to
> +         * allow us to access the Hypervisor CSRs.
>           */
>          effective_priv++;
>      }
> --
> 2.17.1
>
>