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

[Mario Fleischmann]

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.