Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field

[Gavin Shan <gshan@redhat.com>]

Hi Philippe,

On 9/12/23 08:40, Gavin Shan wrote:
> On 9/11/23 19:43, Philippe Mathieu-Daudé wrote:
>> On 11/9/23 01:28, Gavin Shan wrote:
>>> On 9/8/23 21:22, Philippe Mathieu-Daudé wrote:
>>>> Add a field to return the QOM type name of a CPU class.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>   include/hw/core/cpu.h   | 2 ++
>>>>   hw/core/cpu-common.c    | 2 +-
>>>>   target/alpha/cpu.c      | 1 +
>>>>   target/arm/cpu.c        | 1 +
>>>>   target/avr/cpu.c        | 1 +
>>>>   target/cris/cpu.c       | 1 +
>>>>   target/hexagon/cpu.c    | 1 +
>>>>   target/hppa/cpu.c       | 1 +
>>>>   target/i386/cpu.c       | 1 +
>>>>   target/loongarch/cpu.c  | 1 +
>>>>   target/m68k/cpu.c       | 1 +
>>>>   target/microblaze/cpu.c | 1 +
>>>>   target/mips/cpu.c       | 1 +
>>>>   target/nios2/cpu.c      | 1 +
>>>>   target/openrisc/cpu.c   | 1 +
>>>>   target/ppc/cpu_init.c   | 1 +
>>>>   target/riscv/cpu.c      | 1 +
>>>>   target/rx/cpu.c         | 1 +
>>>>   target/s390x/cpu.c      | 1 +
>>>>   target/sh4/cpu.c        | 1 +
>>>>   target/sparc/cpu.c      | 1 +
>>>>   target/tricore/cpu.c    | 1 +
>>>>   target/xtensa/cpu.c     | 1 +
>>>>   23 files changed, 24 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>>>> index 129d179937..e469efd409 100644
>>>> --- a/include/hw/core/cpu.h
>>>> +++ b/include/hw/core/cpu.h
>>>> @@ -100,6 +100,7 @@ struct SysemuCPUOps;
>>>>   /**
>>>>    * CPUClass:
>>>> + * @cpu_resolving_type: CPU QOM type name
>>>>    * @class_by_name: Callback to map -cpu command line model name to an
>>>>    *                 instantiatable CPU type.
>>>>    * @parse_features: Callback to parse command line arguments.
>>>> @@ -148,6 +149,7 @@ struct CPUClass {
>>>>       DeviceClass parent_class;
>>>>       /*< public >*/
>>>> +    const char *cpu_resolving_type;
>>>>       ObjectClass *(*class_by_name)(const char *cpu_model);
>>>>       void (*parse_features)(const char *typename, char *str, Error **errp);
>>>
>>> The question is why not use CPU_RESOLVING_TYPE directly? It seems CPU_RESOLVING_TYPE
>>> is exactly what you want here.
>>
>> Unfortunately CPU_RESOLVING_TYPE is target-specific, we want
>> hw/core/cpu-common.c to be target-agnostic (build once for all
>> targets). This is particularly important in the context of
>> heterogeneous QEMU, where a single binary will be able to create
>> CPUs from different targets.
>>
> 
> CPU_RESOLVING_TYPE and CPUClass::cpu_resolving_type is duplicate to
> each other. There are two options I can figure out to avoid the
> duplication.
> 
> (a) move cpu_class_by_name() from hw/core/cpu-common.c to cpu.c, so that
>      CPU_RESOLVING_TYPE can be seen. cpu.c::list_cpus() is the example.
> 
> (b) remove hw/core/cpu-common.c::cpu_calss_by_name() and squeeze its
>      logic to cpu.c::parse_cpu_option() since there are not too much
>      users for it. target/arm and target/s390 needs some tweaks so that
>      hw/core/cpu-common.c::cpu_calss_by_name() can be removed.
> 
>      [gshan@gshan q]$ git grep \ cpu_class_by_name\(
>      cpu.c:    oc = cpu_class_by_name(CPU_RESOLVING_TYPE, model_pieces[0]);
>      target/arm/arm-qmp-cmds.c:    oc = cpu_class_by_name(TYPE_ARM_CPU, model->name);
>      target/s390x/cpu_models_sysemu.c:    oc = cpu_class_by_name(TYPE_S390_CPU, info->name);
> 
> When option (b) is taken, this series to have the checks against @oc
> in hw/core/cpu-common.c::cpu_calss_by_name() becomes non-sense. Instead,
> we need the same (and complete) checks in CPUClass::class_by_name() for
> individual targets. Further more, an inline helper can be provided to do
> the check in CPUClass::class_by_name() for individual targets.
> 
>     include/hw/core/cpu.h
> 
>     static inline bool cpu_class_is_valid(ObjectClass *oc, const char *parent)
>     {
>         if (!object_class_dynamic_cast(oc, parent) ||
>             object_class_is_abstract(oc)) {
>             return false;
>         }
> 
>         return true;
>     }
> 

Since my series to make CPU type check unified depends on this series, could
you please share your thoughts? If you don't have bandwidth for this, I can
improve the code based on your thoughts, and include your patches to my series
so that they can be reviewed at once. Please just let me know.

Thanks,
Gavin