Re: [PATCH] include virtualization mode as part of priv

[Alistair Francis]

On Thu, Nov 28, 2024 at 12:09 AM Yanfeng <yfliu2008@qq.com> wrote:
>
>
> When debugging hypervisor extension based programs, it is convenient to see 
> the
> current virtualization mode from GDB debugger.
>
> This patch shares the virtualization mode as part of the existing "priv" 
> virtual
> register, or more specifically via bit(8).

Interesting concept. This seems fine to me, I don't think this will
break any existing user.

Another option though is to add a "virt" virtual register, which might
be easier for people to figure out as it isn't obvious from GDB what
bit 8 means. That might be a better approach as then it's really clear
what the register means.

>
>
> >From 0d82561b11e1c2835f14ba5460cfff52f0087530 Mon Sep 17 00:00:00 2001
> From: Yanfeng Liu <yfliu2008@qq.com>
> Date: Mon, 18 Nov 2024 08:03:15 +0800
> Subject: [PATCH] riscv/gdb: share virt mode via priv register

It seems like something strange happened when you submitted the patch.
Can you fix this up?

Alistair

>
> This shares virtualization mode together with privilege mode
> via the `priv` virtual register over the debug interface.
>
> Check logs with gdb-multiarch 12.1:
>
> ```
> (gdb) info registers priv
> priv           0x101    prv:1 [Supervisor]
> (gdb) set $priv = 1
> (gdb) info registers priv
> priv           0x1      prv:1 [Supervisor]
> (gdb) set $priv = 0x101
> (gdb) info registers priv
> priv           0x101    prv:1 [Supervisor]
> (gdb)
> ```
>
> Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
> ---
>  target/riscv/cpu_bits.h |  4 ++++
>  target/riscv/gdbstub.c  | 15 +++++++++++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 385a2c67c2..cc6dece51a 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -623,6 +623,10 @@ typedef enum {
>  #define PRV_RESERVED 2
>  #define PRV_M 3
>
> +/* Share virtualization mode as part of priv register */
> +#define PRV_V                (1 << 8)
> +
> +
>  /* RV32 satp CSR field masks */
>  #define SATP32_MODE         0x80000000
>  #define SATP32_ASID         0x7fc00000
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index c07df972f1..d9e6ad969a 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -212,8 +212,14 @@ static int riscv_gdb_get_virtual(CPUState *cs, GByteArray
> *buf, int n)
>  #else
>          RISCVCPU *cpu = RISCV_CPU(cs);
>          CPURISCVState *env = &cpu->env;
> +        target_ulong ret = env->priv;
>
> -        return gdb_get_regl(buf, env->priv);
> +        /* include virtualization mode */
> +
> +        if (env->virt_enabled) {
> +            ret |= PRV_V;
> +        }
> +        return gdb_get_regl(buf, ret);
>  #endif
>      }
>      return 0;
> @@ -225,11 +231,16 @@ static int riscv_gdb_set_virtual(CPUState *cs, uint8_t
> *mem_buf, int n)
>  #ifndef CONFIG_USER_ONLY
>          RISCVCPU *cpu = RISCV_CPU(cs);
>          CPURISCVState *env = &cpu->env;
> +        target_ulong val = ldtul_p(mem_buf);
>
> -        env->priv = ldtul_p(mem_buf) & 0x3;
> +        env->priv = val & 0x3;
>          if (env->priv == PRV_RESERVED) {
>              env->priv = PRV_S;
>          }
> +
> +        /* Update virtualization mode */
> +
> +        env->virt_enabled = (env->priv != PRV_M && (val & PRV_V) != 0);
>  #endif
>          return sizeof(target_ulong);
>      }
> --
> 2.34.1
>
>
>
>