Re: [PATCH v2] riscv/gdb: add virt mode debug interface
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
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
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
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
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
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
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
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
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
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
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