* [PATCH v2] target/riscv: Expose "priv" register for GDB
@ 2019-10-04 15:16 ` Jonathan Behrens
0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Behrens @ 2019-10-04 15:16 UTC (permalink / raw)
To: qemu-devel, qemu-riscv
Cc: Sagar Karandikar, Jonathan Behrens, Palmer Dabbelt,
Philippe Mathieu-Daudé,
Alistair Francis, Bastian Koppelmann, Alex Bennée
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;
+ }
+#endif
+ return sizeof(target_ulong);
}
return 0;
}
--
2.23.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2] target/riscv: Expose "priv" register for GDB
@ 2019-10-04 15:16 ` Jonathan Behrens
0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Behrens @ 2019-10-04 15:16 UTC (permalink / raw)
To: qemu-devel, qemu-riscv
Cc: Jonathan Behrens, Alex Bennée, Philippe Mathieu-Daudé,
Palmer Dabbelt, Alistair Francis, Sagar Karandikar,
Bastian Koppelmann
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;
+ }
+#endif
+ return sizeof(target_ulong);
}
return 0;
}
--
2.23.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] target/riscv: Expose "priv" register for GDB
2019-10-04 15:16 ` Jonathan Behrens
@ 2019-10-05 12:08 ` Bin Meng
-1 siblings, 0 replies; 16+ messages in thread
From: Bin Meng @ 2019-10-05 12:08 UTC (permalink / raw)
To: Jonathan Behrens
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers,
Alex Bennée, Alistair Francis, Philippe Mathieu-Daudé
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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] target/riscv: Expose "priv" register for GDB
@ 2019-10-05 12:08 ` Bin Meng
0 siblings, 0 replies; 16+ messages in thread
From: Bin Meng @ 2019-10-05 12:08 UTC (permalink / raw)
To: Jonathan Behrens
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V,
Sagar Karandikar, Palmer Dabbelt, Philippe Mathieu-Daudé,
Alistair Francis, Bastian Koppelmann, Alex Bennée
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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] target/riscv: Expose "priv" register for GDB
2019-10-04 15:16 ` Jonathan Behrens
@ 2019-10-07 18:31 ` Alistair Francis
-1 siblings, 0 replies; 16+ messages in thread
From: Alistair Francis @ 2019-10-07 18:31 UTC (permalink / raw)
To: Jonathan Behrens
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers,
Alex Bennée, Alistair Francis, Philippe Mathieu-Daudé
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] target/riscv: Expose "priv" register for GDB
@ 2019-10-07 18:31 ` Alistair Francis
0 siblings, 0 replies; 16+ messages in thread
From: Alistair Francis @ 2019-10-07 18:31 UTC (permalink / raw)
To: Jonathan Behrens
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V,
Sagar Karandikar, Palmer Dabbelt, Philippe Mathieu-Daudé,
Alistair Francis, Bastian Koppelmann, Alex Bennée
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] target/riscv: Expose "priv" register for GDB
2019-10-04 15:16 ` Jonathan Behrens
@ 2019-10-07 21:16 ` Jim Wilson
-1 siblings, 0 replies; 16+ messages in thread
From: Jim Wilson @ 2019-10-07 21:16 UTC (permalink / raw)
To: Jonathan Behrens, qemu-devel, qemu-riscv
Cc: Sagar Karandikar, Philippe Mathieu-Daudé,
Palmer Dabbelt, Alistair Francis, Bastian Koppelmann,
Alex Bennée
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] target/riscv: Expose "priv" register for GDB
@ 2019-10-07 21:16 ` Jim Wilson
0 siblings, 0 replies; 16+ messages in thread
From: Jim Wilson @ 2019-10-07 21:16 UTC (permalink / raw)
To: Jonathan Behrens, qemu-devel, qemu-riscv
Cc: Sagar Karandikar, Palmer Dabbelt, Philippe Mathieu-Daudé,
Alistair Francis, Bastian Koppelmann, Alex Bennée
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] target/riscv: Expose "priv" register for GDB
2019-10-07 21:16 ` Jim Wilson
@ 2019-10-08 0:19 ` Jonathan Behrens
-1 siblings, 0 replies; 16+ messages in thread
From: Jonathan Behrens @ 2019-10-08 0:19 UTC (permalink / raw)
To: Jim Wilson
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers,
Alex Bennée, Alistair Francis, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] target/riscv: Expose "priv" register for GDB
@ 2019-10-08 0:19 ` Jonathan Behrens
0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Behrens @ 2019-10-08 0:19 UTC (permalink / raw)
To: Jim Wilson
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V,
Sagar Karandikar, Palmer Dabbelt, Philippe Mathieu-Daudé,
Alistair Francis, Bastian Koppelmann, Alex Bennée
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] target/riscv: Expose "priv" register for GDB
2019-10-07 18:31 ` Alistair Francis
@ 2019-10-08 0:22 ` Jonathan Behrens
-1 siblings, 0 replies; 16+ messages in thread
From: Jonathan Behrens @ 2019-10-08 0:22 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers,
Alex Bennée, Alistair Francis, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] target/riscv: Expose "priv" register for GDB
@ 2019-10-08 0:22 ` Jonathan Behrens
0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Behrens @ 2019-10-08 0:22 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V,
Sagar Karandikar, Palmer Dabbelt, Philippe Mathieu-Daudé,
Alistair Francis, Bastian Koppelmann, Alex Bennée
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] target/riscv: Expose "priv" register for GDB
2019-10-07 21:16 ` Jim Wilson
@ 2019-10-08 9:00 ` Bin Meng
-1 siblings, 0 replies; 16+ messages in thread
From: Bin Meng @ 2019-10-08 9:00 UTC (permalink / raw)
To: Jim Wilson
Cc: open list:RISC-V, Sagar Karandikar, Jonathan Behrens,
Palmer Dabbelt, qemu-devel@nongnu.org Developers,
Alex Bennée, Alistair Francis, Bastian Koppelmann,
Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] target/riscv: Expose "priv" register for GDB
@ 2019-10-08 9:00 ` Bin Meng
0 siblings, 0 replies; 16+ messages in thread
From: Bin Meng @ 2019-10-08 9:00 UTC (permalink / raw)
To: Jim Wilson
Cc: Jonathan Behrens, qemu-devel@nongnu.org Developers,
open list:RISC-V, Sagar Karandikar, Philippe Mathieu-Daudé,
Palmer Dabbelt, Alistair Francis, Bastian Koppelmann,
Alex Bennée
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] target/riscv: Expose "priv" register for GDB
2019-10-08 9:00 ` Bin Meng
@ 2019-10-08 16:42 ` Jim Wilson
-1 siblings, 0 replies; 16+ messages in thread
From: Jim Wilson @ 2019-10-08 16:42 UTC (permalink / raw)
To: Bin Meng
Cc: open list:RISC-V, Sagar Karandikar, Jonathan Behrens,
Palmer Dabbelt, qemu-devel@nongnu.org Developers,
Alex Bennée, Alistair Francis, Bastian Koppelmann,
Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] target/riscv: Expose "priv" register for GDB
@ 2019-10-08 16:42 ` Jim Wilson
0 siblings, 0 replies; 16+ messages in thread
From: Jim Wilson @ 2019-10-08 16:42 UTC (permalink / raw)
To: Bin Meng
Cc: Jonathan Behrens, qemu-devel@nongnu.org Developers,
open list:RISC-V, Sagar Karandikar, Philippe Mathieu-Daudé,
Palmer Dabbelt, Alistair Francis, Bastian Koppelmann,
Alex Bennée
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
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-10-08 16:43 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-04 15:16 [PATCH v2] target/riscv: Expose "priv" register for GDB Jonathan Behrens
2019-10-04 15:16 ` Jonathan Behrens
2019-10-05 12:08 ` Bin Meng
2019-10-05 12:08 ` Bin Meng
2019-10-07 18:31 ` Alistair Francis
2019-10-07 18:31 ` Alistair Francis
2019-10-08 0:22 ` Jonathan Behrens
2019-10-08 0:22 ` Jonathan Behrens
2019-10-07 21:16 ` Jim Wilson
2019-10-07 21:16 ` Jim Wilson
2019-10-08 0:19 ` Jonathan Behrens
2019-10-08 0:19 ` Jonathan Behrens
2019-10-08 9:00 ` Bin Meng
2019-10-08 9:00 ` Bin Meng
2019-10-08 16:42 ` Jim Wilson
2019-10-08 16:42 ` Jim Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.