[PATCH v3 0/3] target/riscv: Expose "priv" register for GDB

[PATCH v3 0/3] target/riscv: Expose "priv" register for GDB

[Jonathan Behrens]

The third patch in this series makes the priv virtual register writitable. I'm
not entirely sure this is a good idea, so I split it out into its own patch. In
particular, this change will conflict with the hypervisor extension work which
assumes that the privilege mode does not change in unexpected cases.

As pointed out in a previous version of this series, GDB actually contains some
support already for the accessing the privilege mode via a virtual "priv"
register, including to convert the values into human readable forms:

(gdb) info reg priv
priv           0x3	prv:3 [Machine]

Changlog V3:
- Break patch into series
- Make priv a virtual register

Changelog V2:
- Use PRV_H and PRV_S instead of integer literals

Jonathan Behrens (3)
  target/riscv: Tell gdbstub the correct number of CSRs
  target/riscv: Expose priv register for GDB for reads
  target/riscv: Make the priv register writable by GDB

 configure                       |  4 ++--
 gdb-xml/riscv-32bit-virtual.xml | 11 +++++++++++
 gdb-xml/riscv-64bit-virtual.xml | 11 +++++++++++
 target/riscv/gdbstub.c          | 36 ++++++++++++++++++++++++++++++++++--
 4 files changed, 58 insertions(+), 4 deletions(-)

[PATCH v3 1/3] target/riscv: Tell gdbstub the correct number of CSRs

[Jonathan Behrens]

If the number of registers reported to the gdbstub code does not match the
number in the associated XML file, then the register numbers used by the stub
may get out of sync with a remote GDB instance.

Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
---
 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 ded140e8d8..cb5bfd3d50 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -384,7 +384,7 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
     }
 
     gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
-                             4096, "riscv-32bit-csr.xml", 0);
+                             240, "riscv-32bit-csr.xml", 0);
 #elif defined(TARGET_RISCV64)
     if (env->misa & RVF) {
         gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
@@ -392,6 +392,6 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
     }
 
     gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
-                             4096, "riscv-64bit-csr.xml", 0);
+                             240, "riscv-64bit-csr.xml", 0);
 #endif
 }
-- 
2.23.0

Re: [PATCH v3 1/3] target/riscv: Tell gdbstub the correct number of CSRs

[Bin Meng]

On Tue, Oct 8, 2019 at 8:15 AM Jonathan Behrens <jonathan@fintelia.io> wrote:
>
> If the number of registers reported to the gdbstub code does not match the
> number in the associated XML file, then the register numbers used by the stub
> may get out of sync with a remote GDB instance.

I am not sure how to trigger the out of sync issue. Do you know how?

>
> Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
> ---
>  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 ded140e8d8..cb5bfd3d50 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -384,7 +384,7 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
>      }
>
>      gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
> -                             4096, "riscv-32bit-csr.xml", 0);
> +                             240, "riscv-32bit-csr.xml", 0);
>  #elif defined(TARGET_RISCV64)
>      if (env->misa & RVF) {
>          gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
> @@ -392,6 +392,6 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
>      }
>
>      gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
> -                             4096, "riscv-64bit-csr.xml", 0);
> +                             240, "riscv-64bit-csr.xml", 0);
>  #endif
>  }

The change looks good to me.

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH v3 1/3] target/riscv: Tell gdbstub the correct number of CSRs

[Jonathan Behrens]

On Tue, Oct 8, 2019 at 5:54 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Tue, Oct 8, 2019 at 8:15 AM Jonathan Behrens <jonathan@fintelia.io> wrote:
> >
> > If the number of registers reported to the gdbstub code does not match the
> > number in the associated XML file, then the register numbers used by the stub
> > may get out of sync with a remote GDB instance.
>
> I am not sure how to trigger the out of sync issue. Do you know how?

Try applying the next patch in the series without this one. Attempting
to access the priv register will return E14 because GDB will think
that priv has register number 309 (since the last register with an
assigned regnum is fcsr=68 and then none of the 240 CSRs have assigned
numbers) while the GDB stub will think that the number is 4165 (=33 +
36 + 4096).

Re: [PATCH v3 1/3] target/riscv: Tell gdbstub the correct number of CSRs

[Alistair Francis]

On Mon, Oct 7, 2019 at 5:16 PM Jonathan Behrens <jonathan@fintelia.io> wrote:
>
> If the number of registers reported to the gdbstub code does not match the
> number in the associated XML file, then the register numbers used by the stub
> may get out of sync with a remote GDB instance.
>
> Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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 ded140e8d8..cb5bfd3d50 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -384,7 +384,7 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
>      }
>
>      gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
> -                             4096, "riscv-32bit-csr.xml", 0);
> +                             240, "riscv-32bit-csr.xml", 0);
>  #elif defined(TARGET_RISCV64)
>      if (env->misa & RVF) {
>          gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
> @@ -392,6 +392,6 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
>      }
>
>      gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
> -                             4096, "riscv-64bit-csr.xml", 0);
> +                             240, "riscv-64bit-csr.xml", 0);
>  #endif
>  }
> --
> 2.23.0
>

[PATCH v3 2/3] target/riscv: Expose priv register for GDB for reads

[Jonathan Behrens]

This patch enables a debugger to read the current privilege level via a virtual
"priv" register. When compiled with CONFIG_USER_ONLY the register is still
visible but always reports the value zero.

Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
---
 configure                       |  4 ++--
 gdb-xml/riscv-32bit-virtual.xml | 11 +++++++++++
 gdb-xml/riscv-64bit-virtual.xml | 11 +++++++++++
 target/riscv/gdbstub.c          | 23 +++++++++++++++++++++++
 4 files changed, 47 insertions(+), 2 deletions(-)
 create mode 100644 gdb-xml/riscv-32bit-virtual.xml
 create mode 100644 gdb-xml/riscv-64bit-virtual.xml

diff --git a/configure b/configure
index 30544f52e6..6118a6a045 100755
--- a/configure
+++ b/configure
@@ -7520,13 +7520,13 @@ case "$target_name" in
     TARGET_BASE_ARCH=riscv
     TARGET_ABI_DIR=riscv
     mttcg=yes
-    gdb_xml_files="riscv-32bit-cpu.xml riscv-32bit-fpu.xml riscv-32bit-csr.xml"
+    gdb_xml_files="riscv-32bit-cpu.xml riscv-32bit-fpu.xml riscv-32bit-csr.xml riscv-32bit-virtual.xml"
   ;;
   riscv64)
     TARGET_BASE_ARCH=riscv
     TARGET_ABI_DIR=riscv
     mttcg=yes
-    gdb_xml_files="riscv-64bit-cpu.xml riscv-64bit-fpu.xml riscv-64bit-csr.xml"
+    gdb_xml_files="riscv-64bit-cpu.xml riscv-64bit-fpu.xml riscv-64bit-csr.xml riscv-64bit-virtual.xml"
   ;;
   sh4|sh4eb)
     TARGET_ARCH=sh4
diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-xml/riscv-32bit-virtual.xml
new file mode 100644
index 0000000000..905f1c555d
--- /dev/null
+++ b/gdb-xml/riscv-32bit-virtual.xml
@@ -0,0 +1,11 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2018-2019 Free Software Foundation, Inc.
+
+     Copying and distribution of this file, with or without modification,
+     are permitted in any medium without royalty provided the copyright
+     notice and this notice are preserved.  -->
+
+<!DOCTYPE feature SYSTEM "gdb-target.dtd">
+<feature name="org.gnu.gdb.riscv.virtual">
+  <reg name="priv" bitsize="32"/>
+</feature>
diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-xml/riscv-64bit-virtual.xml
new file mode 100644
index 0000000000..62d86c237b
--- /dev/null
+++ b/gdb-xml/riscv-64bit-virtual.xml
@@ -0,0 +1,11 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2018-2019 Free Software Foundation, Inc.
+
+     Copying and distribution of this file, with or without modification,
+     are permitted in any medium without royalty provided the copyright
+     notice and this notice are preserved.  -->
+
+<!DOCTYPE feature SYSTEM "gdb-target.dtd">
+<feature name="org.gnu.gdb.riscv.virtual">
+  <reg name="priv" bitsize="64"/>
+</feature>
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index cb5bfd3d50..33cf7c4c7d 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -373,6 +373,23 @@ static int riscv_gdb_set_csr(CPURISCVState *env, uint8_t *mem_buf, int n)
     return 0;
 }
 
+static int riscv_gdb_get_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
+{
+    if (n == 0) {
+#ifdef CONFIG_USER_ONLY
+        return gdb_get_regl(mem_buf, 0);
+#else
+        return gdb_get_regl(mem_buf, cs->priv);
+#endif
+    }
+    return 0;
+}
+
+static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
+{
+    return 0;
+}
+
 void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
 {
     RISCVCPU *cpu = RISCV_CPU(cs);
@@ -385,6 +402,9 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
 
     gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
                              240, "riscv-32bit-csr.xml", 0);
+
+    gdb_register_coprocessor(cs, riscv_gdb_get_virtual, riscv_gdb_set_virtual,
+                             1, "riscv-32bit-csr.xml", 0);
 #elif defined(TARGET_RISCV64)
     if (env->misa & RVF) {
         gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
@@ -393,5 +413,8 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
 
     gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
                              240, "riscv-64bit-csr.xml", 0);
+
+    gdb_register_coprocessor(cs, riscv_gdb_get_virtual, riscv_gdb_set_virtual,
+                             1, "riscv-64bit-virtual.xml", 0);
 #endif
 }
-- 
2.23.0

Re: [PATCH v3 2/3] target/riscv: Expose priv register for GDB for reads

[Bin Meng]

On Tue, Oct 8, 2019 at 8:18 AM Jonathan Behrens <jonathan@fintelia.io> wrote:
>
> This patch enables a debugger to read the current privilege level via a virtual
> "priv" register. When compiled with CONFIG_USER_ONLY the register is still
> visible but always reports the value zero.
>
> Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
> ---
>  configure                       |  4 ++--
>  gdb-xml/riscv-32bit-virtual.xml | 11 +++++++++++
>  gdb-xml/riscv-64bit-virtual.xml | 11 +++++++++++
>  target/riscv/gdbstub.c          | 23 +++++++++++++++++++++++
>  4 files changed, 47 insertions(+), 2 deletions(-)
>  create mode 100644 gdb-xml/riscv-32bit-virtual.xml
>  create mode 100644 gdb-xml/riscv-64bit-virtual.xml
>
> diff --git a/configure b/configure
> index 30544f52e6..6118a6a045 100755
> --- a/configure
> +++ b/configure
> @@ -7520,13 +7520,13 @@ case "$target_name" in
>      TARGET_BASE_ARCH=riscv
>      TARGET_ABI_DIR=riscv
>      mttcg=yes
> -    gdb_xml_files="riscv-32bit-cpu.xml riscv-32bit-fpu.xml riscv-32bit-csr.xml"
> +    gdb_xml_files="riscv-32bit-cpu.xml riscv-32bit-fpu.xml riscv-32bit-csr.xml riscv-32bit-virtual.xml"
>    ;;
>    riscv64)
>      TARGET_BASE_ARCH=riscv
>      TARGET_ABI_DIR=riscv
>      mttcg=yes
> -    gdb_xml_files="riscv-64bit-cpu.xml riscv-64bit-fpu.xml riscv-64bit-csr.xml"
> +    gdb_xml_files="riscv-64bit-cpu.xml riscv-64bit-fpu.xml riscv-64bit-csr.xml riscv-64bit-virtual.xml"
>    ;;
>    sh4|sh4eb)
>      TARGET_ARCH=sh4
> diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-xml/riscv-32bit-virtual.xml
> new file mode 100644
> index 0000000000..905f1c555d
> --- /dev/null
> +++ b/gdb-xml/riscv-32bit-virtual.xml
> @@ -0,0 +1,11 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2018-2019 Free Software Foundation, Inc.
> +
> +     Copying and distribution of this file, with or without modification,
> +     are permitted in any medium without royalty provided the copyright
> +     notice and this notice are preserved.  -->
> +
> +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
> +<feature name="org.gnu.gdb.riscv.virtual">
> +  <reg name="priv" bitsize="32"/>
> +</feature>
> diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-xml/riscv-64bit-virtual.xml
> new file mode 100644
> index 0000000000..62d86c237b
> --- /dev/null
> +++ b/gdb-xml/riscv-64bit-virtual.xml
> @@ -0,0 +1,11 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2018-2019 Free Software Foundation, Inc.
> +
> +     Copying and distribution of this file, with or without modification,
> +     are permitted in any medium without royalty provided the copyright
> +     notice and this notice are preserved.  -->
> +
> +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
> +<feature name="org.gnu.gdb.riscv.virtual">
> +  <reg name="priv" bitsize="64"/>
> +</feature>
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index cb5bfd3d50..33cf7c4c7d 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -373,6 +373,23 @@ static int riscv_gdb_set_csr(CPURISCVState *env, uint8_t *mem_buf, int n)
>      return 0;
>  }
>
> +static int riscv_gdb_get_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
> +{
> +    if (n == 0) {
> +#ifdef CONFIG_USER_ONLY
> +        return gdb_get_regl(mem_buf, 0);
> +#else
> +        return gdb_get_regl(mem_buf, cs->priv);
> +#endif
> +    }
> +    return 0;
> +}
> +
> +static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
> +{
> +    return 0;
> +}
> +
>  void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
>  {
>      RISCVCPU *cpu = RISCV_CPU(cs);
> @@ -385,6 +402,9 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
>
>      gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
>                               240, "riscv-32bit-csr.xml", 0);
> +
> +    gdb_register_coprocessor(cs, riscv_gdb_get_virtual, riscv_gdb_set_virtual,
> +                             1, "riscv-32bit-csr.xml", 0);

This should be riscv-32bit-virtual.xml

>  #elif defined(TARGET_RISCV64)
>      if (env->misa & RVF) {
>          gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
> @@ -393,5 +413,8 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
>
>      gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
>                               240, "riscv-64bit-csr.xml", 0);
> +
> +    gdb_register_coprocessor(cs, riscv_gdb_get_virtual, riscv_gdb_set_virtual,
> +                             1, "riscv-64bit-virtual.xml", 0);
>  #endif
>  }

Regards,
Bin

Re: [PATCH v3 2/3] target/riscv: Expose priv register for GDB for reads

[Jonathan Behrens]

On Tue, Oct 8, 2019 at 8:27 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Tue, Oct 8, 2019 at 8:18 AM Jonathan Behrens <jonathan@fintelia.io> wrote:
> >
> > This patch enables a debugger to read the current privilege level via a virtual
> > "priv" register. When compiled with CONFIG_USER_ONLY the register is still
> > visible but always reports the value zero.
> >
> > Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
> > ---
> >  configure                       |  4 ++--
> >  gdb-xml/riscv-32bit-virtual.xml | 11 +++++++++++
> >  gdb-xml/riscv-64bit-virtual.xml | 11 +++++++++++
> >  target/riscv/gdbstub.c          | 23 +++++++++++++++++++++++
> >  4 files changed, 47 insertions(+), 2 deletions(-)
> >  create mode 100644 gdb-xml/riscv-32bit-virtual.xml
> >  create mode 100644 gdb-xml/riscv-64bit-virtual.xml
> >
> > diff --git a/configure b/configure
> > index 30544f52e6..6118a6a045 100755
> > --- a/configure
> > +++ b/configure
> > @@ -7520,13 +7520,13 @@ case "$target_name" in
> >      TARGET_BASE_ARCH=riscv
> >      TARGET_ABI_DIR=riscv
> >      mttcg=yes
> > -    gdb_xml_files="riscv-32bit-cpu.xml riscv-32bit-fpu.xml riscv-32bit-csr.xml"
> > +    gdb_xml_files="riscv-32bit-cpu.xml riscv-32bit-fpu.xml riscv-32bit-csr.xml riscv-32bit-virtual.xml"
> >    ;;
> >    riscv64)
> >      TARGET_BASE_ARCH=riscv
> >      TARGET_ABI_DIR=riscv
> >      mttcg=yes
> > -    gdb_xml_files="riscv-64bit-cpu.xml riscv-64bit-fpu.xml riscv-64bit-csr.xml"
> > +    gdb_xml_files="riscv-64bit-cpu.xml riscv-64bit-fpu.xml riscv-64bit-csr.xml riscv-64bit-virtual.xml"
> >    ;;
> >    sh4|sh4eb)
> >      TARGET_ARCH=sh4
> > diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-xml/riscv-32bit-virtual.xml
> > new file mode 100644
> > index 0000000000..905f1c555d
> > --- /dev/null
> > +++ b/gdb-xml/riscv-32bit-virtual.xml
> > @@ -0,0 +1,11 @@
> > +<?xml version="1.0"?>
> > +<!-- Copyright (C) 2018-2019 Free Software Foundation, Inc.
> > +
> > +     Copying and distribution of this file, with or without modification,
> > +     are permitted in any medium without royalty provided the copyright
> > +     notice and this notice are preserved.  -->
> > +
> > +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > +<feature name="org.gnu.gdb.riscv.virtual">
> > +  <reg name="priv" bitsize="32"/>
> > +</feature>
> > diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-xml/riscv-64bit-virtual.xml
> > new file mode 100644
> > index 0000000000..62d86c237b
> > --- /dev/null
> > +++ b/gdb-xml/riscv-64bit-virtual.xml
> > @@ -0,0 +1,11 @@
> > +<?xml version="1.0"?>
> > +<!-- Copyright (C) 2018-2019 Free Software Foundation, Inc.
> > +
> > +     Copying and distribution of this file, with or without modification,
> > +     are permitted in any medium without royalty provided the copyright
> > +     notice and this notice are preserved.  -->
> > +
> > +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > +<feature name="org.gnu.gdb.riscv.virtual">
> > +  <reg name="priv" bitsize="64"/>
> > +</feature>
> > diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> > index cb5bfd3d50..33cf7c4c7d 100644
> > --- a/target/riscv/gdbstub.c
> > +++ b/target/riscv/gdbstub.c
> > @@ -373,6 +373,23 @@ static int riscv_gdb_set_csr(CPURISCVState *env, uint8_t *mem_buf, int n)
> >      return 0;
> >  }
> >
> > +static int riscv_gdb_get_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
> > +{
> > +    if (n == 0) {
> > +#ifdef CONFIG_USER_ONLY
> > +        return gdb_get_regl(mem_buf, 0);
> > +#else
> > +        return gdb_get_regl(mem_buf, cs->priv);
> > +#endif
> > +    }
> > +    return 0;
> > +}
> > +
> > +static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
> > +{
> > +    return 0;
> > +}
> > +
> >  void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
> >  {
> >      RISCVCPU *cpu = RISCV_CPU(cs);
> > @@ -385,6 +402,9 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
> >
> >      gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
> >                               240, "riscv-32bit-csr.xml", 0);
> > +
> > +    gdb_register_coprocessor(cs, riscv_gdb_get_virtual, riscv_gdb_set_virtual,
> > +                             1, "riscv-32bit-csr.xml", 0);
>
> This should be riscv-32bit-virtual.xml

Good catch. I'll fix this in the next version.


>
> >  #elif defined(TARGET_RISCV64)
> >      if (env->misa & RVF) {
> >          gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
> > @@ -393,5 +413,8 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
> >
> >      gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
> >                               240, "riscv-64bit-csr.xml", 0);
> > +
> > +    gdb_register_coprocessor(cs, riscv_gdb_get_virtual, riscv_gdb_set_virtual,
> > +                             1, "riscv-64bit-virtual.xml", 0);
> >  #endif
> >  }
>
> Regards,
> Bin
>

[PATCH v3 3/3] target/riscv: Make the priv register writable by GDB

[Jonathan Behrens]

Currently only PRV_U, PRV_S and PRV_M are supported, so this patch ensures that
the privilege mode is set to one of them. Once support for the H-extension is
added, this code will also need to properly update the virtualization status
when switching between VU/VS-modes and M-mode.

Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
---
 target/riscv/gdbstub.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index 33cf7c4c7d..bc84b599c2 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -387,6 +387,15 @@ static int riscv_gdb_get_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
 
 static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
 {
+    if (n == 0) {
+#ifndef CONFIG_USER_ONLY
+        cs->priv = ldtul_p(mem_buf) & 0x3;
+        if (cs->priv == PRV_H) {
+            cs->priv = PRV_S;
+        }
+#endif
+        return sizeof(target_ulong);
+    }
     return 0;
 }
 
-- 
2.23.0

Re: [PATCH v3 3/3] target/riscv: Make the priv register writable by GDB

[Bin Meng]

On Tue, Oct 8, 2019 at 8:20 AM Jonathan Behrens <jonathan@fintelia.io> wrote:
>
> Currently only PRV_U, PRV_S and PRV_M are supported, so this patch ensures that
> the privilege mode is set to one of them. Once support for the H-extension is
> added, this code will also need to properly update the virtualization status
> when switching between VU/VS-modes and M-mode.
>
> Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
> ---
>  target/riscv/gdbstub.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Tested-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH v3 3/3] target/riscv: Make the priv register writable by GDB

[Alistair Francis]

On Mon, Oct 7, 2019 at 5:20 PM Jonathan Behrens <jonathan@fintelia.io> wrote:
>
> Currently only PRV_U, PRV_S and PRV_M are supported, so this patch ensures that
> the privilege mode is set to one of them. Once support for the H-extension is
> added, this code will also need to properly update the virtualization status
> when switching between VU/VS-modes and M-mode.
>
> Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/gdbstub.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index 33cf7c4c7d..bc84b599c2 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -387,6 +387,15 @@ static int riscv_gdb_get_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
>
>  static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
>  {
> +    if (n == 0) {
> +#ifndef CONFIG_USER_ONLY
> +        cs->priv = ldtul_p(mem_buf) & 0x3;
> +        if (cs->priv == PRV_H) {
> +            cs->priv = PRV_S;
> +        }
> +#endif
> +        return sizeof(target_ulong);
> +    }
>      return 0;
>  }
>
> --
> 2.23.0
>

Re: [PATCH v3 0/3] target/riscv: Expose "priv" register for GDB

[Jonathan Behrens]

The first paragraph seems to have gone missing. Repeated below:

> This series adds a new "priv" virtual register that reports the current
> privilege mode. This is helpful for debugging purposes because that information
> is not actually available in any of the real CSRs.


On Mon, Oct 7, 2019 at 8:15 PM Jonathan Behrens <jonathan@fintelia.io> wrote:
>
> The third patch in this series makes the priv virtual register writitable. I'm
> not entirely sure this is a good idea, so I split it out into its own patch. In
> particular, this change will conflict with the hypervisor extension work which
> assumes that the privilege mode does not change in unexpected cases.
>
> As pointed out in a previous version of this series, GDB actually contains some
> support already for the accessing the privilege mode via a virtual "priv"
> register, including to convert the values into human readable forms:
>
> (gdb) info reg priv
> priv           0x3      prv:3 [Machine]
>
> Changlog V3:
> - Break patch into series
> - Make priv a virtual register
>
> Changelog V2:
> - Use PRV_H and PRV_S instead of integer literals
>
> Jonathan Behrens (3)
>   target/riscv: Tell gdbstub the correct number of CSRs
>   target/riscv: Expose priv register for GDB for reads
>   target/riscv: Make the priv register writable by GDB
>
>  configure                       |  4 ++--
>  gdb-xml/riscv-32bit-virtual.xml | 11 +++++++++++
>  gdb-xml/riscv-64bit-virtual.xml | 11 +++++++++++
>  target/riscv/gdbstub.c          | 36 ++++++++++++++++++++++++++++++++++--
>  4 files changed, 58 insertions(+), 4 deletions(-)
>
>