Re: [RFC PATCH-for-9.1 14/21] system: Introduce QMP generic_query_cpu_definitions()

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Each target use a common template for qmp_query_cpu_definitions().
>
> Extract it as generic_query_cpu_definitions(), keeping the
> target-specific implementations as the following SysemuCPUOps
> handlers:
>  - cpu_list_compare()
>  - add_definition()
>  - add_alias_definitions()
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  MAINTAINERS                           |  2 +
>  include/hw/core/sysemu-cpu-ops.h      | 14 ++++++
>  include/qapi/commands-target-compat.h | 14 ++++++
>  system/cpu-qmp-cmds.c                 | 71 +++++++++++++++++++++++++++
>  system/meson.build                    |  1 +
>  5 files changed, 102 insertions(+)
>  create mode 100644 include/qapi/commands-target-compat.h
>  create mode 100644 system/cpu-qmp-cmds.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index af27490243..39d7c14d98 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -148,6 +148,7 @@ M: Richard Henderson <richard.henderson@linaro.org>
>  R: Paolo Bonzini <pbonzini@redhat.com>
>  S: Maintained
>  F: system/cpus.c
> +F: system/cpu-qmp-cmds.c
>  F: system/cpu-qom-helpers.c
>  F: system/watchpoint.c
>  F: cpu-common.c
> @@ -1894,6 +1895,7 @@ F: qapi/machine-target.json
>  F: include/hw/boards.h
>  F: include/hw/core/cpu.h
>  F: include/hw/cpu/cluster.h
> +F: include/qapi/commands-target-compat.h
>  F: include/sysemu/numa.h
>  F: tests/unit/test-smp-parse.c
>  T: git https://gitlab.com/ehabkost/qemu.git machine-next
> diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
> index 24d003fe04..2173226e97 100644
> --- a/include/hw/core/sysemu-cpu-ops.h
> +++ b/include/hw/core/sysemu-cpu-ops.h
> @@ -11,6 +11,7 @@
>  #define SYSEMU_CPU_OPS_H
>  
>  #include "hw/core/cpu.h"
> +#include "qapi/qapi-types-machine.h"
>  
>  /*
>   * struct SysemuCPUOps: System operations specific to a CPU class
> @@ -81,6 +82,19 @@ typedef struct SysemuCPUOps {
>       */
>      bool (*virtio_is_big_endian)(CPUState *cpu);
>  
> +    /**
> +     * @cpu_list_compare: Sort alphabetically by type name,
> +     *                    respecting CPUClass::ordering.
> +     */
> +    gint (*cpu_list_compare)(gconstpointer cpu_class_a, gconstpointer cpu_class_b);
> +    /**
> +     * @add_definition: Add the @cpu_class definition to @cpu_list.
> +     */
> +    void (*add_definition)(gpointer cpu_class, gpointer cpu_list);
> +    /**
> +     * @add_alias_definitions: Add CPU alias definitions to @cpu_list.
> +     */
> +    void (*add_alias_definitions)(CpuDefinitionInfoList **cpu_list);
>      /**
>       * @legacy_vmsd: Legacy state for migration.
>       *               Do not use in new targets, use #DeviceClass::vmsd instead.
> diff --git a/include/qapi/commands-target-compat.h b/include/qapi/commands-target-compat.h
> new file mode 100644
> index 0000000000..86d45d8fcc
> --- /dev/null
> +++ b/include/qapi/commands-target-compat.h
> @@ -0,0 +1,14 @@
> +/*
> + * QAPI helpers for target specific QMP commands
> + *
> + * SPDX-FileCopyrightText: 2024 Linaro Ltd.
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#ifndef QAPI_COMPAT_TARGET_H
> +#define QAPI_COMPAT_TARGET_H
> +
> +#include "qapi/qapi-types-machine.h"
> +
> +CpuDefinitionInfoList *generic_query_cpu_definitions(Error **errp);
> +
> +#endif
> diff --git a/system/cpu-qmp-cmds.c b/system/cpu-qmp-cmds.c
> new file mode 100644
> index 0000000000..daeb131159
> --- /dev/null
> +++ b/system/cpu-qmp-cmds.c
> @@ -0,0 +1,71 @@
> +/*
> + * QAPI helpers for target specific QMP commands
> + *
> + * SPDX-FileCopyrightText: 2024 Linaro Ltd.
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qom/object.h"
> +#include "qapi/commands-target-compat.h"
> +#include "sysemu/arch_init.h"
> +#include "hw/core/cpu.h"
> +#include "hw/core/sysemu-cpu-ops.h"
> +
> +static void cpu_common_add_definition(gpointer data, gpointer user_data)
> +{
> +    ObjectClass *oc = data;
> +    CpuDefinitionInfoList **cpu_list = user_data;
> +    CpuDefinitionInfo *info;
> +    const char *typename;
> +
> +    typename = object_class_get_name(oc);
> +    info = g_malloc0(sizeof(*info));
> +    info->name = cpu_model_from_type(typename);
> +    info->q_typename = g_strdup(typename);
> +
> +    QAPI_LIST_PREPEND(*cpu_list, info);
> +}
> +
> +static void arch_add_cpu_definitions(CpuDefinitionInfoList **cpu_list,
> +                                     const char *cpu_typename)
> +{
> +    ObjectClass *oc;
> +    GSList *list;
> +    const struct SysemuCPUOps *ops;
> +
> +    oc = object_class_by_name(cpu_typename);
> +    if (!oc) {
> +        return;
> +    }
> +    ops = CPU_CLASS(oc)->sysemu_ops;
> +
> +    list = object_class_get_list(cpu_typename, false);
> +    if (ops->cpu_list_compare) {
> +        list = g_slist_sort(list, ops->cpu_list_compare);
> +    }
> +    g_slist_foreach(list, ops->add_definition ? : cpu_common_add_definition,
> +                    cpu_list);
> +    g_slist_free(list);
> +
> +    if (ops->add_alias_definitions) {
> +        ops->add_alias_definitions(cpu_list);
> +    }
> +}
> +
> +CpuDefinitionInfoList *generic_query_cpu_definitions(Error **errp)
> +{
> +    CpuDefinitionInfoList *cpu_list = NULL;
> +
> +    for (unsigned i = 0; i <= QEMU_ARCH_BIT_LAST; i++) {
> +        const char *cpu_typename;
> +
> +        cpu_typename = cpu_typename_by_arch_bit(i);
> +        if (!cpu_typename) {
> +            continue;
> +        }
> +        arch_add_cpu_definitions(&cpu_list, cpu_typename);
> +    }
> +
> +    return cpu_list;
> +}

The target-specific qmp_query_cpu_definitions() this is going to replace
each execute the equivalent of *one* loop iteration: the one
corresponding to their own arch bit.

For the replacement to be faithful, as cpu_typename_by_arch_bit() must
return non-null exactly once.

This is the case for the qemu-system-TARGET.  The solution feels
overengineered there.

I figure cpu_typename_by_arch_bit() will return non-null multiple times
in a future single binary supporting heterogeneous machines.

Such a single binary then can't serve as drop-in replacement for the
qemu-system-TARGET, because query-cpu-definitions returns more.

To get a drop-in replacement, we'll need additional logic to restrict
the query for the homogeneous use case.

I think this needs to be discussed in the commit message.

Possibly easier: don't loop over the bits, relying on
cpu_typename_by_arch_bit() to select the right one.  Instead get the
right bit from somewhere.

We can switch to a loop when we need it for the heterogeneous case.

> diff --git a/system/meson.build b/system/meson.build
> index c6ee97e3b2..dd78caa9b7 100644
> --- a/system/meson.build
> +++ b/system/meson.build
> @@ -10,6 +10,7 @@ system_ss.add(files(
>    'balloon.c',
>    'bootdevice.c',
>    'cpus.c',
> +  'cpu-qmp-cmds.c',
>    'cpu-qom-helpers.c',
>    'cpu-throttle.c',
>    'cpu-timers.c',