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(-)
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.1Yanfeng 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
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);
>
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
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. >
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?
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
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
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
>
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
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
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~
© 2016 - 2024 Red Hat, Inc.