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

[Yanfeng Liu]

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(&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?
> > > > > 
> > > > > 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