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

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

Hi Gavin,

On 25/9/23 02:24, Gavin Shan wrote:
> 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>
>>>>> ---


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

You seem to prove (b) is not useful, so we have to do (a).

Unfortunately at this moment I feel hopeless with this topic.

I don't want to delay your work further. If you find a way to integrate
both series, please go ahead. Otherwise let's drop my approach and
continue with your previous work.

I apologize I kept you waiting that long.

Regards,

Phil.