diff mbox series

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

Message ID 20240315130910.15750-15-philmd@linaro.org (mailing list archive)
State New, archived
Headers show
Series qapi: Make @query-cpu-definitions command target-agnostic | expand

Commit Message

Philippe Mathieu-Daudé March 15, 2024, 1:09 p.m. UTC 
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

Comments

Markus Armbruster March 26, 2024, 12:54 p.m. UTC | #1 
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);

Peeking ahead...  This is used for sorting the subtypes of the CPU type
returned by cpu_typename_by_arch_bit().  Implementing the callback is
optional, and when absent, we don't sort, i.e. we get hash table order.

Worth mentioning it's optional?

> +    /**
> +     * @add_definition: Add the @cpu_class definition to @cpu_list.
> +     */
> +    void (*add_definition)(gpointer cpu_class, gpointer cpu_list);

This one appears to default to cpu_common_add_definition().  Worth
mentioning?

I despise Glib's pointless typedefs for ordinary C types.

> +    /**
> +     * @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

Why "compat"?

> @@ -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;
> +}
> 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',

The commit message made me expect the complete refactoring in this
patch.  In fact, it's just a first step: the new
generic_query_cpu_definitions() remains unused, and no target implements
the new callbacks.  We can worry about this later.

Subsequent patches convert targets to generic_query_cpu_definitions()
one by one.  To convince myself the replacement is faithful, I'll have
to refer back to this patch.  That's fine.
Markus Armbruster March 26, 2024, 1:28 p.m. UTC | #2 
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',
Philippe Mathieu-Daudé March 29, 2024, 1:03 p.m. UTC | #3 
Hi Markus,

On 26/3/24 14:28, Markus Armbruster wrote:
> 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/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.

Can we ask the management layer to provide the current homogeneous
target via argument? Otherwise we can add a new query-cpu-definitions-v2
command requiring an explicit target argument, allowing 'all', and
deprecate the current query-cpu-definitions.

> 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.

Alex suggested to consider heterogeneous emulation the new default,
and the current homogeneous use as a particular case. I'd rather not
plan on a "heterogeneous switch day" and get things integrated in
the way, otherwise we'll never get there...

Regards,

Phil.
Markus Armbruster April 2, 2024, 9:37 a.m. UTC | #4 
Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Hi Markus,
>
> On 26/3/24 14:28, Markus Armbruster wrote:
>> 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/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.
>
> Can we ask the management layer to provide the current homogeneous
> target via argument? Otherwise we can add a new query-cpu-definitions-v2
> command requiring an explicit target argument, allowing 'all', and
> deprecate the current query-cpu-definitions.

Is "new binary can serve as drop-in replacement for the
qemu-system-TARGET" a goal?

I'm asking because a new binary is also an opportunity to improve
things, and backward compatibility conflicts with that.

Where would we want to go if backward compatibility was not a concern?
I guess we'd want a single binary capable of running any machine,
homogeneous or not.

Your query-cpu-definitions would be fine for that binary, as long as its
users can filter out the CPUs they're interested in.

I prefer client-side filtering whenever practical, since it keeps the
interface simple.  We do have to provide clients the information they
need to filter.  How would a client filter the result of your
query-cpu-definitions for target?

Since backward compatibility is a concern, and we don't want to provide
per target binaries forever, we need to think about how to provide
suitable replacements based on the single binary.  I'm not sure drop-in
replacement is feasible, given the complexity of the external
interfaces.

>> 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.
>
> Alex suggested to consider heterogeneous emulation the new default,
> and the current homogeneous use as a particular case. I'd rather not
> plan on a "heterogeneous switch day" and get things integrated in
> the way, otherwise we'll never get there...

I guess it's okay to overengineer certain things so they're ready for
the heterogenous case when it comes.  We may want to explain this in
comments, so people don't simplify the overengineered code :)
diff mbox series

Patch

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;
+}
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',