1. Patchew
  2. QEMU
  3. View series

[PATCH v3] riscv/gdb: add V bit to priv register

Yanfeng Liu posted 1 patch 1 year ago
Failed in applying to current master (apply log)
target/riscv/gdbstub.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
[PATCH v3] riscv/gdb: add V bit to priv register
Posted by Yanfeng Liu 1 year ago 
This adds virtualization mode (V bit) as bit(2) of register `priv`
per RiscV debug spec v1.0.0-rc3. Checked with gdb-multiarch v12.1.

Note that GDB may display `INVALID` tag for the value when V bit
is set, this doesn't affect accessing to the bit.

Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
---
 target/riscv/gdbstub.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index c07df972f1..8cc095cda3 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -213,7 +213,10 @@ static int riscv_gdb_get_virtual(CPUState *cs, GByteArray *buf, int n)
         RISCVCPU *cpu = RISCV_CPU(cs);
         CPURISCVState *env = &cpu->env;
 
-        return gdb_get_regl(buf, env->priv);
+        /* Per RiscV debug spec v1.0.0 rc3 */
+        target_ulong vbit = (env->virt_enabled) ? BIT(2) : 0;
+
+        return gdb_get_regl(buf, env->priv | vbit);
 #endif
     }
     return 0;
@@ -230,6 +233,8 @@ static int riscv_gdb_set_virtual(CPUState *cs, uint8_t *mem_buf, int n)
         if (env->priv == PRV_RESERVED) {
             env->priv = PRV_S;
         }
+        env->virt_enabled = (env->priv == PRV_M) ? 0 :
+                            ((ldtul_p(mem_buf) & BIT(2)) >> 2);
 #endif
         return sizeof(target_ulong);
     }
-- 
2.34.1
Re: [PATCH v3] riscv/gdb: add V bit to priv register
Posted by Mario Fleischmann 1 year ago 
Hi,

apologies for the delayed review; I've just gotten to it now.

On 06.12.2024 01:14, Yanfeng Liu wrote:
> This adds virtualization mode (V bit) as bit(2) of register `priv`
> per RiscV debug spec v1.0.0-rc3. Checked with gdb-multiarch v12.1.
> 
> Note that GDB may display `INVALID` tag for the value when V bit
> is set, this doesn't affect accessing to the bit.
> 
> Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
> ---
>   target/riscv/gdbstub.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index c07df972f1..8cc095cda3 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -213,7 +213,10 @@ static int riscv_gdb_get_virtual(CPUState *cs, GByteArray *buf, int n)
>           RISCVCPU *cpu = RISCV_CPU(cs);
>           CPURISCVState *env = &cpu->env;
>   
> -        return gdb_get_regl(buf, env->priv);
> +        /* Per RiscV debug spec v1.0.0 rc3 */

Now that rc4 is released, you might also cite "RISC-V Debug
Specification v1.0.0 rc4".

> +        target_ulong vbit = (env->virt_enabled) ? BIT(2) : 0;
> +
> +        return gdb_get_regl(buf, env->priv | vbit);
>   #endif
>       }
>       return 0;
> @@ -230,6 +233,8 @@ static int riscv_gdb_set_virtual(CPUState *cs, uint8_t *mem_buf, int n)
>           if (env->priv == PRV_RESERVED) {
>               env->priv = PRV_S;
>           }
> +        env->virt_enabled = (env->priv == PRV_M) ? 0 :
> +                            ((ldtul_p(mem_buf) & BIT(2)) >> 2);

Looking at the other places in the source code where virt_enabled is 
set, we should also check here if the H extension is active.
Alternatively, you might also consider using riscv_cpu_set_mode():

Message-ID: <20240711-smcntrpmf_v7-v8-1-b7c38ae7b263@rivosinc.com>
Date: Thu, 11 Jul 2024 15:31:04 -0700
Subject: [PATCH v8 01/13] target/riscv: Combine set_mode and set_virt 
functions.
From: Atish Patra <atishp@rivosinc.com>

>   #endif
>           return sizeof(target_ulong);
>       }

In addition, we need to swap the virtual supervisor registers from the H 
extension, e.g. vsstatus:

"When V=1, vsstatus substitutes for the usual sstatus, so instructions 
that normally read or modify sstatus actually access vsstatus instead." 
(privileged spec)

With the current patch, I was able to read and modify V, but the 
registers were not changing:

(gdb) info register $priv
priv           0x4      prv:4 [INVALID]
(gdb) info register $sstatus
sstatus        0x200004022      8589951010
(gdb) set $priv = 0x0
(gdb) info register $priv
priv           0x0      prv:0 [User/Application]
(gdb) info register $sstatus
sstatus        0x200004022      8589951010

Take a look at riscv_cpu_swap_hypervisor_regs() which I believe does the 
thing we need here. Note that the function is supposed be called before 
the mode switch.
Re: [PATCH v3] riscv/gdb: add V bit to priv register
Posted by Yanfeng Liu 1 year ago 
On Fri, 2024-12-13 at 10:04 +0100, Mario Fleischmann wrote:
> Hi,
> 
> apologies for the delayed review; I've just gotten to it now.
> 
> On 06.12.2024 01:14, Yanfeng Liu wrote:
> > This adds virtualization mode (V bit) as bit(2) of register `priv`
> > per RiscV debug spec v1.0.0-rc3. Checked with gdb-multiarch v12.1.
> > 
> > Note that GDB may display `INVALID` tag for the value when V bit
> > is set, this doesn't affect accessing to the bit.
> > 
> > Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
> > ---
> >   target/riscv/gdbstub.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> > index c07df972f1..8cc095cda3 100644
> > --- a/target/riscv/gdbstub.c
> > +++ b/target/riscv/gdbstub.c
> > @@ -213,7 +213,10 @@ static int riscv_gdb_get_virtual(CPUState *cs,
> > GByteArray *buf, int n)
> >           RISCVCPU *cpu = RISCV_CPU(cs);
> >           CPURISCVState *env = &cpu->env;
> >   
> > -        return gdb_get_regl(buf, env->priv);
> > +        /* Per RiscV debug spec v1.0.0 rc3 */
> 
> Now that rc4 is released, you might also cite "RISC-V Debug
> Specification v1.0.0 rc4".
Okay, will do.
> > +        target_ulong vbit = (env->virt_enabled) ? BIT(2) : 0;
> > +
> > +        return gdb_get_regl(buf, env->priv | vbit);
> >   #endif
> >       }
> >       return 0;
> > @@ -230,6 +233,8 @@ static int riscv_gdb_set_virtual(CPUState *cs, uint8_t
> > *mem_buf, int n)
> >           if (env->priv == PRV_RESERVED) {
> >               env->priv = PRV_S;
> >           }
> > +        env->virt_enabled = (env->priv == PRV_M) ? 0 :
> > +                            ((ldtul_p(mem_buf) & BIT(2)) >> 2);
> 
> Looking at the other places in the source code where virt_enabled is 
> set, we should also check here if the H extension is active.
> Alternatively, you might also consider using riscv_cpu_set_mode():
> 
> Message-ID: <20240711-smcntrpmf_v7-v8-1-b7c38ae7b263@rivosinc.com>
> Date: Thu, 11 Jul 2024 15:31:04 -0700
> Subject: [PATCH v8 01/13] target/riscv: Combine set_mode and set_virt 
> functions.
> From: Atish Patra <atishp@rivosinc.com>
> 
Thanks for this, I will check both and see what is easier for a QEMU newbie
later.

> > 
> >   #endif
> >           return sizeof(target_ulong);
> >       }
> 
> In addition, we need to swap the virtual supervisor registers from the H 
> extension, e.g. vsstatus:
> 
> "When V=1, vsstatus substitutes for the usual sstatus, so instructions 
> that normally read or modify sstatus actually access vsstatus instead." 
> (privileged spec)
> 
> With the current patch, I was able to read and modify V, but the 
> registers were not changing:
> 
> (gdb) info register $priv
> priv           0x4      prv:4 [INVALID]
> (gdb) info register $sstatus
> sstatus        0x200004022      8589951010
> (gdb) set $priv = 0x0
> (gdb) info register $priv
> priv           0x0      prv:0 [User/Application]
> (gdb) info register $sstatus
> sstatus        0x200004022      8589951010
> 
> Take a look at riscv_cpu_swap_hypervisor_regs() which I believe does the 
> thing we need here. Note that the function is supposed be called before 
> the mode switch.

thanks again, I will check that swapping method and use it before changing the v
bit. 

Regards,
yf

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.