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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 11/9/23 01:28, Gavin Shan wrote:
> Hi Philippe,
> 
> 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.

> Besides, I guess the changes can be 
> squeezed into two
> patches (commits) as below:
> 
> PATCH[1] target/alpha: Tidy up alpha_cpu_class_by_name()
> PATCH[2] Move the checks (oc == NULL || object_class_is_abstract() || 
> !object_class_dynamic_cast())
>           from individual targets to 
> hw/core/cpu-common.c::cpu_class_by_name()
> 
> I rebase my series of '[PATCH v3 00/32] Unified CPU type check' on top 
> of yours. Please
> let me know if I need to include your patch into my v4 series for 
> review. In that case,
> I can include your patches with above changes applied.
> 
> Thanks,
> Gavin
> 
>> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
>> index c6a0c9390c..2d24261a6a 100644
>> --- a/hw/core/cpu-common.c
>> +++ b/hw/core/cpu-common.c
>> @@ -155,7 +155,7 @@ ObjectClass *cpu_class_by_name(const char 
>> *typename, const char *cpu_model)
>>       assert(cpu_model);
>>       oc = object_class_by_name(typename);
>>       cc = CPU_CLASS(oc);
>> -    assert(cc->class_by_name);
>> +    assert(cc->cpu_resolving_type && cc->class_by_name);
>>       oc = cc->class_by_name(cpu_model);
>>       if (oc == NULL || object_class_is_abstract(oc)) {
>>           return NULL;