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

[Alex Bennée]

Yanfeng Liu <yfliu2008@qq.com> 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 <yfliu2008@qq.com>
> ---
>  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 @@
>  <!DOCTYPE feature SYSTEM "gdb-target.dtd">
>  <feature name="org.gnu.gdb.riscv.virtual">
>    <reg name="priv" bitsize="32"/>
> +  <reg name="virt" bitsize="32"/>
>  </feature>
> 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 @@
>  <!DOCTYPE feature SYSTEM "gdb-target.dtd">
>  <feature name="org.gnu.gdb.riscv.virtual">
>    <reg name="priv" bitsize="64"/>
> +  <reg name="virt" bitsize="64"/>
>  </feature>

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