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

[Andrew Jones]

Hi Weiwei,

We still need the s/resue/reuse/ typo fix in $SUBJECT.

(That's maybe something that could be fixed up while applying?)

Thanks,
drew

On Mon, Jul 18, 2022 at 09:09:55PM +0800, Weiwei Li wrote:
> Just add 1 to the effective privledge level when in HS mode, then reuse
> the check of '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 accessing 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 accessing from HS privilege level, effective_priv will add 1,
> the check passes and also returns hmode(...) too.
>    3) If accessing from VS/VU privilege level, the check fails, and
> returns RISCV_EXCP_VIRT_INSTRUCTION_FAULT
>    4) If accessing 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>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>  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 cf15aa67b7..0fb042b2fd 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;
> @@ -3279,13 +3273,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
> 
>