Re: [PATCH v2] riscv/gdb: add virt mode debug interface

[Yanfeng Liu]

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(&param.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. 

>