| Message ID | 20191004151614.81516-1-jonathan@fintelia.io |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] target/riscv: Expose "priv" register for GDB | expand |
On Fri, Oct 4, 2019 at 11:18 PM Jonathan Behrens <jonathan@fintelia.io> wrote: > > This patch enables a debugger to read and write the current privilege level via > a special "priv" register. When compiled with CONFIG_USER_ONLY the register is > still visible but is hardwired to zero. > > Signed-off-by: Jonathan Behrens <jonathan@fintelia.io> > --- > gdb-xml/riscv-32bit-cpu.xml | 1 + > gdb-xml/riscv-64bit-cpu.xml | 1 + > target/riscv/cpu.c | 2 +- > target/riscv/gdbstub.c | 14 ++++++++++++++ > 4 files changed, 17 insertions(+), 1 deletion(-) > --- > Changelog V2: > - Use PRV_H and PRV_S instead of integer literals > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> Tested-by: Bin Meng <bmeng.cn@gmail.com>
On Fri, Oct 4, 2019 at 8:18 AM Jonathan Behrens <jonathan@fintelia.io> wrote: > > This patch enables a debugger to read and write the current privilege level via > a special "priv" register. When compiled with CONFIG_USER_ONLY the register is > still visible but is hardwired to zero. > > Signed-off-by: Jonathan Behrens <jonathan@fintelia.io> > --- > gdb-xml/riscv-32bit-cpu.xml | 1 + > gdb-xml/riscv-64bit-cpu.xml | 1 + > target/riscv/cpu.c | 2 +- > target/riscv/gdbstub.c | 14 ++++++++++++++ > 4 files changed, 17 insertions(+), 1 deletion(-) > --- > Changelog V2: > - Use PRV_H and PRV_S instead of integer literals > > diff --git a/gdb-xml/riscv-32bit-cpu.xml b/gdb-xml/riscv-32bit-cpu.xml > index 0d07aaec85..d6d76aafd8 100644 > --- a/gdb-xml/riscv-32bit-cpu.xml > +++ b/gdb-xml/riscv-32bit-cpu.xml > @@ -44,4 +44,5 @@ > <reg name="t5" bitsize="32" type="int"/> > <reg name="t6" bitsize="32" type="int"/> > <reg name="pc" bitsize="32" type="code_ptr"/> > + <reg name="priv" bitsize="32" type="int"/> > </feature> > diff --git a/gdb-xml/riscv-64bit-cpu.xml b/gdb-xml/riscv-64bit-cpu.xml > index b8aa424ae4..0758d1b5fe 100644 > --- a/gdb-xml/riscv-64bit-cpu.xml > +++ b/gdb-xml/riscv-64bit-cpu.xml > @@ -44,4 +44,5 @@ > <reg name="t5" bitsize="64" type="int"/> > <reg name="t6" bitsize="64" type="int"/> > <reg name="pc" bitsize="64" type="code_ptr"/> > + <reg name="priv" bitsize="64" type="int" /> > </feature> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index f13e298a36..347989858f 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -475,7 +475,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data) > cc->synchronize_from_tb = riscv_cpu_synchronize_from_tb; > cc->gdb_read_register = riscv_cpu_gdb_read_register; > cc->gdb_write_register = riscv_cpu_gdb_write_register; > - cc->gdb_num_core_regs = 33; > + cc->gdb_num_core_regs = 34; > #if defined(TARGET_RISCV32) > cc->gdb_core_xml_file = "riscv-32bit-cpu.xml"; > #elif defined(TARGET_RISCV64) > diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c > index ded140e8d8..7e0822145d 100644 > --- a/target/riscv/gdbstub.c > +++ b/target/riscv/gdbstub.c > @@ -278,6 +278,12 @@ int riscv_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n) > return gdb_get_regl(mem_buf, env->gpr[n]); > } else if (n == 32) { > return gdb_get_regl(mem_buf, env->pc); > + } else if (n == 33) { > +#ifdef CONFIG_USER_ONLY > + return gdb_get_regl(mem_buf, 0); > +#else > + return gdb_get_regl(mem_buf, env->priv); > +#endif > } > return 0; > } > @@ -296,6 +302,14 @@ int riscv_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n) > } else if (n == 32) { > env->pc = ldtul_p(mem_buf); > return sizeof(target_ulong); > + } else if (n == 33) { > +#ifndef CONFIG_USER_ONLY > + env->priv = ldtul_p(mem_buf) & 0x3; > + if (env->priv == PRV_H) { > + env->priv = PRV_S; > + } Why have this? There is no PRV_H so we should never be in that privilege mode. Alistair > +#endif > + return sizeof(target_ulong); > } > return 0; > } > -- > 2.23.0 >
On 10/4/19 8:16 AM, Jonathan Behrens wrote: > diff --git a/gdb-xml/riscv-32bit-cpu.xml b/gdb-xml/riscv-32bit-cpu.xml > index 0d07aaec85..d6d76aafd8 100644 > --- a/gdb-xml/riscv-32bit-cpu.xml > +++ b/gdb-xml/riscv-32bit-cpu.xml > @@ -44,4 +44,5 @@ > <reg name="t5" bitsize="32" type="int"/> > <reg name="t6" bitsize="32" type="int"/> > <reg name="pc" bitsize="32" type="code_ptr"/> > + <reg name="priv" bitsize="32" type="int"/> > </feature> Adding this to the cpu register set means that the gdb "info registers" command will now list a value for the mysterious undocumented "priv" register. That is likely to result in user confusion, and a few gdb bug reports. Gdb incidentally already has support for a virtual priv register. From gdb/riscv-tdep.c: static const struct riscv_register_feature riscv_virtual_feature = { "org.gnu.gdb.riscv.virtual", { { RISCV_PRIV_REGNUM, { "priv" }, false } } }; So the correct way to fix this is to add a gdb-xml/riscv-32bit-virtual.xml file, along with code to handle this new xml file and the registers in it. Likewise for the 64-bit support. The main advantage of doing things this way is that only people that care about the priv register will see it, and this will interoperate with other RISC-V debuggers and targets (if any) that already have virtual priv register support. Jim
On Mon, Oct 7, 2019 at 5:17 PM Jim Wilson <jimw@sifive.com> wrote: > On 10/4/19 8:16 AM, Jonathan Behrens wrote: > > diff --git a/gdb-xml/riscv-32bit-cpu.xml b/gdb-xml/riscv-32bit-cpu.xml > > index 0d07aaec85..d6d76aafd8 100644 > > --- a/gdb-xml/riscv-32bit-cpu.xml > > +++ b/gdb-xml/riscv-32bit-cpu.xml > > @@ -44,4 +44,5 @@ > > <reg name="t5" bitsize="32" type="int"/> > > <reg name="t6" bitsize="32" type="int"/> > > <reg name="pc" bitsize="32" type="code_ptr"/> > > + <reg name="priv" bitsize="32" type="int"/> > > </feature> > > Adding this to the cpu register set means that the gdb "info registers" > command will now list a value for the mysterious undocumented "priv" > register. That is likely to result in user confusion, and a few gdb bug > reports. > > Gdb incidentally already has support for a virtual priv register. From > gdb/riscv-tdep.c: > > static const struct riscv_register_feature riscv_virtual_feature = > { > "org.gnu.gdb.riscv.virtual", > { > { RISCV_PRIV_REGNUM, { "priv" }, false } > } > }; > > So the correct way to fix this is to add a > gdb-xml/riscv-32bit-virtual.xml file, along with code to handle this new > xml file and the registers in it. Likewise for the 64-bit support. > > The main advantage of doing things this way is that only people that > care about the priv register will see it, and this will interoperate > with other RISC-V debuggers and targets (if any) that already have > virtual priv register support. > > Jim Thanks for this suggestion, I've incorporated it into the next version. Jonathan
On Mon, Oct 7, 2019 at 2:36 PM Alistair Francis <alistair23@gmail.com> wrote: > On Fri, Oct 4, 2019 at 8:18 AM Jonathan Behrens <jonathan@fintelia.io> wrote: > > @@ -296,6 +302,14 @@ int riscv_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n) > > } else if (n == 32) { > > env->pc = ldtul_p(mem_buf); > > return sizeof(target_ulong); > > + } else if (n == 33) { > > +#ifndef CONFIG_USER_ONLY > > + env->priv = ldtul_p(mem_buf) & 0x3; > > + if (env->priv == PRV_H) { > > + env->priv = PRV_S; > > + } > > Why have this? There is no PRV_H so we should never be in that privilege mode. > > Alistair This is hopefully more clear in the next version, but the idea is that since GDB can try to set the privilege mode to *any* value this function needs to make sure that it isn't set to something unsupported like PRV_H. Jonathan
Hi Jim, On Tue, Oct 8, 2019 at 5:17 AM Jim Wilson <jimw@sifive.com> wrote: > > On 10/4/19 8:16 AM, Jonathan Behrens wrote: > > diff --git a/gdb-xml/riscv-32bit-cpu.xml b/gdb-xml/riscv-32bit-cpu.xml > > index 0d07aaec85..d6d76aafd8 100644 > > --- a/gdb-xml/riscv-32bit-cpu.xml > > +++ b/gdb-xml/riscv-32bit-cpu.xml > > @@ -44,4 +44,5 @@ > > <reg name="t5" bitsize="32" type="int"/> > > <reg name="t6" bitsize="32" type="int"/> > > <reg name="pc" bitsize="32" type="code_ptr"/> > > + <reg name="priv" bitsize="32" type="int"/> > > </feature> > > Adding this to the cpu register set means that the gdb "info registers" > command will now list a value for the mysterious undocumented "priv" My gdb does not list "priv" register after applying this patch. >>> info registers ra 0x0 0x0 sp 0x0 0x0 gp 0x0 0x0 tp 0x0 0x0 t0 0x1000 4096 t1 0x0 0 t2 0x0 0 fp 0x0 0x0 s1 0x0 0 a0 0x0 0 a1 0x1020 4128 a2 0x0 0 a3 0x0 0 a4 0x0 0 a5 0x0 0 a6 0x0 0 a7 0x0 0 s2 0x0 0 s3 0x0 0 s4 0x0 0 s5 0x0 0 s6 0x0 0 s7 0x0 0 s8 0x0 0 s9 0x0 0 s10 0x0 0 s11 0x0 0 t3 0x0 0 t4 0x0 0 t5 0x0 0 t6 0x0 0 pc 0x1008 0x1008 Anything I was missing? > register. That is likely to result in user confusion, and a few gdb bug > reports. > > Gdb incidentally already has support for a virtual priv register. From > gdb/riscv-tdep.c: > > static const struct riscv_register_feature riscv_virtual_feature = > { > "org.gnu.gdb.riscv.virtual", > { > { RISCV_PRIV_REGNUM, { "priv" }, false } > } > }; > > So the correct way to fix this is to add a > gdb-xml/riscv-32bit-virtual.xml file, along with code to handle this new > xml file and the registers in it. Likewise for the 64-bit support. > > The main advantage of doing things this way is that only people that > care about the priv register will see it, and this will interoperate > with other RISC-V debuggers and targets (if any) that already have > virtual priv register support. Regards, Bin
On Tue, Oct 8, 2019 at 2:00 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> My gdb does not list "priv" register after applying this patch.
I didn't try the patch, I didn't have time for that. I would expect
priv to be in the "info registers" output if you are adding it to the
cpu register set. Shrug. Anyways, defining priv as a virtual
register is the right way to do this, as it isn't a cpu register, and
gdb already has support for virtual registers like priv.
Jim
diff --git a/gdb-xml/riscv-32bit-cpu.xml b/gdb-xml/riscv-32bit-cpu.xml index 0d07aaec85..d6d76aafd8 100644 --- a/gdb-xml/riscv-32bit-cpu.xml +++ b/gdb-xml/riscv-32bit-cpu.xml @@ -44,4 +44,5 @@ <reg name="t5" bitsize="32" type="int"/> <reg name="t6" bitsize="32" type="int"/> <reg name="pc" bitsize="32" type="code_ptr"/> + <reg name="priv" bitsize="32" type="int"/> </feature> diff --git a/gdb-xml/riscv-64bit-cpu.xml b/gdb-xml/riscv-64bit-cpu.xml index b8aa424ae4..0758d1b5fe 100644 --- a/gdb-xml/riscv-64bit-cpu.xml +++ b/gdb-xml/riscv-64bit-cpu.xml @@ -44,4 +44,5 @@ <reg name="t5" bitsize="64" type="int"/> <reg name="t6" bitsize="64" type="int"/> <reg name="pc" bitsize="64" type="code_ptr"/> + <reg name="priv" bitsize="64" type="int" /> </feature> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index f13e298a36..347989858f 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -475,7 +475,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data) cc->synchronize_from_tb = riscv_cpu_synchronize_from_tb; cc->gdb_read_register = riscv_cpu_gdb_read_register; cc->gdb_write_register = riscv_cpu_gdb_write_register; - cc->gdb_num_core_regs = 33; + cc->gdb_num_core_regs = 34; #if defined(TARGET_RISCV32) cc->gdb_core_xml_file = "riscv-32bit-cpu.xml"; #elif defined(TARGET_RISCV64) diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c index ded140e8d8..7e0822145d 100644 --- a/target/riscv/gdbstub.c +++ b/target/riscv/gdbstub.c @@ -278,6 +278,12 @@ int riscv_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n) return gdb_get_regl(mem_buf, env->gpr[n]); } else if (n == 32) { return gdb_get_regl(mem_buf, env->pc); + } else if (n == 33) { +#ifdef CONFIG_USER_ONLY + return gdb_get_regl(mem_buf, 0); +#else + return gdb_get_regl(mem_buf, env->priv); +#endif } return 0; } @@ -296,6 +302,14 @@ int riscv_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n) } else if (n == 32) { env->pc = ldtul_p(mem_buf); return sizeof(target_ulong); + } else if (n == 33) { +#ifndef CONFIG_USER_ONLY + env->priv = ldtul_p(mem_buf) & 0x3; + if (env->priv == PRV_H) { + env->priv = PRV_S; + } +#endif + return sizeof(target_ulong); } return 0; }
This patch enables a debugger to read and write the current privilege level via a special "priv" register. When compiled with CONFIG_USER_ONLY the register is still visible but is hardwired to zero. Signed-off-by: Jonathan Behrens <jonathan@fintelia.io> --- gdb-xml/riscv-32bit-cpu.xml | 1 + gdb-xml/riscv-64bit-cpu.xml | 1 + target/riscv/cpu.c | 2 +- target/riscv/gdbstub.c | 14 ++++++++++++++ 4 files changed, 17 insertions(+), 1 deletion(-) --- Changelog V2: - Use PRV_H and PRV_S instead of integer literals