Re: [RFC PATCH v2 1/2] Add RISC-V CSR qtest support

[Thomas Huth <thuth@redhat.com>]

On 18/06/2024 08.44, Ivan Klokov wrote:
> The RISC-V architecture supports the creation of custom
> CSR-mapped devices. It would be convenient to test them in the same way
> as MMIO-mapped devices. To do this, a new call has been added
> to read/write CSR registers.
> 
> Signed-off-by: Ivan Klokov <ivan.klokov@syntacore.com>
> ---
...
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 58ef7079dc..82540ae5dc 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -29,7 +29,7 @@
>   #include "sysemu/cpu-timers.h"
>   #include "qemu/guest-random.h"
>   #include "qapi/error.h"
> -
> +#include "tests/qtest/libqtest.h"
>   
>   /* CSR function table public API */
>   void riscv_get_csr_ops(int csrno, riscv_csr_operations *ops)
> @@ -4549,6 +4549,53 @@ static RISCVException write_jvt(CPURISCVState *env, int csrno,
>       return RISCV_EXCP_NONE;
>   }
>   
> +static uint64_t csr_call(char *cmd, uint64_t cpu_num, int csrno,
> +                                uint64_t *val)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(cpu_by_arch_id(cpu_num));
> +    CPURISCVState *env = &cpu->env;
> +
> +    int ret = RISCV_EXCP_NONE;
> +    if (strcmp(cmd, "get_csr") == 0) {
> +        ret = riscv_csrrw(env, csrno, (target_ulong *)val, 0, 0);
> +
> +    } else if (strcmp(cmd, "set_csr") == 0) {
> +        ret = riscv_csrrw(env, csrno, NULL, *(target_ulong *)val, MAKE_64BIT_MASK(0, TARGET_LONG_BITS));
> +    }
> +
> +    if (ret == RISCV_EXCP_NONE) {
> +        ret = 0;
> +    }

Is there a reason for ignoring errors here? If not, I'd rather replace that 
final if-statement with:

     else {
         g_assert_not_reached();
     }

to make sure that mistakes in setting the right sub-command don't get 
ignored without any error message.

> +    return ret;
> +}
> +
> +bool csr_qtest_callback(CharBackend *chr, gchar **words)
> +{
> +    if (strcmp(words[0], "csr") == 0) {
> +
> +        uint64_t res, cpu;
> +
> +        uint64_t val;
> +        int rc, csr;
> +
> +        rc = qemu_strtou64(words[2], NULL, 0, &cpu);
> +        g_assert(rc == 0);
> +        rc = qemu_strtoi(words[3], NULL, 0, &csr);
> +        g_assert(rc == 0);
> +        rc = qemu_strtou64(words[4], NULL, 0, &val);
> +        g_assert(rc == 0);
> +        res = csr_call(words[1], cpu, csr, &val);
> +
> +        qtest_send_prefix(chr);
> +        qtest_sendf(chr, "OK %"PRIx64" "TARGET_FMT_lx"\n", res, (target_ulong)val);
> +
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
>   /*
>    * Control and Status Register function table
>    * riscv_csr_operations::predicate() must be provided for an implemented CSR
> diff --git a/tests/qtest/libqos/csr.c b/tests/qtest/libqos/csr.c
> new file mode 100644
> index 0000000000..2dc52fc442
> --- /dev/null
> +++ b/tests/qtest/libqos/csr.c
> @@ -0,0 +1,42 @@
> +/*
> + * QTest RISC-V CSR driver
> + *
> + * Copyright (c) 2024 Syntacore
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "../libqtest.h"
> +#include "csr.h"
> +
> +static uint64_t qcsr_call(QTestState *qts, const char *name, uint64_t cpu,
> +                           int csrno, uint64_t *val)
> +{
> +    uint64_t res = 0;
> +
> +    res = qtest_csr_call(qts, name, cpu, csrno, val);
> +
> +    return res;
> +}
> +
> +int qcsr_get_csr(QTestState *qts, uint64_t cpu,
> +        int csrno, uint64_t *val)
> +{
> +    int res;
> +
> +    res = qcsr_call(qts, "get_csr", cpu, csrno, val);
> +
> +    return res;
> +}
> +
> +int qcsr_set_csr(QTestState *qts, uint64_t cpu,
> +        int csrno, uint64_t *val)
> +{
> +    int res;
> +
> +    res = qcsr_call(qts, "set_csr", cpu, csrno, val);
> +
> +    return res;
> +}

Technically, there does not seem to be anything related to libqos in your 
patch set. libqos is a framework for executing tests on various buses, e.g. 
to test PCI devices on various host PCI bus implementations. All that is 
triggered via qos-test.c. Your CSR test does not seem to fit into that 
catogory, so please put that code rather directly in your riscv-csr-test.c 
file instead. (unless you want to use it in a lot of other tests in the 
future, too, then maybe you could move them as static inlines into the csr.h 
header instead).

> diff --git a/tests/qtest/libqos/csr.h b/tests/qtest/libqos/csr.h
> new file mode 100644
> index 0000000000..d953735fe8
> --- /dev/null
> +++ b/tests/qtest/libqos/csr.h

Again, not related to libqos, please move it up to the qtest folder itself.

> @@ -0,0 +1,16 @@
> +/*
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef LIBQOS_CSR_H
> +#define LIBQOS_CSR_H
> +
> +int qcsr_get_csr(QTestState *qts, uint64_t cpu,
> +        int csrno, uint64_t *val);
> +
> +int qcsr_set_csr(QTestState *qts, uint64_t cpu,
> +        int csrno, uint64_t *val);
> +
> +
> +#endif /* LIBQOS_CSR_H */
> diff --git a/tests/qtest/libqos/meson.build b/tests/qtest/libqos/meson.build
> index 558eb4c24b..a944febbd8 100644
> --- a/tests/qtest/libqos/meson.build
> +++ b/tests/qtest/libqos/meson.build
> @@ -25,6 +25,9 @@ libqos_srcs = files(
>           # usb
>           'usb.c',
>   
> +        #riscv csr
> +        'csr.c',
> +
>           # qgraph devices:
>           'e1000e.c',
>           'i2c.c',
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 18e2f7f282..4667d8d873 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -1200,6 +1200,33 @@ uint64_t qtest_rtas_call(QTestState *s, const char *name,
>       return 0;
>   }
>   
> +static void qtest_rsp_csr(QTestState *s, uint64_t *val)
> +{
> +    gchar **args;
> +    uint64_t ret;
> +    int rc;
> +
> +    args = qtest_rsp_args(s, 3);
> +
> +    rc = qemu_strtou64(args[1], NULL, 16, &ret);
> +    g_assert(rc == 0);
> +    rc = qemu_strtou64(args[2], NULL, 16, val);
> +    g_assert(rc == 0);
> +
> +    g_strfreev(args);
> +}
> +
> +uint64_t qtest_csr_call(QTestState *s, const char *name,
> +                         uint64_t cpu, int csr,
> +                         uint64_t *val)
> +{
> +    qtest_sendf(s, "csr %s 0x%"PRIx64" %d 0x%"PRIx64"\n",
> +                    name, cpu, csr, *val);
> +
> +    qtest_rsp_csr(s, val);

Just a matter of taste, but I think I'd rather inline the contents of 
qtest_rsp_csr() here since both functions are not very big yet.
(unless you need qtest_rsp_csr() in another function later, then it's of 
course better to keep it separate)

> +    return 0;
> +}

  Thomas