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

2024-12-17 Thread 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  wrote:
> > 
> > On Thu, 2024-12-05 at 08:10 +, Alex Bennée wrote:
> > > Yanfeng Liu  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 +, Alex Bennée wrote:
> > > > > > > Yanfeng  writes:
> > > > > > > 
> > > > > > > > On Thu, 2024-11-28 at 14:21 +, Alex Bennée wrote:
> > > > > > > > > Yanfeng Liu  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 
> > > > > > > > > > ---
> > > > > > > > > >   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 @@
> > > > > > > > > >   
> > > > > > > > > >   
> > > > > > > > > >     
> > > > > > > > > > +  
> > > > > > > > > >   
> > > > > > > > > > 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 @@
> > > > > > > > > >   
> > > > > > > > > >   
> > > > > > > > > >     
> > > > > > > > > > +  
> > > > > > > > > >   
> > > > > > > > > 
> > > > > > > > > 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 ti

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

2024-12-15 Thread Alistair Francis
On Thu, Dec 5, 2024 at 7:17 PM Yanfeng Liu  wrote:
>
> On Thu, 2024-12-05 at 08:10 +, Alex Bennée wrote:
> > Yanfeng Liu  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 +, Alex Bennée wrote:
> > > > > > Yanfeng  writes:
> > > > > >
> > > > > > > On Thu, 2024-11-28 at 14:21 +, Alex Bennée wrote:
> > > > > > > > Yanfeng Liu  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 
> > > > > > > > > ---
> > > > > > > > >   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 @@
> > > > > > > > >   
> > > > > > > > >   
> > > > > > > > > 
> > > > > > > > > +  
> > > > > > > > >   
> > > > > > > > > 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 @@
> > > > > > > > >   
> > > > > > > > >   
> > > > > > > > > 
> > > > > > > > > +  
> > > > > > > > >   
> > > > > > > >
> > > > > > > > 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?
> > > >
> > > >

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

2024-12-05 Thread Yanfeng Liu
On Thu, 2024-12-05 at 08:10 +, Alex Bennée wrote:
> Yanfeng Liu  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 +, Alex Bennée wrote:
> > > > > Yanfeng  writes:
> > > > > 
> > > > > > On Thu, 2024-11-28 at 14:21 +, Alex Bennée wrote:
> > > > > > > Yanfeng Liu  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 
> > > > > > > > ---
> > > > > > > >   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 @@
> > > > > > > >   
> > > > > > > >   
> > > > > > > >     
> > > > > > > > +  
> > > > > > > >   
> > > > > > > > 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 @@
> > > > > > > >   
> > > > > > > >   
> > > > > > > >     
> > > > > > > > +  
> > > > > > > >   
> > > > > > > 
> > > > > > > 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?

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

2024-12-05 Thread Alex Bennée
Yanfeng Liu  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 +, Alex Bennée wrote:
>> > > Yanfeng  writes:
>> > > 
>> > > > On Thu, 2024-11-28 at 14:21 +, Alex Bennée wrote:
>> > > > > Yanfeng Liu  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 
>> > > > > > ---
>> > > > > >   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 @@
>> > > > > >   
>> > > > > >   
>> > > > > >     
>> > > > > > +  
>> > > > > >   
>> > > > > > 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 @@
>> > > > > >   
>> > > > > >   
>> > > > > >     
>> > > > > > +  
>> > > > > >   
>> > > > > 
>> > > > > 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,

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

2024-12-04 Thread Yanfeng Liu
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 +, Alex Bennée wrote:
> > > Yanfeng  writes:
> > > 
> > > > On Thu, 2024-11-28 at 14:21 +, Alex Bennée wrote:
> > > > > Yanfeng Liu  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 
> > > > > > ---
> > > > > >   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 @@
> > > > > >   
> > > > > >   
> > > > > >     
> > > > > > +  
> > > > > >   
> > > > > > 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 @@
> > > > > >   
> > > > > >   
> > > > > >     
> > > > > > +  
> > > > > >   
> > > > > 
> > > > > 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 v

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

2024-12-04 Thread Mario Fleischmann

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 +, Alex Bennée wrote:

Yanfeng  writes:


On Thu, 2024-11-28 at 14:21 +, Alex Bennée wrote:

Yanfeng Liu  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 
---
  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 @@
  
  
    
+  
  
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 @@
  
  
    
+  
  


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?






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

2024-11-29 Thread Yanfeng Liu
On Fri, 2024-11-29 at 09:59 +, Alex Bennée wrote:
> Yanfeng  writes:
> 
> > On Thu, 2024-11-28 at 14:21 +, Alex Bennée wrote:
> > > Yanfeng Liu  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 
> > > > ---
> > > >  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 @@
> > > >  
> > > >  
> > > >    
> > > > +  
> > > >  
> > > > 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 @@
> > > >  
> > > >  
> > > >    
> > > > +  
> > > >  
> > > 
> > > 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. 

> 









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

2024-11-29 Thread Alex Bennée
Yanfeng  writes:

> On Thu, 2024-11-28 at 14:21 +, Alex Bennée wrote:
>> Yanfeng Liu  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 
>> > ---
>> >  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 @@
>> >  
>> >  
>> >    
>> > +  
>> >  
>> > 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 @@
>> >  
>> >  
>> >    
>> > +  
>> >  
>> 
>> 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   0x3prv:3 [Machine]
> virt   0x00
> (gdb) set $priv = 2
> (gdb) ir virt
> priv   0x1prv:1 [Supervisor]
> virt   0x00
> (gdb) set $virt = 1
> (gdb) ir virt
> priv   0x1prv:1 [Supervisor]
> virt   0x11
> (gdb) set $virt = 0
> (gdb) ir virt
> priv   0x1prv:1 [Supervisor]
> virt   0x00
> (gdb) set $virt = 1
> (gdb) ir virt
> priv   0x1prv:1 [Supervisor]
> virt   0x11
> (gdb) set $priv = 3
> (gdb) ir virt
> priv   0x3prv:3 [Machine]
> virt   0x00
> ```

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



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

2024-11-28 Thread Yanfeng
On Thu, 2024-11-28 at 14:21 +, Alex Bennée wrote:
> Yanfeng Liu  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 
> > ---
> >  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 @@
> >  
> >  
> >    
> > +  
> >  
> > 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 @@
> >  
> >  
> >    
> > +  
> >  
> 
> 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);
> 




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

2024-11-28 Thread Richard Henderson

On 11/28/24 08:21, Alex Bennée wrote:

Yanfeng Liu  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 
---
  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 @@
  
  

+  
  
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 @@
  
  

+  
  


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~



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

2024-11-28 Thread Alex Bennée
Yanfeng Liu  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 
> ---
>  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 @@
>  
>  
>
> +  
>  
> 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 @@
>  
>  
>
> +  
>  

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