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

[Yanfeng]

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);
>