Re: [PATCH 1/3] softmmu/qtest: Move the target-specific pseries RTAS code out of qtest.c

[Cédric Le Goater]

On 4/11/23 20:34, Thomas Huth wrote:
> Ideally, qtest.c should be independent from target specific code, so
> we only have to compile it once for all targets. Thus start improving
> the situation by moving the pseries related code to hw/ppc/spapr_rtas.c
> instead and allow target code to register a callback handler for such
> target specific commands.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

That #ifdef CONFIG_PSERIES was really ugly.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>   include/sysemu/qtest.h |  4 ++++
>   hw/ppc/spapr_rtas.c    | 29 +++++++++++++++++++++++++++++
>   softmmu/qtest.c        | 39 +++++++++++++--------------------------
>   3 files changed, 46 insertions(+), 26 deletions(-)
>
> diff --git a/include/sysemu/qtest.h b/include/sysemu/qtest.h
> index 4c53537ef3..85f05b0e46 100644
> --- a/include/sysemu/qtest.h
> +++ b/include/sysemu/qtest.h
> @@ -14,6 +14,7 @@
>   #ifndef QTEST_H
>   #define QTEST_H
>   
> +#include "chardev/char.h"
>   
>   extern bool qtest_allowed;
>   
> @@ -22,6 +23,9 @@ static inline bool qtest_enabled(void)
>       return qtest_allowed;
>   }
>   
> +void qtest_send_prefix(CharBackend *chr);
> +void G_GNUC_PRINTF(2, 3) qtest_sendf(CharBackend *chr, const char *fmt, ...);
> +void qtest_set_command_cb(bool (*pc_cb)(CharBackend *chr, gchar **words));
>   bool qtest_driver(void);
>   
>   void qtest_server_init(const char *qtest_chrdev, const char *qtest_log, Error **errp);
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 3f664ea02c..7df21581c2 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -33,6 +33,7 @@
>   #include "sysemu/cpus.h"
>   #include "sysemu/hw_accel.h"
>   #include "sysemu/runstate.h"
> +#include "sysemu/qtest.h"
>   #include "kvm_ppc.h"
>   
>   #include "hw/ppc/spapr.h"
> @@ -548,6 +549,32 @@ uint64_t qtest_rtas_call(char *cmd, uint32_t nargs, 
> uint64_t args,
>       return H_PARAMETER;
>   }
>   
> +static bool spapr_qtest_callback(CharBackend *chr, gchar **words)
> +{
> +    if (strcmp(words[0], "rtas") == 0) {
> +        uint64_t res, args, ret;
> +        unsigned long nargs, nret;
> +        int rc;
> +
> +        rc = qemu_strtoul(words[2], NULL, 0, &nargs);
> +        g_assert(rc == 0);
> +        rc = qemu_strtou64(words[3], NULL, 0, &args);
> +        g_assert(rc == 0);
> +        rc = qemu_strtoul(words[4], NULL, 0, &nret);
> +        g_assert(rc == 0);
> +        rc = qemu_strtou64(words[5], NULL, 0, &ret);
> +        g_assert(rc == 0);
> +        res = qtest_rtas_call(words[1], nargs, args, nret, ret);
> +
> +        qtest_send_prefix(chr);
> +        qtest_sendf(chr, "OK %"PRIu64"\n", res);
> +
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
>   void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
>   {
>       assert((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX));
> @@ -630,6 +657,8 @@ static void core_rtas_register_types(void)
>                           rtas_ibm_nmi_register);
>       spapr_rtas_register(RTAS_IBM_NMI_INTERLOCK, "ibm,nmi-interlock",
>                           rtas_ibm_nmi_interlock);
> +
> +    qtest_set_command_cb(spapr_qtest_callback);
>   }
>   
>   type_init(core_rtas_register_types)
> diff --git a/softmmu/qtest.c b/softmmu/qtest.c
> index 34bd2a33a7..76cbb8bcee 100644
> --- a/softmmu/qtest.c
> +++ b/softmmu/qtest.c
> @@ -29,10 +29,6 @@
>   #include "qemu/module.h"
>   #include "qemu/cutils.h"
>   #include "qom/object_interfaces.h"
> -#include CONFIG_DEVICES
> -#ifdef CONFIG_PSERIES
> -#include "hw/ppc/spapr_rtas.h"
> -#endif
>   
>   #define MAX_IRQ 256
>   
> @@ -263,7 +259,7 @@ static int hex2nib(char ch)
>       }
>   }
>   
> -static void qtest_send_prefix(CharBackend *chr)
> +void qtest_send_prefix(CharBackend *chr)
>   {
>       if (!qtest_log_fp || !qtest_opened) {
>           return;
> @@ -302,8 +298,7 @@ static void qtest_send(CharBackend *chr, const char *str)
>       qtest_server_send(qtest_server_send_opaque, str);
>   }
>   
> -static void G_GNUC_PRINTF(2, 3) qtest_sendf(CharBackend *chr,
> -                                           const char *fmt, ...)
> +void qtest_sendf(CharBackend *chr, const char *fmt, ...)
>   {
>       va_list ap;
>       gchar *buffer;
> @@ -361,6 +356,15 @@ static void qtest_clock_warp(int64_t dest)
>       qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
>   }
>   
> +static bool (*process_command_cb)(CharBackend *chr, gchar **words);
> +
> +void qtest_set_command_cb(bool (*pc_cb)(CharBackend *chr, gchar **words))
> +{
> +    assert(!process_command_cb);  /* Switch to a list if we need more than one 
> */
> +
> +    process_command_cb = pc_cb;
> +}
> +
>   static void qtest_process_command(CharBackend *chr, gchar **words)
>   {
>       const gchar *command;
> @@ -717,25 +721,6 @@ static void qtest_process_command(CharBackend *chr, gchar 
> **words)
>           qtest_sendf(chr, "OK big\n");
>   #else
>           qtest_sendf(chr, "OK little\n");
> -#endif
> -#ifdef CONFIG_PSERIES
> -    } else if (strcmp(words[0], "rtas") == 0) {
> -        uint64_t res, args, ret;
> -        unsigned long nargs, nret;
> -        int rc;
> -
> -        rc = qemu_strtoul(words[2], NULL, 0, &nargs);
> -        g_assert(rc == 0);
> -        rc = qemu_strtou64(words[3], NULL, 0, &args);
> -        g_assert(rc == 0);
> -        rc = qemu_strtoul(words[4], NULL, 0, &nret);
> -        g_assert(rc == 0);
> -        rc = qemu_strtou64(words[5], NULL, 0, &ret);
> -        g_assert(rc == 0);
> -        res = qtest_rtas_call(words[1], nargs, args, nret, ret);
> -
> -        qtest_send_prefix(chr);
> -        qtest_sendf(chr, "OK %"PRIu64"\n", res);
>   #endif
>       } else if (qtest_enabled() && strcmp(words[0], "clock_step") == 0) {
>           int64_t ns;
> @@ -777,6 +762,8 @@ static void qtest_process_command(CharBackend *chr, gchar 
> **words)
>           qtest_send_prefix(chr);
>           qtest_sendf(chr, "OK %"PRIi64"\n",
>                       (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> +    } else if (process_command_cb && process_command_cb(chr, words)) {
> +        /* Command got consumed by the callback handler */
>       } else {
>           qtest_send_prefix(chr);
>           qtest_sendf(chr, "FAIL Unknown command '%s'\n", words[0]);