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

["Alex Bennée" <alex.bennee@linaro.org>]

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.

>
>
> Regards,
> yf

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro