* [PATCH v2] riscv/gdb: add virt mode debug interface
@ 2024-11-28 7:21 Yanfeng Liu
2024-11-28 14:21 ` Alex Bennée
0 siblings, 1 reply; 12+ messages in thread
From: Yanfeng Liu @ 2024-11-28 7:21 UTC (permalink / raw)
To: qemu-riscv; +Cc: qemu-devel, alistair.francis, Yanfeng Liu
This adds `virt` virtual register on debug interface so that users
can access current virtualization mode for debugging purposes.
Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
---
gdb-xml/riscv-32bit-virtual.xml | 1 +
gdb-xml/riscv-64bit-virtual.xml | 1 +
target/riscv/gdbstub.c | 18 ++++++++++++------
3 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-xml/riscv-32bit-virtual.xml
index 905f1c555d..d44b6ca2dc 100644
--- a/gdb-xml/riscv-32bit-virtual.xml
+++ b/gdb-xml/riscv-32bit-virtual.xml
@@ -8,4 +8,5 @@
<!DOCTYPE feature SYSTEM "gdb-target.dtd">
<feature name="org.gnu.gdb.riscv.virtual">
<reg name="priv" bitsize="32"/>
+ <reg name="virt" bitsize="32"/>
</feature>
diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-xml/riscv-64bit-virtual.xml
index 62d86c237b..7c9b63d5b6 100644
--- a/gdb-xml/riscv-64bit-virtual.xml
+++ b/gdb-xml/riscv-64bit-virtual.xml
@@ -8,4 +8,5 @@
<!DOCTYPE feature SYSTEM "gdb-target.dtd">
<feature name="org.gnu.gdb.riscv.virtual">
<reg name="priv" bitsize="64"/>
+ <reg name="virt" bitsize="64"/>
</feature>
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index c07df972f1..7399003e04 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -206,14 +206,14 @@ static int riscv_gdb_set_csr(CPUState *cs, uint8_t *mem_buf, int n)
static int riscv_gdb_get_virtual(CPUState *cs, GByteArray *buf, int n)
{
- if (n == 0) {
+ if (n >= 0 && n <= 1) {
#ifdef CONFIG_USER_ONLY
return gdb_get_regl(buf, 0);
#else
RISCVCPU *cpu = RISCV_CPU(cs);
CPURISCVState *env = &cpu->env;
- return gdb_get_regl(buf, env->priv);
+ return gdb_get_regl(buf, n ? env->virt_enabled : env->priv);
#endif
}
return 0;
@@ -221,14 +221,20 @@ static int riscv_gdb_get_virtual(CPUState *cs, GByteArray *buf, int n)
static int riscv_gdb_set_virtual(CPUState *cs, uint8_t *mem_buf, int n)
{
- if (n == 0) {
+ if (n >= 0 && n <= 1) {
#ifndef CONFIG_USER_ONLY
RISCVCPU *cpu = RISCV_CPU(cs);
CPURISCVState *env = &cpu->env;
- env->priv = ldtul_p(mem_buf) & 0x3;
- if (env->priv == PRV_RESERVED) {
- env->priv = PRV_S;
+ if (n == 0) {
+ env->priv = ldtul_p(mem_buf) & 0x3;
+ if (env->priv == PRV_RESERVED) {
+ env->priv = PRV_S;
+ }
+ } else {
+ if (env->priv != PRV_M) {
+ env->virt_enabled = ldtul_p(mem_buf) & 0x1;
+ }
}
#endif
return sizeof(target_ulong);
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] riscv/gdb: add virt mode debug interface
2024-11-28 7:21 [PATCH v2] riscv/gdb: add virt mode debug interface Yanfeng Liu
@ 2024-11-28 14:21 ` Alex Bennée
2024-11-28 18:01 ` Richard Henderson
2024-11-29 2:11 ` Yanfeng
0 siblings, 2 replies; 12+ messages in thread
From: Alex Bennée @ 2024-11-28 14:21 UTC (permalink / raw)
To: Yanfeng Liu; +Cc: qemu-riscv, qemu-devel, alistair.francis
Yanfeng Liu <yfliu2008@qq.com> writes:
> This adds `virt` virtual register on debug interface so that users
> can access current virtualization mode for debugging purposes.
>
> Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
> ---
> gdb-xml/riscv-32bit-virtual.xml | 1 +
> gdb-xml/riscv-64bit-virtual.xml | 1 +
> target/riscv/gdbstub.c | 18 ++++++++++++------
> 3 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-xml/riscv-32bit-virtual.xml
> index 905f1c555d..d44b6ca2dc 100644
> --- a/gdb-xml/riscv-32bit-virtual.xml
> +++ b/gdb-xml/riscv-32bit-virtual.xml
> @@ -8,4 +8,5 @@
> <!DOCTYPE feature SYSTEM "gdb-target.dtd">
> <feature name="org.gnu.gdb.riscv.virtual">
> <reg name="priv" bitsize="32"/>
> + <reg name="virt" bitsize="32"/>
> </feature>
> diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-xml/riscv-64bit-virtual.xml
> index 62d86c237b..7c9b63d5b6 100644
> --- a/gdb-xml/riscv-64bit-virtual.xml
> +++ b/gdb-xml/riscv-64bit-virtual.xml
> @@ -8,4 +8,5 @@
> <!DOCTYPE feature SYSTEM "gdb-target.dtd">
> <feature name="org.gnu.gdb.riscv.virtual">
> <reg name="priv" bitsize="64"/>
> + <reg name="virt" bitsize="64"/>
> </feature>
I assume these are mirrored in gdb not a QEMU only extension?
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index c07df972f1..7399003e04 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -206,14 +206,14 @@ static int riscv_gdb_set_csr(CPUState *cs, uint8_t *mem_buf, int n)
>
> static int riscv_gdb_get_virtual(CPUState *cs, GByteArray *buf, int n)
> {
> - if (n == 0) {
> + if (n >= 0 && n <= 1) {
> #ifdef CONFIG_USER_ONLY
> return gdb_get_regl(buf, 0);
> #else
> RISCVCPU *cpu = RISCV_CPU(cs);
> CPURISCVState *env = &cpu->env;
>
> - return gdb_get_regl(buf, env->priv);
> + return gdb_get_regl(buf, n ? env->virt_enabled : env->priv);
The range checking of n and ternary op are a bit magical here. Whats
wrong with a good old fashioned switch/case statement?
> #endif
> }
> return 0;
> @@ -221,14 +221,20 @@ static int riscv_gdb_get_virtual(CPUState *cs, GByteArray *buf, int n)
>
> static int riscv_gdb_set_virtual(CPUState *cs, uint8_t *mem_buf, int n)
> {
> - if (n == 0) {
> + if (n >= 0 && n <= 1) {
> #ifndef CONFIG_USER_ONLY
> RISCVCPU *cpu = RISCV_CPU(cs);
> CPURISCVState *env = &cpu->env;
>
> - env->priv = ldtul_p(mem_buf) & 0x3;
> - if (env->priv == PRV_RESERVED) {
> - env->priv = PRV_S;
> + if (n == 0) {
> + env->priv = ldtul_p(mem_buf) & 0x3;
> + if (env->priv == PRV_RESERVED) {
> + env->priv = PRV_S;
> + }
> + } else {
> + if (env->priv != PRV_M) {
> + env->virt_enabled = ldtul_p(mem_buf) & 0x1;
> + }
ditto here.
> }
> #endif
> return sizeof(target_ulong);
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] riscv/gdb: add virt mode debug interface
2024-11-28 14:21 ` Alex Bennée
@ 2024-11-28 18:01 ` Richard Henderson
2024-11-29 2:11 ` Yanfeng
1 sibling, 0 replies; 12+ messages in thread
From: Richard Henderson @ 2024-11-28 18:01 UTC (permalink / raw)
To: qemu-devel
On 11/28/24 08:21, Alex Bennée wrote:
> Yanfeng Liu <yfliu2008@qq.com> writes:
>
>> This adds `virt` virtual register on debug interface so that users
>> can access current virtualization mode for debugging purposes.
>>
>> Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
>> ---
>> gdb-xml/riscv-32bit-virtual.xml | 1 +
>> gdb-xml/riscv-64bit-virtual.xml | 1 +
>> target/riscv/gdbstub.c | 18 ++++++++++++------
>> 3 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-xml/riscv-32bit-virtual.xml
>> index 905f1c555d..d44b6ca2dc 100644
>> --- a/gdb-xml/riscv-32bit-virtual.xml
>> +++ b/gdb-xml/riscv-32bit-virtual.xml
>> @@ -8,4 +8,5 @@
>> <!DOCTYPE feature SYSTEM "gdb-target.dtd">
>> <feature name="org.gnu.gdb.riscv.virtual">
>> <reg name="priv" bitsize="32"/>
>> + <reg name="virt" bitsize="32"/>
>> </feature>
>> diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-xml/riscv-64bit-virtual.xml
>> index 62d86c237b..7c9b63d5b6 100644
>> --- a/gdb-xml/riscv-64bit-virtual.xml
>> +++ b/gdb-xml/riscv-64bit-virtual.xml
>> @@ -8,4 +8,5 @@
>> <!DOCTYPE feature SYSTEM "gdb-target.dtd">
>> <feature name="org.gnu.gdb.riscv.virtual">
>> <reg name="priv" bitsize="64"/>
>> + <reg name="virt" bitsize="64"/>
>> </feature>
>
> I assume these are mirrored in gdb not a QEMU only extension?
No, we're making this up fresh. This needs to go into a new feature, at minimum.
But buy-in from upstream gdb is always better.
r~
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] riscv/gdb: add virt mode debug interface
2024-11-28 14:21 ` Alex Bennée
2024-11-28 18:01 ` Richard Henderson
@ 2024-11-29 2:11 ` Yanfeng
2024-11-29 9:59 ` Alex Bennée
1 sibling, 1 reply; 12+ messages in thread
From: Yanfeng @ 2024-11-29 2:11 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-riscv, qemu-devel, alistair.francis
On Thu, 2024-11-28 at 14:21 +0000, Alex Bennée wrote:
> Yanfeng Liu <yfliu2008@qq.com> writes:
>
> > This adds `virt` virtual register on debug interface so that users
> > can access current virtualization mode for debugging purposes.
> >
> > Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
> > ---
> > gdb-xml/riscv-32bit-virtual.xml | 1 +
> > gdb-xml/riscv-64bit-virtual.xml | 1 +
> > target/riscv/gdbstub.c | 18 ++++++++++++------
> > 3 files changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-xml/riscv-32bit-
> > virtual.xml
> > index 905f1c555d..d44b6ca2dc 100644
> > --- a/gdb-xml/riscv-32bit-virtual.xml
> > +++ b/gdb-xml/riscv-32bit-virtual.xml
> > @@ -8,4 +8,5 @@
> > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > <feature name="org.gnu.gdb.riscv.virtual">
> > <reg name="priv" bitsize="32"/>
> > + <reg name="virt" bitsize="32"/>
> > </feature>
> > diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-xml/riscv-64bit-
> > virtual.xml
> > index 62d86c237b..7c9b63d5b6 100644
> > --- a/gdb-xml/riscv-64bit-virtual.xml
> > +++ b/gdb-xml/riscv-64bit-virtual.xml
> > @@ -8,4 +8,5 @@
> > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > <feature name="org.gnu.gdb.riscv.virtual">
> > <reg name="priv" bitsize="64"/>
> > + <reg name="virt" bitsize="64"/>
> > </feature>
>
> I assume these are mirrored in gdb not a QEMU only extension?
So far I think it is a QEMU extension and the `gdb-multiarch` doesn't treat is
specially. My tests shows it basically works:
```
(gdb) ir virt
priv 0x3 prv:3 [Machine]
virt 0x0 0
(gdb) set $priv = 2
(gdb) ir virt
priv 0x1 prv:1 [Supervisor]
virt 0x0 0
(gdb) set $virt = 1
(gdb) ir virt
priv 0x1 prv:1 [Supervisor]
virt 0x1 1
(gdb) set $virt = 0
(gdb) ir virt
priv 0x1 prv:1 [Supervisor]
virt 0x0 0
(gdb) set $virt = 1
(gdb) ir virt
priv 0x1 prv:1 [Supervisor]
virt 0x1 1
(gdb) set $priv = 3
(gdb) ir virt
priv 0x3 prv:3 [Machine]
virt 0x0 0
```
As I am rather new to QEMU, please teach how we can add it as a QEMU only
extension.
>
> > diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> > index c07df972f1..7399003e04 100644
> > --- a/target/riscv/gdbstub.c
> > +++ b/target/riscv/gdbstub.c
> > @@ -206,14 +206,14 @@ static int riscv_gdb_set_csr(CPUState *cs, uint8_t
> > *mem_buf, int n)
> >
> > static int riscv_gdb_get_virtual(CPUState *cs, GByteArray *buf, int n)
> > {
> > - if (n == 0) {
> > + if (n >= 0 && n <= 1) {
> > #ifdef CONFIG_USER_ONLY
> > return gdb_get_regl(buf, 0);
> > #else
> > RISCVCPU *cpu = RISCV_CPU(cs);
> > CPURISCVState *env = &cpu->env;
> >
> > - return gdb_get_regl(buf, env->priv);
> > + return gdb_get_regl(buf, n ? env->virt_enabled : env->priv);
>
> The range checking of n and ternary op are a bit magical here. Whats
> wrong with a good old fashioned switch/case statement?
thanks, I can rewrite with switch statement.
>
> > #endif
> > }
> > return 0;
> > @@ -221,14 +221,20 @@ static int riscv_gdb_get_virtual(CPUState *cs,
> > GByteArray *buf, int n)
> >
> > static int riscv_gdb_set_virtual(CPUState *cs, uint8_t *mem_buf, int n)
> > {
> > - if (n == 0) {
> > + if (n >= 0 && n <= 1) {
> > #ifndef CONFIG_USER_ONLY
> > RISCVCPU *cpu = RISCV_CPU(cs);
> > CPURISCVState *env = &cpu->env;
> >
> > - env->priv = ldtul_p(mem_buf) & 0x3;
> > - if (env->priv == PRV_RESERVED) {
> > - env->priv = PRV_S;
> > + if (n == 0) {
> > + env->priv = ldtul_p(mem_buf) & 0x3;
> > + if (env->priv == PRV_RESERVED) {
> > + env->priv = PRV_S;
> > + }
> > + } else {
> > + if (env->priv != PRV_M) {
> > + env->virt_enabled = ldtul_p(mem_buf) & 0x1;
> > + }
>
> ditto here.
>
> > }
> > #endif
> > return sizeof(target_ulong);
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] riscv/gdb: add virt mode debug interface
2024-11-29 2:11 ` Yanfeng
@ 2024-11-29 9:59 ` Alex Bennée
2024-11-30 0:14 ` Yanfeng Liu
0 siblings, 1 reply; 12+ messages in thread
From: Alex Bennée @ 2024-11-29 9:59 UTC (permalink / raw)
To: Yanfeng; +Cc: qemu-riscv, qemu-devel, alistair.francis
Yanfeng <yfliu2008@qq.com> writes:
> On Thu, 2024-11-28 at 14:21 +0000, Alex Bennée wrote:
>> Yanfeng Liu <yfliu2008@qq.com> writes:
>>
>> > This adds `virt` virtual register on debug interface so that users
>> > can access current virtualization mode for debugging purposes.
>> >
>> > Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
>> > ---
>> > gdb-xml/riscv-32bit-virtual.xml | 1 +
>> > gdb-xml/riscv-64bit-virtual.xml | 1 +
>> > target/riscv/gdbstub.c | 18 ++++++++++++------
>> > 3 files changed, 14 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-xml/riscv-32bit-
>> > virtual.xml
>> > index 905f1c555d..d44b6ca2dc 100644
>> > --- a/gdb-xml/riscv-32bit-virtual.xml
>> > +++ b/gdb-xml/riscv-32bit-virtual.xml
>> > @@ -8,4 +8,5 @@
>> > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
>> > <feature name="org.gnu.gdb.riscv.virtual">
>> > <reg name="priv" bitsize="32"/>
>> > + <reg name="virt" bitsize="32"/>
>> > </feature>
>> > diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-xml/riscv-64bit-
>> > virtual.xml
>> > index 62d86c237b..7c9b63d5b6 100644
>> > --- a/gdb-xml/riscv-64bit-virtual.xml
>> > +++ b/gdb-xml/riscv-64bit-virtual.xml
>> > @@ -8,4 +8,5 @@
>> > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
>> > <feature name="org.gnu.gdb.riscv.virtual">
>> > <reg name="priv" bitsize="64"/>
>> > + <reg name="virt" bitsize="64"/>
>> > </feature>
>>
>> I assume these are mirrored in gdb not a QEMU only extension?
>
> So far I think it is a QEMU extension and the `gdb-multiarch` doesn't treat is
> specially. My tests shows it basically works:
>
> ```
> (gdb) ir virt
> priv 0x3 prv:3 [Machine]
> virt 0x0 0
> (gdb) set $priv = 2
> (gdb) ir virt
> priv 0x1 prv:1 [Supervisor]
> virt 0x0 0
> (gdb) set $virt = 1
> (gdb) ir virt
> priv 0x1 prv:1 [Supervisor]
> virt 0x1 1
> (gdb) set $virt = 0
> (gdb) ir virt
> priv 0x1 prv:1 [Supervisor]
> virt 0x0 0
> (gdb) set $virt = 1
> (gdb) ir virt
> priv 0x1 prv:1 [Supervisor]
> virt 0x1 1
> (gdb) set $priv = 3
> (gdb) ir virt
> priv 0x3 prv:3 [Machine]
> virt 0x0 0
> ```
A gdbstub test case would be useful for this although I don't know if
the RiscV check-tcg tests switch mode at all.
>
> As I am rather new to QEMU, please teach how we can add it as a QEMU only
> extension.
You don't need to extend the XML from GDB, you can build a specific one
for QEMU extensions. For example:
gdb_feature_builder_init(¶m.builder,
&cpu->dyn_sysreg_feature.desc,
"org.qemu.gdb.arm.sys.regs",
"system-registers.xml",
base_reg);
This exports all the system registers QEMU knows about and GDB can
access generically. Note the id is org.qemu..., indicating its our
schema not gdbs.
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] riscv/gdb: add virt mode debug interface
2024-11-29 9:59 ` Alex Bennée
@ 2024-11-30 0:14 ` Yanfeng Liu
2024-12-04 16:03 ` Mario Fleischmann
0 siblings, 1 reply; 12+ messages in thread
From: Yanfeng Liu @ 2024-11-30 0:14 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-riscv, qemu-devel, alistair.francis
On Fri, 2024-11-29 at 09:59 +0000, Alex Bennée wrote:
> Yanfeng <yfliu2008@qq.com> writes:
>
> > On Thu, 2024-11-28 at 14:21 +0000, Alex Bennée wrote:
> > > Yanfeng Liu <yfliu2008@qq.com> writes:
> > >
> > > > This adds `virt` virtual register on debug interface so that users
> > > > can access current virtualization mode for debugging purposes.
> > > >
> > > > Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
> > > > ---
> > > > gdb-xml/riscv-32bit-virtual.xml | 1 +
> > > > gdb-xml/riscv-64bit-virtual.xml | 1 +
> > > > target/riscv/gdbstub.c | 18 ++++++++++++------
> > > > 3 files changed, 14 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-xml/riscv-32bit-
> > > > virtual.xml
> > > > index 905f1c555d..d44b6ca2dc 100644
> > > > --- a/gdb-xml/riscv-32bit-virtual.xml
> > > > +++ b/gdb-xml/riscv-32bit-virtual.xml
> > > > @@ -8,4 +8,5 @@
> > > > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > > > <feature name="org.gnu.gdb.riscv.virtual">
> > > > <reg name="priv" bitsize="32"/>
> > > > + <reg name="virt" bitsize="32"/>
> > > > </feature>
> > > > diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-xml/riscv-64bit-
> > > > virtual.xml
> > > > index 62d86c237b..7c9b63d5b6 100644
> > > > --- a/gdb-xml/riscv-64bit-virtual.xml
> > > > +++ b/gdb-xml/riscv-64bit-virtual.xml
> > > > @@ -8,4 +8,5 @@
> > > > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > > > <feature name="org.gnu.gdb.riscv.virtual">
> > > > <reg name="priv" bitsize="64"/>
> > > > + <reg name="virt" bitsize="64"/>
> > > > </feature>
> > >
> > > I assume these are mirrored in gdb not a QEMU only extension?
> >
> > So far I think it is a QEMU extension and the `gdb-multiarch` doesn't treat
> > is
> > specially. My tests shows it basically works:
> >
> > ```
> > (gdb) ir virt
> > priv 0x3 prv:3 [Machine]
> > virt 0x0 0
> > (gdb) set $priv = 2
> > (gdb) ir virt
> > priv 0x1 prv:1 [Supervisor]
> > virt 0x0 0
> > (gdb) set $virt = 1
> > (gdb) ir virt
> > priv 0x1 prv:1 [Supervisor]
> > virt 0x1 1
> > (gdb) set $virt = 0
> > (gdb) ir virt
> > priv 0x1 prv:1 [Supervisor]
> > virt 0x0 0
> > (gdb) set $virt = 1
> > (gdb) ir virt
> > priv 0x1 prv:1 [Supervisor]
> > virt 0x1 1
> > (gdb) set $priv = 3
> > (gdb) ir virt
> > priv 0x3 prv:3 [Machine]
> > virt 0x0 0
> > ```
>
> A gdbstub test case would be useful for this although I don't know if
> the RiscV check-tcg tests switch mode at all.
>
> >
> > As I am rather new to QEMU, please teach how we can add it as a QEMU only
> > extension.
>
> You don't need to extend the XML from GDB, you can build a specific one
> for QEMU extensions. For example:
>
> gdb_feature_builder_init(¶m.builder,
> &cpu->dyn_sysreg_feature.desc,
> "org.qemu.gdb.arm.sys.regs",
> "system-registers.xml",
> base_reg);
>
> This exports all the system registers QEMU knows about and GDB can
> access generically. Note the id is org.qemu..., indicating its our
> schema not gdbs.
Thanks for teaching, I need time to digest. I guess more feature builder APIs
are needed (like append_reg) and the getter/setter callbacks might be at a
different place.
BTW, compared to adding virtual register `virt`, how do you think if we share
the V bit as part of existing `priv` register?
Or maybe we shall talk to GDB community to get their opinions? If they agree to
add a few words about V bit here
https://sourceware.org/gdb/current/onlinedocs/gdb.html/RISC_002dV-Features.html,
then it saves us a lot.
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] riscv/gdb: add virt mode debug interface
2024-11-30 0:14 ` Yanfeng Liu
@ 2024-12-04 16:03 ` Mario Fleischmann
2024-12-05 1:29 ` Yanfeng Liu
0 siblings, 1 reply; 12+ messages in thread
From: Mario Fleischmann @ 2024-12-04 16:03 UTC (permalink / raw)
To: Yanfeng Liu, Alex Bennée; +Cc: qemu-riscv, qemu-devel, alistair.francis
Hi everyone,
I'd like to chime in here because we are sitting on a similar patch
which I wanted to send to the mailing list as soon as riscv-debug-spec
v1.0.0 becomes ratified.
For hypervisor support, `(qemu) info registers` isn't enough. We need to
have both read and write access to the V-bit.
On 04.12.2024 14:43, Yanfeng Liu wrote:
> On Fri, 2024-11-29 at 09:59 +0000, Alex Bennée wrote:
>> Yanfeng <yfliu2008@qq.com> writes:
>>
>>> On Thu, 2024-11-28 at 14:21 +0000, Alex Bennée wrote:
>>>> Yanfeng Liu <yfliu2008@qq.com> writes:
>>>>
>>>>> This adds `virt` virtual register on debug interface so that users
>>>>> can access current virtualization mode for debugging purposes.
>>>>>
>>>>> Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
>>>>> ---
>>>>> gdb-xml/riscv-32bit-virtual.xml | 1 +
>>>>> gdb-xml/riscv-64bit-virtual.xml | 1 +
>>>>> target/riscv/gdbstub.c | 18 ++++++++++++------
>>>>> 3 files changed, 14 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-xml/riscv-32bit-
>>>>> virtual.xml
>>>>> index 905f1c555d..d44b6ca2dc 100644
>>>>> --- a/gdb-xml/riscv-32bit-virtual.xml
>>>>> +++ b/gdb-xml/riscv-32bit-virtual.xml
>>>>> @@ -8,4 +8,5 @@
>>>>> <!DOCTYPE feature SYSTEM "gdb-target.dtd">
>>>>> <feature name="org.gnu.gdb.riscv.virtual">
>>>>> <reg name="priv" bitsize="32"/>
>>>>> + <reg name="virt" bitsize="32"/>
>>>>> </feature>
>>>>> diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-xml/riscv-64bit-
>>>>> virtual.xml
>>>>> index 62d86c237b..7c9b63d5b6 100644
>>>>> --- a/gdb-xml/riscv-64bit-virtual.xml
>>>>> +++ b/gdb-xml/riscv-64bit-virtual.xml
>>>>> @@ -8,4 +8,5 @@
>>>>> <!DOCTYPE feature SYSTEM "gdb-target.dtd">
>>>>> <feature name="org.gnu.gdb.riscv.virtual">
>>>>> <reg name="priv" bitsize="64"/>
>>>>> + <reg name="virt" bitsize="64"/>
>>>>> </feature>
>>>>
>>>> I assume these are mirrored in gdb not a QEMU only extension?
>>>
>>> So far I think it is a QEMU extension and the `gdb-multiarch` doesn't treat
>>> is
>>> specially. My tests shows it basically works:
>>>
>>> ```
>>> (gdb) ir virt
>>> priv 0x3 prv:3 [Machine]
>>> virt 0x0 0
>>> (gdb) set $priv = 2
>>> (gdb) ir virt
>>> priv 0x1 prv:1 [Supervisor]
>>> virt 0x0 0
>>> (gdb) set $virt = 1
>>> (gdb) ir virt
>>> priv 0x1 prv:1 [Supervisor]
>>> virt 0x1 1
>>> (gdb) set $virt = 0
>>> (gdb) ir virt
>>> priv 0x1 prv:1 [Supervisor]
>>> virt 0x0 0
>>> (gdb) set $virt = 1
>>> (gdb) ir virt
>>> priv 0x1 prv:1 [Supervisor]
>>> virt 0x1 1
>>> (gdb) set $priv = 3
>>> (gdb) ir virt
>>> priv 0x3 prv:3 [Machine]
>>> virt 0x0 0
>>> ```
>>
>> A gdbstub test case would be useful for this although I don't know if
>> the RiscV check-tcg tests switch mode at all.
>>
>>>
>>> As I am rather new to QEMU, please teach how we can add it as a QEMU only
>>> extension.
>>
>> You don't need to extend the XML from GDB, you can build a specific one
>> for QEMU extensions. For example:
>>
>> gdb_feature_builder_init(¶m.builder,
>> &cpu->dyn_sysreg_feature.desc,
>> "org.qemu.gdb.arm.sys.regs",
>> "system-registers.xml",
>> base_reg);
>>
>> This exports all the system registers QEMU knows about and GDB can
>> access generically. Note the id is org.qemu..., indicating its our
>> schema not gdbs.
> Thanks for teaching, I need time to digest. I guess more feature builder APIs
> are needed (like append_reg) and the getter/setter callbacks might be at a
> different place.
>
> BTW, compared to adding virtual register `virt`, how do you think if we share
> the V bit as part of existing `priv` register?
IMHO this is a very good idea since the latest release candidate of
riscv-debug-spec also includes the V bit in priv:2.
> Or maybe we shall talk to GDB community to get their opinions? If they agree to
> add a few words about V bit here
> https://sourceware.org/gdb/current/onlinedocs/gdb.html/RISC_002dV-Features.html,
> then it saves us a lot.
Except being currently not supported by GDB
(gdb) info register $priv
priv 0x5 prv:5 [INVALID]
are there any reasons from QEMU's side that would speak against
including V in priv?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] riscv/gdb: add virt mode debug interface
2024-12-04 16:03 ` Mario Fleischmann
@ 2024-12-05 1:29 ` Yanfeng Liu
2024-12-05 8:10 ` Alex Bennée
0 siblings, 1 reply; 12+ messages in thread
From: Yanfeng Liu @ 2024-12-05 1:29 UTC (permalink / raw)
To: Mario Fleischmann, Alex Bennée
Cc: qemu-riscv, qemu-devel, alistair.francis
On Wed, 2024-12-04 at 17:03 +0100, Mario Fleischmann wrote:
> Hi everyone,
>
> I'd like to chime in here because we are sitting on a similar patch
> which I wanted to send to the mailing list as soon as riscv-debug-spec
> v1.0.0 becomes ratified.
>
> For hypervisor support, `(qemu) info registers` isn't enough. We need to
> have both read and write access to the V-bit.
>
> On 04.12.2024 14:43, Yanfeng Liu wrote:
> > On Fri, 2024-11-29 at 09:59 +0000, Alex Bennée wrote:
> > > Yanfeng <yfliu2008@qq.com> writes:
> > >
> > > > On Thu, 2024-11-28 at 14:21 +0000, Alex Bennée wrote:
> > > > > Yanfeng Liu <yfliu2008@qq.com> writes:
> > > > >
> > > > > > This adds `virt` virtual register on debug interface so that users
> > > > > > can access current virtualization mode for debugging purposes.
> > > > > >
> > > > > > Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
> > > > > > ---
> > > > > > gdb-xml/riscv-32bit-virtual.xml | 1 +
> > > > > > gdb-xml/riscv-64bit-virtual.xml | 1 +
> > > > > > target/riscv/gdbstub.c | 18 ++++++++++++------
> > > > > > 3 files changed, 14 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-xml/riscv-32bit-
> > > > > > virtual.xml
> > > > > > index 905f1c555d..d44b6ca2dc 100644
> > > > > > --- a/gdb-xml/riscv-32bit-virtual.xml
> > > > > > +++ b/gdb-xml/riscv-32bit-virtual.xml
> > > > > > @@ -8,4 +8,5 @@
> > > > > > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > > > > > <feature name="org.gnu.gdb.riscv.virtual">
> > > > > > <reg name="priv" bitsize="32"/>
> > > > > > + <reg name="virt" bitsize="32"/>
> > > > > > </feature>
> > > > > > diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-xml/riscv-64bit-
> > > > > > virtual.xml
> > > > > > index 62d86c237b..7c9b63d5b6 100644
> > > > > > --- a/gdb-xml/riscv-64bit-virtual.xml
> > > > > > +++ b/gdb-xml/riscv-64bit-virtual.xml
> > > > > > @@ -8,4 +8,5 @@
> > > > > > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > > > > > <feature name="org.gnu.gdb.riscv.virtual">
> > > > > > <reg name="priv" bitsize="64"/>
> > > > > > + <reg name="virt" bitsize="64"/>
> > > > > > </feature>
> > > > >
> > > > > I assume these are mirrored in gdb not a QEMU only extension?
> > > >
> > > > So far I think it is a QEMU extension and the `gdb-multiarch` doesn't
> > > > treat
> > > > is
> > > > specially. My tests shows it basically works:
> > > >
> > > > ```
> > > > (gdb) ir virt
> > > > priv 0x3 prv:3 [Machine]
> > > > virt 0x0 0
> > > > (gdb) set $priv = 2
> > > > (gdb) ir virt
> > > > priv 0x1 prv:1 [Supervisor]
> > > > virt 0x0 0
> > > > (gdb) set $virt = 1
> > > > (gdb) ir virt
> > > > priv 0x1 prv:1 [Supervisor]
> > > > virt 0x1 1
> > > > (gdb) set $virt = 0
> > > > (gdb) ir virt
> > > > priv 0x1 prv:1 [Supervisor]
> > > > virt 0x0 0
> > > > (gdb) set $virt = 1
> > > > (gdb) ir virt
> > > > priv 0x1 prv:1 [Supervisor]
> > > > virt 0x1 1
> > > > (gdb) set $priv = 3
> > > > (gdb) ir virt
> > > > priv 0x3 prv:3 [Machine]
> > > > virt 0x0 0
> > > > ```
> > >
> > > A gdbstub test case would be useful for this although I don't know if
> > > the RiscV check-tcg tests switch mode at all.
> > >
> > > >
> > > > As I am rather new to QEMU, please teach how we can add it as a QEMU
> > > > only
> > > > extension.
> > >
> > > You don't need to extend the XML from GDB, you can build a specific one
> > > for QEMU extensions. For example:
> > >
> > > gdb_feature_builder_init(¶m.builder,
> > > &cpu->dyn_sysreg_feature.desc,
> > > "org.qemu.gdb.arm.sys.regs",
> > > "system-registers.xml",
> > > base_reg);
> > >
> > > This exports all the system registers QEMU knows about and GDB can
> > > access generically. Note the id is org.qemu..., indicating its our
> > > schema not gdbs.
> > Thanks for teaching, I need time to digest. I guess more feature builder
> > APIs
> > are needed (like append_reg) and the getter/setter callbacks might be at a
> > different place.
> >
> > BTW, compared to adding virtual register `virt`, how do you think if we
> > share
> > the V bit as part of existing `priv` register?
>
> IMHO this is a very good idea since the latest release candidate of
> riscv-debug-spec also includes the V bit in priv:2.
>
Thanks for this information, I noticed the bit(2) of `priv` register is for the
V bit as per section 4.10.1.
> > Or maybe we shall talk to GDB community to get their opinions? If they agree
> > to
> > add a few words about V bit here
> > https://sourceware.org/gdb/current/onlinedocs/gdb.html/RISC_002dV-Features.html
> > ,
> > then it saves us a lot.
>
> Except being currently not supported by GDB
>
> (gdb) info register $priv
> priv 0x5 prv:5 [INVALID]
>
> are there any reasons from QEMU's side that would speak against
> including V in priv?
>
My v1 patch used `bit(8)` to avoid seeing the `[INVALID]` thing at GDB side,
though that is due to GDB isn't in line with its own manual (i.e. use the two
lowest bits only).
Without a doc or specification. we felt people may not know `bit(8)` in v1 patch
was for the V bit, so I drafted patch v2 as Alistair suggested. However, as Alex
pointed out, directly adding `virt` register in "org.gnu.gdb.riscv.virtual" XML
is improper. I also wanted to raise this in GDB side but my application to join
the mail list is still pending.
Alex and Alistair, now I am wondering if we can follow the RiscV debug
specification to use `bit(2)` of `priv` virtual register? My test shows except
for the `[INVALID]` label, both set/get access seems working.
Regards,
yf
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] riscv/gdb: add virt mode debug interface
2024-12-05 1:29 ` Yanfeng Liu
@ 2024-12-05 8:10 ` Alex Bennée
2024-12-05 9:15 ` Yanfeng Liu
0 siblings, 1 reply; 12+ messages in thread
From: Alex Bennée @ 2024-12-05 8:10 UTC (permalink / raw)
To: Yanfeng Liu; +Cc: Mario Fleischmann, qemu-riscv, qemu-devel, alistair.francis
Yanfeng Liu <yfliu2008@qq.com> writes:
> On Wed, 2024-12-04 at 17:03 +0100, Mario Fleischmann wrote:
>> Hi everyone,
>>
>> I'd like to chime in here because we are sitting on a similar patch
>> which I wanted to send to the mailing list as soon as riscv-debug-spec
>> v1.0.0 becomes ratified.
>>
>> For hypervisor support, `(qemu) info registers` isn't enough. We need to
>> have both read and write access to the V-bit.
>>
>> On 04.12.2024 14:43, Yanfeng Liu wrote:
>> > On Fri, 2024-11-29 at 09:59 +0000, Alex Bennée wrote:
>> > > Yanfeng <yfliu2008@qq.com> writes:
>> > >
>> > > > On Thu, 2024-11-28 at 14:21 +0000, Alex Bennée wrote:
>> > > > > Yanfeng Liu <yfliu2008@qq.com> writes:
>> > > > >
>> > > > > > This adds `virt` virtual register on debug interface so that users
>> > > > > > can access current virtualization mode for debugging purposes.
>> > > > > >
>> > > > > > Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
>> > > > > > ---
>> > > > > > gdb-xml/riscv-32bit-virtual.xml | 1 +
>> > > > > > gdb-xml/riscv-64bit-virtual.xml | 1 +
>> > > > > > target/riscv/gdbstub.c | 18 ++++++++++++------
>> > > > > > 3 files changed, 14 insertions(+), 6 deletions(-)
>> > > > > >
>> > > > > > diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-xml/riscv-32bit-
>> > > > > > virtual.xml
>> > > > > > index 905f1c555d..d44b6ca2dc 100644
>> > > > > > --- a/gdb-xml/riscv-32bit-virtual.xml
>> > > > > > +++ b/gdb-xml/riscv-32bit-virtual.xml
>> > > > > > @@ -8,4 +8,5 @@
>> > > > > > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
>> > > > > > <feature name="org.gnu.gdb.riscv.virtual">
>> > > > > > <reg name="priv" bitsize="32"/>
>> > > > > > + <reg name="virt" bitsize="32"/>
>> > > > > > </feature>
>> > > > > > diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-xml/riscv-64bit-
>> > > > > > virtual.xml
>> > > > > > index 62d86c237b..7c9b63d5b6 100644
>> > > > > > --- a/gdb-xml/riscv-64bit-virtual.xml
>> > > > > > +++ b/gdb-xml/riscv-64bit-virtual.xml
>> > > > > > @@ -8,4 +8,5 @@
>> > > > > > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
>> > > > > > <feature name="org.gnu.gdb.riscv.virtual">
>> > > > > > <reg name="priv" bitsize="64"/>
>> > > > > > + <reg name="virt" bitsize="64"/>
>> > > > > > </feature>
>> > > > >
>> > > > > I assume these are mirrored in gdb not a QEMU only extension?
>> > > >
>> > > > So far I think it is a QEMU extension and the `gdb-multiarch` doesn't
>> > > > treat
>> > > > is
>> > > > specially. My tests shows it basically works:
>> > > >
>> > > > ```
>> > > > (gdb) ir virt
>> > > > priv 0x3 prv:3 [Machine]
>> > > > virt 0x0 0
>> > > > (gdb) set $priv = 2
>> > > > (gdb) ir virt
>> > > > priv 0x1 prv:1 [Supervisor]
>> > > > virt 0x0 0
>> > > > (gdb) set $virt = 1
>> > > > (gdb) ir virt
>> > > > priv 0x1 prv:1 [Supervisor]
>> > > > virt 0x1 1
>> > > > (gdb) set $virt = 0
>> > > > (gdb) ir virt
>> > > > priv 0x1 prv:1 [Supervisor]
>> > > > virt 0x0 0
>> > > > (gdb) set $virt = 1
>> > > > (gdb) ir virt
>> > > > priv 0x1 prv:1 [Supervisor]
>> > > > virt 0x1 1
>> > > > (gdb) set $priv = 3
>> > > > (gdb) ir virt
>> > > > priv 0x3 prv:3 [Machine]
>> > > > virt 0x0 0
>> > > > ```
>> > >
>> > > A gdbstub test case would be useful for this although I don't know if
>> > > the RiscV check-tcg tests switch mode at all.
>> > >
>> > > >
>> > > > As I am rather new to QEMU, please teach how we can add it as a QEMU
>> > > > only
>> > > > extension.
>> > >
>> > > You don't need to extend the XML from GDB, you can build a specific one
>> > > for QEMU extensions. For example:
>> > >
>> > > gdb_feature_builder_init(¶m.builder,
>> > > &cpu->dyn_sysreg_feature.desc,
>> > > "org.qemu.gdb.arm.sys.regs",
>> > > "system-registers.xml",
>> > > base_reg);
>> > >
>> > > This exports all the system registers QEMU knows about and GDB can
>> > > access generically. Note the id is org.qemu..., indicating its our
>> > > schema not gdbs.
>> > Thanks for teaching, I need time to digest. I guess more feature builder
>> > APIs
>> > are needed (like append_reg) and the getter/setter callbacks might be at a
>> > different place.
>> >
>> > BTW, compared to adding virtual register `virt`, how do you think if we
>> > share
>> > the V bit as part of existing `priv` register?
>>
>> IMHO this is a very good idea since the latest release candidate of
>> riscv-debug-spec also includes the V bit in priv:2.
>>
>
> Thanks for this information, I noticed the bit(2) of `priv` register is for the
> V bit as per section 4.10.1.
>
>> > Or maybe we shall talk to GDB community to get their opinions? If they agree
>> > to
>> > add a few words about V bit here
>> > https://sourceware.org/gdb/current/onlinedocs/gdb.html/RISC_002dV-Features.html
>> > ,
>> > then it saves us a lot.
>>
>> Except being currently not supported by GDB
>>
>> (gdb) info register $priv
>> priv 0x5 prv:5 [INVALID]
>>
>> are there any reasons from QEMU's side that would speak against
>> including V in priv?
>>
>
> My v1 patch used `bit(8)` to avoid seeing the `[INVALID]` thing at GDB side,
> though that is due to GDB isn't in line with its own manual (i.e. use the two
> lowest bits only).
>
> Without a doc or specification. we felt people may not know `bit(8)` in v1 patch
> was for the V bit, so I drafted patch v2 as Alistair suggested. However, as Alex
> pointed out, directly adding `virt` register in "org.gnu.gdb.riscv.virtual" XML
> is improper. I also wanted to raise this in GDB side but my application to join
> the mail list is still pending.
>
> Alex and Alistair, now I am wondering if we can follow the RiscV debug
> specification to use `bit(2)` of `priv` virtual register? My test shows except
> for the `[INVALID]` label, both set/get access seems working.
I guess the INVALID just means gdb needs teaching about the format of
the register.
>
>
> Regards,
> yf
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] riscv/gdb: add virt mode debug interface
2024-12-05 8:10 ` Alex Bennée
@ 2024-12-05 9:15 ` Yanfeng Liu
2024-12-16 5:33 ` Alistair Francis
0 siblings, 1 reply; 12+ messages in thread
From: Yanfeng Liu @ 2024-12-05 9:15 UTC (permalink / raw)
To: Alex Bennée
Cc: Mario Fleischmann, qemu-riscv, qemu-devel, alistair.francis
On Thu, 2024-12-05 at 08:10 +0000, Alex Bennée wrote:
> Yanfeng Liu <yfliu2008@qq.com> writes:
>
> > On Wed, 2024-12-04 at 17:03 +0100, Mario Fleischmann wrote:
> > > Hi everyone,
> > >
> > > I'd like to chime in here because we are sitting on a similar patch
> > > which I wanted to send to the mailing list as soon as riscv-debug-spec
> > > v1.0.0 becomes ratified.
> > >
> > > For hypervisor support, `(qemu) info registers` isn't enough. We need to
> > > have both read and write access to the V-bit.
> > >
> > > On 04.12.2024 14:43, Yanfeng Liu wrote:
> > > > On Fri, 2024-11-29 at 09:59 +0000, Alex Bennée wrote:
> > > > > Yanfeng <yfliu2008@qq.com> writes:
> > > > >
> > > > > > On Thu, 2024-11-28 at 14:21 +0000, Alex Bennée wrote:
> > > > > > > Yanfeng Liu <yfliu2008@qq.com> writes:
> > > > > > >
> > > > > > > > This adds `virt` virtual register on debug interface so that
> > > > > > > > users
> > > > > > > > can access current virtualization mode for debugging purposes.
> > > > > > > >
> > > > > > > > Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
> > > > > > > > ---
> > > > > > > > gdb-xml/riscv-32bit-virtual.xml | 1 +
> > > > > > > > gdb-xml/riscv-64bit-virtual.xml | 1 +
> > > > > > > > target/riscv/gdbstub.c | 18 ++++++++++++------
> > > > > > > > 3 files changed, 14 insertions(+), 6 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-xml/riscv-
> > > > > > > > 32bit-
> > > > > > > > virtual.xml
> > > > > > > > index 905f1c555d..d44b6ca2dc 100644
> > > > > > > > --- a/gdb-xml/riscv-32bit-virtual.xml
> > > > > > > > +++ b/gdb-xml/riscv-32bit-virtual.xml
> > > > > > > > @@ -8,4 +8,5 @@
> > > > > > > > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > > > > > > > <feature name="org.gnu.gdb.riscv.virtual">
> > > > > > > > <reg name="priv" bitsize="32"/>
> > > > > > > > + <reg name="virt" bitsize="32"/>
> > > > > > > > </feature>
> > > > > > > > diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-xml/riscv-
> > > > > > > > 64bit-
> > > > > > > > virtual.xml
> > > > > > > > index 62d86c237b..7c9b63d5b6 100644
> > > > > > > > --- a/gdb-xml/riscv-64bit-virtual.xml
> > > > > > > > +++ b/gdb-xml/riscv-64bit-virtual.xml
> > > > > > > > @@ -8,4 +8,5 @@
> > > > > > > > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > > > > > > > <feature name="org.gnu.gdb.riscv.virtual">
> > > > > > > > <reg name="priv" bitsize="64"/>
> > > > > > > > + <reg name="virt" bitsize="64"/>
> > > > > > > > </feature>
> > > > > > >
> > > > > > > I assume these are mirrored in gdb not a QEMU only extension?
> > > > > >
> > > > > > So far I think it is a QEMU extension and the `gdb-multiarch`
> > > > > > doesn't
> > > > > > treat
> > > > > > is
> > > > > > specially. My tests shows it basically works:
> > > > > >
> > > > > > ```
> > > > > > (gdb) ir virt
> > > > > > priv 0x3 prv:3 [Machine]
> > > > > > virt 0x0 0
> > > > > > (gdb) set $priv = 2
> > > > > > (gdb) ir virt
> > > > > > priv 0x1 prv:1 [Supervisor]
> > > > > > virt 0x0 0
> > > > > > (gdb) set $virt = 1
> > > > > > (gdb) ir virt
> > > > > > priv 0x1 prv:1 [Supervisor]
> > > > > > virt 0x1 1
> > > > > > (gdb) set $virt = 0
> > > > > > (gdb) ir virt
> > > > > > priv 0x1 prv:1 [Supervisor]
> > > > > > virt 0x0 0
> > > > > > (gdb) set $virt = 1
> > > > > > (gdb) ir virt
> > > > > > priv 0x1 prv:1 [Supervisor]
> > > > > > virt 0x1 1
> > > > > > (gdb) set $priv = 3
> > > > > > (gdb) ir virt
> > > > > > priv 0x3 prv:3 [Machine]
> > > > > > virt 0x0 0
> > > > > > ```
> > > > >
> > > > > A gdbstub test case would be useful for this although I don't know if
> > > > > the RiscV check-tcg tests switch mode at all.
> > > > >
> > > > > >
> > > > > > As I am rather new to QEMU, please teach how we can add it as a QEMU
> > > > > > only
> > > > > > extension.
> > > > >
> > > > > You don't need to extend the XML from GDB, you can build a specific
> > > > > one
> > > > > for QEMU extensions. For example:
> > > > >
> > > > > gdb_feature_builder_init(¶m.builder,
> > > > > &cpu->dyn_sysreg_feature.desc,
> > > > > "org.qemu.gdb.arm.sys.regs",
> > > > > "system-registers.xml",
> > > > > base_reg);
> > > > >
> > > > > This exports all the system registers QEMU knows about and GDB can
> > > > > access generically. Note the id is org.qemu..., indicating its our
> > > > > schema not gdbs.
> > > > Thanks for teaching, I need time to digest. I guess more feature builder
> > > > APIs
> > > > are needed (like append_reg) and the getter/setter callbacks might be at
> > > > a
> > > > different place.
> > > >
> > > > BTW, compared to adding virtual register `virt`, how do you think if we
> > > > share
> > > > the V bit as part of existing `priv` register?
> > >
> > > IMHO this is a very good idea since the latest release candidate of
> > > riscv-debug-spec also includes the V bit in priv:2.
> > >
> >
> > Thanks for this information, I noticed the bit(2) of `priv` register is for
> > the
> > V bit as per section 4.10.1.
> >
> > > > Or maybe we shall talk to GDB community to get their opinions? If they
> > > > agree
> > > > to
> > > > add a few words about V bit here
> > > > https://sourceware.org/gdb/current/onlinedocs/gdb.html/RISC_002dV-Features.html
> > > > ,
> > > > then it saves us a lot.
> > >
> > > Except being currently not supported by GDB
> > >
> > > (gdb) info register $priv
> > > priv 0x5 prv:5 [INVALID]
> > >
> > > are there any reasons from QEMU's side that would speak against
> > > including V in priv?
> > >
> >
> > My v1 patch used `bit(8)` to avoid seeing the `[INVALID]` thing at GDB side,
> > though that is due to GDB isn't in line with its own manual (i.e. use the
> > two
> > lowest bits only).
> >
> > Without a doc or specification. we felt people may not know `bit(8)` in v1
> > patch
> > was for the V bit, so I drafted patch v2 as Alistair suggested. However, as
> > Alex
> > pointed out, directly adding `virt` register in "org.gnu.gdb.riscv.virtual"
> > XML
> > is improper. I also wanted to raise this in GDB side but my application to
> > join
> > the mail list is still pending.
> >
> > Alex and Alistair, now I am wondering if we can follow the RiscV debug
> > specification to use `bit(2)` of `priv` virtual register? My test shows
> > except
> > for the `[INVALID]` label, both set/get access seems working.
>
> I guess the INVALID just means gdb needs teaching about the format of
> the register.
Yes, GDB currently uses mask `0xff`(instead of `0x3`) to get the mode value when
adding the string label, this violates its own manual:
1303 else if (regnum == RISCV_PRIV_REGNUM)
1304 {
1305 LONGEST d;
1306 uint8_t priv;
1307
1308 d = value_as_long (val);
1309 priv = d & 0xff;
1310
1311 if (priv < 4)
1312 {
1313 static const char * const sprv[] =
1314 {
1315 "User/Application",
1316 "Supervisor",
1317 "Hypervisor",
1318 "Machine"
1319 };
1320 gdb_printf (file, "\tprv:%d [%s]",
1321 priv, sprv[priv]);
1322 }
1323 else
1324 gdb_printf (file, "\tprv:%d [INVALID]", priv);
1325 }
I am wondering if we can go ahead to follow RiscV debug specification and sync
with GDB later?
>
> >
> >
> > Regards,
> > yf
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] riscv/gdb: add virt mode debug interface
2024-12-05 9:15 ` Yanfeng Liu
@ 2024-12-16 5:33 ` Alistair Francis
2024-12-18 0:49 ` Yanfeng Liu
0 siblings, 1 reply; 12+ messages in thread
From: Alistair Francis @ 2024-12-16 5:33 UTC (permalink / raw)
To: Yanfeng Liu
Cc: Alex Bennée, Mario Fleischmann, qemu-riscv, qemu-devel,
alistair.francis
On Thu, Dec 5, 2024 at 7:17 PM Yanfeng Liu <yfliu2008@qq.com> wrote:
>
> On Thu, 2024-12-05 at 08:10 +0000, Alex Bennée wrote:
> > Yanfeng Liu <yfliu2008@qq.com> writes:
> >
> > > On Wed, 2024-12-04 at 17:03 +0100, Mario Fleischmann wrote:
> > > > Hi everyone,
> > > >
> > > > I'd like to chime in here because we are sitting on a similar patch
> > > > which I wanted to send to the mailing list as soon as riscv-debug-spec
> > > > v1.0.0 becomes ratified.
> > > >
> > > > For hypervisor support, `(qemu) info registers` isn't enough. We need to
> > > > have both read and write access to the V-bit.
> > > >
> > > > On 04.12.2024 14:43, Yanfeng Liu wrote:
> > > > > On Fri, 2024-11-29 at 09:59 +0000, Alex Bennée wrote:
> > > > > > Yanfeng <yfliu2008@qq.com> writes:
> > > > > >
> > > > > > > On Thu, 2024-11-28 at 14:21 +0000, Alex Bennée wrote:
> > > > > > > > Yanfeng Liu <yfliu2008@qq.com> writes:
> > > > > > > >
> > > > > > > > > This adds `virt` virtual register on debug interface so that
> > > > > > > > > users
> > > > > > > > > can access current virtualization mode for debugging purposes.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
> > > > > > > > > ---
> > > > > > > > > gdb-xml/riscv-32bit-virtual.xml | 1 +
> > > > > > > > > gdb-xml/riscv-64bit-virtual.xml | 1 +
> > > > > > > > > target/riscv/gdbstub.c | 18 ++++++++++++------
> > > > > > > > > 3 files changed, 14 insertions(+), 6 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-xml/riscv-
> > > > > > > > > 32bit-
> > > > > > > > > virtual.xml
> > > > > > > > > index 905f1c555d..d44b6ca2dc 100644
> > > > > > > > > --- a/gdb-xml/riscv-32bit-virtual.xml
> > > > > > > > > +++ b/gdb-xml/riscv-32bit-virtual.xml
> > > > > > > > > @@ -8,4 +8,5 @@
> > > > > > > > > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > > > > > > > > <feature name="org.gnu.gdb.riscv.virtual">
> > > > > > > > > <reg name="priv" bitsize="32"/>
> > > > > > > > > + <reg name="virt" bitsize="32"/>
> > > > > > > > > </feature>
> > > > > > > > > diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-xml/riscv-
> > > > > > > > > 64bit-
> > > > > > > > > virtual.xml
> > > > > > > > > index 62d86c237b..7c9b63d5b6 100644
> > > > > > > > > --- a/gdb-xml/riscv-64bit-virtual.xml
> > > > > > > > > +++ b/gdb-xml/riscv-64bit-virtual.xml
> > > > > > > > > @@ -8,4 +8,5 @@
> > > > > > > > > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > > > > > > > > <feature name="org.gnu.gdb.riscv.virtual">
> > > > > > > > > <reg name="priv" bitsize="64"/>
> > > > > > > > > + <reg name="virt" bitsize="64"/>
> > > > > > > > > </feature>
> > > > > > > >
> > > > > > > > I assume these are mirrored in gdb not a QEMU only extension?
> > > > > > >
> > > > > > > So far I think it is a QEMU extension and the `gdb-multiarch`
> > > > > > > doesn't
> > > > > > > treat
> > > > > > > is
> > > > > > > specially. My tests shows it basically works:
> > > > > > >
> > > > > > > ```
> > > > > > > (gdb) ir virt
> > > > > > > priv 0x3 prv:3 [Machine]
> > > > > > > virt 0x0 0
> > > > > > > (gdb) set $priv = 2
> > > > > > > (gdb) ir virt
> > > > > > > priv 0x1 prv:1 [Supervisor]
> > > > > > > virt 0x0 0
> > > > > > > (gdb) set $virt = 1
> > > > > > > (gdb) ir virt
> > > > > > > priv 0x1 prv:1 [Supervisor]
> > > > > > > virt 0x1 1
> > > > > > > (gdb) set $virt = 0
> > > > > > > (gdb) ir virt
> > > > > > > priv 0x1 prv:1 [Supervisor]
> > > > > > > virt 0x0 0
> > > > > > > (gdb) set $virt = 1
> > > > > > > (gdb) ir virt
> > > > > > > priv 0x1 prv:1 [Supervisor]
> > > > > > > virt 0x1 1
> > > > > > > (gdb) set $priv = 3
> > > > > > > (gdb) ir virt
> > > > > > > priv 0x3 prv:3 [Machine]
> > > > > > > virt 0x0 0
> > > > > > > ```
> > > > > >
> > > > > > A gdbstub test case would be useful for this although I don't know if
> > > > > > the RiscV check-tcg tests switch mode at all.
> > > > > >
> > > > > > >
> > > > > > > As I am rather new to QEMU, please teach how we can add it as a QEMU
> > > > > > > only
> > > > > > > extension.
> > > > > >
> > > > > > You don't need to extend the XML from GDB, you can build a specific
> > > > > > one
> > > > > > for QEMU extensions. For example:
> > > > > >
> > > > > > gdb_feature_builder_init(¶m.builder,
> > > > > > &cpu->dyn_sysreg_feature.desc,
> > > > > > "org.qemu.gdb.arm.sys.regs",
> > > > > > "system-registers.xml",
> > > > > > base_reg);
> > > > > >
> > > > > > This exports all the system registers QEMU knows about and GDB can
> > > > > > access generically. Note the id is org.qemu..., indicating its our
> > > > > > schema not gdbs.
> > > > > Thanks for teaching, I need time to digest. I guess more feature builder
> > > > > APIs
> > > > > are needed (like append_reg) and the getter/setter callbacks might be at
> > > > > a
> > > > > different place.
> > > > >
> > > > > BTW, compared to adding virtual register `virt`, how do you think if we
> > > > > share
> > > > > the V bit as part of existing `priv` register?
> > > >
> > > > IMHO this is a very good idea since the latest release candidate of
> > > > riscv-debug-spec also includes the V bit in priv:2.
> > > >
> > >
> > > Thanks for this information, I noticed the bit(2) of `priv` register is for
> > > the
> > > V bit as per section 4.10.1.
> > >
> > > > > Or maybe we shall talk to GDB community to get their opinions? If they
> > > > > agree
> > > > > to
> > > > > add a few words about V bit here
> > > > > https://sourceware.org/gdb/current/onlinedocs/gdb.html/RISC_002dV-Features.html
> > > > > ,
> > > > > then it saves us a lot.
> > > >
> > > > Except being currently not supported by GDB
> > > >
> > > > (gdb) info register $priv
> > > > priv 0x5 prv:5 [INVALID]
> > > >
> > > > are there any reasons from QEMU's side that would speak against
> > > > including V in priv?
> > > >
> > >
> > > My v1 patch used `bit(8)` to avoid seeing the `[INVALID]` thing at GDB side,
> > > though that is due to GDB isn't in line with its own manual (i.e. use the
> > > two
> > > lowest bits only).
> > >
> > > Without a doc or specification. we felt people may not know `bit(8)` in v1
> > > patch
> > > was for the V bit, so I drafted patch v2 as Alistair suggested. However, as
> > > Alex
> > > pointed out, directly adding `virt` register in "org.gnu.gdb.riscv.virtual"
> > > XML
> > > is improper. I also wanted to raise this in GDB side but my application to
> > > join
> > > the mail list is still pending.
> > >
> > > Alex and Alistair, now I am wondering if we can follow the RiscV debug
> > > specification to use `bit(2)` of `priv` virtual register? My test shows
> > > except
> > > for the `[INVALID]` label, both set/get access seems working.
> >
> > I guess the INVALID just means gdb needs teaching about the format of
> > the register.
>
> Yes, GDB currently uses mask `0xff`(instead of `0x3`) to get the mode value when
> adding the string label, this violates its own manual:
>
> 1303 else if (regnum == RISCV_PRIV_REGNUM)
> 1304 {
> 1305 LONGEST d;
> 1306 uint8_t priv;
> 1307
> 1308 d = value_as_long (val);
> 1309 priv = d & 0xff;
> 1310
> 1311 if (priv < 4)
> 1312 {
> 1313 static const char * const sprv[] =
> 1314 {
> 1315 "User/Application",
> 1316 "Supervisor",
> 1317 "Hypervisor",
> 1318 "Machine"
> 1319 };
> 1320 gdb_printf (file, "\tprv:%d [%s]",
> 1321 priv, sprv[priv]);
> 1322 }
> 1323 else
> 1324 gdb_printf (file, "\tprv:%d [INVALID]", priv);
> 1325 }
>
>
> I am wondering if we can go ahead to follow RiscV debug specification and sync
> with GDB later?
If there is a spec then that is the way to go. Just link to it when
submitting the patch.
We also at least want to be sending a patch to GDB to fix it,
otherwise we are going to break people.
Alistair
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] riscv/gdb: add virt mode debug interface
2024-12-16 5:33 ` Alistair Francis
@ 2024-12-18 0:49 ` Yanfeng Liu
0 siblings, 0 replies; 12+ messages in thread
From: Yanfeng Liu @ 2024-12-18 0:49 UTC (permalink / raw)
To: Alistair Francis
Cc: Alex Bennée, Mario Fleischmann, qemu-riscv, qemu-devel,
alistair.francis
On Mon, 2024-12-16 at 15:33 +1000, Alistair Francis wrote:
> On Thu, Dec 5, 2024 at 7:17 PM Yanfeng Liu <yfliu2008@qq.com> wrote:
> >
> > On Thu, 2024-12-05 at 08:10 +0000, Alex Bennée wrote:
> > > Yanfeng Liu <yfliu2008@qq.com> writes:
> > >
> > > > On Wed, 2024-12-04 at 17:03 +0100, Mario Fleischmann wrote:
> > > > > Hi everyone,
> > > > >
> > > > > I'd like to chime in here because we are sitting on a similar patch
> > > > > which I wanted to send to the mailing list as soon as riscv-debug-spec
> > > > > v1.0.0 becomes ratified.
> > > > >
> > > > > For hypervisor support, `(qemu) info registers` isn't enough. We need
> > > > > to
> > > > > have both read and write access to the V-bit.
> > > > >
> > > > > On 04.12.2024 14:43, Yanfeng Liu wrote:
> > > > > > On Fri, 2024-11-29 at 09:59 +0000, Alex Bennée wrote:
> > > > > > > Yanfeng <yfliu2008@qq.com> writes:
> > > > > > >
> > > > > > > > On Thu, 2024-11-28 at 14:21 +0000, Alex Bennée wrote:
> > > > > > > > > Yanfeng Liu <yfliu2008@qq.com> writes:
> > > > > > > > >
> > > > > > > > > > This adds `virt` virtual register on debug interface so that
> > > > > > > > > > users
> > > > > > > > > > can access current virtualization mode for debugging
> > > > > > > > > > purposes.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
> > > > > > > > > > ---
> > > > > > > > > > gdb-xml/riscv-32bit-virtual.xml | 1 +
> > > > > > > > > > gdb-xml/riscv-64bit-virtual.xml | 1 +
> > > > > > > > > > target/riscv/gdbstub.c | 18 ++++++++++++------
> > > > > > > > > > 3 files changed, 14 insertions(+), 6 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-
> > > > > > > > > > xml/riscv-
> > > > > > > > > > 32bit-
> > > > > > > > > > virtual.xml
> > > > > > > > > > index 905f1c555d..d44b6ca2dc 100644
> > > > > > > > > > --- a/gdb-xml/riscv-32bit-virtual.xml
> > > > > > > > > > +++ b/gdb-xml/riscv-32bit-virtual.xml
> > > > > > > > > > @@ -8,4 +8,5 @@
> > > > > > > > > > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > > > > > > > > > <feature name="org.gnu.gdb.riscv.virtual">
> > > > > > > > > > <reg name="priv" bitsize="32"/>
> > > > > > > > > > + <reg name="virt" bitsize="32"/>
> > > > > > > > > > </feature>
> > > > > > > > > > diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-
> > > > > > > > > > xml/riscv-
> > > > > > > > > > 64bit-
> > > > > > > > > > virtual.xml
> > > > > > > > > > index 62d86c237b..7c9b63d5b6 100644
> > > > > > > > > > --- a/gdb-xml/riscv-64bit-virtual.xml
> > > > > > > > > > +++ b/gdb-xml/riscv-64bit-virtual.xml
> > > > > > > > > > @@ -8,4 +8,5 @@
> > > > > > > > > > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > > > > > > > > > <feature name="org.gnu.gdb.riscv.virtual">
> > > > > > > > > > <reg name="priv" bitsize="64"/>
> > > > > > > > > > + <reg name="virt" bitsize="64"/>
> > > > > > > > > > </feature>
> > > > > > > > >
> > > > > > > > > I assume these are mirrored in gdb not a QEMU only extension?
> > > > > > > >
> > > > > > > > So far I think it is a QEMU extension and the `gdb-multiarch`
> > > > > > > > doesn't
> > > > > > > > treat
> > > > > > > > is
> > > > > > > > specially. My tests shows it basically works:
> > > > > > > >
> > > > > > > > ```
> > > > > > > > (gdb) ir virt
> > > > > > > > priv 0x3 prv:3 [Machine]
> > > > > > > > virt 0x0 0
> > > > > > > > (gdb) set $priv = 2
> > > > > > > > (gdb) ir virt
> > > > > > > > priv 0x1 prv:1 [Supervisor]
> > > > > > > > virt 0x0 0
> > > > > > > > (gdb) set $virt = 1
> > > > > > > > (gdb) ir virt
> > > > > > > > priv 0x1 prv:1 [Supervisor]
> > > > > > > > virt 0x1 1
> > > > > > > > (gdb) set $virt = 0
> > > > > > > > (gdb) ir virt
> > > > > > > > priv 0x1 prv:1 [Supervisor]
> > > > > > > > virt 0x0 0
> > > > > > > > (gdb) set $virt = 1
> > > > > > > > (gdb) ir virt
> > > > > > > > priv 0x1 prv:1 [Supervisor]
> > > > > > > > virt 0x1 1
> > > > > > > > (gdb) set $priv = 3
> > > > > > > > (gdb) ir virt
> > > > > > > > priv 0x3 prv:3 [Machine]
> > > > > > > > virt 0x0 0
> > > > > > > > ```
> > > > > > >
> > > > > > > A gdbstub test case would be useful for this although I don't know
> > > > > > > if
> > > > > > > the RiscV check-tcg tests switch mode at all.
> > > > > > >
> > > > > > > >
> > > > > > > > As I am rather new to QEMU, please teach how we can add it as a
> > > > > > > > QEMU
> > > > > > > > only
> > > > > > > > extension.
> > > > > > >
> > > > > > > You don't need to extend the XML from GDB, you can build a
> > > > > > > specific
> > > > > > > one
> > > > > > > for QEMU extensions. For example:
> > > > > > >
> > > > > > > gdb_feature_builder_init(¶m.builder,
> > > > > > > &cpu->dyn_sysreg_feature.desc,
> > > > > > > "org.qemu.gdb.arm.sys.regs",
> > > > > > > "system-registers.xml",
> > > > > > > base_reg);
> > > > > > >
> > > > > > > This exports all the system registers QEMU knows about and GDB can
> > > > > > > access generically. Note the id is org.qemu..., indicating its our
> > > > > > > schema not gdbs.
> > > > > > Thanks for teaching, I need time to digest. I guess more feature
> > > > > > builder
> > > > > > APIs
> > > > > > are needed (like append_reg) and the getter/setter callbacks might
> > > > > > be at
> > > > > > a
> > > > > > different place.
> > > > > >
> > > > > > BTW, compared to adding virtual register `virt`, how do you think if
> > > > > > we
> > > > > > share
> > > > > > the V bit as part of existing `priv` register?
> > > > >
> > > > > IMHO this is a very good idea since the latest release candidate of
> > > > > riscv-debug-spec also includes the V bit in priv:2.
> > > > >
> > > >
> > > > Thanks for this information, I noticed the bit(2) of `priv` register is
> > > > for
> > > > the
> > > > V bit as per section 4.10.1.
> > > >
> > > > > > Or maybe we shall talk to GDB community to get their opinions? If
> > > > > > they
> > > > > > agree
> > > > > > to
> > > > > > add a few words about V bit here
> > > > > > https://sourceware.org/gdb/current/onlinedocs/gdb.html/RISC_002dV-Features.html
> > > > > > ,
> > > > > > then it saves us a lot.
> > > > >
> > > > > Except being currently not supported by GDB
> > > > >
> > > > > (gdb) info register $priv
> > > > > priv 0x5 prv:5 [INVALID]
> > > > >
> > > > > are there any reasons from QEMU's side that would speak against
> > > > > including V in priv?
> > > > >
> > > >
> > > > My v1 patch used `bit(8)` to avoid seeing the `[INVALID]` thing at GDB
> > > > side,
> > > > though that is due to GDB isn't in line with its own manual (i.e. use
> > > > the
> > > > two
> > > > lowest bits only).
> > > >
> > > > Without a doc or specification. we felt people may not know `bit(8)` in
> > > > v1
> > > > patch
> > > > was for the V bit, so I drafted patch v2 as Alistair suggested. However,
> > > > as
> > > > Alex
> > > > pointed out, directly adding `virt` register in
> > > > "org.gnu.gdb.riscv.virtual"
> > > > XML
> > > > is improper. I also wanted to raise this in GDB side but my application
> > > > to
> > > > join
> > > > the mail list is still pending.
> > > >
> > > > Alex and Alistair, now I am wondering if we can follow the RiscV debug
> > > > specification to use `bit(2)` of `priv` virtual register? My test shows
> > > > except
> > > > for the `[INVALID]` label, both set/get access seems working.
> > >
> > > I guess the INVALID just means gdb needs teaching about the format of
> > > the register.
> >
> > Yes, GDB currently uses mask `0xff`(instead of `0x3`) to get the mode value
> > when
> > adding the string label, this violates its own manual:
> >
> > 1303 else if (regnum == RISCV_PRIV_REGNUM)
> > 1304 {
> > 1305 LONGEST d;
> > 1306 uint8_t priv;
> > 1307
> > 1308 d = value_as_long (val);
> > 1309 priv = d & 0xff;
> > 1310
> > 1311 if (priv < 4)
> > 1312 {
> > 1313 static const char * const sprv[] =
> > 1314 {
> > 1315 "User/Application",
> > 1316 "Supervisor",
> > 1317 "Hypervisor",
> > 1318 "Machine"
> > 1319 };
> > 1320 gdb_printf (file, "\tprv:%d [%s]",
> > 1321 priv, sprv[priv]);
> > 1322 }
> > 1323 else
> > 1324 gdb_printf (file, "\tprv:%d [INVALID]", priv);
> > 1325 }
> >
> >
> > I am wondering if we can go ahead to follow RiscV debug specification and
> > sync
> > with GDB later?
>
> If there is a spec then that is the way to go. Just link to it when
> submitting the patch.
>
> We also at least want to be sending a patch to GDB to fix it,
> otherwise we are going to break people.
>
Thanks for the suggestion, a patch was sent to GDB:
https://sourceware.org/pipermail/gdb-patches/2024-December/214221.html
> Alistair
Regards,
yf
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-12-18 0:51 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-28 7:21 [PATCH v2] riscv/gdb: add virt mode debug interface Yanfeng Liu
2024-11-28 14:21 ` Alex Bennée
2024-11-28 18:01 ` Richard Henderson
2024-11-29 2:11 ` Yanfeng
2024-11-29 9:59 ` Alex Bennée
2024-11-30 0:14 ` Yanfeng Liu
2024-12-04 16:03 ` Mario Fleischmann
2024-12-05 1:29 ` Yanfeng Liu
2024-12-05 8:10 ` Alex Bennée
2024-12-05 9:15 ` Yanfeng Liu
2024-12-16 5:33 ` Alistair Francis
2024-12-18 0:49 ` Yanfeng Liu
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.