[v3] Improve the U/S/H extension related check

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

Posted by Weiwei Li 3 years, 6 months ago

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

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

Posted by Andrew Jones 3 years, 6 months ago

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

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

Posted by Weiwei Li 3 years, 6 months ago

在 2022/7/18 下午9:47, 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

Thanks a lot.

Sorry for not finding this typo. I'll fix it in next version.

Regards,

Weiwei Li

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

[PATCH V3 1/6] target/riscv: Add check for supported privilege mode combinations
[PATCH V3 2/6] target/riscv: H extension depends on I extension
[PATCH V3 3/6] target/riscv: Fix checkpatch warning may triggered in csr_ops table
[PATCH V3 4/6] target/riscv: Add check for csrs existed with U extension
[PATCH V3 5/6] target/riscv: Fix checks in hmode/hmode32
[PATCH V3 6/6] target/riscv: Simplify the check in hmode to resue the check in riscv_csrrw_check