[PATCH 00/18] target/riscv: Various fixes to gdbstub and CSR access

[PATCH 00/18] target/riscv: Various fixes to gdbstub and CSR access

[Bin Meng]

At present gdbstub reports an incorrect / incomplete CSR list in the
target description XML, for example:

- menvcfg is reported in 'sifive_u' machine
- fcsr is missing in a F/D enabled processor

The issue is caused by:
- priv spec version check is missing when reporting CSRs
- CSR predicate() routine is called without turning on the debugger flag

This series aims to generate a correct and complete CSR list for gdbstub.


Bin Meng (18):
  target/riscv: gdbstub: Check priv spec version before reporting CSR
  target/riscv: Correct the priority policy of riscv_csrrw_check()
  target/riscv: gdbstub: Minor change for better readability
  target/riscv: gdbstub: Do not generate CSR XML if Zicsr is disabled
  target/riscv: Coding style fixes in csr.c
  target/riscv: Use 'bool' type for read_only
  target/riscv: Simplify {read,write}_pmpcfg() a little bit
  target/riscv: Simplify getting RISCVCPU pointer from env
  target/riscv: Avoid reporting odd-numbered pmpcfgX in the CSR XML for
    RV64
  target/riscv: gdbstub: Turn on debugger mode before calling CSR
    predicate()
  target/riscv: gdbstub: Drop the vector CSRs in riscv-vector.xml
  target/riscv: Allow debugger to access user timer and counter CSRs
  target/riscv: Allow debugger to access seed CSR
  target/riscv: Allow debugger to access {h,s}stateen CSRs
  target/riscv: Allow debugger to access sstc CSRs
  target/riscv: Drop priv level check in mseccfg predicate()
  target/riscv: Group all predicate() routines together
  target/riscv: Move configuration check to envcfg CSRs predicate()

 target/riscv/csr.c     | 360 +++++++++++++++++++++++------------------
 target/riscv/gdbstub.c | 100 +++---------
 2 files changed, 221 insertions(+), 239 deletions(-)

-- 
2.25.1

[PATCH 01/18] target/riscv: gdbstub: Check priv spec version before reporting CSR

[Bin Meng]

The gdbstub CSR XML is dynamically generated according to the result
of the CSR predicate() result. This has been working fine until
commit 7100fe6c2441 ("target/riscv: Enable privileged spec version 1.12")
introduced the privilege spec version check in riscv_csrrw_check().

When debugging the 'sifive_u' machine whose priv spec is at 1.10,
gdbstub reports priv spec 1.12 CSRs like menvcfg in the XML, hence
we see "remote failure reply 'E14'" message when examining all CSRs
via "info register system" from gdb.

Add the priv spec version check in the CSR XML generation logic to
fix this issue.

Fixes: 7100fe6c2441 ("target/riscv: Enable privileged spec version 1.12")
Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/gdbstub.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index 6e7bbdbd5e..e57372db38 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -290,6 +290,9 @@ static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
     g_string_append_printf(s, "<feature name=\"org.gnu.gdb.riscv.csr\">");
 
     for (i = 0; i < CSR_TABLE_SIZE; i++) {
+        if (env->priv_ver < csr_ops[i].min_priv_ver) {
+            continue;
+        }
         predicate = csr_ops[i].predicate;
         if (predicate && (predicate(env, i) == RISCV_EXCP_NONE)) {
             if (csr_ops[i].name) {
-- 
2.25.1

Re: [PATCH 01/18] target/riscv: gdbstub: Check priv spec version before reporting CSR

[weiwei]

On 2023/2/14 02:01, Bin Meng wrote:
> The gdbstub CSR XML is dynamically generated according to the result
> of the CSR predicate() result. This has been working fine until
> commit 7100fe6c2441 ("target/riscv: Enable privileged spec version 1.12")
> introduced the privilege spec version check in riscv_csrrw_check().
>
> When debugging the 'sifive_u' machine whose priv spec is at 1.10,
> gdbstub reports priv spec 1.12 CSRs like menvcfg in the XML, hence
> we see "remote failure reply 'E14'" message when examining all CSRs
> via "info register system" from gdb.
>
> Add the priv spec version check in the CSR XML generation logic to
> fix this issue.
>
> Fixes: 7100fe6c2441 ("target/riscv: Enable privileged spec version 1.12")
> Signed-off-by: Bin Meng <bmeng@tinylab.org>

Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,
Weiwei Li
> ---
>
>   target/riscv/gdbstub.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index 6e7bbdbd5e..e57372db38 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -290,6 +290,9 @@ static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
>       g_string_append_printf(s, "<feature name=\"org.gnu.gdb.riscv.csr\">");
>   
>       for (i = 0; i < CSR_TABLE_SIZE; i++) {
> +        if (env->priv_ver < csr_ops[i].min_priv_ver) {
> +            continue;
> +        }
>           predicate = csr_ops[i].predicate;
>           if (predicate && (predicate(env, i) == RISCV_EXCP_NONE)) {
>               if (csr_ops[i].name) {

Re: [PATCH 01/18] target/riscv: gdbstub: Check priv spec version before reporting CSR

[LIU Zhiwei]

On 2023/2/14 2:01, Bin Meng wrote:
> The gdbstub CSR XML is dynamically generated according to the result
> of the CSR predicate() result. This has been working fine until
> commit 7100fe6c2441 ("target/riscv: Enable privileged spec version 1.12")
> introduced the privilege spec version check in riscv_csrrw_check().
>
> When debugging the 'sifive_u' machine whose priv spec is at 1.10,
> gdbstub reports priv spec 1.12 CSRs like menvcfg in the XML, hence
> we see "remote failure reply 'E14'" message when examining all CSRs
> via "info register system" from gdb.
>
> Add the priv spec version check in the CSR XML generation logic to
> fix this issue.
>
> Fixes: 7100fe6c2441 ("target/riscv: Enable privileged spec version 1.12")
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/gdbstub.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index 6e7bbdbd5e..e57372db38 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -290,6 +290,9 @@ static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
>       g_string_append_printf(s, "<feature name=\"org.gnu.gdb.riscv.csr\">");
>   
>       for (i = 0; i < CSR_TABLE_SIZE; i++) {
> +        if (env->priv_ver < csr_ops[i].min_priv_ver) {
> +            continue;
> +        }
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>           predicate = csr_ops[i].predicate;
>           if (predicate && (predicate(env, i) == RISCV_EXCP_NONE)) {
>               if (csr_ops[i].name) {

[PATCH 02/18] target/riscv: Correct the priority policy of riscv_csrrw_check()

[Bin Meng]

The priority policy of riscv_csrrw_check() was once adjusted in
commit eacaf4401956 ("target/riscv: Fix priority of csr related check in riscv_csrrw_check")
whose commit message says the CSR existence check should come
before the access control check, but the code changes did not
agree with the commit message, that the predicate() check came
after the read / write check.

Fixes: eacaf4401956 ("target/riscv: Fix priority of csr related check in riscv_csrrw_check")
Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/csr.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 1b0a0c1693..c2dd9d5af0 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3793,15 +3793,15 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
         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 (write_mask && read_only) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
 #if !defined(CONFIG_USER_ONLY)
     int csr_priv, effective_priv = env->priv;
 
-- 
2.25.1

Re: [PATCH 02/18] target/riscv: Correct the priority policy of riscv_csrrw_check()

[weiwei]

On 2023/2/14 02:01, Bin Meng wrote:
> The priority policy of riscv_csrrw_check() was once adjusted in
> commit eacaf4401956 ("target/riscv: Fix priority of csr related check in riscv_csrrw_check")
> whose commit message says the CSR existence check should come
> before the access control check, but the code changes did not
> agree with the commit message, that the predicate() check came
> after the read / write check.
Hi Bin Meng,

Let me explain why I put read-only check before predicate() check in 
commit eacaf4401956:

*  The predicates don't do existence check only. They also do some 
access-control check , and

     will trigger virtual instruction exception in come cases.  I think 
read-only check should be done

     before these access-control check, and trigger illegal instruction 
exception instead of virtual

    instruction exception when writing to read-only CSRs in these cases.

  *  Read-only  check will trigger ILLEGAL_INST exception which is  also 
the exception triggered when

      CSR is not existed, so put this check before existence check will 
not affect the final exception.

Regards,

Weiwei Li

> Fixes: eacaf4401956 ("target/riscv: Fix priority of csr related check in riscv_csrrw_check")
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/csr.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 1b0a0c1693..c2dd9d5af0 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3793,15 +3793,15 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>           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 (write_mask && read_only) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
>   #if !defined(CONFIG_USER_ONLY)
>       int csr_priv, effective_priv = env->priv;
>   

Re: [PATCH 02/18] target/riscv: Correct the priority policy of riscv_csrrw_check()

[LIU Zhiwei]

On 2023/2/14 2:01, Bin Meng wrote:
> The priority policy of riscv_csrrw_check() was once adjusted in
> commit eacaf4401956 ("target/riscv: Fix priority of csr related check in riscv_csrrw_check")
> whose commit message says the CSR existence check should come
> before the access control check, but the code changes did not
> agree with the commit message, that the predicate() check came
> after the read / write check.
>
> Fixes: eacaf4401956 ("target/riscv: Fix priority of csr related check in riscv_csrrw_check")
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/csr.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 1b0a0c1693..c2dd9d5af0 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3793,15 +3793,15 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>           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 (write_mask && read_only) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>   #if !defined(CONFIG_USER_ONLY)
>       int csr_priv, effective_priv = env->priv;
>   

[PATCH 03/18] target/riscv: gdbstub: Minor change for better readability

[Bin Meng]

Use a variable 'base_reg' to represent cs->gdb_num_regs so that
the call to ricsv_gen_dynamic_vector_xml() can be placed in one
single line for better readability.

Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/gdbstub.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index e57372db38..704f3d6922 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -385,9 +385,9 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
                                  32, "riscv-32bit-fpu.xml", 0);
     }
     if (env->misa_ext & RVV) {
+        int base_reg = cs->gdb_num_regs;
         gdb_register_coprocessor(cs, riscv_gdb_get_vector, riscv_gdb_set_vector,
-                                 ricsv_gen_dynamic_vector_xml(cs,
-                                                              cs->gdb_num_regs),
+                                 ricsv_gen_dynamic_vector_xml(cs, base_reg),
                                  "riscv-vector.xml", 0);
     }
     switch (env->misa_mxl_max) {
-- 
2.25.1

Re: [PATCH 03/18] target/riscv: gdbstub: Minor change for better readability

[weiwei]

On 2023/2/14 02:01, Bin Meng wrote:
> Use a variable 'base_reg' to represent cs->gdb_num_regs so that
> the call to ricsv_gen_dynamic_vector_xml() can be placed in one
> single line for better readability.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,
Weiwei Li
> ---
>
>   target/riscv/gdbstub.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index e57372db38..704f3d6922 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -385,9 +385,9 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
>                                    32, "riscv-32bit-fpu.xml", 0);
>       }
>       if (env->misa_ext & RVV) {
> +        int base_reg = cs->gdb_num_regs;
>           gdb_register_coprocessor(cs, riscv_gdb_get_vector, riscv_gdb_set_vector,
> -                                 ricsv_gen_dynamic_vector_xml(cs,
> -                                                              cs->gdb_num_regs),
> +                                 ricsv_gen_dynamic_vector_xml(cs, base_reg),
>                                    "riscv-vector.xml", 0);
>       }
>       switch (env->misa_mxl_max) {

Re: [PATCH 03/18] target/riscv: gdbstub: Minor change for better readability

[LIU Zhiwei]

On 2023/2/14 2:01, Bin Meng wrote:
> Use a variable 'base_reg' to represent cs->gdb_num_regs so that
> the call to ricsv_gen_dynamic_vector_xml() can be placed in one
> single line for better readability.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/gdbstub.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index e57372db38..704f3d6922 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -385,9 +385,9 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
>                                    32, "riscv-32bit-fpu.xml", 0);
>       }
>       if (env->misa_ext & RVV) {
> +        int base_reg = cs->gdb_num_regs;
>           gdb_register_coprocessor(cs, riscv_gdb_get_vector, riscv_gdb_set_vector,
> -                                 ricsv_gen_dynamic_vector_xml(cs,
> -                                                              cs->gdb_num_regs),
> +                                 ricsv_gen_dynamic_vector_xml(cs, base_reg),

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>                                    "riscv-vector.xml", 0);
>       }
>       switch (env->misa_mxl_max) {

[PATCH 04/18] target/riscv: gdbstub: Do not generate CSR XML if Zicsr is disabled

[Bin Meng]

There is no need to generate the CSR XML if the Zicsr extension
is not enabled.

Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/gdbstub.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index 704f3d6922..294f0ceb1c 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -406,7 +406,10 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
         g_assert_not_reached();
     }
 
-    gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
-                             riscv_gen_dynamic_csr_xml(cs, cs->gdb_num_regs),
-                             "riscv-csr.xml", 0);
+    if (cpu->cfg.ext_icsr) {
+        int base_reg = cs->gdb_num_regs;
+        gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
+                                 riscv_gen_dynamic_csr_xml(cs, base_reg),
+                                 "riscv-csr.xml", 0);
+    }
 }
-- 
2.25.1

Re: [PATCH 04/18] target/riscv: gdbstub: Do not generate CSR XML if Zicsr is disabled

[weiwei]

On 2023/2/14 02:02, Bin Meng wrote:
> There is no need to generate the CSR XML if the Zicsr extension
> is not enabled.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,
Weiwei Li
> ---
>
>   target/riscv/gdbstub.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index 704f3d6922..294f0ceb1c 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -406,7 +406,10 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
>           g_assert_not_reached();
>       }
>   
> -    gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
> -                             riscv_gen_dynamic_csr_xml(cs, cs->gdb_num_regs),
> -                             "riscv-csr.xml", 0);
> +    if (cpu->cfg.ext_icsr) {
> +        int base_reg = cs->gdb_num_regs;
> +        gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
> +                                 riscv_gen_dynamic_csr_xml(cs, base_reg),
> +                                 "riscv-csr.xml", 0);
> +    }
>   }

Re: [PATCH 04/18] target/riscv: gdbstub: Do not generate CSR XML if Zicsr is disabled

[LIU Zhiwei]

On 2023/2/14 2:02, Bin Meng wrote:
> There is no need to generate the CSR XML if the Zicsr extension
> is not enabled.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/gdbstub.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index 704f3d6922..294f0ceb1c 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -406,7 +406,10 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
>           g_assert_not_reached();
>       }
>   
> -    gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
> -                             riscv_gen_dynamic_csr_xml(cs, cs->gdb_num_regs),
> -                             "riscv-csr.xml", 0);
> +    if (cpu->cfg.ext_icsr) {
> +        int base_reg = cs->gdb_num_regs;
> +        gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
> +                                 riscv_gen_dynamic_csr_xml(cs, base_reg),
> +                                 "riscv-csr.xml", 0);
> +    }

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>   }

[PATCH 05/18] target/riscv: Coding style fixes in csr.c

[Bin Meng]

Fix various places that violate QEMU coding style:

- correct multi-line comment format
- indent to opening parenthesis

Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/csr.c | 62 ++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 30 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index c2dd9d5af0..cc74819759 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -963,7 +963,7 @@ static RISCVException sstc_32(CPURISCVState *env, int csrno)
 }
 
 static RISCVException read_vstimecmp(CPURISCVState *env, int csrno,
-                                    target_ulong *val)
+                                     target_ulong *val)
 {
     *val = env->vstimecmp;
 
@@ -971,7 +971,7 @@ static RISCVException read_vstimecmp(CPURISCVState *env, int csrno,
 }
 
 static RISCVException read_vstimecmph(CPURISCVState *env, int csrno,
-                                    target_ulong *val)
+                                      target_ulong *val)
 {
     *val = env->vstimecmp >> 32;
 
@@ -979,7 +979,7 @@ static RISCVException read_vstimecmph(CPURISCVState *env, int csrno,
 }
 
 static RISCVException write_vstimecmp(CPURISCVState *env, int csrno,
-                                    target_ulong val)
+                                      target_ulong val)
 {
     RISCVCPU *cpu = env_archcpu(env);
 
@@ -996,7 +996,7 @@ static RISCVException write_vstimecmp(CPURISCVState *env, int csrno,
 }
 
 static RISCVException write_vstimecmph(CPURISCVState *env, int csrno,
-                                    target_ulong val)
+                                       target_ulong val)
 {
     RISCVCPU *cpu = env_archcpu(env);
 
@@ -1020,7 +1020,7 @@ static RISCVException read_stimecmp(CPURISCVState *env, int csrno,
 }
 
 static RISCVException read_stimecmph(CPURISCVState *env, int csrno,
-                                    target_ulong *val)
+                                     target_ulong *val)
 {
     if (riscv_cpu_virt_enabled(env)) {
         *val = env->vstimecmp >> 32;
@@ -1032,7 +1032,7 @@ static RISCVException read_stimecmph(CPURISCVState *env, int csrno,
 }
 
 static RISCVException write_stimecmp(CPURISCVState *env, int csrno,
-                                    target_ulong val)
+                                     target_ulong val)
 {
     RISCVCPU *cpu = env_archcpu(env);
 
@@ -1055,7 +1055,7 @@ static RISCVException write_stimecmp(CPURISCVState *env, int csrno,
 }
 
 static RISCVException write_stimecmph(CPURISCVState *env, int csrno,
-                                    target_ulong val)
+                                      target_ulong val)
 {
     RISCVCPU *cpu = env_archcpu(env);
 
@@ -1342,7 +1342,8 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
 
     /* 'E' excludes all other extensions */
     if (val & RVE) {
-        /* when we support 'E' we can do "val = RVE;" however
+        /*
+         * when we support 'E' we can do "val = RVE;" however
          * for now we just drop writes if 'E' is present.
          */
         return RISCV_EXCP_NONE;
@@ -1364,7 +1365,8 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
         val &= ~RVD;
     }
 
-    /* Suppress 'C' if next instruction is not aligned
+    /*
+     * Suppress 'C' if next instruction is not aligned
      * TODO: this should check next_pc
      */
     if ((val & RVC) && (GETPC() & ~3) != 0) {
@@ -1833,28 +1835,28 @@ static RISCVException write_mscratch(CPURISCVState *env, int csrno,
 }
 
 static RISCVException read_mepc(CPURISCVState *env, int csrno,
-                                     target_ulong *val)
+                                target_ulong *val)
 {
     *val = env->mepc;
     return RISCV_EXCP_NONE;
 }
 
 static RISCVException write_mepc(CPURISCVState *env, int csrno,
-                                     target_ulong val)
+                                 target_ulong val)
 {
     env->mepc = val;
     return RISCV_EXCP_NONE;
 }
 
 static RISCVException read_mcause(CPURISCVState *env, int csrno,
-                                     target_ulong *val)
+                                  target_ulong *val)
 {
     *val = env->mcause;
     return RISCV_EXCP_NONE;
 }
 
 static RISCVException write_mcause(CPURISCVState *env, int csrno,
-                                     target_ulong val)
+                                   target_ulong val)
 {
     env->mcause = val;
     return RISCV_EXCP_NONE;
@@ -1876,14 +1878,14 @@ static RISCVException write_mtval(CPURISCVState *env, int csrno,
 
 /* Execution environment configuration setup */
 static RISCVException read_menvcfg(CPURISCVState *env, int csrno,
-                                 target_ulong *val)
+                                   target_ulong *val)
 {
     *val = env->menvcfg;
     return RISCV_EXCP_NONE;
 }
 
 static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
-                                  target_ulong val)
+                                    target_ulong val)
 {
     uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE | MENVCFG_CBZE;
 
@@ -1896,14 +1898,14 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
 }
 
 static RISCVException read_menvcfgh(CPURISCVState *env, int csrno,
-                                 target_ulong *val)
+                                    target_ulong *val)
 {
     *val = env->menvcfg >> 32;
     return RISCV_EXCP_NONE;
 }
 
 static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
-                                  target_ulong val)
+                                     target_ulong val)
 {
     uint64_t mask = MENVCFG_PBMTE | MENVCFG_STCE;
     uint64_t valh = (uint64_t)val << 32;
@@ -1914,7 +1916,7 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
 }
 
 static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
-                                 target_ulong *val)
+                                   target_ulong *val)
 {
     RISCVException ret;
 
@@ -1928,7 +1930,7 @@ static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
 }
 
 static RISCVException write_senvcfg(CPURISCVState *env, int csrno,
-                                  target_ulong val)
+                                    target_ulong val)
 {
     uint64_t mask = SENVCFG_FIOM | SENVCFG_CBIE | SENVCFG_CBCFE | SENVCFG_CBZE;
     RISCVException ret;
@@ -1943,7 +1945,7 @@ static RISCVException write_senvcfg(CPURISCVState *env, int csrno,
 }
 
 static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
-                                 target_ulong *val)
+                                   target_ulong *val)
 {
     RISCVException ret;
 
@@ -1957,7 +1959,7 @@ static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
 }
 
 static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
-                                  target_ulong val)
+                                    target_ulong val)
 {
     uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
     RISCVException ret;
@@ -1977,7 +1979,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
 }
 
 static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
-                                 target_ulong *val)
+                                    target_ulong *val)
 {
     RISCVException ret;
 
@@ -1991,7 +1993,7 @@ static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
 }
 
 static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
-                                  target_ulong val)
+                                     target_ulong val)
 {
     uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE;
     uint64_t valh = (uint64_t)val << 32;
@@ -2034,13 +2036,13 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
 }
 
 static RISCVException write_mstateen_1_3(CPURISCVState *env, int csrno,
-                                      target_ulong new_val)
+                                         target_ulong new_val)
 {
     return write_mstateen(env, csrno, SMSTATEEN_STATEEN, new_val);
 }
 
 static RISCVException read_mstateenh(CPURISCVState *env, int csrno,
-                                      target_ulong *val)
+                                     target_ulong *val)
 {
     *val = env->mstateen[csrno - CSR_MSTATEEN0H] >> 32;
 
@@ -2061,7 +2063,7 @@ static RISCVException write_mstateenh(CPURISCVState *env, int csrno,
 }
 
 static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
-                                      target_ulong new_val)
+                                       target_ulong new_val)
 {
     uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
@@ -2069,7 +2071,7 @@ static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
 }
 
 static RISCVException write_mstateenh_1_3(CPURISCVState *env, int csrno,
-                                      target_ulong new_val)
+                                          target_ulong new_val)
 {
     return write_mstateenh(env, csrno, SMSTATEEN_STATEEN, new_val);
 }
@@ -2106,7 +2108,7 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
 }
 
 static RISCVException write_hstateen_1_3(CPURISCVState *env, int csrno,
-                                      target_ulong new_val)
+                                         target_ulong new_val)
 {
     return write_hstateen(env, csrno, SMSTATEEN_STATEEN, new_val);
 }
@@ -2145,7 +2147,7 @@ static RISCVException write_hstateen0h(CPURISCVState *env, int csrno,
 }
 
 static RISCVException write_hstateenh_1_3(CPURISCVState *env, int csrno,
-                                       target_ulong new_val)
+                                          target_ulong new_val)
 {
     return write_hstateenh(env, csrno, SMSTATEEN_STATEEN, new_val);
 }
@@ -3338,7 +3340,7 @@ static RISCVException read_mseccfg(CPURISCVState *env, int csrno,
 }
 
 static RISCVException write_mseccfg(CPURISCVState *env, int csrno,
-                         target_ulong val)
+                                    target_ulong val)
 {
     mseccfg_csr_write(env, val);
     return RISCV_EXCP_NONE;
-- 
2.25.1

Re: [PATCH 05/18] target/riscv: Coding style fixes in csr.c

[weiwei]

On 2023/2/14 02:02, Bin Meng wrote:
> Fix various places that violate QEMU coding style:
>
> - correct multi-line comment format
> - indent to opening parenthesis
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,
Weiwei Li
> ---
>
>   target/riscv/csr.c | 62 ++++++++++++++++++++++++----------------------
>   1 file changed, 32 insertions(+), 30 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index c2dd9d5af0..cc74819759 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -963,7 +963,7 @@ static RISCVException sstc_32(CPURISCVState *env, int csrno)
>   }
>   
>   static RISCVException read_vstimecmp(CPURISCVState *env, int csrno,
> -                                    target_ulong *val)
> +                                     target_ulong *val)
>   {
>       *val = env->vstimecmp;
>   
> @@ -971,7 +971,7 @@ static RISCVException read_vstimecmp(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException read_vstimecmph(CPURISCVState *env, int csrno,
> -                                    target_ulong *val)
> +                                      target_ulong *val)
>   {
>       *val = env->vstimecmp >> 32;
>   
> @@ -979,7 +979,7 @@ static RISCVException read_vstimecmph(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_vstimecmp(CPURISCVState *env, int csrno,
> -                                    target_ulong val)
> +                                      target_ulong val)
>   {
>       RISCVCPU *cpu = env_archcpu(env);
>   
> @@ -996,7 +996,7 @@ static RISCVException write_vstimecmp(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_vstimecmph(CPURISCVState *env, int csrno,
> -                                    target_ulong val)
> +                                       target_ulong val)
>   {
>       RISCVCPU *cpu = env_archcpu(env);
>   
> @@ -1020,7 +1020,7 @@ static RISCVException read_stimecmp(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException read_stimecmph(CPURISCVState *env, int csrno,
> -                                    target_ulong *val)
> +                                     target_ulong *val)
>   {
>       if (riscv_cpu_virt_enabled(env)) {
>           *val = env->vstimecmp >> 32;
> @@ -1032,7 +1032,7 @@ static RISCVException read_stimecmph(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_stimecmp(CPURISCVState *env, int csrno,
> -                                    target_ulong val)
> +                                     target_ulong val)
>   {
>       RISCVCPU *cpu = env_archcpu(env);
>   
> @@ -1055,7 +1055,7 @@ static RISCVException write_stimecmp(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_stimecmph(CPURISCVState *env, int csrno,
> -                                    target_ulong val)
> +                                      target_ulong val)
>   {
>       RISCVCPU *cpu = env_archcpu(env);
>   
> @@ -1342,7 +1342,8 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>   
>       /* 'E' excludes all other extensions */
>       if (val & RVE) {
> -        /* when we support 'E' we can do "val = RVE;" however
> +        /*
> +         * when we support 'E' we can do "val = RVE;" however
>            * for now we just drop writes if 'E' is present.
>            */
>           return RISCV_EXCP_NONE;
> @@ -1364,7 +1365,8 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>           val &= ~RVD;
>       }
>   
> -    /* Suppress 'C' if next instruction is not aligned
> +    /*
> +     * Suppress 'C' if next instruction is not aligned
>        * TODO: this should check next_pc
>        */
>       if ((val & RVC) && (GETPC() & ~3) != 0) {
> @@ -1833,28 +1835,28 @@ static RISCVException write_mscratch(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException read_mepc(CPURISCVState *env, int csrno,
> -                                     target_ulong *val)
> +                                target_ulong *val)
>   {
>       *val = env->mepc;
>       return RISCV_EXCP_NONE;
>   }
>   
>   static RISCVException write_mepc(CPURISCVState *env, int csrno,
> -                                     target_ulong val)
> +                                 target_ulong val)
>   {
>       env->mepc = val;
>       return RISCV_EXCP_NONE;
>   }
>   
>   static RISCVException read_mcause(CPURISCVState *env, int csrno,
> -                                     target_ulong *val)
> +                                  target_ulong *val)
>   {
>       *val = env->mcause;
>       return RISCV_EXCP_NONE;
>   }
>   
>   static RISCVException write_mcause(CPURISCVState *env, int csrno,
> -                                     target_ulong val)
> +                                   target_ulong val)
>   {
>       env->mcause = val;
>       return RISCV_EXCP_NONE;
> @@ -1876,14 +1878,14 @@ static RISCVException write_mtval(CPURISCVState *env, int csrno,
>   
>   /* Execution environment configuration setup */
>   static RISCVException read_menvcfg(CPURISCVState *env, int csrno,
> -                                 target_ulong *val)
> +                                   target_ulong *val)
>   {
>       *val = env->menvcfg;
>       return RISCV_EXCP_NONE;
>   }
>   
>   static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
> -                                  target_ulong val)
> +                                    target_ulong val)
>   {
>       uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE | MENVCFG_CBZE;
>   
> @@ -1896,14 +1898,14 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException read_menvcfgh(CPURISCVState *env, int csrno,
> -                                 target_ulong *val)
> +                                    target_ulong *val)
>   {
>       *val = env->menvcfg >> 32;
>       return RISCV_EXCP_NONE;
>   }
>   
>   static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
> -                                  target_ulong val)
> +                                     target_ulong val)
>   {
>       uint64_t mask = MENVCFG_PBMTE | MENVCFG_STCE;
>       uint64_t valh = (uint64_t)val << 32;
> @@ -1914,7 +1916,7 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
> -                                 target_ulong *val)
> +                                   target_ulong *val)
>   {
>       RISCVException ret;
>   
> @@ -1928,7 +1930,7 @@ static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_senvcfg(CPURISCVState *env, int csrno,
> -                                  target_ulong val)
> +                                    target_ulong val)
>   {
>       uint64_t mask = SENVCFG_FIOM | SENVCFG_CBIE | SENVCFG_CBCFE | SENVCFG_CBZE;
>       RISCVException ret;
> @@ -1943,7 +1945,7 @@ static RISCVException write_senvcfg(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
> -                                 target_ulong *val)
> +                                   target_ulong *val)
>   {
>       RISCVException ret;
>   
> @@ -1957,7 +1959,7 @@ static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
> -                                  target_ulong val)
> +                                    target_ulong val)
>   {
>       uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
>       RISCVException ret;
> @@ -1977,7 +1979,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
> -                                 target_ulong *val)
> +                                    target_ulong *val)
>   {
>       RISCVException ret;
>   
> @@ -1991,7 +1993,7 @@ static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
> -                                  target_ulong val)
> +                                     target_ulong val)
>   {
>       uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE;
>       uint64_t valh = (uint64_t)val << 32;
> @@ -2034,13 +2036,13 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_mstateen_1_3(CPURISCVState *env, int csrno,
> -                                      target_ulong new_val)
> +                                         target_ulong new_val)
>   {
>       return write_mstateen(env, csrno, SMSTATEEN_STATEEN, new_val);
>   }
>   
>   static RISCVException read_mstateenh(CPURISCVState *env, int csrno,
> -                                      target_ulong *val)
> +                                     target_ulong *val)
>   {
>       *val = env->mstateen[csrno - CSR_MSTATEEN0H] >> 32;
>   
> @@ -2061,7 +2063,7 @@ static RISCVException write_mstateenh(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
> -                                      target_ulong new_val)
> +                                       target_ulong new_val)
>   {
>       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
>   
> @@ -2069,7 +2071,7 @@ static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_mstateenh_1_3(CPURISCVState *env, int csrno,
> -                                      target_ulong new_val)
> +                                          target_ulong new_val)
>   {
>       return write_mstateenh(env, csrno, SMSTATEEN_STATEEN, new_val);
>   }
> @@ -2106,7 +2108,7 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_hstateen_1_3(CPURISCVState *env, int csrno,
> -                                      target_ulong new_val)
> +                                         target_ulong new_val)
>   {
>       return write_hstateen(env, csrno, SMSTATEEN_STATEEN, new_val);
>   }
> @@ -2145,7 +2147,7 @@ static RISCVException write_hstateen0h(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_hstateenh_1_3(CPURISCVState *env, int csrno,
> -                                       target_ulong new_val)
> +                                          target_ulong new_val)
>   {
>       return write_hstateenh(env, csrno, SMSTATEEN_STATEEN, new_val);
>   }
> @@ -3338,7 +3340,7 @@ static RISCVException read_mseccfg(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_mseccfg(CPURISCVState *env, int csrno,
> -                         target_ulong val)
> +                                    target_ulong val)
>   {
>       mseccfg_csr_write(env, val);
>       return RISCV_EXCP_NONE;

Re: [PATCH 05/18] target/riscv: Coding style fixes in csr.c

[LIU Zhiwei]

On 2023/2/14 2:02, Bin Meng wrote:
> Fix various places that violate QEMU coding style:
>
> - correct multi-line comment format
> - indent to opening parenthesis
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/csr.c | 62 ++++++++++++++++++++++++----------------------
>   1 file changed, 32 insertions(+), 30 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index c2dd9d5af0..cc74819759 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -963,7 +963,7 @@ static RISCVException sstc_32(CPURISCVState *env, int csrno)
>   }
>   
>   static RISCVException read_vstimecmp(CPURISCVState *env, int csrno,
> -                                    target_ulong *val)
> +                                     target_ulong *val)
>   {
>       *val = env->vstimecmp;
>   
> @@ -971,7 +971,7 @@ static RISCVException read_vstimecmp(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException read_vstimecmph(CPURISCVState *env, int csrno,
> -                                    target_ulong *val)
> +                                      target_ulong *val)
>   {
>       *val = env->vstimecmp >> 32;
>   
> @@ -979,7 +979,7 @@ static RISCVException read_vstimecmph(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_vstimecmp(CPURISCVState *env, int csrno,
> -                                    target_ulong val)
> +                                      target_ulong val)
>   {
>       RISCVCPU *cpu = env_archcpu(env);
>   
> @@ -996,7 +996,7 @@ static RISCVException write_vstimecmp(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_vstimecmph(CPURISCVState *env, int csrno,
> -                                    target_ulong val)
> +                                       target_ulong val)
>   {
>       RISCVCPU *cpu = env_archcpu(env);
>   
> @@ -1020,7 +1020,7 @@ static RISCVException read_stimecmp(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException read_stimecmph(CPURISCVState *env, int csrno,
> -                                    target_ulong *val)
> +                                     target_ulong *val)
>   {
>       if (riscv_cpu_virt_enabled(env)) {
>           *val = env->vstimecmp >> 32;
> @@ -1032,7 +1032,7 @@ static RISCVException read_stimecmph(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_stimecmp(CPURISCVState *env, int csrno,
> -                                    target_ulong val)
> +                                     target_ulong val)
>   {
>       RISCVCPU *cpu = env_archcpu(env);
>   
> @@ -1055,7 +1055,7 @@ static RISCVException write_stimecmp(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_stimecmph(CPURISCVState *env, int csrno,
> -                                    target_ulong val)
> +                                      target_ulong val)
>   {
>       RISCVCPU *cpu = env_archcpu(env);
>   
> @@ -1342,7 +1342,8 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>   
>       /* 'E' excludes all other extensions */
>       if (val & RVE) {
> -        /* when we support 'E' we can do "val = RVE;" however
> +        /*
> +         * when we support 'E' we can do "val = RVE;" however
>            * for now we just drop writes if 'E' is present.
>            */
>           return RISCV_EXCP_NONE;
> @@ -1364,7 +1365,8 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>           val &= ~RVD;
>       }
>   
> -    /* Suppress 'C' if next instruction is not aligned
> +    /*
> +     * Suppress 'C' if next instruction is not aligned
>        * TODO: this should check next_pc
>        */
>       if ((val & RVC) && (GETPC() & ~3) != 0) {
> @@ -1833,28 +1835,28 @@ static RISCVException write_mscratch(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException read_mepc(CPURISCVState *env, int csrno,
> -                                     target_ulong *val)
> +                                target_ulong *val)
>   {
>       *val = env->mepc;
>       return RISCV_EXCP_NONE;
>   }
>   
>   static RISCVException write_mepc(CPURISCVState *env, int csrno,
> -                                     target_ulong val)
> +                                 target_ulong val)
>   {
>       env->mepc = val;
>       return RISCV_EXCP_NONE;
>   }
>   
>   static RISCVException read_mcause(CPURISCVState *env, int csrno,
> -                                     target_ulong *val)
> +                                  target_ulong *val)
>   {
>       *val = env->mcause;
>       return RISCV_EXCP_NONE;
>   }
>   
>   static RISCVException write_mcause(CPURISCVState *env, int csrno,
> -                                     target_ulong val)
> +                                   target_ulong val)
>   {
>       env->mcause = val;
>       return RISCV_EXCP_NONE;
> @@ -1876,14 +1878,14 @@ static RISCVException write_mtval(CPURISCVState *env, int csrno,
>   
>   /* Execution environment configuration setup */
>   static RISCVException read_menvcfg(CPURISCVState *env, int csrno,
> -                                 target_ulong *val)
> +                                   target_ulong *val)
>   {
>       *val = env->menvcfg;
>       return RISCV_EXCP_NONE;
>   }
>   
>   static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
> -                                  target_ulong val)
> +                                    target_ulong val)
>   {
>       uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE | MENVCFG_CBZE;
>   
> @@ -1896,14 +1898,14 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException read_menvcfgh(CPURISCVState *env, int csrno,
> -                                 target_ulong *val)
> +                                    target_ulong *val)
>   {
>       *val = env->menvcfg >> 32;
>       return RISCV_EXCP_NONE;
>   }
>   
>   static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
> -                                  target_ulong val)
> +                                     target_ulong val)
>   {
>       uint64_t mask = MENVCFG_PBMTE | MENVCFG_STCE;
>       uint64_t valh = (uint64_t)val << 32;
> @@ -1914,7 +1916,7 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
> -                                 target_ulong *val)
> +                                   target_ulong *val)
>   {
>       RISCVException ret;
>   
> @@ -1928,7 +1930,7 @@ static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_senvcfg(CPURISCVState *env, int csrno,
> -                                  target_ulong val)
> +                                    target_ulong val)
>   {
>       uint64_t mask = SENVCFG_FIOM | SENVCFG_CBIE | SENVCFG_CBCFE | SENVCFG_CBZE;
>       RISCVException ret;
> @@ -1943,7 +1945,7 @@ static RISCVException write_senvcfg(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
> -                                 target_ulong *val)
> +                                   target_ulong *val)
>   {
>       RISCVException ret;
>   
> @@ -1957,7 +1959,7 @@ static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
> -                                  target_ulong val)
> +                                    target_ulong val)
>   {
>       uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
>       RISCVException ret;
> @@ -1977,7 +1979,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
> -                                 target_ulong *val)
> +                                    target_ulong *val)
>   {
>       RISCVException ret;
>   
> @@ -1991,7 +1993,7 @@ static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
> -                                  target_ulong val)
> +                                     target_ulong val)
>   {
>       uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE;
>       uint64_t valh = (uint64_t)val << 32;
> @@ -2034,13 +2036,13 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_mstateen_1_3(CPURISCVState *env, int csrno,
> -                                      target_ulong new_val)
> +                                         target_ulong new_val)
>   {
>       return write_mstateen(env, csrno, SMSTATEEN_STATEEN, new_val);
>   }
>   
>   static RISCVException read_mstateenh(CPURISCVState *env, int csrno,
> -                                      target_ulong *val)
> +                                     target_ulong *val)
>   {
>       *val = env->mstateen[csrno - CSR_MSTATEEN0H] >> 32;
>   
> @@ -2061,7 +2063,7 @@ static RISCVException write_mstateenh(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
> -                                      target_ulong new_val)
> +                                       target_ulong new_val)
>   {
>       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
>   
> @@ -2069,7 +2071,7 @@ static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_mstateenh_1_3(CPURISCVState *env, int csrno,
> -                                      target_ulong new_val)
> +                                          target_ulong new_val)
>   {
>       return write_mstateenh(env, csrno, SMSTATEEN_STATEEN, new_val);
>   }
> @@ -2106,7 +2108,7 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_hstateen_1_3(CPURISCVState *env, int csrno,
> -                                      target_ulong new_val)
> +                                         target_ulong new_val)
>   {
>       return write_hstateen(env, csrno, SMSTATEEN_STATEEN, new_val);
>   }
> @@ -2145,7 +2147,7 @@ static RISCVException write_hstateen0h(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_hstateenh_1_3(CPURISCVState *env, int csrno,
> -                                       target_ulong new_val)
> +                                          target_ulong new_val)
>   {
>       return write_hstateenh(env, csrno, SMSTATEEN_STATEEN, new_val);
>   }
> @@ -3338,7 +3340,7 @@ static RISCVException read_mseccfg(CPURISCVState *env, int csrno,
>   }
>   
>   static RISCVException write_mseccfg(CPURISCVState *env, int csrno,
> -                         target_ulong val)
> +                                    target_ulong val)

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>   {
>       mseccfg_csr_write(env, val);
>       return RISCV_EXCP_NONE;

[PATCH 06/18] target/riscv: Use 'bool' type for read_only

[Bin Meng]

The read_only variable is currently declared as an 'int', but it
should really be a 'bool'.

Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/csr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index cc74819759..8bbc75cbfa 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3778,7 +3778,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
                                                RISCVCPU *cpu)
 {
     /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
-    int read_only = get_field(csrno, 0xC00) == 3;
+    bool read_only = get_field(csrno, 0xC00) == 3;
     int csr_min_priv = csr_ops[csrno].min_priv_ver;
 
     /* ensure the CSR extension is enabled. */
-- 
2.25.1

Re: [PATCH 06/18] target/riscv: Use 'bool' type for read_only

[weiwei]

On 2023/2/14 02:02, Bin Meng wrote:
> The read_only variable is currently declared as an 'int', but it
> should really be a 'bool'.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,
Weiwei Li
> ---
>
>   target/riscv/csr.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index cc74819759..8bbc75cbfa 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3778,7 +3778,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>                                                  RISCVCPU *cpu)
>   {
>       /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
> -    int read_only = get_field(csrno, 0xC00) == 3;
> +    bool read_only = get_field(csrno, 0xC00) == 3;
>       int csr_min_priv = csr_ops[csrno].min_priv_ver;
>   
>       /* ensure the CSR extension is enabled. */

Re: [PATCH 06/18] target/riscv: Use 'bool' type for read_only

[LIU Zhiwei]

On 2023/2/14 2:02, Bin Meng wrote:
> The read_only variable is currently declared as an 'int', but it
> should really be a 'bool'.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/csr.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index cc74819759..8bbc75cbfa 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3778,7 +3778,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>                                                  RISCVCPU *cpu)
>   {
>       /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
> -    int read_only = get_field(csrno, 0xC00) == 3;
> +    bool read_only = get_field(csrno, 0xC00) == 3;

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>       int csr_min_priv = csr_ops[csrno].min_priv_ver;
>   
>       /* ensure the CSR extension is enabled. */

[PATCH 07/18] target/riscv: Simplify {read, write}_pmpcfg() a little bit

[Bin Meng]

Use the register index that has already been calculated in the
pmpcfg_csr_{read,write} call.

Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/csr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 8bbc75cbfa..da3b770894 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3363,7 +3363,7 @@ static RISCVException read_pmpcfg(CPURISCVState *env, int csrno,
     if (!check_pmp_reg_index(env, reg_index)) {
         return RISCV_EXCP_ILLEGAL_INST;
     }
-    *val = pmpcfg_csr_read(env, csrno - CSR_PMPCFG0);
+    *val = pmpcfg_csr_read(env, reg_index);
     return RISCV_EXCP_NONE;
 }
 
@@ -3375,7 +3375,7 @@ static RISCVException write_pmpcfg(CPURISCVState *env, int csrno,
     if (!check_pmp_reg_index(env, reg_index)) {
         return RISCV_EXCP_ILLEGAL_INST;
     }
-    pmpcfg_csr_write(env, csrno - CSR_PMPCFG0, val);
+    pmpcfg_csr_write(env, reg_index, val);
     return RISCV_EXCP_NONE;
 }
 
-- 
2.25.1

Re: [PATCH 07/18] target/riscv: Simplify {read,write}_pmpcfg() a little bit

[weiwei]

On 2023/2/14 02:02, Bin Meng wrote:
> Use the register index that has already been calculated in the
> pmpcfg_csr_{read,write} call.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,
Weiwei Li
> ---
>
>   target/riscv/csr.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 8bbc75cbfa..da3b770894 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3363,7 +3363,7 @@ static RISCVException read_pmpcfg(CPURISCVState *env, int csrno,
>       if (!check_pmp_reg_index(env, reg_index)) {
>           return RISCV_EXCP_ILLEGAL_INST;
>       }
> -    *val = pmpcfg_csr_read(env, csrno - CSR_PMPCFG0);
> +    *val = pmpcfg_csr_read(env, reg_index);
>       return RISCV_EXCP_NONE;
>   }
>   
> @@ -3375,7 +3375,7 @@ static RISCVException write_pmpcfg(CPURISCVState *env, int csrno,
>       if (!check_pmp_reg_index(env, reg_index)) {
>           return RISCV_EXCP_ILLEGAL_INST;
>       }
> -    pmpcfg_csr_write(env, csrno - CSR_PMPCFG0, val);
> +    pmpcfg_csr_write(env, reg_index, val);
>       return RISCV_EXCP_NONE;
>   }
>   

Re: [PATCH 07/18] target/riscv: Simplify {read,write}_pmpcfg() a little bit

[LIU Zhiwei]

On 2023/2/14 2:02, Bin Meng wrote:
> Use the register index that has already been calculated in the
> pmpcfg_csr_{read,write} call.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/csr.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 8bbc75cbfa..da3b770894 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3363,7 +3363,7 @@ static RISCVException read_pmpcfg(CPURISCVState *env, int csrno,
>       if (!check_pmp_reg_index(env, reg_index)) {
>           return RISCV_EXCP_ILLEGAL_INST;
>       }
> -    *val = pmpcfg_csr_read(env, csrno - CSR_PMPCFG0);
> +    *val = pmpcfg_csr_read(env, reg_index);
>       return RISCV_EXCP_NONE;
>   }
>   
> @@ -3375,7 +3375,7 @@ static RISCVException write_pmpcfg(CPURISCVState *env, int csrno,
>       if (!check_pmp_reg_index(env, reg_index)) {
>           return RISCV_EXCP_ILLEGAL_INST;
>       }
> -    pmpcfg_csr_write(env, csrno - CSR_PMPCFG0, val);
> +    pmpcfg_csr_write(env, reg_index, val);

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>       return RISCV_EXCP_NONE;
>   }
>   

[PATCH 08/18] target/riscv: Simplify getting RISCVCPU pointer from env

[Bin Meng]

Use env_archcpu() to get RISCVCPU pointer from env directly.

Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/csr.c | 36 ++++++++++++------------------------
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index da3b770894..0a3f2bef6f 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -46,8 +46,7 @@ static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
                                        uint64_t bit)
 {
     bool virt = riscv_cpu_virt_enabled(env);
-    CPUState *cs = env_cpu(env);
-    RISCVCPU *cpu = RISCV_CPU(cs);
+    RISCVCPU *cpu = env_archcpu(env);
 
     if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
         return RISCV_EXCP_NONE;
@@ -90,8 +89,7 @@ static RISCVException fs(CPURISCVState *env, int csrno)
 
 static RISCVException vs(CPURISCVState *env, int csrno)
 {
-    CPUState *cs = env_cpu(env);
-    RISCVCPU *cpu = RISCV_CPU(cs);
+    RISCVCPU *cpu = env_archcpu(env);
 
     if (env->misa_ext & RVV ||
         cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) {
@@ -108,8 +106,7 @@ static RISCVException vs(CPURISCVState *env, int csrno)
 static RISCVException ctr(CPURISCVState *env, int csrno)
 {
 #if !defined(CONFIG_USER_ONLY)
-    CPUState *cs = env_cpu(env);
-    RISCVCPU *cpu = RISCV_CPU(cs);
+    RISCVCPU *cpu = env_archcpu(env);
     int ctr_index;
     target_ulong ctr_mask;
     int base_csrno = CSR_CYCLE;
@@ -166,8 +163,7 @@ static RISCVException ctr32(CPURISCVState *env, int csrno)
 #if !defined(CONFIG_USER_ONLY)
 static RISCVException mctr(CPURISCVState *env, int csrno)
 {
-    CPUState *cs = env_cpu(env);
-    RISCVCPU *cpu = RISCV_CPU(cs);
+    RISCVCPU *cpu = env_archcpu(env);
     int ctr_index;
     int base_csrno = CSR_MHPMCOUNTER3;
 
@@ -195,8 +191,7 @@ static RISCVException mctr32(CPURISCVState *env, int csrno)
 
 static RISCVException sscofpmf(CPURISCVState *env, int csrno)
 {
-    CPUState *cs = env_cpu(env);
-    RISCVCPU *cpu = RISCV_CPU(cs);
+    RISCVCPU *cpu = env_archcpu(env);
 
     if (!cpu->cfg.ext_sscofpmf) {
         return RISCV_EXCP_ILLEGAL_INST;
@@ -321,8 +316,7 @@ static RISCVException umode32(CPURISCVState *env, int csrno)
 
 static RISCVException mstateen(CPURISCVState *env, int csrno)
 {
-    CPUState *cs = env_cpu(env);
-    RISCVCPU *cpu = RISCV_CPU(cs);
+    RISCVCPU *cpu = env_archcpu(env);
 
     if (!cpu->cfg.ext_smstateen) {
         return RISCV_EXCP_ILLEGAL_INST;
@@ -333,8 +327,7 @@ static RISCVException mstateen(CPURISCVState *env, int csrno)
 
 static RISCVException hstateen_pred(CPURISCVState *env, int csrno, int base)
 {
-    CPUState *cs = env_cpu(env);
-    RISCVCPU *cpu = RISCV_CPU(cs);
+    RISCVCPU *cpu = env_archcpu(env);
 
     if (!cpu->cfg.ext_smstateen) {
         return RISCV_EXCP_ILLEGAL_INST;
@@ -363,8 +356,7 @@ static RISCVException sstateen(CPURISCVState *env, int csrno)
 {
     bool virt = riscv_cpu_virt_enabled(env);
     int index = csrno - CSR_SSTATEEN0;
-    CPUState *cs = env_cpu(env);
-    RISCVCPU *cpu = RISCV_CPU(cs);
+    RISCVCPU *cpu = env_archcpu(env);
 
     if (!cpu->cfg.ext_smstateen) {
         return RISCV_EXCP_ILLEGAL_INST;
@@ -918,8 +910,7 @@ static RISCVException read_timeh(CPURISCVState *env, int csrno,
 
 static RISCVException sstc(CPURISCVState *env, int csrno)
 {
-    CPUState *cs = env_cpu(env);
-    RISCVCPU *cpu = RISCV_CPU(cs);
+    RISCVCPU *cpu = env_archcpu(env);
     bool hmode_check = false;
 
     if (!cpu->cfg.ext_sstc || !env->rdtime_fn) {
@@ -1152,8 +1143,7 @@ static RISCVException write_ignore(CPURISCVState *env, int csrno,
 static RISCVException read_mvendorid(CPURISCVState *env, int csrno,
                                      target_ulong *val)
 {
-    CPUState *cs = env_cpu(env);
-    RISCVCPU *cpu = RISCV_CPU(cs);
+    RISCVCPU *cpu = env_archcpu(env);
 
     *val = cpu->cfg.mvendorid;
     return RISCV_EXCP_NONE;
@@ -1162,8 +1152,7 @@ static RISCVException read_mvendorid(CPURISCVState *env, int csrno,
 static RISCVException read_marchid(CPURISCVState *env, int csrno,
                                    target_ulong *val)
 {
-    CPUState *cs = env_cpu(env);
-    RISCVCPU *cpu = RISCV_CPU(cs);
+    RISCVCPU *cpu = env_archcpu(env);
 
     *val = cpu->cfg.marchid;
     return RISCV_EXCP_NONE;
@@ -1172,8 +1161,7 @@ static RISCVException read_marchid(CPURISCVState *env, int csrno,
 static RISCVException read_mimpid(CPURISCVState *env, int csrno,
                                   target_ulong *val)
 {
-    CPUState *cs = env_cpu(env);
-    RISCVCPU *cpu = RISCV_CPU(cs);
+    RISCVCPU *cpu = env_archcpu(env);
 
     *val = cpu->cfg.mimpid;
     return RISCV_EXCP_NONE;
-- 
2.25.1

Re: [PATCH 08/18] target/riscv: Simplify getting RISCVCPU pointer from env

[weiwei]

On 2023/2/14 02:02, Bin Meng wrote:
> Use env_archcpu() to get RISCVCPU pointer from env directly.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,
Weiwei Li
> ---
>
>   target/riscv/csr.c | 36 ++++++++++++------------------------
>   1 file changed, 12 insertions(+), 24 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index da3b770894..0a3f2bef6f 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -46,8 +46,7 @@ static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
>                                          uint64_t bit)
>   {
>       bool virt = riscv_cpu_virt_enabled(env);
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>   
>       if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
>           return RISCV_EXCP_NONE;
> @@ -90,8 +89,7 @@ static RISCVException fs(CPURISCVState *env, int csrno)
>   
>   static RISCVException vs(CPURISCVState *env, int csrno)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>   
>       if (env->misa_ext & RVV ||
>           cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) {
> @@ -108,8 +106,7 @@ static RISCVException vs(CPURISCVState *env, int csrno)
>   static RISCVException ctr(CPURISCVState *env, int csrno)
>   {
>   #if !defined(CONFIG_USER_ONLY)
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>       int ctr_index;
>       target_ulong ctr_mask;
>       int base_csrno = CSR_CYCLE;
> @@ -166,8 +163,7 @@ static RISCVException ctr32(CPURISCVState *env, int csrno)
>   #if !defined(CONFIG_USER_ONLY)
>   static RISCVException mctr(CPURISCVState *env, int csrno)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>       int ctr_index;
>       int base_csrno = CSR_MHPMCOUNTER3;
>   
> @@ -195,8 +191,7 @@ static RISCVException mctr32(CPURISCVState *env, int csrno)
>   
>   static RISCVException sscofpmf(CPURISCVState *env, int csrno)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>   
>       if (!cpu->cfg.ext_sscofpmf) {
>           return RISCV_EXCP_ILLEGAL_INST;
> @@ -321,8 +316,7 @@ static RISCVException umode32(CPURISCVState *env, int csrno)
>   
>   static RISCVException mstateen(CPURISCVState *env, int csrno)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>   
>       if (!cpu->cfg.ext_smstateen) {
>           return RISCV_EXCP_ILLEGAL_INST;
> @@ -333,8 +327,7 @@ static RISCVException mstateen(CPURISCVState *env, int csrno)
>   
>   static RISCVException hstateen_pred(CPURISCVState *env, int csrno, int base)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>   
>       if (!cpu->cfg.ext_smstateen) {
>           return RISCV_EXCP_ILLEGAL_INST;
> @@ -363,8 +356,7 @@ static RISCVException sstateen(CPURISCVState *env, int csrno)
>   {
>       bool virt = riscv_cpu_virt_enabled(env);
>       int index = csrno - CSR_SSTATEEN0;
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>   
>       if (!cpu->cfg.ext_smstateen) {
>           return RISCV_EXCP_ILLEGAL_INST;
> @@ -918,8 +910,7 @@ static RISCVException read_timeh(CPURISCVState *env, int csrno,
>   
>   static RISCVException sstc(CPURISCVState *env, int csrno)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>       bool hmode_check = false;
>   
>       if (!cpu->cfg.ext_sstc || !env->rdtime_fn) {
> @@ -1152,8 +1143,7 @@ static RISCVException write_ignore(CPURISCVState *env, int csrno,
>   static RISCVException read_mvendorid(CPURISCVState *env, int csrno,
>                                        target_ulong *val)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>   
>       *val = cpu->cfg.mvendorid;
>       return RISCV_EXCP_NONE;
> @@ -1162,8 +1152,7 @@ static RISCVException read_mvendorid(CPURISCVState *env, int csrno,
>   static RISCVException read_marchid(CPURISCVState *env, int csrno,
>                                      target_ulong *val)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>   
>       *val = cpu->cfg.marchid;
>       return RISCV_EXCP_NONE;
> @@ -1172,8 +1161,7 @@ static RISCVException read_marchid(CPURISCVState *env, int csrno,
>   static RISCVException read_mimpid(CPURISCVState *env, int csrno,
>                                     target_ulong *val)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>   
>       *val = cpu->cfg.mimpid;
>       return RISCV_EXCP_NONE;

Re: [PATCH 08/18] target/riscv: Simplify getting RISCVCPU pointer from env

[LIU Zhiwei]

On 2023/2/14 2:02, Bin Meng wrote:
> Use env_archcpu() to get RISCVCPU pointer from env directly.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/csr.c | 36 ++++++++++++------------------------
>   1 file changed, 12 insertions(+), 24 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index da3b770894..0a3f2bef6f 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -46,8 +46,7 @@ static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
>                                          uint64_t bit)
>   {
>       bool virt = riscv_cpu_virt_enabled(env);
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>   
>       if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
>           return RISCV_EXCP_NONE;
> @@ -90,8 +89,7 @@ static RISCVException fs(CPURISCVState *env, int csrno)
>   
>   static RISCVException vs(CPURISCVState *env, int csrno)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);

I see many RISCVCPU pointers are just for cfg in this patch. We can also 
use the new interface by Dianel, which will unify the interface for same 
function.

Otherwise,

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>   
>       if (env->misa_ext & RVV ||
>           cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) {
> @@ -108,8 +106,7 @@ static RISCVException vs(CPURISCVState *env, int csrno)
>   static RISCVException ctr(CPURISCVState *env, int csrno)
>   {
>   #if !defined(CONFIG_USER_ONLY)
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>       int ctr_index;
>       target_ulong ctr_mask;
>       int base_csrno = CSR_CYCLE;
> @@ -166,8 +163,7 @@ static RISCVException ctr32(CPURISCVState *env, int csrno)
>   #if !defined(CONFIG_USER_ONLY)
>   static RISCVException mctr(CPURISCVState *env, int csrno)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>       int ctr_index;
>       int base_csrno = CSR_MHPMCOUNTER3;
>   
> @@ -195,8 +191,7 @@ static RISCVException mctr32(CPURISCVState *env, int csrno)
>   
>   static RISCVException sscofpmf(CPURISCVState *env, int csrno)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>   
>       if (!cpu->cfg.ext_sscofpmf) {
>           return RISCV_EXCP_ILLEGAL_INST;
> @@ -321,8 +316,7 @@ static RISCVException umode32(CPURISCVState *env, int csrno)
>   
>   static RISCVException mstateen(CPURISCVState *env, int csrno)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>   
>       if (!cpu->cfg.ext_smstateen) {
>           return RISCV_EXCP_ILLEGAL_INST;
> @@ -333,8 +327,7 @@ static RISCVException mstateen(CPURISCVState *env, int csrno)
>   
>   static RISCVException hstateen_pred(CPURISCVState *env, int csrno, int base)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>   
>       if (!cpu->cfg.ext_smstateen) {
>           return RISCV_EXCP_ILLEGAL_INST;
> @@ -363,8 +356,7 @@ static RISCVException sstateen(CPURISCVState *env, int csrno)
>   {
>       bool virt = riscv_cpu_virt_enabled(env);
>       int index = csrno - CSR_SSTATEEN0;
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>   
>       if (!cpu->cfg.ext_smstateen) {
>           return RISCV_EXCP_ILLEGAL_INST;
> @@ -918,8 +910,7 @@ static RISCVException read_timeh(CPURISCVState *env, int csrno,
>   
>   static RISCVException sstc(CPURISCVState *env, int csrno)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>       bool hmode_check = false;
>   
>       if (!cpu->cfg.ext_sstc || !env->rdtime_fn) {
> @@ -1152,8 +1143,7 @@ static RISCVException write_ignore(CPURISCVState *env, int csrno,
>   static RISCVException read_mvendorid(CPURISCVState *env, int csrno,
>                                        target_ulong *val)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>   
>       *val = cpu->cfg.mvendorid;
>       return RISCV_EXCP_NONE;
> @@ -1162,8 +1152,7 @@ static RISCVException read_mvendorid(CPURISCVState *env, int csrno,
>   static RISCVException read_marchid(CPURISCVState *env, int csrno,
>                                      target_ulong *val)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>   
>       *val = cpu->cfg.marchid;
>       return RISCV_EXCP_NONE;
> @@ -1172,8 +1161,7 @@ static RISCVException read_marchid(CPURISCVState *env, int csrno,
>   static RISCVException read_mimpid(CPURISCVState *env, int csrno,
>                                     target_ulong *val)
>   {
> -    CPUState *cs = env_cpu(env);
> -    RISCVCPU *cpu = RISCV_CPU(cs);
> +    RISCVCPU *cpu = env_archcpu(env);
>   
>       *val = cpu->cfg.mimpid;
>       return RISCV_EXCP_NONE;

[PATCH 09/18] target/riscv: Avoid reporting odd-numbered pmpcfgX in the CSR XML for RV64

[Bin Meng]

At present the odd-numbered PMP configuration registers for RV64 are
reported in the CSR XML by QEMU gdbstub. However these registers do
not exist on RV64 so trying to access them from gdb results in 'E14'.

Move the pmpcfgX index check from the actual read/write routine to
the PMP CSR predicate() routine, so that non-existent pmpcfgX won't
be reported in the CSR XML for RV64.

Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/csr.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 0a3f2bef6f..749d0ef83e 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -412,6 +412,14 @@ static int aia_hmode32(CPURISCVState *env, int csrno)
 static RISCVException pmp(CPURISCVState *env, int csrno)
 {
     if (riscv_feature(env, RISCV_FEATURE_PMP)) {
+        if (csrno <= CSR_PMPCFG3) {
+            uint32_t reg_index = csrno - CSR_PMPCFG0;
+
+            if ((reg_index & 1) && (riscv_cpu_mxl(env) == MXL_RV64)) {
+                return RISCV_EXCP_ILLEGAL_INST;
+            }
+        }
+
         return RISCV_EXCP_NONE;
     }
 
@@ -3334,23 +3342,11 @@ static RISCVException write_mseccfg(CPURISCVState *env, int csrno,
     return RISCV_EXCP_NONE;
 }
 
-static bool check_pmp_reg_index(CPURISCVState *env, uint32_t reg_index)
-{
-    /* TODO: RV128 restriction check */
-    if ((reg_index & 1) && (riscv_cpu_mxl(env) == MXL_RV64)) {
-        return false;
-    }
-    return true;
-}
-
 static RISCVException read_pmpcfg(CPURISCVState *env, int csrno,
                                   target_ulong *val)
 {
     uint32_t reg_index = csrno - CSR_PMPCFG0;
 
-    if (!check_pmp_reg_index(env, reg_index)) {
-        return RISCV_EXCP_ILLEGAL_INST;
-    }
     *val = pmpcfg_csr_read(env, reg_index);
     return RISCV_EXCP_NONE;
 }
@@ -3360,9 +3356,6 @@ static RISCVException write_pmpcfg(CPURISCVState *env, int csrno,
 {
     uint32_t reg_index = csrno - CSR_PMPCFG0;
 
-    if (!check_pmp_reg_index(env, reg_index)) {
-        return RISCV_EXCP_ILLEGAL_INST;
-    }
     pmpcfg_csr_write(env, reg_index, val);
     return RISCV_EXCP_NONE;
 }
-- 
2.25.1

Re: [PATCH 09/18] target/riscv: Avoid reporting odd-numbered pmpcfgX in the CSR XML for RV64

[weiwei]

[-- Attachment #1: Type: text/plain, Size: 2414 bytes --]


On 2023/2/14 02:02, Bin Meng wrote:
> At present the odd-numbered PMP configuration registers for RV64 are
> reported in the CSR XML by QEMU gdbstub. However these registers do
> not exist on RV64 so trying to access them from gdb results in 'E14'.
>
> Move the pmpcfgX index check from the actual read/write routine to
> the PMP CSR predicate() routine, so that non-existent pmpcfgX won't
> be reported in the CSR XML for RV64.
>
> Signed-off-by: Bin Meng<bmeng@tinylab.org>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,
Weiwei Li
> ---
>
>   target/riscv/csr.c | 23 ++++++++---------------
>   1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 0a3f2bef6f..749d0ef83e 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -412,6 +412,14 @@ static int aia_hmode32(CPURISCVState *env, int csrno)
>   static RISCVException pmp(CPURISCVState *env, int csrno)
>   {
>       if (riscv_feature(env, RISCV_FEATURE_PMP)) {
> +        if (csrno <= CSR_PMPCFG3) {
> +            uint32_t reg_index = csrno - CSR_PMPCFG0;
> +
> +            if ((reg_index & 1) && (riscv_cpu_mxl(env) == MXL_RV64)) {
> +                return RISCV_EXCP_ILLEGAL_INST;
> +            }
> +        }
> +
>           return RISCV_EXCP_NONE;
>       }
>   
> @@ -3334,23 +3342,11 @@ static RISCVException write_mseccfg(CPURISCVState *env, int csrno,
>       return RISCV_EXCP_NONE;
>   }
>   
> -static bool check_pmp_reg_index(CPURISCVState *env, uint32_t reg_index)
> -{
> -    /* TODO: RV128 restriction check */
> -    if ((reg_index & 1) && (riscv_cpu_mxl(env) == MXL_RV64)) {
> -        return false;
> -    }
> -    return true;
> -}
> -
>   static RISCVException read_pmpcfg(CPURISCVState *env, int csrno,
>                                     target_ulong *val)
>   {
>       uint32_t reg_index = csrno - CSR_PMPCFG0;
>   
> -    if (!check_pmp_reg_index(env, reg_index)) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
>       *val = pmpcfg_csr_read(env, reg_index);
>       return RISCV_EXCP_NONE;
>   }
> @@ -3360,9 +3356,6 @@ static RISCVException write_pmpcfg(CPURISCVState *env, int csrno,
>   {
>       uint32_t reg_index = csrno - CSR_PMPCFG0;
>   
> -    if (!check_pmp_reg_index(env, reg_index)) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
>       pmpcfg_csr_write(env, reg_index, val);
>       return RISCV_EXCP_NONE;
>   }

[-- Attachment #2: Type: text/html, Size: 3231 bytes --]

Re: [PATCH 09/18] target/riscv: Avoid reporting odd-numbered pmpcfgX in the CSR XML for RV64

[LIU Zhiwei]

On 2023/2/14 2:02, Bin Meng wrote:
> At present the odd-numbered PMP configuration registers for RV64 are
> reported in the CSR XML by QEMU gdbstub. However these registers do
> not exist on RV64 so trying to access them from gdb results in 'E14'.
>
> Move the pmpcfgX index check from the actual read/write routine to
> the PMP CSR predicate() routine, so that non-existent pmpcfgX won't
> be reported in the CSR XML for RV64.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/csr.c | 23 ++++++++---------------
>   1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 0a3f2bef6f..749d0ef83e 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -412,6 +412,14 @@ static int aia_hmode32(CPURISCVState *env, int csrno)
>   static RISCVException pmp(CPURISCVState *env, int csrno)
>   {
>       if (riscv_feature(env, RISCV_FEATURE_PMP)) {
> +        if (csrno <= CSR_PMPCFG3) {
> +            uint32_t reg_index = csrno - CSR_PMPCFG0;
> +
> +            if ((reg_index & 1) && (riscv_cpu_mxl(env) == MXL_RV64)) {
> +                return RISCV_EXCP_ILLEGAL_INST;
> +            }
> +        }
> +
>           return RISCV_EXCP_NONE;
>       }
>   
> @@ -3334,23 +3342,11 @@ static RISCVException write_mseccfg(CPURISCVState *env, int csrno,
>       return RISCV_EXCP_NONE;
>   }
>   
> -static bool check_pmp_reg_index(CPURISCVState *env, uint32_t reg_index)
> -{
> -    /* TODO: RV128 restriction check */

Should keep this comment. Otherwise,

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

> -    if ((reg_index & 1) && (riscv_cpu_mxl(env) == MXL_RV64)) {
> -        return false;
> -    }
> -    return true;
> -}
> -
>   static RISCVException read_pmpcfg(CPURISCVState *env, int csrno,
>                                     target_ulong *val)
>   {
>       uint32_t reg_index = csrno - CSR_PMPCFG0;
>   
> -    if (!check_pmp_reg_index(env, reg_index)) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
>       *val = pmpcfg_csr_read(env, reg_index);
>       return RISCV_EXCP_NONE;
>   }
> @@ -3360,9 +3356,6 @@ static RISCVException write_pmpcfg(CPURISCVState *env, int csrno,
>   {
>       uint32_t reg_index = csrno - CSR_PMPCFG0;
>   
> -    if (!check_pmp_reg_index(env, reg_index)) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
>       pmpcfg_csr_write(env, reg_index, val);
>       return RISCV_EXCP_NONE;
>   }

[PATCH 10/18] target/riscv: gdbstub: Turn on debugger mode before calling CSR predicate()

[Bin Meng]

Since commit 94452ac4cf26 ("target/riscv: remove fflags, frm, and fcsr from riscv-*-fpu.xml")
the 3 FPU CSRs are removed from the XML target decription. The
original intent of that commit was based on the assumption that
the 3 FPU CSRs will show up in the riscv-csr.xml so the ones in
riscv-*-fpu.xml are redundant. But unforuantely that is not ture.
As the FPU CSR predicate() has a run-time check on MSTATUS.FS,
at the time when CSR XML is generated MSTATUS.FS is unset, hence
no FPU CSRs will be reported.

The FPU CSR predicate() already considered such a case of being
accessed by a debugger. All we need to do is to turn on debugger
mode before calling predicate().

Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/gdbstub.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index 294f0ceb1c..ef52f41460 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -280,6 +280,10 @@ static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
     int bitsize = 16 << env->misa_mxl_max;
     int i;
 
+#if !defined(CONFIG_USER_ONLY)
+    env->debugger = true;
+#endif
+
     /* Until gdb knows about 128-bit registers */
     if (bitsize > 64) {
         bitsize = 64;
@@ -308,6 +312,11 @@ static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
     g_string_append_printf(s, "</feature>");
 
     cpu->dyn_csr_xml = g_string_free(s, false);
+
+#if !defined(CONFIG_USER_ONLY)
+    env->debugger = false;
+#endif
+
     return CSR_TABLE_SIZE;
 }
 
-- 
2.25.1

Re: [PATCH 10/18] target/riscv: gdbstub: Turn on debugger mode before calling CSR predicate()

[weiwei]

On 2023/2/14 02:02, Bin Meng wrote:
> Since commit 94452ac4cf26 ("target/riscv: remove fflags, frm, and fcsr from riscv-*-fpu.xml")
> the 3 FPU CSRs are removed from the XML target decription. The
> original intent of that commit was based on the assumption that
> the 3 FPU CSRs will show up in the riscv-csr.xml so the ones in
> riscv-*-fpu.xml are redundant. But unforuantely that is not ture.

typo here -> true

otherwise, Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,
Weiwei Li

> As the FPU CSR predicate() has a run-time check on MSTATUS.FS,
> at the time when CSR XML is generated MSTATUS.FS is unset, hence
> no FPU CSRs will be reported.
>
> The FPU CSR predicate() already considered such a case of being
> accessed by a debugger. All we need to do is to turn on debugger
> mode before calling predicate().
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/gdbstub.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index 294f0ceb1c..ef52f41460 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -280,6 +280,10 @@ static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
>       int bitsize = 16 << env->misa_mxl_max;
>       int i;
>   
> +#if !defined(CONFIG_USER_ONLY)
> +    env->debugger = true;
> +#endif
> +
>       /* Until gdb knows about 128-bit registers */
>       if (bitsize > 64) {
>           bitsize = 64;
> @@ -308,6 +312,11 @@ static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
>       g_string_append_printf(s, "</feature>");
>   
>       cpu->dyn_csr_xml = g_string_free(s, false);
> +
> +#if !defined(CONFIG_USER_ONLY)
> +    env->debugger = false;
> +#endif
> +
>       return CSR_TABLE_SIZE;
>   }
>   

Re: [PATCH 10/18] target/riscv: gdbstub: Turn on debugger mode before calling CSR predicate()

[LIU Zhiwei]

On 2023/2/14 2:02, Bin Meng wrote:
> Since commit 94452ac4cf26 ("target/riscv: remove fflags, frm, and fcsr from riscv-*-fpu.xml")
> the 3 FPU CSRs are removed from the XML target decription. The
> original intent of that commit was based on the assumption that
> the 3 FPU CSRs will show up in the riscv-csr.xml so the ones in
> riscv-*-fpu.xml are redundant. But unforuantely that is not ture.
> As the FPU CSR predicate() has a run-time check on MSTATUS.FS,
> at the time when CSR XML is generated MSTATUS.FS is unset, hence
> no FPU CSRs will be reported.
>
> The FPU CSR predicate() already considered such a case of being
> accessed by a debugger. All we need to do is to turn on debugger
> mode before calling predicate().
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/gdbstub.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index 294f0ceb1c..ef52f41460 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -280,6 +280,10 @@ static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
>       int bitsize = 16 << env->misa_mxl_max;
>       int i;
>   
> +#if !defined(CONFIG_USER_ONLY)
> +    env->debugger = true;
> +#endif
> +
>       /* Until gdb knows about 128-bit registers */
>       if (bitsize > 64) {
>           bitsize = 64;
> @@ -308,6 +312,11 @@ static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
>       g_string_append_printf(s, "</feature>");
>   
>       cpu->dyn_csr_xml = g_string_free(s, false);
> +
> +#if !defined(CONFIG_USER_ONLY)
> +    env->debugger = false;
> +#endif
> +

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>       return CSR_TABLE_SIZE;
>   }
>   

[PATCH 11/18] target/riscv: gdbstub: Drop the vector CSRs in riscv-vector.xml

[Bin Meng]

It's worth noting that the vector CSR predicate() has a similar
run-time check logic to the FPU CSR. With the previous patch our
gdbstub can correctly report these vector CSRs via the CSR xml.

Commit 719d3561b269 ("target/riscv: gdb: support vector registers for rv64 & rv32")
inserted these vector CSRs in an ad-hoc, non-standard way in the
riscv-vector.xml. Now we can treat these CSRs no different from
other CSRs.

Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/gdbstub.c | 75 ------------------------------------------
 1 file changed, 75 deletions(-)

diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index ef52f41460..6048541606 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -127,40 +127,6 @@ static int riscv_gdb_set_fpu(CPURISCVState *env, uint8_t *mem_buf, int n)
     return 0;
 }
 
-/*
- * Convert register index number passed by GDB to the correspond
- * vector CSR number. Vector CSRs are defined after vector registers
- * in dynamic generated riscv-vector.xml, thus the starting register index
- * of vector CSRs is 32.
- * Return 0 if register index number is out of range.
- */
-static int riscv_gdb_vector_csrno(int num_regs)
-{
-    /*
-     * The order of vector CSRs in the switch case
-     * should match with the order defined in csr_ops[].
-     */
-    switch (num_regs) {
-    case 32:
-        return CSR_VSTART;
-    case 33:
-        return CSR_VXSAT;
-    case 34:
-        return CSR_VXRM;
-    case 35:
-        return CSR_VCSR;
-    case 36:
-        return CSR_VL;
-    case 37:
-        return CSR_VTYPE;
-    case 38:
-        return CSR_VLENB;
-    default:
-        /* Unknown register. */
-        return 0;
-    }
-}
-
 static int riscv_gdb_get_vector(CPURISCVState *env, GByteArray *buf, int n)
 {
     uint16_t vlenb = env_archcpu(env)->cfg.vlen >> 3;
@@ -174,19 +140,6 @@ static int riscv_gdb_get_vector(CPURISCVState *env, GByteArray *buf, int n)
         return cnt;
     }
 
-    int csrno = riscv_gdb_vector_csrno(n);
-
-    if (!csrno) {
-        return 0;
-    }
-
-    target_ulong val = 0;
-    int result = riscv_csrrw_debug(env, csrno, &val, 0, 0);
-
-    if (result == RISCV_EXCP_NONE) {
-        return gdb_get_regl(buf, val);
-    }
-
     return 0;
 }
 
@@ -201,19 +154,6 @@ static int riscv_gdb_set_vector(CPURISCVState *env, uint8_t *mem_buf, int n)
         return vlenb;
     }
 
-    int csrno = riscv_gdb_vector_csrno(n);
-
-    if (!csrno) {
-        return 0;
-    }
-
-    target_ulong val = ldtul_p(mem_buf);
-    int result = riscv_csrrw_debug(env, csrno, NULL, val, -1);
-
-    if (result == RISCV_EXCP_NONE) {
-        return sizeof(target_ulong);
-    }
-
     return 0;
 }
 
@@ -361,21 +301,6 @@ static int ricsv_gen_dynamic_vector_xml(CPUState *cs, int base_reg)
         num_regs++;
     }
 
-    /* Define vector CSRs */
-    const char *vector_csrs[7] = {
-        "vstart", "vxsat", "vxrm", "vcsr",
-        "vl", "vtype", "vlenb"
-    };
-
-    for (i = 0; i < 7; i++) {
-        g_string_append_printf(s,
-                               "<reg name=\"%s\" bitsize=\"%d\""
-                               " regnum=\"%d\" group=\"vector\""
-                               " type=\"int\"/>",
-                               vector_csrs[i], TARGET_LONG_BITS, base_reg++);
-        num_regs++;
-    }
-
     g_string_append_printf(s, "</feature>");
 
     cpu->dyn_vreg_xml = g_string_free(s, false);
-- 
2.25.1

Re: [PATCH 11/18] target/riscv: gdbstub: Drop the vector CSRs in riscv-vector.xml

[weiwei]

On 2023/2/14 02:02, Bin Meng wrote:
> It's worth noting that the vector CSR predicate() has a similar
> run-time check logic to the FPU CSR. With the previous patch our
> gdbstub can correctly report these vector CSRs via the CSR xml.
>
> Commit 719d3561b269 ("target/riscv: gdb: support vector registers for rv64 & rv32")
> inserted these vector CSRs in an ad-hoc, non-standard way in the
> riscv-vector.xml. Now we can treat these CSRs no different from
> other CSRs.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,
Weiwei Li
> ---
>
>   target/riscv/gdbstub.c | 75 ------------------------------------------
>   1 file changed, 75 deletions(-)
>
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index ef52f41460..6048541606 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -127,40 +127,6 @@ static int riscv_gdb_set_fpu(CPURISCVState *env, uint8_t *mem_buf, int n)
>       return 0;
>   }
>   
> -/*
> - * Convert register index number passed by GDB to the correspond
> - * vector CSR number. Vector CSRs are defined after vector registers
> - * in dynamic generated riscv-vector.xml, thus the starting register index
> - * of vector CSRs is 32.
> - * Return 0 if register index number is out of range.
> - */
> -static int riscv_gdb_vector_csrno(int num_regs)
> -{
> -    /*
> -     * The order of vector CSRs in the switch case
> -     * should match with the order defined in csr_ops[].
> -     */
> -    switch (num_regs) {
> -    case 32:
> -        return CSR_VSTART;
> -    case 33:
> -        return CSR_VXSAT;
> -    case 34:
> -        return CSR_VXRM;
> -    case 35:
> -        return CSR_VCSR;
> -    case 36:
> -        return CSR_VL;
> -    case 37:
> -        return CSR_VTYPE;
> -    case 38:
> -        return CSR_VLENB;
> -    default:
> -        /* Unknown register. */
> -        return 0;
> -    }
> -}
> -
>   static int riscv_gdb_get_vector(CPURISCVState *env, GByteArray *buf, int n)
>   {
>       uint16_t vlenb = env_archcpu(env)->cfg.vlen >> 3;
> @@ -174,19 +140,6 @@ static int riscv_gdb_get_vector(CPURISCVState *env, GByteArray *buf, int n)
>           return cnt;
>       }
>   
> -    int csrno = riscv_gdb_vector_csrno(n);
> -
> -    if (!csrno) {
> -        return 0;
> -    }
> -
> -    target_ulong val = 0;
> -    int result = riscv_csrrw_debug(env, csrno, &val, 0, 0);
> -
> -    if (result == RISCV_EXCP_NONE) {
> -        return gdb_get_regl(buf, val);
> -    }
> -
>       return 0;
>   }
>   
> @@ -201,19 +154,6 @@ static int riscv_gdb_set_vector(CPURISCVState *env, uint8_t *mem_buf, int n)
>           return vlenb;
>       }
>   
> -    int csrno = riscv_gdb_vector_csrno(n);
> -
> -    if (!csrno) {
> -        return 0;
> -    }
> -
> -    target_ulong val = ldtul_p(mem_buf);
> -    int result = riscv_csrrw_debug(env, csrno, NULL, val, -1);
> -
> -    if (result == RISCV_EXCP_NONE) {
> -        return sizeof(target_ulong);
> -    }
> -
>       return 0;
>   }
>   
> @@ -361,21 +301,6 @@ static int ricsv_gen_dynamic_vector_xml(CPUState *cs, int base_reg)
>           num_regs++;
>       }
>   
> -    /* Define vector CSRs */
> -    const char *vector_csrs[7] = {
> -        "vstart", "vxsat", "vxrm", "vcsr",
> -        "vl", "vtype", "vlenb"
> -    };
> -
> -    for (i = 0; i < 7; i++) {
> -        g_string_append_printf(s,
> -                               "<reg name=\"%s\" bitsize=\"%d\""
> -                               " regnum=\"%d\" group=\"vector\""
> -                               " type=\"int\"/>",
> -                               vector_csrs[i], TARGET_LONG_BITS, base_reg++);
> -        num_regs++;
> -    }
> -
>       g_string_append_printf(s, "</feature>");
>   
>       cpu->dyn_vreg_xml = g_string_free(s, false);

Re: [PATCH 11/18] target/riscv: gdbstub: Drop the vector CSRs in riscv-vector.xml

[LIU Zhiwei]

On 2023/2/14 2:02, Bin Meng wrote:
> It's worth noting that the vector CSR predicate() has a similar
> run-time check logic to the FPU CSR. With the previous patch our
> gdbstub can correctly report these vector CSRs via the CSR xml.
>
> Commit 719d3561b269 ("target/riscv: gdb: support vector registers for rv64 & rv32")
> inserted these vector CSRs in an ad-hoc, non-standard way in the
> riscv-vector.xml. Now we can treat these CSRs no different from
> other CSRs.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/gdbstub.c | 75 ------------------------------------------
>   1 file changed, 75 deletions(-)
>
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index ef52f41460..6048541606 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -127,40 +127,6 @@ static int riscv_gdb_set_fpu(CPURISCVState *env, uint8_t *mem_buf, int n)
>       return 0;
>   }
>   
> -/*
> - * Convert register index number passed by GDB to the correspond
> - * vector CSR number. Vector CSRs are defined after vector registers
> - * in dynamic generated riscv-vector.xml, thus the starting register index
> - * of vector CSRs is 32.
> - * Return 0 if register index number is out of range.
> - */
> -static int riscv_gdb_vector_csrno(int num_regs)
> -{
> -    /*
> -     * The order of vector CSRs in the switch case
> -     * should match with the order defined in csr_ops[].
> -     */
> -    switch (num_regs) {
> -    case 32:
> -        return CSR_VSTART;
> -    case 33:
> -        return CSR_VXSAT;
> -    case 34:
> -        return CSR_VXRM;
> -    case 35:
> -        return CSR_VCSR;
> -    case 36:
> -        return CSR_VL;
> -    case 37:
> -        return CSR_VTYPE;
> -    case 38:
> -        return CSR_VLENB;
> -    default:
> -        /* Unknown register. */
> -        return 0;
> -    }
> -}
> -
>   static int riscv_gdb_get_vector(CPURISCVState *env, GByteArray *buf, int n)
>   {
>       uint16_t vlenb = env_archcpu(env)->cfg.vlen >> 3;
> @@ -174,19 +140,6 @@ static int riscv_gdb_get_vector(CPURISCVState *env, GByteArray *buf, int n)
>           return cnt;
>       }
>   
> -    int csrno = riscv_gdb_vector_csrno(n);
> -
> -    if (!csrno) {
> -        return 0;
> -    }
> -
> -    target_ulong val = 0;
> -    int result = riscv_csrrw_debug(env, csrno, &val, 0, 0);
> -
> -    if (result == RISCV_EXCP_NONE) {
> -        return gdb_get_regl(buf, val);
> -    }
> -
>       return 0;
>   }
>   
> @@ -201,19 +154,6 @@ static int riscv_gdb_set_vector(CPURISCVState *env, uint8_t *mem_buf, int n)
>           return vlenb;
>       }
>   
> -    int csrno = riscv_gdb_vector_csrno(n);
> -
> -    if (!csrno) {
> -        return 0;
> -    }
> -
> -    target_ulong val = ldtul_p(mem_buf);
> -    int result = riscv_csrrw_debug(env, csrno, NULL, val, -1);
> -
> -    if (result == RISCV_EXCP_NONE) {
> -        return sizeof(target_ulong);
> -    }
> -
>       return 0;
>   }
>   
> @@ -361,21 +301,6 @@ static int ricsv_gen_dynamic_vector_xml(CPUState *cs, int base_reg)
>           num_regs++;
>       }
>   
> -    /* Define vector CSRs */
> -    const char *vector_csrs[7] = {
> -        "vstart", "vxsat", "vxrm", "vcsr",
> -        "vl", "vtype", "vlenb"
> -    };
> -
> -    for (i = 0; i < 7; i++) {
> -        g_string_append_printf(s,
> -                               "<reg name=\"%s\" bitsize=\"%d\""
> -                               " regnum=\"%d\" group=\"vector\""
> -                               " type=\"int\"/>",
> -                               vector_csrs[i], TARGET_LONG_BITS, base_reg++);
> -        num_regs++;
> -    }
> -

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>       g_string_append_printf(s, "</feature>");
>   
>       cpu->dyn_vreg_xml = g_string_free(s, false);

Re: [PATCH 00/18] target/riscv: Various fixes to gdbstub and CSR access

[Daniel Henrique Barboza]

Bin,


I received only patches 1-11. I don't see the remaining patches in patchwork:


https://patchwork.kernel.org/project/qemu-devel/list/?series=721372


or in the qemu-devel archives:


https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg03461.html


Can you please verify? Thanks,



Daniel


On 2/13/23 15:01, Bin Meng wrote:
> At present gdbstub reports an incorrect / incomplete CSR list in the
> target description XML, for example:
> 
> - menvcfg is reported in 'sifive_u' machine
> - fcsr is missing in a F/D enabled processor
> 
> The issue is caused by:
> - priv spec version check is missing when reporting CSRs
> - CSR predicate() routine is called without turning on the debugger flag
> 
> This series aims to generate a correct and complete CSR list for gdbstub.
> 
> 
> Bin Meng (18):
>    target/riscv: gdbstub: Check priv spec version before reporting CSR
>    target/riscv: Correct the priority policy of riscv_csrrw_check()
>    target/riscv: gdbstub: Minor change for better readability
>    target/riscv: gdbstub: Do not generate CSR XML if Zicsr is disabled
>    target/riscv: Coding style fixes in csr.c
>    target/riscv: Use 'bool' type for read_only
>    target/riscv: Simplify {read,write}_pmpcfg() a little bit
>    target/riscv: Simplify getting RISCVCPU pointer from env
>    target/riscv: Avoid reporting odd-numbered pmpcfgX in the CSR XML for
>      RV64
>    target/riscv: gdbstub: Turn on debugger mode before calling CSR
>      predicate()
>    target/riscv: gdbstub: Drop the vector CSRs in riscv-vector.xml
>    target/riscv: Allow debugger to access user timer and counter CSRs
>    target/riscv: Allow debugger to access seed CSR
>    target/riscv: Allow debugger to access {h,s}stateen CSRs
>    target/riscv: Allow debugger to access sstc CSRs
>    target/riscv: Drop priv level check in mseccfg predicate()
>    target/riscv: Group all predicate() routines together
>    target/riscv: Move configuration check to envcfg CSRs predicate()
> 
>   target/riscv/csr.c     | 360 +++++++++++++++++++++++------------------
>   target/riscv/gdbstub.c | 100 +++---------
>   2 files changed, 221 insertions(+), 239 deletions(-)
> 

Re: [PATCH 00/18] target/riscv: Various fixes to gdbstub and CSR access

[Bin Meng]

Hi Daniel,

On Tue, Feb 14, 2023 at 3:20 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Bin,
>
>
> I received only patches 1-11. I don't see the remaining patches in patchwork:
>
>
> https://patchwork.kernel.org/project/qemu-devel/list/?series=721372
>
>
> or in the qemu-devel archives:
>
>
> https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg03461.html
>
>
> Can you please verify? Thanks,
>

Somehow my email service provider blocked some of my patches. I've now
resent the missing patches, and I just verified that it showed up in
the patchwork.

Regards,
Bin

[PATCH 12/18] target/riscv: Allow debugger to access user timer and counter CSRs

[Bin Meng]

At present user timer and counter CSRs are not reported in the
CSR XML hence gdb cannot access them.

Fix it by addding a debugger check in their predicate() routine.

Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/csr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 749d0ef83e..515b05348b 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -131,6 +131,10 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
 
 skip_ext_pmu_check:
 
+    if (env->debugger) {
+        return RISCV_EXCP_NONE;
+    }
+
     if (env->priv < PRV_M && !get_field(env->mcounteren, ctr_mask)) {
         return RISCV_EXCP_ILLEGAL_INST;
     }
-- 
2.25.1

Re: [PATCH 12/18] target/riscv: Allow debugger to access user timer and counter CSRs

[weiwei]

On 2023/2/14 09:09, Bin Meng wrote:
> At present user timer and counter CSRs are not reported in the
> CSR XML hence gdb cannot access them.
>
> Fix it by addding a debugger check in their predicate() routine.

typo: adding

Otherwise, Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,
Weiwei Li

> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/csr.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 749d0ef83e..515b05348b 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -131,6 +131,10 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
>   
>   skip_ext_pmu_check:
>   
> +    if (env->debugger) {
> +        return RISCV_EXCP_NONE;
> +    }
> +
>       if (env->priv < PRV_M && !get_field(env->mcounteren, ctr_mask)) {
>           return RISCV_EXCP_ILLEGAL_INST;
>       }

Re: [PATCH 12/18] target/riscv: Allow debugger to access user timer and counter CSRs

[LIU Zhiwei]

On 2023/2/14 9:09, Bin Meng wrote:
> At present user timer and counter CSRs are not reported in the
> CSR XML hence gdb cannot access them.
>
> Fix it by addding a debugger check in their predicate() routine.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/csr.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 749d0ef83e..515b05348b 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -131,6 +131,10 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
>   
>   skip_ext_pmu_check:
>   
> +    if (env->debugger) {
> +        return RISCV_EXCP_NONE;
> +    }
> +

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>


Zhiwei

>       if (env->priv < PRV_M && !get_field(env->mcounteren, ctr_mask)) {
>           return RISCV_EXCP_ILLEGAL_INST;
>       }

[PATCH 13/18] target/riscv: Allow debugger to access seed CSR

[Bin Meng]

At present seed CSR is not reported in the CSR XML hence gdb cannot
access it.

Fix it by addding a debugger check in its predicate() routine.

Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/csr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 515b05348b..f1075b5728 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -458,6 +458,10 @@ static RISCVException seed(CPURISCVState *env, int csrno)
     }
 
 #if !defined(CONFIG_USER_ONLY)
+    if (env->debugger) {
+        return RISCV_EXCP_NONE;
+    }
+
     /*
      * With a CSR read-write instruction:
      * 1) The seed CSR is always available in machine mode as normal.
-- 
2.25.1

Re: [PATCH 13/18] target/riscv: Allow debugger to access seed CSR

[weiwei]

On 2023/2/14 09:09, Bin Meng wrote:
> At present seed CSR is not reported in the CSR XML hence gdb cannot
> access it.
>
> Fix it by addding a debugger check in its predicate() routine.

typo: adding

Otherwise, Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,
Weiwei Li

>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/csr.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 515b05348b..f1075b5728 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -458,6 +458,10 @@ static RISCVException seed(CPURISCVState *env, int csrno)
>       }
>   
>   #if !defined(CONFIG_USER_ONLY)
> +    if (env->debugger) {
> +        return RISCV_EXCP_NONE;
> +    }
> +
>       /*
>        * With a CSR read-write instruction:
>        * 1) The seed CSR is always available in machine mode as normal.

Re: [PATCH 13/18] target/riscv: Allow debugger to access seed CSR

[LIU Zhiwei]

On 2023/2/14 9:09, Bin Meng wrote:
> At present seed CSR is not reported in the CSR XML hence gdb cannot
> access it.
>
> Fix it by addding a debugger check in its predicate() routine.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/csr.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 515b05348b..f1075b5728 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -458,6 +458,10 @@ static RISCVException seed(CPURISCVState *env, int csrno)
>       }
>   
>   #if !defined(CONFIG_USER_ONLY)
> +    if (env->debugger) {
> +        return RISCV_EXCP_NONE;
> +    }
> +

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>       /*
>        * With a CSR read-write instruction:
>        * 1) The seed CSR is always available in machine mode as normal.

[PATCH 14/18] target/riscv: Allow debugger to access {h, s}stateen CSRs

[Bin Meng]

At present {h,s}stateen CSRs are not reported in the CSR XML
hence gdb cannot access them.

Fix it by adjusting their predicate() routine logic so that the
static config check comes before the run-time check, as well as
addding a debugger check.

Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/csr.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index f1075b5728..d6bcb7f275 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -337,13 +337,22 @@ static RISCVException hstateen_pred(CPURISCVState *env, int csrno, int base)
         return RISCV_EXCP_ILLEGAL_INST;
     }
 
+    RISCVException ret = hmode(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
+    if (env->debugger) {
+        return RISCV_EXCP_NONE;
+    }
+
     if (env->priv < PRV_M) {
         if (!(env->mstateen[csrno - base] & SMSTATEEN_STATEEN)) {
             return RISCV_EXCP_ILLEGAL_INST;
         }
     }
 
-    return hmode(env, csrno);
+    return RISCV_EXCP_NONE;
 }
 
 static RISCVException hstateen(CPURISCVState *env, int csrno)
@@ -366,6 +375,15 @@ static RISCVException sstateen(CPURISCVState *env, int csrno)
         return RISCV_EXCP_ILLEGAL_INST;
     }
 
+    RISCVException ret = smode(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
+    if (env->debugger) {
+        return RISCV_EXCP_NONE;
+    }
+
     if (env->priv < PRV_M) {
         if (!(env->mstateen[index] & SMSTATEEN_STATEEN)) {
             return RISCV_EXCP_ILLEGAL_INST;
@@ -378,7 +396,7 @@ static RISCVException sstateen(CPURISCVState *env, int csrno)
         }
     }
 
-    return smode(env, csrno);
+    return RISCV_EXCP_NONE;
 }
 
 /* Checks if PointerMasking registers could be accessed */
-- 
2.25.1

Re: [PATCH 14/18] target/riscv: Allow debugger to access {h, s}stateen CSRs

[weiwei]

On 2023/2/14 11:06, Bin Meng wrote:
> At present {h,s}stateen CSRs are not reported in the CSR XML
> hence gdb cannot access them.
>
> Fix it by adjusting their predicate() routine logic so that the
> static config check comes before the run-time check, as well as
> addding a debugger check.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>

Similar typo,

Otherwise, Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,
Weiwei Li

> ---
>
>   target/riscv/csr.c | 22 ++++++++++++++++++++--
>   1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index f1075b5728..d6bcb7f275 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -337,13 +337,22 @@ static RISCVException hstateen_pred(CPURISCVState *env, int csrno, int base)
>           return RISCV_EXCP_ILLEGAL_INST;
>       }
>   
> +    RISCVException ret = hmode(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
> +    if (env->debugger) {
> +        return RISCV_EXCP_NONE;
> +    }
> +
>       if (env->priv < PRV_M) {
>           if (!(env->mstateen[csrno - base] & SMSTATEEN_STATEEN)) {
>               return RISCV_EXCP_ILLEGAL_INST;
>           }
>       }
>   
> -    return hmode(env, csrno);
> +    return RISCV_EXCP_NONE;
>   }
>   
>   static RISCVException hstateen(CPURISCVState *env, int csrno)
> @@ -366,6 +375,15 @@ static RISCVException sstateen(CPURISCVState *env, int csrno)
>           return RISCV_EXCP_ILLEGAL_INST;
>       }
>   
> +    RISCVException ret = smode(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
> +    if (env->debugger) {
> +        return RISCV_EXCP_NONE;
> +    }
> +
>       if (env->priv < PRV_M) {
>           if (!(env->mstateen[index] & SMSTATEEN_STATEEN)) {
>               return RISCV_EXCP_ILLEGAL_INST;
> @@ -378,7 +396,7 @@ static RISCVException sstateen(CPURISCVState *env, int csrno)
>           }
>       }
>   
> -    return smode(env, csrno);
> +    return RISCV_EXCP_NONE;
>   }
>   
>   /* Checks if PointerMasking registers could be accessed */

[PATCH 15/18] target/riscv: Allow debugger to access sstc CSRs

[Bin Meng]

At present with a debugger attached sstc CSRs can only be accssed
when CPU is in M-mode, or configured correctly.

Fix it by adjusting their predicate() routine logic so that the
static config check comes before the run-time check, as well as
addding a debugger check.

Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/csr.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index d6bcb7f275..c6a7745cb2 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -951,6 +951,19 @@ static RISCVException sstc(CPURISCVState *env, int csrno)
         return RISCV_EXCP_ILLEGAL_INST;
     }
 
+    if ((csrno == CSR_VSTIMECMP) || (csrno == CSR_VSTIMECMPH)) {
+        hmode_check = true;
+    }
+
+    RISCVException ret = hmode_check ? hmode(env, csrno) : smode(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
+    if (env->debugger) {
+        return RISCV_EXCP_NONE;
+    }
+
     if (env->priv == PRV_M) {
         return RISCV_EXCP_NONE;
     }
@@ -971,11 +984,7 @@ static RISCVException sstc(CPURISCVState *env, int csrno)
         }
     }
 
-    if ((csrno == CSR_VSTIMECMP) || (csrno == CSR_VSTIMECMPH)) {
-        hmode_check = true;
-    }
-
-    return hmode_check ? hmode(env, csrno) : smode(env, csrno);
+    return RISCV_EXCP_NONE;
 }
 
 static RISCVException sstc_32(CPURISCVState *env, int csrno)
-- 
2.25.1

Re: [PATCH 15/18] target/riscv: Allow debugger to access sstc CSRs

[weiwei]

On 2023/2/14 12:12, Bin Meng wrote:
> At present with a debugger attached sstc CSRs can only be accssed
> when CPU is in M-mode, or configured correctly.
>
> Fix it by adjusting their predicate() routine logic so that the
> static config check comes before the run-time check, as well as
> addding a debugger check.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>

Similar typo, otherwise Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,
Weiwei Li
> ---
>
>   target/riscv/csr.c | 19 ++++++++++++++-----
>   1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index d6bcb7f275..c6a7745cb2 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -951,6 +951,19 @@ static RISCVException sstc(CPURISCVState *env, int csrno)
>           return RISCV_EXCP_ILLEGAL_INST;
>       }
>   
> +    if ((csrno == CSR_VSTIMECMP) || (csrno == CSR_VSTIMECMPH)) {
> +        hmode_check = true;
> +    }
> +
> +    RISCVException ret = hmode_check ? hmode(env, csrno) : smode(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
> +    if (env->debugger) {
> +        return RISCV_EXCP_NONE;
> +    }
> +
>       if (env->priv == PRV_M) {
>           return RISCV_EXCP_NONE;
>       }
> @@ -971,11 +984,7 @@ static RISCVException sstc(CPURISCVState *env, int csrno)
>           }
>       }
>   
> -    if ((csrno == CSR_VSTIMECMP) || (csrno == CSR_VSTIMECMPH)) {
> -        hmode_check = true;
> -    }
> -
> -    return hmode_check ? hmode(env, csrno) : smode(env, csrno);
> +    return RISCV_EXCP_NONE;
>   }
>   
>   static RISCVException sstc_32(CPURISCVState *env, int csrno)

[PATCH 16/18] target/riscv: Drop priv level check in mseccfg predicate()

[Bin Meng]

riscv_csrrw_check() already does the generic privilege level check
hence there is no need to do the specific M-mode access check in
the mseccfg predicate().

With this change debugger can access the mseccfg CSR anytime.

Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/csr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index c6a7745cb2..40aae9e7b3 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -450,7 +450,7 @@ static RISCVException pmp(CPURISCVState *env, int csrno)
 
 static RISCVException epmp(CPURISCVState *env, int csrno)
 {
-    if (env->priv == PRV_M && riscv_feature(env, RISCV_FEATURE_EPMP)) {
+    if (riscv_feature(env, RISCV_FEATURE_EPMP)) {
         return RISCV_EXCP_NONE;
     }
 
-- 
2.25.1

Re: [PATCH 16/18] target/riscv: Drop priv level check in mseccfg predicate()

[weiwei]

On 2023/2/14 12:12, Bin Meng wrote:
> riscv_csrrw_check() already does the generic privilege level check
> hence there is no need to do the specific M-mode access check in
> the mseccfg predicate().
>
> With this change debugger can access the mseccfg CSR anytime.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,
Weiwei Li
> ---
>
>   target/riscv/csr.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index c6a7745cb2..40aae9e7b3 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -450,7 +450,7 @@ static RISCVException pmp(CPURISCVState *env, int csrno)
>   
>   static RISCVException epmp(CPURISCVState *env, int csrno)
>   {
> -    if (env->priv == PRV_M && riscv_feature(env, RISCV_FEATURE_EPMP)) {
> +    if (riscv_feature(env, RISCV_FEATURE_EPMP)) {
>           return RISCV_EXCP_NONE;
>       }
>   

[PATCH 17/18] target/riscv: Group all predicate() routines together

[Bin Meng]

Move sstc()/sstc32() to where all predicate() routines live.

Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/csr.c | 108 ++++++++++++++++++++++-----------------------
 1 file changed, 54 insertions(+), 54 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 40aae9e7b3..37350b8a6d 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -399,6 +399,60 @@ static RISCVException sstateen(CPURISCVState *env, int csrno)
     return RISCV_EXCP_NONE;
 }
 
+static RISCVException sstc(CPURISCVState *env, int csrno)
+{
+    RISCVCPU *cpu = env_archcpu(env);
+    bool hmode_check = false;
+
+    if (!cpu->cfg.ext_sstc || !env->rdtime_fn) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    if ((csrno == CSR_VSTIMECMP) || (csrno == CSR_VSTIMECMPH)) {
+        hmode_check = true;
+    }
+
+    RISCVException ret = hmode_check ? hmode(env, csrno) : smode(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
+    if (env->debugger) {
+        return RISCV_EXCP_NONE;
+    }
+
+    if (env->priv == PRV_M) {
+        return RISCV_EXCP_NONE;
+    }
+
+    /*
+     * No need of separate function for rv32 as menvcfg stores both menvcfg
+     * menvcfgh for RV32.
+     */
+    if (!(get_field(env->mcounteren, COUNTEREN_TM) &&
+          get_field(env->menvcfg, MENVCFG_STCE))) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    if (riscv_cpu_virt_enabled(env)) {
+        if (!(get_field(env->hcounteren, COUNTEREN_TM) &&
+              get_field(env->henvcfg, HENVCFG_STCE))) {
+            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+        }
+    }
+
+    return RISCV_EXCP_NONE;
+}
+
+static RISCVException sstc_32(CPURISCVState *env, int csrno)
+{
+    if (riscv_cpu_mxl(env) != MXL_RV32) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    return sstc(env, csrno);
+}
+
 /* Checks if PointerMasking registers could be accessed */
 static RISCVException pointer_masking(CPURISCVState *env, int csrno)
 {
@@ -942,60 +996,6 @@ static RISCVException read_timeh(CPURISCVState *env, int csrno,
     return RISCV_EXCP_NONE;
 }
 
-static RISCVException sstc(CPURISCVState *env, int csrno)
-{
-    RISCVCPU *cpu = env_archcpu(env);
-    bool hmode_check = false;
-
-    if (!cpu->cfg.ext_sstc || !env->rdtime_fn) {
-        return RISCV_EXCP_ILLEGAL_INST;
-    }
-
-    if ((csrno == CSR_VSTIMECMP) || (csrno == CSR_VSTIMECMPH)) {
-        hmode_check = true;
-    }
-
-    RISCVException ret = hmode_check ? hmode(env, csrno) : smode(env, csrno);
-    if (ret != RISCV_EXCP_NONE) {
-        return ret;
-    }
-
-    if (env->debugger) {
-        return RISCV_EXCP_NONE;
-    }
-
-    if (env->priv == PRV_M) {
-        return RISCV_EXCP_NONE;
-    }
-
-    /*
-     * No need of separate function for rv32 as menvcfg stores both menvcfg
-     * menvcfgh for RV32.
-     */
-    if (!(get_field(env->mcounteren, COUNTEREN_TM) &&
-          get_field(env->menvcfg, MENVCFG_STCE))) {
-        return RISCV_EXCP_ILLEGAL_INST;
-    }
-
-    if (riscv_cpu_virt_enabled(env)) {
-        if (!(get_field(env->hcounteren, COUNTEREN_TM) &&
-              get_field(env->henvcfg, HENVCFG_STCE))) {
-            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
-        }
-    }
-
-    return RISCV_EXCP_NONE;
-}
-
-static RISCVException sstc_32(CPURISCVState *env, int csrno)
-{
-    if (riscv_cpu_mxl(env) != MXL_RV32) {
-        return RISCV_EXCP_ILLEGAL_INST;
-    }
-
-    return sstc(env, csrno);
-}
-
 static RISCVException read_vstimecmp(CPURISCVState *env, int csrno,
                                      target_ulong *val)
 {
-- 
2.25.1

Re: [PATCH 17/18] target/riscv: Group all predicate() routines together

[weiwei]

On 2023/2/14 12:31, Bin Meng wrote:
> Move sstc()/sstc32() to where all predicate() routines live.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,
Weiwei Li
> ---
>
>   target/riscv/csr.c | 108 ++++++++++++++++++++++-----------------------
>   1 file changed, 54 insertions(+), 54 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 40aae9e7b3..37350b8a6d 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -399,6 +399,60 @@ static RISCVException sstateen(CPURISCVState *env, int csrno)
>       return RISCV_EXCP_NONE;
>   }
>   
> +static RISCVException sstc(CPURISCVState *env, int csrno)
> +{
> +    RISCVCPU *cpu = env_archcpu(env);
> +    bool hmode_check = false;
> +
> +    if (!cpu->cfg.ext_sstc || !env->rdtime_fn) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    if ((csrno == CSR_VSTIMECMP) || (csrno == CSR_VSTIMECMPH)) {
> +        hmode_check = true;
> +    }
> +
> +    RISCVException ret = hmode_check ? hmode(env, csrno) : smode(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
> +    if (env->debugger) {
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +    if (env->priv == PRV_M) {
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +    /*
> +     * No need of separate function for rv32 as menvcfg stores both menvcfg
> +     * menvcfgh for RV32.
> +     */
> +    if (!(get_field(env->mcounteren, COUNTEREN_TM) &&
> +          get_field(env->menvcfg, MENVCFG_STCE))) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    if (riscv_cpu_virt_enabled(env)) {
> +        if (!(get_field(env->hcounteren, COUNTEREN_TM) &&
> +              get_field(env->henvcfg, HENVCFG_STCE))) {
> +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> +        }
> +    }
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException sstc_32(CPURISCVState *env, int csrno)
> +{
> +    if (riscv_cpu_mxl(env) != MXL_RV32) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    return sstc(env, csrno);
> +}
> +
>   /* Checks if PointerMasking registers could be accessed */
>   static RISCVException pointer_masking(CPURISCVState *env, int csrno)
>   {
> @@ -942,60 +996,6 @@ static RISCVException read_timeh(CPURISCVState *env, int csrno,
>       return RISCV_EXCP_NONE;
>   }
>   
> -static RISCVException sstc(CPURISCVState *env, int csrno)
> -{
> -    RISCVCPU *cpu = env_archcpu(env);
> -    bool hmode_check = false;
> -
> -    if (!cpu->cfg.ext_sstc || !env->rdtime_fn) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
> -    if ((csrno == CSR_VSTIMECMP) || (csrno == CSR_VSTIMECMPH)) {
> -        hmode_check = true;
> -    }
> -
> -    RISCVException ret = hmode_check ? hmode(env, csrno) : smode(env, csrno);
> -    if (ret != RISCV_EXCP_NONE) {
> -        return ret;
> -    }
> -
> -    if (env->debugger) {
> -        return RISCV_EXCP_NONE;
> -    }
> -
> -    if (env->priv == PRV_M) {
> -        return RISCV_EXCP_NONE;
> -    }
> -
> -    /*
> -     * No need of separate function for rv32 as menvcfg stores both menvcfg
> -     * menvcfgh for RV32.
> -     */
> -    if (!(get_field(env->mcounteren, COUNTEREN_TM) &&
> -          get_field(env->menvcfg, MENVCFG_STCE))) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
> -    if (riscv_cpu_virt_enabled(env)) {
> -        if (!(get_field(env->hcounteren, COUNTEREN_TM) &&
> -              get_field(env->henvcfg, HENVCFG_STCE))) {
> -            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> -        }
> -    }
> -
> -    return RISCV_EXCP_NONE;
> -}
> -
> -static RISCVException sstc_32(CPURISCVState *env, int csrno)
> -{
> -    if (riscv_cpu_mxl(env) != MXL_RV32) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
> -    return sstc(env, csrno);
> -}
> -
>   static RISCVException read_vstimecmp(CPURISCVState *env, int csrno,
>                                        target_ulong *val)
>   {

[PATCH 18/18] target/riscv: Move configuration check to envcfg CSRs predicate()

[Bin Meng]

At present the envcfg CSRs predicate() routines are generic one like
smode(), hmode. The configuration check is done in the read / write
routine. Create a new predicate routine to cover such check, so that
gdbstub can correctly report its existence.

Signed-off-by: Bin Meng <bmeng@tinylab.org>

---

 target/riscv/csr.c | 98 +++++++++++++++++++++++++++++-----------------
 1 file changed, 61 insertions(+), 37 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 37350b8a6d..284ccc09dd 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -41,40 +41,6 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
 }
 
 /* Predicates */
-#if !defined(CONFIG_USER_ONLY)
-static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
-                                       uint64_t bit)
-{
-    bool virt = riscv_cpu_virt_enabled(env);
-    RISCVCPU *cpu = env_archcpu(env);
-
-    if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
-        return RISCV_EXCP_NONE;
-    }
-
-    if (!(env->mstateen[index] & bit)) {
-        return RISCV_EXCP_ILLEGAL_INST;
-    }
-
-    if (virt) {
-        if (!(env->hstateen[index] & bit)) {
-            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
-        }
-
-        if (env->priv == PRV_U && !(env->sstateen[index] & bit)) {
-            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
-        }
-    }
-
-    if (env->priv == PRV_U && riscv_has_ext(env, RVS)) {
-        if (!(env->sstateen[index] & bit)) {
-            return RISCV_EXCP_ILLEGAL_INST;
-        }
-    }
-
-    return RISCV_EXCP_NONE;
-}
-#endif
 
 static RISCVException fs(CPURISCVState *env, int csrno)
 {
@@ -318,6 +284,32 @@ static RISCVException umode32(CPURISCVState *env, int csrno)
     return umode(env, csrno);
 }
 
+static RISCVException envcfg(CPURISCVState *env, int csrno)
+{
+    RISCVCPU *cpu = env_archcpu(env);
+    riscv_csr_predicate_fn predicate;
+
+    if (cpu->cfg.ext_smstateen) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    switch (csrno) {
+    case CSR_SENVCFG:
+        predicate = smode;
+        break;
+    case CSR_HENVCFG:
+        predicate = hmode;
+        break;
+    case CSR_HENVCFGH:
+        predicate = hmode32;
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    return predicate(env, csrno);
+}
+
 static RISCVException mstateen(CPURISCVState *env, int csrno)
 {
     RISCVCPU *cpu = env_archcpu(env);
@@ -1946,6 +1938,38 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
     return RISCV_EXCP_NONE;
 }
 
+static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
+                                       uint64_t bit)
+{
+    bool virt = riscv_cpu_virt_enabled(env);
+
+    if (env->priv == PRV_M) {
+        return RISCV_EXCP_NONE;
+    }
+
+    if (!(env->mstateen[index] & bit)) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    if (virt) {
+        if (!(env->hstateen[index] & bit)) {
+            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+        }
+
+        if (env->priv == PRV_U && !(env->sstateen[index] & bit)) {
+            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+        }
+    }
+
+    if (env->priv == PRV_U && riscv_has_ext(env, RVS)) {
+        if (!(env->sstateen[index] & bit)) {
+            return RISCV_EXCP_ILLEGAL_INST;
+        }
+    }
+
+    return RISCV_EXCP_NONE;
+}
+
 static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
                                    target_ulong *val)
 {
@@ -4087,11 +4111,11 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
                        .min_priv_ver = PRIV_VERSION_1_12_0              },
     [CSR_MENVCFGH] = { "menvcfgh", umode32, read_menvcfgh, write_menvcfgh,
                        .min_priv_ver = PRIV_VERSION_1_12_0              },
-    [CSR_SENVCFG]  = { "senvcfg",  smode, read_senvcfg,  write_senvcfg,
+    [CSR_SENVCFG]  = { "senvcfg",  envcfg, read_senvcfg,  write_senvcfg,
                        .min_priv_ver = PRIV_VERSION_1_12_0              },
-    [CSR_HENVCFG]  = { "henvcfg",  hmode, read_henvcfg, write_henvcfg,
+    [CSR_HENVCFG]  = { "henvcfg",  envcfg, read_henvcfg,  write_henvcfg,
                        .min_priv_ver = PRIV_VERSION_1_12_0              },
-    [CSR_HENVCFGH] = { "henvcfgh", hmode32, read_henvcfgh, write_henvcfgh,
+    [CSR_HENVCFGH] = { "henvcfgh", envcfg, read_henvcfgh, write_henvcfgh,
                        .min_priv_ver = PRIV_VERSION_1_12_0              },
 
     /* Smstateen extension CSRs */
-- 
2.25.1

Re: [PATCH 18/18] target/riscv: Move configuration check to envcfg CSRs predicate()

[weiwei]

On 2023/2/14 22:27, Bin Meng wrote:
> At present the envcfg CSRs predicate() routines are generic one like
> smode(), hmode. The configuration check is done in the read / write
> routine. Create a new predicate routine to cover such check, so that
> gdbstub can correctly report its existence.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
>
> ---
>
>   target/riscv/csr.c | 98 +++++++++++++++++++++++++++++-----------------
>   1 file changed, 61 insertions(+), 37 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 37350b8a6d..284ccc09dd 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -41,40 +41,6 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
>   }
>   
>   /* Predicates */
> -#if !defined(CONFIG_USER_ONLY)
> -static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
> -                                       uint64_t bit)
> -{
> -    bool virt = riscv_cpu_virt_enabled(env);
> -    RISCVCPU *cpu = env_archcpu(env);
> -
> -    if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
> -        return RISCV_EXCP_NONE;
> -    }
> -
> -    if (!(env->mstateen[index] & bit)) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
> -    if (virt) {
> -        if (!(env->hstateen[index] & bit)) {
> -            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> -        }
> -
> -        if (env->priv == PRV_U && !(env->sstateen[index] & bit)) {
> -            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> -        }
> -    }
> -
> -    if (env->priv == PRV_U && riscv_has_ext(env, RVS)) {
> -        if (!(env->sstateen[index] & bit)) {
> -            return RISCV_EXCP_ILLEGAL_INST;
> -        }
> -    }
> -
> -    return RISCV_EXCP_NONE;
> -}
> -#endif
>   
>   static RISCVException fs(CPURISCVState *env, int csrno)
>   {
> @@ -318,6 +284,32 @@ static RISCVException umode32(CPURISCVState *env, int csrno)
>       return umode(env, csrno);
>   }
>   
> +static RISCVException envcfg(CPURISCVState *env, int csrno)
> +{
> +    RISCVCPU *cpu = env_archcpu(env);
> +    riscv_csr_predicate_fn predicate;
> +
> +    if (cpu->cfg.ext_smstateen) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }

This check seems not right here.  Why  ILLEGAL_INST is directly 
triggered if smstateen is enabled?

It seems that smstateen related check will be done  for 
senvcfg/henvcfg{h} when smstateen is enabled.

Regards,

Weiwei Li

> +
> +    switch (csrno) {
> +    case CSR_SENVCFG:
> +        predicate = smode;
> +        break;
> +    case CSR_HENVCFG:
> +        predicate = hmode;
> +        break;
> +    case CSR_HENVCFGH:
> +        predicate = hmode32;
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    return predicate(env, csrno);
> +}
> +
>   static RISCVException mstateen(CPURISCVState *env, int csrno)
>   {
>       RISCVCPU *cpu = env_archcpu(env);
> @@ -1946,6 +1938,38 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>       return RISCV_EXCP_NONE;
>   }
>   
> +static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
> +                                       uint64_t bit)
> +{
> +    bool virt = riscv_cpu_virt_enabled(env);
> +
> +    if (env->priv == PRV_M) {
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +    if (!(env->mstateen[index] & bit)) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    if (virt) {
> +        if (!(env->hstateen[index] & bit)) {
> +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> +        }
> +
> +        if (env->priv == PRV_U && !(env->sstateen[index] & bit)) {
> +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> +        }
> +    }
> +
> +    if (env->priv == PRV_U && riscv_has_ext(env, RVS)) {
> +        if (!(env->sstateen[index] & bit)) {
> +            return RISCV_EXCP_ILLEGAL_INST;
> +        }
> +    }
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
>   static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
>                                      target_ulong *val)
>   {
> @@ -4087,11 +4111,11 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>                          .min_priv_ver = PRIV_VERSION_1_12_0              },
>       [CSR_MENVCFGH] = { "menvcfgh", umode32, read_menvcfgh, write_menvcfgh,
>                          .min_priv_ver = PRIV_VERSION_1_12_0              },
> -    [CSR_SENVCFG]  = { "senvcfg",  smode, read_senvcfg,  write_senvcfg,
> +    [CSR_SENVCFG]  = { "senvcfg",  envcfg, read_senvcfg,  write_senvcfg,
>                          .min_priv_ver = PRIV_VERSION_1_12_0              },
> -    [CSR_HENVCFG]  = { "henvcfg",  hmode, read_henvcfg, write_henvcfg,
> +    [CSR_HENVCFG]  = { "henvcfg",  envcfg, read_henvcfg,  write_henvcfg,
>                          .min_priv_ver = PRIV_VERSION_1_12_0              },
> -    [CSR_HENVCFGH] = { "henvcfgh", hmode32, read_henvcfgh, write_henvcfgh,
> +    [CSR_HENVCFGH] = { "henvcfgh", envcfg, read_henvcfgh, write_henvcfgh,
>                          .min_priv_ver = PRIV_VERSION_1_12_0              },
>   
>       /* Smstateen extension CSRs */

Re: [PATCH 18/18] target/riscv: Move configuration check to envcfg CSRs predicate()

[Bin Meng]

On Tue, Feb 14, 2023 at 10:59 PM weiwei <liweiwei@iscas.ac.cn> wrote:
>
>
> On 2023/2/14 22:27, Bin Meng wrote:
> > At present the envcfg CSRs predicate() routines are generic one like
> > smode(), hmode. The configuration check is done in the read / write
> > routine. Create a new predicate routine to cover such check, so that
> > gdbstub can correctly report its existence.
> >
> > Signed-off-by: Bin Meng <bmeng@tinylab.org>
> >
> > ---
> >
> >   target/riscv/csr.c | 98 +++++++++++++++++++++++++++++-----------------
> >   1 file changed, 61 insertions(+), 37 deletions(-)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 37350b8a6d..284ccc09dd 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -41,40 +41,6 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
> >   }
> >
> >   /* Predicates */
> > -#if !defined(CONFIG_USER_ONLY)
> > -static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
> > -                                       uint64_t bit)
> > -{
> > -    bool virt = riscv_cpu_virt_enabled(env);
> > -    RISCVCPU *cpu = env_archcpu(env);
> > -
> > -    if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
> > -        return RISCV_EXCP_NONE;
> > -    }
> > -
> > -    if (!(env->mstateen[index] & bit)) {
> > -        return RISCV_EXCP_ILLEGAL_INST;
> > -    }
> > -
> > -    if (virt) {
> > -        if (!(env->hstateen[index] & bit)) {
> > -            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > -        }
> > -
> > -        if (env->priv == PRV_U && !(env->sstateen[index] & bit)) {
> > -            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > -        }
> > -    }
> > -
> > -    if (env->priv == PRV_U && riscv_has_ext(env, RVS)) {
> > -        if (!(env->sstateen[index] & bit)) {
> > -            return RISCV_EXCP_ILLEGAL_INST;
> > -        }
> > -    }
> > -
> > -    return RISCV_EXCP_NONE;
> > -}
> > -#endif
> >
> >   static RISCVException fs(CPURISCVState *env, int csrno)
> >   {
> > @@ -318,6 +284,32 @@ static RISCVException umode32(CPURISCVState *env, int csrno)
> >       return umode(env, csrno);
> >   }
> >
> > +static RISCVException envcfg(CPURISCVState *env, int csrno)
> > +{
> > +    RISCVCPU *cpu = env_archcpu(env);
> > +    riscv_csr_predicate_fn predicate;
> > +
> > +    if (cpu->cfg.ext_smstateen) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
>
> This check seems not right here.  Why  ILLEGAL_INST is directly
> triggered if smstateen is enabled?

This logic was there in the original codes. I was confused when I
looked at this as well.

Anyway, if it is an issue, it should be a separate patch.

>
> It seems that smstateen related check will be done  for
> senvcfg/henvcfg{h} when smstateen is enabled.
>

Regards,
Bin

Re: [PATCH 18/18] target/riscv: Move configuration check to envcfg CSRs predicate()

[weiwei]

On 2023/2/15 10:22, Bin Meng wrote:
> On Tue, Feb 14, 2023 at 10:59 PM weiwei <liweiwei@iscas.ac.cn> wrote:
>>
>> On 2023/2/14 22:27, Bin Meng wrote:
>>> At present the envcfg CSRs predicate() routines are generic one like
>>> smode(), hmode. The configuration check is done in the read / write
>>> routine. Create a new predicate routine to cover such check, so that
>>> gdbstub can correctly report its existence.
>>>
>>> Signed-off-by: Bin Meng <bmeng@tinylab.org>
>>>
>>> ---
>>>
>>>    target/riscv/csr.c | 98 +++++++++++++++++++++++++++++-----------------
>>>    1 file changed, 61 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>> index 37350b8a6d..284ccc09dd 100644
>>> --- a/target/riscv/csr.c
>>> +++ b/target/riscv/csr.c
>>> @@ -41,40 +41,6 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
>>>    }
>>>
>>>    /* Predicates */
>>> -#if !defined(CONFIG_USER_ONLY)
>>> -static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
>>> -                                       uint64_t bit)
>>> -{
>>> -    bool virt = riscv_cpu_virt_enabled(env);
>>> -    RISCVCPU *cpu = env_archcpu(env);
>>> -
>>> -    if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
>>> -        return RISCV_EXCP_NONE;
>>> -    }
>>> -
>>> -    if (!(env->mstateen[index] & bit)) {
>>> -        return RISCV_EXCP_ILLEGAL_INST;
>>> -    }
>>> -
>>> -    if (virt) {
>>> -        if (!(env->hstateen[index] & bit)) {
>>> -            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>>> -        }
>>> -
>>> -        if (env->priv == PRV_U && !(env->sstateen[index] & bit)) {
>>> -            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>>> -        }
>>> -    }
>>> -
>>> -    if (env->priv == PRV_U && riscv_has_ext(env, RVS)) {
>>> -        if (!(env->sstateen[index] & bit)) {
>>> -            return RISCV_EXCP_ILLEGAL_INST;
>>> -        }
>>> -    }
>>> -
>>> -    return RISCV_EXCP_NONE;
>>> -}
>>> -#endif
>>>
>>>    static RISCVException fs(CPURISCVState *env, int csrno)
>>>    {
>>> @@ -318,6 +284,32 @@ static RISCVException umode32(CPURISCVState *env, int csrno)
>>>        return umode(env, csrno);
>>>    }
>>>
>>> +static RISCVException envcfg(CPURISCVState *env, int csrno)
>>> +{
>>> +    RISCVCPU *cpu = env_archcpu(env);
>>> +    riscv_csr_predicate_fn predicate;
>>> +
>>> +    if (cpu->cfg.ext_smstateen) {
>>> +        return RISCV_EXCP_ILLEGAL_INST;
>>> +    }
>> This check seems not right here.  Why  ILLEGAL_INST is directly
>> triggered if smstateen is enabled?
> This logic was there in the original codes. I was confused when I
> looked at this as well.

Sorry, I didn't find the original codes. Do you mean this:

     if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
         return RISCV_EXCP_NONE;
     }

If so, I think the check here is to make the following *stateen related
check ignored when smstateen extension is disabled.

Regards,

Weiwei Li

> Anyway, if it is an issue, it should be a separate patch.
>
>> It seems that smstateen related check will be done  for
>> senvcfg/henvcfg{h} when smstateen is enabled.
>>
> Regards,
> Bin

Re: [PATCH 18/18] target/riscv: Move configuration check to envcfg CSRs predicate()

[Palmer Dabbelt]

On Tue, 14 Feb 2023 18:22:21 PST (-0800), Bin Meng wrote:
> On Tue, Feb 14, 2023 at 10:59 PM weiwei <liweiwei@iscas.ac.cn> wrote:
>>
>>
>> On 2023/2/14 22:27, Bin Meng wrote:
>> > At present the envcfg CSRs predicate() routines are generic one like
>> > smode(), hmode. The configuration check is done in the read / write
>> > routine. Create a new predicate routine to cover such check, so that
>> > gdbstub can correctly report its existence.
>> >
>> > Signed-off-by: Bin Meng <bmeng@tinylab.org>
>> >
>> > ---
>> >
>> >   target/riscv/csr.c | 98 +++++++++++++++++++++++++++++-----------------
>> >   1 file changed, 61 insertions(+), 37 deletions(-)
>> >
>> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> > index 37350b8a6d..284ccc09dd 100644
>> > --- a/target/riscv/csr.c
>> > +++ b/target/riscv/csr.c
>> > @@ -41,40 +41,6 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
>> >   }
>> >
>> >   /* Predicates */
>> > -#if !defined(CONFIG_USER_ONLY)
>> > -static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
>> > -                                       uint64_t bit)
>> > -{
>> > -    bool virt = riscv_cpu_virt_enabled(env);
>> > -    RISCVCPU *cpu = env_archcpu(env);
>> > -
>> > -    if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
>> > -        return RISCV_EXCP_NONE;
>> > -    }
>> > -
>> > -    if (!(env->mstateen[index] & bit)) {
>> > -        return RISCV_EXCP_ILLEGAL_INST;
>> > -    }
>> > -
>> > -    if (virt) {
>> > -        if (!(env->hstateen[index] & bit)) {
>> > -            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>> > -        }
>> > -
>> > -        if (env->priv == PRV_U && !(env->sstateen[index] & bit)) {
>> > -            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>> > -        }
>> > -    }
>> > -
>> > -    if (env->priv == PRV_U && riscv_has_ext(env, RVS)) {
>> > -        if (!(env->sstateen[index] & bit)) {
>> > -            return RISCV_EXCP_ILLEGAL_INST;
>> > -        }
>> > -    }
>> > -
>> > -    return RISCV_EXCP_NONE;
>> > -}
>> > -#endif
>> >
>> >   static RISCVException fs(CPURISCVState *env, int csrno)
>> >   {
>> > @@ -318,6 +284,32 @@ static RISCVException umode32(CPURISCVState *env, int csrno)
>> >       return umode(env, csrno);
>> >   }
>> >
>> > +static RISCVException envcfg(CPURISCVState *env, int csrno)
>> > +{
>> > +    RISCVCPU *cpu = env_archcpu(env);
>> > +    riscv_csr_predicate_fn predicate;
>> > +
>> > +    if (cpu->cfg.ext_smstateen) {
>> > +        return RISCV_EXCP_ILLEGAL_INST;
>> > +    }
>>
>> This check seems not right here.  Why  ILLEGAL_INST is directly
>> triggered if smstateen is enabled?
>
> This logic was there in the original codes. I was confused when I
> looked at this as well.
>
> Anyway, if it is an issue, it should be a separate patch.

Seems reasonable to me, it's always nice to split up the refactoring types.  So
I queued this up as 4ac6c32224 ("Merge patch series "target/riscv: Various
fixes to gdbstub and CSR access"").

I had to fix up the From address on the patch you re-sent and there was a minor
merge conflict, but otherwise things look sane to me.  I'll hold off on sending
anything for a bit just in case, though.

Thanks!

>
>>
>> It seems that smstateen related check will be done  for
>> senvcfg/henvcfg{h} when smstateen is enabled.
>>
>
> Regards,
> Bin

Re: [PATCH 18/18] target/riscv: Move configuration check to envcfg CSRs predicate()

[Bin Meng]

Hi Palmer,

On Fri, Feb 17, 2023 at 12:40 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Tue, 14 Feb 2023 18:22:21 PST (-0800), Bin Meng wrote:
> > On Tue, Feb 14, 2023 at 10:59 PM weiwei <liweiwei@iscas.ac.cn> wrote:
> >>
> >>
> >> On 2023/2/14 22:27, Bin Meng wrote:
> >> > At present the envcfg CSRs predicate() routines are generic one like
> >> > smode(), hmode. The configuration check is done in the read / write
> >> > routine. Create a new predicate routine to cover such check, so that
> >> > gdbstub can correctly report its existence.
> >> >
> >> > Signed-off-by: Bin Meng <bmeng@tinylab.org>
> >> >
> >> > ---
> >> >
> >> >   target/riscv/csr.c | 98 +++++++++++++++++++++++++++++-----------------
> >> >   1 file changed, 61 insertions(+), 37 deletions(-)
> >> >
> >> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> > index 37350b8a6d..284ccc09dd 100644
> >> > --- a/target/riscv/csr.c
> >> > +++ b/target/riscv/csr.c
> >> > @@ -41,40 +41,6 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
> >> >   }
> >> >
> >> >   /* Predicates */
> >> > -#if !defined(CONFIG_USER_ONLY)
> >> > -static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
> >> > -                                       uint64_t bit)
> >> > -{
> >> > -    bool virt = riscv_cpu_virt_enabled(env);
> >> > -    RISCVCPU *cpu = env_archcpu(env);
> >> > -
> >> > -    if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
> >> > -        return RISCV_EXCP_NONE;
> >> > -    }
> >> > -
> >> > -    if (!(env->mstateen[index] & bit)) {
> >> > -        return RISCV_EXCP_ILLEGAL_INST;
> >> > -    }
> >> > -
> >> > -    if (virt) {
> >> > -        if (!(env->hstateen[index] & bit)) {
> >> > -            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> >> > -        }
> >> > -
> >> > -        if (env->priv == PRV_U && !(env->sstateen[index] & bit)) {
> >> > -            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> >> > -        }
> >> > -    }
> >> > -
> >> > -    if (env->priv == PRV_U && riscv_has_ext(env, RVS)) {
> >> > -        if (!(env->sstateen[index] & bit)) {
> >> > -            return RISCV_EXCP_ILLEGAL_INST;
> >> > -        }
> >> > -    }
> >> > -
> >> > -    return RISCV_EXCP_NONE;
> >> > -}
> >> > -#endif
> >> >
> >> >   static RISCVException fs(CPURISCVState *env, int csrno)
> >> >   {
> >> > @@ -318,6 +284,32 @@ static RISCVException umode32(CPURISCVState *env, int csrno)
> >> >       return umode(env, csrno);
> >> >   }
> >> >
> >> > +static RISCVException envcfg(CPURISCVState *env, int csrno)
> >> > +{
> >> > +    RISCVCPU *cpu = env_archcpu(env);
> >> > +    riscv_csr_predicate_fn predicate;
> >> > +
> >> > +    if (cpu->cfg.ext_smstateen) {
> >> > +        return RISCV_EXCP_ILLEGAL_INST;
> >> > +    }
> >>
> >> This check seems not right here.  Why  ILLEGAL_INST is directly
> >> triggered if smstateen is enabled?
> >
> > This logic was there in the original codes. I was confused when I
> > looked at this as well.
> >
> > Anyway, if it is an issue, it should be a separate patch.
>
> Seems reasonable to me, it's always nice to split up the refactoring types.  So
> I queued this up as 4ac6c32224 ("Merge patch series "target/riscv: Various
> fixes to gdbstub and CSR access"").
>
> I had to fix up the From address on the patch you re-sent and there was a minor
> merge conflict, but otherwise things look sane to me.  I'll hold off on sending
> anything for a bit just in case, though.
>

There are some open comments in this series I need to address. Please
drop this v1. I will send v2 soon.

Regards,
Bin

Re: [PATCH 18/18] target/riscv: Move configuration check to envcfg CSRs predicate()

[Palmer Dabbelt]

On Thu, 16 Feb 2023 17:59:42 PST (-0800), Bin Meng wrote:
> Hi Palmer,
>
> On Fri, Feb 17, 2023 at 12:40 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Tue, 14 Feb 2023 18:22:21 PST (-0800), Bin Meng wrote:
>> > On Tue, Feb 14, 2023 at 10:59 PM weiwei <liweiwei@iscas.ac.cn> wrote:
>> >>
>> >>
>> >> On 2023/2/14 22:27, Bin Meng wrote:
>> >> > At present the envcfg CSRs predicate() routines are generic one like
>> >> > smode(), hmode. The configuration check is done in the read / write
>> >> > routine. Create a new predicate routine to cover such check, so that
>> >> > gdbstub can correctly report its existence.
>> >> >
>> >> > Signed-off-by: Bin Meng <bmeng@tinylab.org>
>> >> >
>> >> > ---
>> >> >
>> >> >   target/riscv/csr.c | 98 +++++++++++++++++++++++++++++-----------------
>> >> >   1 file changed, 61 insertions(+), 37 deletions(-)
>> >> >
>> >> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> >> > index 37350b8a6d..284ccc09dd 100644
>> >> > --- a/target/riscv/csr.c
>> >> > +++ b/target/riscv/csr.c
>> >> > @@ -41,40 +41,6 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
>> >> >   }
>> >> >
>> >> >   /* Predicates */
>> >> > -#if !defined(CONFIG_USER_ONLY)
>> >> > -static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
>> >> > -                                       uint64_t bit)
>> >> > -{
>> >> > -    bool virt = riscv_cpu_virt_enabled(env);
>> >> > -    RISCVCPU *cpu = env_archcpu(env);
>> >> > -
>> >> > -    if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
>> >> > -        return RISCV_EXCP_NONE;
>> >> > -    }
>> >> > -
>> >> > -    if (!(env->mstateen[index] & bit)) {
>> >> > -        return RISCV_EXCP_ILLEGAL_INST;
>> >> > -    }
>> >> > -
>> >> > -    if (virt) {
>> >> > -        if (!(env->hstateen[index] & bit)) {
>> >> > -            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>> >> > -        }
>> >> > -
>> >> > -        if (env->priv == PRV_U && !(env->sstateen[index] & bit)) {
>> >> > -            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>> >> > -        }
>> >> > -    }
>> >> > -
>> >> > -    if (env->priv == PRV_U && riscv_has_ext(env, RVS)) {
>> >> > -        if (!(env->sstateen[index] & bit)) {
>> >> > -            return RISCV_EXCP_ILLEGAL_INST;
>> >> > -        }
>> >> > -    }
>> >> > -
>> >> > -    return RISCV_EXCP_NONE;
>> >> > -}
>> >> > -#endif
>> >> >
>> >> >   static RISCVException fs(CPURISCVState *env, int csrno)
>> >> >   {
>> >> > @@ -318,6 +284,32 @@ static RISCVException umode32(CPURISCVState *env, int csrno)
>> >> >       return umode(env, csrno);
>> >> >   }
>> >> >
>> >> > +static RISCVException envcfg(CPURISCVState *env, int csrno)
>> >> > +{
>> >> > +    RISCVCPU *cpu = env_archcpu(env);
>> >> > +    riscv_csr_predicate_fn predicate;
>> >> > +
>> >> > +    if (cpu->cfg.ext_smstateen) {
>> >> > +        return RISCV_EXCP_ILLEGAL_INST;
>> >> > +    }
>> >>
>> >> This check seems not right here.  Why  ILLEGAL_INST is directly
>> >> triggered if smstateen is enabled?
>> >
>> > This logic was there in the original codes. I was confused when I
>> > looked at this as well.
>> >
>> > Anyway, if it is an issue, it should be a separate patch.
>>
>> Seems reasonable to me, it's always nice to split up the refactoring types.  So
>> I queued this up as 4ac6c32224 ("Merge patch series "target/riscv: Various
>> fixes to gdbstub and CSR access"").
>>
>> I had to fix up the From address on the patch you re-sent and there was a minor
>> merge conflict, but otherwise things look sane to me.  I'll hold off on sending
>> anything for a bit just in case, though.
>>
>
> There are some open comments in this series I need to address. Please
> drop this v1. I will send v2 soon.

Sorry aobut that, I'd thought they were all reviewed.  I've dropped it 
from the queue.

Thanks!

>
> Regards,
> Bin