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

[Markus Armbruster]

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 :)