[PULL 04/26] target/riscv: implement MonitorDef HMP API

alistair23@gmail.com posted 26 patches 3 weeks, 2 days ago
There is a newer version of this series
[PULL 04/26] target/riscv: implement MonitorDef HMP API
Posted by alistair23@gmail.com 3 weeks, 2 days ago
From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

The MonitorDef API is related to two HMP monitor commands: 'p' and 'x':

(qemu) help p
print|p /fmt expr -- print expression value (use $reg for CPU register access)
(qemu) help x
x /fmt addr -- virtual memory dump starting at 'addr'

For x86, one of the few targets that implements it, it is possible to
print the PC register value with $pc and use the PC value in the 'x'
command as well.

Those 2 commands are hooked into get_monitor_def(), called by
exp_unary() in hmp.c. The function tries to fetch a reg value in two
ways: by reading them directly via a target_monitor_defs array or using
a target_get_monitor_def() helper. In RISC-V we have *A LOT* of
registers and this number will keep getting bigger, so we're opting out
of an array declaration.

We're able to retrieve all regs but vregs because the API only fits an
uint64_t and vregs have 'vlen' size that are bigger than that.

With this patch we can do things such as:

- print CSRs and use their val in expressions:
(qemu) p $mstatus
0xa000000a0
(qemu) p $mstatus & 0xFF
0xa0

- dump the next 10 insn from virtual memory starting at x1 (ra):

(qemu) x/10i $ra
0xffffffff80958aea:  a9bff0ef          jal                     ra,-1382                # 0xffffffff80958584
0xffffffff80958aee:  10016073          csrrsi                  zero,sstatus,2
0xffffffff80958af2:  60a2              ld                      ra,8(sp)
0xffffffff80958af4:  6402              ld                      s0,0(sp)
0xffffffff80958af6:  0141              addi                    sp,sp,16
0xffffffff80958af8:  8082              ret
0xffffffff80958afa:  10016073          csrrsi                  zero,sstatus,2
0xffffffff80958afe:  8082              ret
0xffffffff80958b00:  1141              addi                    sp,sp,-16
0xffffffff80958b02:  e422              sd                      s0,8(sp)
(qemu)

Suggested-by: Dr. David Alan Gilbert <dave@treblig.org>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Message-ID: <20250703130815.1592493-1-dbarboza@ventanamicro.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.h            |   1 +
 target/riscv/riscv-qmp-cmds.c | 148 ++++++++++++++++++++++++++++++++++
 2 files changed, 149 insertions(+)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 4a862da615..738e68fa6e 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -592,6 +592,7 @@ static inline int riscv_has_ext(CPURISCVState *env, target_ulong ext)
 extern const char * const riscv_int_regnames[];
 extern const char * const riscv_int_regnamesh[];
 extern const char * const riscv_fpr_regnames[];
+extern const char * const riscv_rvv_regnames[];
 
 const char *riscv_cpu_get_trap_name(target_ulong cause, bool async);
 int riscv_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
diff --git a/target/riscv/riscv-qmp-cmds.c b/target/riscv/riscv-qmp-cmds.c
index 8a1856c50e..b63de8dd45 100644
--- a/target/riscv/riscv-qmp-cmds.c
+++ b/target/riscv/riscv-qmp-cmds.c
@@ -31,6 +31,10 @@
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/visitor.h"
 #include "qom/qom-qobject.h"
+#include "qemu/ctype.h"
+#include "qemu/qemu-print.h"
+#include "monitor/hmp.h"
+#include "monitor/hmp-target.h"
 #include "system/kvm.h"
 #include "system/tcg.h"
 #include "cpu-qom.h"
@@ -240,3 +244,147 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
 
     return expansion_info;
 }
+
+/*
+ * We have way too many potential CSRs and regs being added
+ * regularly to register them in a static array.
+ *
+ * Declare an empty array instead, making get_monitor_def() use
+ * the target_get_monitor_def() API directly.
+ */
+const MonitorDef monitor_defs[] = { { } };
+const MonitorDef *target_monitor_defs(void)
+{
+    return monitor_defs;
+}
+
+static bool reg_is_ulong_integer(CPURISCVState *env, const char *name,
+                                 target_ulong *val, bool is_gprh)
+{
+    const char * const *reg_names;
+    target_ulong *vals;
+
+    if (is_gprh) {
+        reg_names = riscv_int_regnamesh;
+        vals = env->gprh;
+    } else {
+        reg_names = riscv_int_regnames;
+        vals = env->gpr;
+    }
+
+    for (int i = 0; i < 32; i++) {
+        g_autofree char *reg_name = g_strdup(reg_names[i]);
+        char *reg1 = strtok(reg_name, "/");
+        char *reg2 = strtok(NULL, "/");
+
+        if (strcasecmp(reg1, name) == 0 ||
+            (reg2 && strcasecmp(reg2, name) == 0)) {
+            *val = vals[i];
+            return true;
+        }
+    }
+
+    return false;
+}
+
+static bool reg_is_u64_fpu(CPURISCVState *env, const char *name, uint64_t *val)
+{
+    if (qemu_tolower(name[0]) != 'f') {
+        return false;
+    }
+
+    for (int i = 0; i < 32; i++) {
+        g_autofree char *reg_name = g_strdup(riscv_fpr_regnames[i]);
+        char *reg1 = strtok(reg_name, "/");
+        char *reg2 = strtok(NULL, "/");
+
+        if (strcasecmp(reg1, name) == 0 ||
+            (reg2 && strcasecmp(reg2, name) == 0)) {
+            *val = env->fpr[i];
+            return true;
+        }
+    }
+
+    return false;
+}
+
+static bool reg_is_vreg(const char *name)
+{
+    if (qemu_tolower(name[0]) != 'v' || strlen(name) > 3) {
+        return false;
+    }
+
+    for (int i = 0; i < 32; i++) {
+        if (strcasecmp(name, riscv_rvv_regnames[i]) == 0) {
+            return true;
+        }
+    }
+
+    return false;
+}
+
+int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval)
+{
+    CPURISCVState *env = &RISCV_CPU(cs)->env;
+    target_ulong val = 0;
+    uint64_t val64 = 0;
+    int i;
+
+    if (reg_is_ulong_integer(env, name, &val, false) ||
+        reg_is_ulong_integer(env, name, &val, true)) {
+        *pval = val;
+        return 0;
+    }
+
+    if (reg_is_u64_fpu(env, name, &val64)) {
+        *pval = val64;
+        return 0;
+    }
+
+    if (reg_is_vreg(name)) {
+        if (!riscv_has_ext(env, RVV)) {
+            return -EINVAL;
+        }
+
+        qemu_printf("Unable to print the value of vector "
+                    "vreg '%s' from this API\n", name);
+
+        /*
+         * We're returning 0 because returning -EINVAL triggers
+         * an 'unknown register' message in exp_unary() later,
+         * which feels ankward after our own error message.
+         */
+        *pval = 0;
+        return 0;
+    }
+
+    for (i = 0; i < ARRAY_SIZE(csr_ops); i++) {
+        RISCVException res;
+        int csrno = i;
+
+        /*
+         * Early skip when possible since we're going
+         * through a lot of NULL entries.
+         */
+        if (csr_ops[csrno].predicate == NULL) {
+            continue;
+        }
+
+        if (strcasecmp(csr_ops[csrno].name, name) != 0) {
+            continue;
+        }
+
+        res = riscv_csrrw_debug(env, csrno, &val, 0, 0);
+
+        /*
+         * Rely on the smode, hmode, etc, predicates within csr.c
+         * to do the filtering of the registers that are present.
+         */
+        if (res == RISCV_EXCP_NONE) {
+            *pval = val;
+            return 0;
+        }
+    }
+
+    return -EINVAL;
+}
-- 
2.51.0
Re: [PULL 04/26] target/riscv: implement MonitorDef HMP API
Posted by Peter Maydell 5 days, 6 hours ago
On Fri, 3 Oct 2025 at 04:29, <alistair23@gmail.com> wrote:
>
> From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>
> The MonitorDef API is related to two HMP monitor commands: 'p' and 'x':
>
> (qemu) help p
> print|p /fmt expr -- print expression value (use $reg for CPU register access)
> (qemu) help x
> x /fmt addr -- virtual memory dump starting at 'addr'
>
> For x86, one of the few targets that implements it, it is possible to
> print the PC register value with $pc and use the PC value in the 'x'
> command as well.
>
> Those 2 commands are hooked into get_monitor_def(), called by
> exp_unary() in hmp.c. The function tries to fetch a reg value in two
> ways: by reading them directly via a target_monitor_defs array or using
> a target_get_monitor_def() helper. In RISC-V we have *A LOT* of
> registers and this number will keep getting bigger, so we're opting out
> of an array declaration.
>
> We're able to retrieve all regs but vregs because the API only fits an
> uint64_t and vregs have 'vlen' size that are bigger than that.

Hi; Coverity complains about these functions
(CID 161401, CID 1641393):

> +static bool reg_is_ulong_integer(CPURISCVState *env, const char *name,
> +                                 target_ulong *val, bool is_gprh)
> +{
> +    const char * const *reg_names;
> +    target_ulong *vals;
> +
> +    if (is_gprh) {
> +        reg_names = riscv_int_regnamesh;
> +        vals = env->gprh;
> +    } else {
> +        reg_names = riscv_int_regnames;
> +        vals = env->gpr;
> +    }
> +
> +    for (int i = 0; i < 32; i++) {
> +        g_autofree char *reg_name = g_strdup(reg_names[i]);
> +        char *reg1 = strtok(reg_name, "/");
> +        char *reg2 = strtok(NULL, "/");
> +
> +        if (strcasecmp(reg1, name) == 0 ||

Coverity complains because we call strtok() to get reg1,
and that might return NULL, but strcasecmp() wants a
non-NULL pointer.

Similarly with the use of strtok() in reg_is_u64_fpu().

We could fix this with an assert(reg1) since the
names are compile-time fixed and it would be an error
for the string to be empty.

But taking a step back, strtok() isn't thread safe.
Maybe we should use g_strsplit() instead ?

More speculatively, do we need to care about locale here?
strcasecmp() does a locale-aware comparison, which is
probably not what we want. (Notoriously Turkish locales
don't have the upper/lowercase mapping you would expect
for "i" and "I".) glib has a g_ascii_strcasecmp() which
might be helpful here.

> +            (reg2 && strcasecmp(reg2, name) == 0)) {
> +            *val = vals[i];
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}

thanks
-- PMM
Re: [PULL 04/26] target/riscv: implement MonitorDef HMP API
Posted by Daniel Henrique Barboza 5 days, 4 hours ago

On 10/21/25 12:23 PM, Peter Maydell wrote:
> On Fri, 3 Oct 2025 at 04:29, <alistair23@gmail.com> wrote:
>>
>> From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>
>> The MonitorDef API is related to two HMP monitor commands: 'p' and 'x':
>>
>> (qemu) help p
>> print|p /fmt expr -- print expression value (use $reg for CPU register access)
>> (qemu) help x
>> x /fmt addr -- virtual memory dump starting at 'addr'
>>
>> For x86, one of the few targets that implements it, it is possible to
>> print the PC register value with $pc and use the PC value in the 'x'
>> command as well.
>>
>> Those 2 commands are hooked into get_monitor_def(), called by
>> exp_unary() in hmp.c. The function tries to fetch a reg value in two
>> ways: by reading them directly via a target_monitor_defs array or using
>> a target_get_monitor_def() helper. In RISC-V we have *A LOT* of
>> registers and this number will keep getting bigger, so we're opting out
>> of an array declaration.
>>
>> We're able to retrieve all regs but vregs because the API only fits an
>> uint64_t and vregs have 'vlen' size that are bigger than that.
> 
> Hi; Coverity complains about these functions
> (CID 161401, CID 1641393):
> 
>> +static bool reg_is_ulong_integer(CPURISCVState *env, const char *name,
>> +                                 target_ulong *val, bool is_gprh)
>> +{
>> +    const char * const *reg_names;
>> +    target_ulong *vals;
>> +
>> +    if (is_gprh) {
>> +        reg_names = riscv_int_regnamesh;
>> +        vals = env->gprh;
>> +    } else {
>> +        reg_names = riscv_int_regnames;
>> +        vals = env->gpr;
>> +    }
>> +
>> +    for (int i = 0; i < 32; i++) {
>> +        g_autofree char *reg_name = g_strdup(reg_names[i]);
>> +        char *reg1 = strtok(reg_name, "/");
>> +        char *reg2 = strtok(NULL, "/");
>> +
>> +        if (strcasecmp(reg1, name) == 0 ||
> 
> Coverity complains because we call strtok() to get reg1,
> and that might return NULL, but strcasecmp() wants a
> non-NULL pointer.
> 
> Similarly with the use of strtok() in reg_is_u64_fpu().
> 
> We could fix this with an assert(reg1) since the
> names are compile-time fixed and it would be an error
> for the string to be empty.
> 
> But taking a step back, strtok() isn't thread safe.
> Maybe we should use g_strsplit() instead ?
> 
> More speculatively, do we need to care about locale here?
> strcasecmp() does a locale-aware comparison, which is
> probably not what we want. (Notoriously Turkish locales
> don't have the upper/lowercase mapping you would expect
> for "i" and "I".) glib has a g_ascii_strcasecmp() which
> might be helpful here.

I don't mind doing both changes. I'll see if things are still working as
expected after doing them and send a fix.

And thanks for the analysis and heads up! I saw the Coverity Scan email in
my inbox today and haven't opened it yet. Seems like I'm on the hook for
CID 161401 and CID 1641393.


Daniel

> 
>> +            (reg2 && strcasecmp(reg2, name) == 0)) {
>> +            *val = vals[i];
>> +            return true;
>> +        }
>> +    }
>> +
>> +    return false;
>> +}
> 
> thanks
> -- PMM