diff mbox series

target/riscv: Fix priority of csr related check in riscv_csrrw_check

Message ID 20220803123652.3700-1-liweiwei@iscas.ac.cn (mailing list archive)
State New, archived
Headers show
Series target/riscv: Fix priority of csr related check in riscv_csrrw_check | expand

Commit Message

Weiwei Li Aug. 3, 2022, 12:36 p.m. UTC 
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(-)

Comments

Anup Patel Aug. 4, 2022, 3:38 a.m. UTC | #1 
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
>
>
Weiwei Li Aug. 4, 2022, 12:29 p.m. UTC | #2 
在 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
>>
>>
Anup Patel Aug. 4, 2022, 4:31 p.m. UTC | #3 
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
> >>
> >>
>
Alistair Francis Aug. 5, 2022, 4:03 a.m. UTC | #4 
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
>
>
Alistair Francis Aug. 5, 2022, 5:02 a.m. UTC | #5 
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
>
>
diff mbox series

Patch

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,