[PATCH] target/riscv: Fix priority of csr related check in riscv_csrrw

qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed

* [PATCH] target/riscv: Fix priority of csr related check in riscv_csrrw_check
@ 2022-08-03 12:36 Weiwei Li
  2022-08-04  3:38 ` Anup Patel
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Weiwei Li @ 2022-08-03 12:36 UTC (permalink / raw)
  To: palmer, alistair.francis, bin.meng, qemu-riscv, qemu-devel
  Cc: wangjunqiang, lazyparser, Weiwei Li

Normally, riscv_csrrw_check is called when executing Zicsr instructions.
And we can only do access control for existed CSRs. So the priority of
CSR related check, from highest to lowest, should be as follows:
1) check whether Zicsr is supported: raise RISCV_EXCP_ILLEGAL_INST if not
2) check whether csr is existed: raise RISCV_EXCP_ILLEGAL_INST if not
3) do access control: raise RISCV_EXCP_ILLEGAL_INST or RISCV_EXCP_VIRT_
INSTRUCTION_FAULT if not allowed

The predicates contain parts of function of both 2) and 3), So they need
to be placed in the middle of riscv_csrrw_check

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/csr.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 0fb042b2fd..d81f466c80 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3270,6 +3270,30 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
     /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
     int read_only = get_field(csrno, 0xC00) == 3;
     int csr_min_priv = csr_ops[csrno].min_priv_ver;
+
+    /* ensure the CSR extension is enabled. */
+    if (!cpu->cfg.ext_icsr) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    if (env->priv_ver < csr_min_priv) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    /* check predicate */
+    if (!csr_ops[csrno].predicate) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    if (write_mask && read_only) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    RISCVException ret = csr_ops[csrno].predicate(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
 #if !defined(CONFIG_USER_ONLY)
     int csr_priv, effective_priv = env->priv;
 
@@ -3290,25 +3314,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
         return RISCV_EXCP_ILLEGAL_INST;
     }
 #endif
-    if (write_mask && read_only) {
-        return RISCV_EXCP_ILLEGAL_INST;
-    }
-
-    /* ensure the CSR extension is enabled. */
-    if (!cpu->cfg.ext_icsr) {
-        return RISCV_EXCP_ILLEGAL_INST;
-    }
-
-    /* check predicate */
-    if (!csr_ops[csrno].predicate) {
-        return RISCV_EXCP_ILLEGAL_INST;
-    }
-
-    if (env->priv_ver < csr_min_priv) {
-        return RISCV_EXCP_ILLEGAL_INST;
-    }
-
-    return csr_ops[csrno].predicate(env, csrno);
+    return RISCV_EXCP_NONE;
 }
 
 static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] target/riscv: Fix priority of csr related check in riscv_csrrw_check
  2022-08-03 12:36 [PATCH] target/riscv: Fix priority of csr related check in riscv_csrrw_check Weiwei Li
@ 2022-08-04  3:38 ` Anup Patel
  2022-08-04 12:29   ` Weiwei Li
  2022-08-05  4:03 ` Alistair Francis
  2022-08-05  5:02 ` Alistair Francis
  2 siblings, 1 reply; 6+ messages in thread
From: Anup Patel @ 2022-08-04  3:38 UTC (permalink / raw)
  To: Weiwei Li
  Cc: palmer, alistair.francis, bin.meng, qemu-riscv, qemu-devel,
	wangjunqiang, lazyparser

On Wed, Aug 3, 2022 at 6:16 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> Normally, riscv_csrrw_check is called when executing Zicsr instructions.
> And we can only do access control for existed CSRs. So the priority of
> CSR related check, from highest to lowest, should be as follows:
> 1) check whether Zicsr is supported: raise RISCV_EXCP_ILLEGAL_INST if not
> 2) check whether csr is existed: raise RISCV_EXCP_ILLEGAL_INST if not
> 3) do access control: raise RISCV_EXCP_ILLEGAL_INST or RISCV_EXCP_VIRT_
> INSTRUCTION_FAULT if not allowed
>
> The predicates contain parts of function of both 2) and 3), So they need
> to be placed in the middle of riscv_csrrw_check
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
>  target/riscv/csr.c | 44 +++++++++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 0fb042b2fd..d81f466c80 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3270,6 +3270,30 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>      /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
>      int read_only = get_field(csrno, 0xC00) == 3;
>      int csr_min_priv = csr_ops[csrno].min_priv_ver;
> +
> +    /* ensure the CSR extension is enabled. */
> +    if (!cpu->cfg.ext_icsr) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    if (env->priv_ver < csr_min_priv) {
> +        return RISCV_EXCP_ILLEGAL_INST;

This line breaks nested virtualization because for nested virtualization
to work, the guest hypervisor accessing h<xyz> and vs<xyz> CSRs from
VS-mode should result in a virtual instruction trap not illegal
instruction trap.

Regards,
Anup

> +    }
> +
> +    /* check predicate */
> +    if (!csr_ops[csrno].predicate) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    if (write_mask && read_only) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    RISCVException ret = csr_ops[csrno].predicate(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>  #if !defined(CONFIG_USER_ONLY)
>      int csr_priv, effective_priv = env->priv;
>
> @@ -3290,25 +3314,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>          return RISCV_EXCP_ILLEGAL_INST;
>      }
>  #endif
> -    if (write_mask && read_only) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
> -    /* ensure the CSR extension is enabled. */
> -    if (!cpu->cfg.ext_icsr) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
> -    /* check predicate */
> -    if (!csr_ops[csrno].predicate) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
> -    if (env->priv_ver < csr_min_priv) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
> -    return csr_ops[csrno].predicate(env, csrno);
> +    return RISCV_EXCP_NONE;
>  }
>
>  static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
> --
> 2.17.1
>
>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] target/riscv: Fix priority of csr related check in riscv_csrrw_check
  2022-08-04  3:38 ` Anup Patel
@ 2022-08-04 12:29   ` Weiwei Li
  2022-08-04 16:31     ` Anup Patel
  0 siblings, 1 reply; 6+ messages in thread
From: Weiwei Li @ 2022-08-04 12:29 UTC (permalink / raw)
  To: Anup Patel
  Cc: palmer, alistair.francis, bin.meng, qemu-riscv, qemu-devel,
	wangjunqiang, lazyparser


在 2022/8/4 上午11:38, Anup Patel 写道:
> On Wed, Aug 3, 2022 at 6:16 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>> Normally, riscv_csrrw_check is called when executing Zicsr instructions.
>> And we can only do access control for existed CSRs. So the priority of
>> CSR related check, from highest to lowest, should be as follows:
>> 1) check whether Zicsr is supported: raise RISCV_EXCP_ILLEGAL_INST if not
>> 2) check whether csr is existed: raise RISCV_EXCP_ILLEGAL_INST if not
>> 3) do access control: raise RISCV_EXCP_ILLEGAL_INST or RISCV_EXCP_VIRT_
>> INSTRUCTION_FAULT if not allowed
>>
>> The predicates contain parts of function of both 2) and 3), So they need
>> to be placed in the middle of riscv_csrrw_check
>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
>>   target/riscv/csr.c | 44 +++++++++++++++++++++++++-------------------
>>   1 file changed, 25 insertions(+), 19 deletions(-)
>>
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index 0fb042b2fd..d81f466c80 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -3270,6 +3270,30 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>>       /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
>>       int read_only = get_field(csrno, 0xC00) == 3;
>>       int csr_min_priv = csr_ops[csrno].min_priv_ver;
>> +
>> +    /* ensure the CSR extension is enabled. */
>> +    if (!cpu->cfg.ext_icsr) {
>> +        return RISCV_EXCP_ILLEGAL_INST;
>> +    }
>> +
>> +    if (env->priv_ver < csr_min_priv) {
>> +        return RISCV_EXCP_ILLEGAL_INST;
> This line breaks nested virtualization because for nested virtualization
> to work, the guest hypervisor accessing h<xyz> and vs<xyz> CSRs from
> VS-mode should result in a virtual instruction trap not illegal
> instruction trap.
>
> Regards,
> Anup

Do you mean "if (env->priv_ver < csr_min_priv)" ?

If so, I think illegal instruction trap is better, since the csr is not 
implemented(or existed) when

env->priv_ver < csr_min_priv and virtual instruction trap is only raised 
for implemented csr access.

Regards,

Weiwei Li

>> +    }
>> +
>> +    /* check predicate */
>> +    if (!csr_ops[csrno].predicate) {
>> +        return RISCV_EXCP_ILLEGAL_INST;
>> +    }
>> +
>> +    if (write_mask && read_only) {
>> +        return RISCV_EXCP_ILLEGAL_INST;
>> +    }
>> +
>> +    RISCVException ret = csr_ops[csrno].predicate(env, csrno);
>> +    if (ret != RISCV_EXCP_NONE) {
>> +        return ret;
>> +    }
>> +
>>   #if !defined(CONFIG_USER_ONLY)
>>       int csr_priv, effective_priv = env->priv;
>>
>> @@ -3290,25 +3314,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>>           return RISCV_EXCP_ILLEGAL_INST;
>>       }
>>   #endif
>> -    if (write_mask && read_only) {
>> -        return RISCV_EXCP_ILLEGAL_INST;
>> -    }
>> -
>> -    /* ensure the CSR extension is enabled. */
>> -    if (!cpu->cfg.ext_icsr) {
>> -        return RISCV_EXCP_ILLEGAL_INST;
>> -    }
>> -
>> -    /* check predicate */
>> -    if (!csr_ops[csrno].predicate) {
>> -        return RISCV_EXCP_ILLEGAL_INST;
>> -    }
>> -
>> -    if (env->priv_ver < csr_min_priv) {
>> -        return RISCV_EXCP_ILLEGAL_INST;
>> -    }
>> -
>> -    return csr_ops[csrno].predicate(env, csrno);
>> +    return RISCV_EXCP_NONE;
>>   }
>>
>>   static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
>> --
>> 2.17.1
>>
>>



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] target/riscv: Fix priority of csr related check in riscv_csrrw_check
  2022-08-04 12:29   ` Weiwei Li
@ 2022-08-04 16:31     ` Anup Patel
  0 siblings, 0 replies; 6+ messages in thread
From: Anup Patel @ 2022-08-04 16:31 UTC (permalink / raw)
  To: Weiwei Li
  Cc: palmer, alistair.francis, bin.meng, qemu-riscv, qemu-devel,
	wangjunqiang, lazyparser

On Thu, Aug 4, 2022 at 5:59 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
>
> 在 2022/8/4 上午11:38, Anup Patel 写道:
> > On Wed, Aug 3, 2022 at 6:16 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
> >> Normally, riscv_csrrw_check is called when executing Zicsr instructions.
> >> And we can only do access control for existed CSRs. So the priority of
> >> CSR related check, from highest to lowest, should be as follows:
> >> 1) check whether Zicsr is supported: raise RISCV_EXCP_ILLEGAL_INST if not
> >> 2) check whether csr is existed: raise RISCV_EXCP_ILLEGAL_INST if not
> >> 3) do access control: raise RISCV_EXCP_ILLEGAL_INST or RISCV_EXCP_VIRT_
> >> INSTRUCTION_FAULT if not allowed
> >>
> >> The predicates contain parts of function of both 2) and 3), So they need
> >> to be placed in the middle of riscv_csrrw_check
> >>
> >> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> >> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> >> ---
> >>   target/riscv/csr.c | 44 +++++++++++++++++++++++++-------------------
> >>   1 file changed, 25 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index 0fb042b2fd..d81f466c80 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -3270,6 +3270,30 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
> >>       /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
> >>       int read_only = get_field(csrno, 0xC00) == 3;
> >>       int csr_min_priv = csr_ops[csrno].min_priv_ver;
> >> +
> >> +    /* ensure the CSR extension is enabled. */
> >> +    if (!cpu->cfg.ext_icsr) {
> >> +        return RISCV_EXCP_ILLEGAL_INST;
> >> +    }
> >> +
> >> +    if (env->priv_ver < csr_min_priv) {
> >> +        return RISCV_EXCP_ILLEGAL_INST;
> > This line breaks nested virtualization because for nested virtualization
> > to work, the guest hypervisor accessing h<xyz> and vs<xyz> CSRs from
> > VS-mode should result in a virtual instruction trap not illegal
> > instruction trap.
> >
> > Regards,
> > Anup
>
> Do you mean "if (env->priv_ver < csr_min_priv)" ?

I got confused with the csr_min_priv name. This variable holds
min priv spec verison and not the privilege level required for
the CSR.

No issues with the "if (env->priv_ver < csr_min_priv)" check.

Regards,
Anup

>
> If so, I think illegal instruction trap is better, since the csr is not
> implemented(or existed) when
>
> env->priv_ver < csr_min_priv and virtual instruction trap is only raised
> for implemented csr access.
>
> Regards,
>
> Weiwei Li
>
> >> +    }
> >> +
> >> +    /* check predicate */
> >> +    if (!csr_ops[csrno].predicate) {
> >> +        return RISCV_EXCP_ILLEGAL_INST;
> >> +    }
> >> +
> >> +    if (write_mask && read_only) {
> >> +        return RISCV_EXCP_ILLEGAL_INST;
> >> +    }
> >> +
> >> +    RISCVException ret = csr_ops[csrno].predicate(env, csrno);
> >> +    if (ret != RISCV_EXCP_NONE) {
> >> +        return ret;
> >> +    }
> >> +
> >>   #if !defined(CONFIG_USER_ONLY)
> >>       int csr_priv, effective_priv = env->priv;
> >>
> >> @@ -3290,25 +3314,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
> >>           return RISCV_EXCP_ILLEGAL_INST;
> >>       }
> >>   #endif
> >> -    if (write_mask && read_only) {
> >> -        return RISCV_EXCP_ILLEGAL_INST;
> >> -    }
> >> -
> >> -    /* ensure the CSR extension is enabled. */
> >> -    if (!cpu->cfg.ext_icsr) {
> >> -        return RISCV_EXCP_ILLEGAL_INST;
> >> -    }
> >> -
> >> -    /* check predicate */
> >> -    if (!csr_ops[csrno].predicate) {
> >> -        return RISCV_EXCP_ILLEGAL_INST;
> >> -    }
> >> -
> >> -    if (env->priv_ver < csr_min_priv) {
> >> -        return RISCV_EXCP_ILLEGAL_INST;
> >> -    }
> >> -
> >> -    return csr_ops[csrno].predicate(env, csrno);
> >> +    return RISCV_EXCP_NONE;
> >>   }
> >>
> >>   static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
> >> --
> >> 2.17.1
> >>
> >>
>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] target/riscv: Fix priority of csr related check in riscv_csrrw_check
  2022-08-03 12:36 [PATCH] target/riscv: Fix priority of csr related check in riscv_csrrw_check Weiwei Li
  2022-08-04  3:38 ` Anup Patel
@ 2022-08-05  4:03 ` Alistair Francis
  2022-08-05  5:02 ` Alistair Francis
  2 siblings, 0 replies; 6+ messages in thread
From: Alistair Francis @ 2022-08-05  4:03 UTC (permalink / raw)
  To: Weiwei Li
  Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, open list:RISC-V,
	qemu-devel@nongnu.org Developers, wangjunqiang,
	Wei Wu (吴伟)

On Wed, Aug 3, 2022 at 10:56 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> Normally, riscv_csrrw_check is called when executing Zicsr instructions.
> And we can only do access control for existed CSRs. So the priority of
> CSR related check, from highest to lowest, should be as follows:
> 1) check whether Zicsr is supported: raise RISCV_EXCP_ILLEGAL_INST if not
> 2) check whether csr is existed: raise RISCV_EXCP_ILLEGAL_INST if not
> 3) do access control: raise RISCV_EXCP_ILLEGAL_INST or RISCV_EXCP_VIRT_
> INSTRUCTION_FAULT if not allowed
>
> The predicates contain parts of function of both 2) and 3), So they need
> to be placed in the middle of riscv_csrrw_check
>
> 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 | 44 +++++++++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 0fb042b2fd..d81f466c80 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3270,6 +3270,30 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>      /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
>      int read_only = get_field(csrno, 0xC00) == 3;
>      int csr_min_priv = csr_ops[csrno].min_priv_ver;
> +
> +    /* ensure the CSR extension is enabled. */
> +    if (!cpu->cfg.ext_icsr) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    if (env->priv_ver < csr_min_priv) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    /* check predicate */
> +    if (!csr_ops[csrno].predicate) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    if (write_mask && read_only) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    RISCVException ret = csr_ops[csrno].predicate(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>  #if !defined(CONFIG_USER_ONLY)
>      int csr_priv, effective_priv = env->priv;
>
> @@ -3290,25 +3314,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>          return RISCV_EXCP_ILLEGAL_INST;
>      }
>  #endif
> -    if (write_mask && read_only) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
> -    /* ensure the CSR extension is enabled. */
> -    if (!cpu->cfg.ext_icsr) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
> -    /* check predicate */
> -    if (!csr_ops[csrno].predicate) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
> -    if (env->priv_ver < csr_min_priv) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
> -    return csr_ops[csrno].predicate(env, csrno);
> +    return RISCV_EXCP_NONE;
>  }
>
>  static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
> --
> 2.17.1
>
>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] target/riscv: Fix priority of csr related check in riscv_csrrw_check
  2022-08-03 12:36 [PATCH] target/riscv: Fix priority of csr related check in riscv_csrrw_check Weiwei Li
  2022-08-04  3:38 ` Anup Patel
  2022-08-05  4:03 ` Alistair Francis
@ 2022-08-05  5:02 ` Alistair Francis
  2 siblings, 0 replies; 6+ messages in thread
From: Alistair Francis @ 2022-08-05  5:02 UTC (permalink / raw)
  To: Weiwei Li
  Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, open list:RISC-V,
	qemu-devel@nongnu.org Developers, wangjunqiang,
	Wei Wu (吴伟)

On Wed, Aug 3, 2022 at 10:56 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> Normally, riscv_csrrw_check is called when executing Zicsr instructions.
> And we can only do access control for existed CSRs. So the priority of
> CSR related check, from highest to lowest, should be as follows:
> 1) check whether Zicsr is supported: raise RISCV_EXCP_ILLEGAL_INST if not
> 2) check whether csr is existed: raise RISCV_EXCP_ILLEGAL_INST if not
> 3) do access control: raise RISCV_EXCP_ILLEGAL_INST or RISCV_EXCP_VIRT_
> INSTRUCTION_FAULT if not allowed
>
> The predicates contain parts of function of both 2) and 3), So they need
> to be placed in the middle of riscv_csrrw_check
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  target/riscv/csr.c | 44 +++++++++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 0fb042b2fd..d81f466c80 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3270,6 +3270,30 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>      /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
>      int read_only = get_field(csrno, 0xC00) == 3;
>      int csr_min_priv = csr_ops[csrno].min_priv_ver;
> +
> +    /* ensure the CSR extension is enabled. */
> +    if (!cpu->cfg.ext_icsr) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    if (env->priv_ver < csr_min_priv) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    /* check predicate */
> +    if (!csr_ops[csrno].predicate) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    if (write_mask && read_only) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    RISCVException ret = csr_ops[csrno].predicate(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>  #if !defined(CONFIG_USER_ONLY)
>      int csr_priv, effective_priv = env->priv;
>
> @@ -3290,25 +3314,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>          return RISCV_EXCP_ILLEGAL_INST;
>      }
>  #endif
> -    if (write_mask && read_only) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
> -    /* ensure the CSR extension is enabled. */
> -    if (!cpu->cfg.ext_icsr) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
> -    /* check predicate */
> -    if (!csr_ops[csrno].predicate) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
> -    if (env->priv_ver < csr_min_priv) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
> -    return csr_ops[csrno].predicate(env, csrno);
> +    return RISCV_EXCP_NONE;
>  }
>
>  static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
> --
> 2.17.1
>
>


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-08-05  5:04 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-03 12:36 [PATCH] target/riscv: Fix priority of csr related check in riscv_csrrw_check Weiwei Li
2022-08-04  3:38 ` Anup Patel
2022-08-04 12:29   ` Weiwei Li
2022-08-04 16:31     ` Anup Patel
2022-08-05  4:03 ` Alistair Francis
2022-08-05  5:02 ` Alistair Francis

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).