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

[Gavin Shan <gshan@redhat.com>]

Hi Philippe,

On 10/11/23 13:28, Philippe Mathieu-Daudé wrote:
> 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.
> 

Ah, nope, nothing went to wrong here. Thanks for your reply, I will try
to follow (a) and integrate your patches to my series. Please help to
review my series when it's posted :)

Thanks,
Gavin