Re: [RFC v3 9/9] i386: split cpu accelerators from cpu.c

[Claudio Fontana]

On 11/23/20 7:24 PM, Eduardo Habkost wrote:
> On Fri, Nov 20, 2020 at 10:08:46AM +0100, Claudio Fontana wrote:
>> On 11/19/20 8:23 PM, Eduardo Habkost wrote:
>>> On Thu, Nov 19, 2020 at 09:53:09AM +0100, Claudio Fontana wrote:
>>>> Hi,
>>>>
>>>> On 11/18/20 7:28 PM, Eduardo Habkost wrote:
>>>>> On Wed, Nov 18, 2020 at 11:29:36AM +0100, Claudio Fontana wrote:
>>>>>> split cpu.c into:
>>>>>>
>>>>>> cpu.c            cpuid and common x86 cpu functionality
>>>>>> host-cpu.c       host x86 cpu functions and "host" cpu type
>>>>>> kvm/cpu.c        KVM x86 cpu type
>>>>>> hvf/cpu.c        HVF x86 cpu type
>>>>>> tcg/cpu.c        TCG x86 cpu type
>>>>>>
>>>>>> The accel interface of the X86CPUClass is set at MODULE_INIT_ACCEL_CPU
>>>>>> time, when the accelerator is known.
>>>>>>
>>>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>>>>> ---
>>>>> [...]
>>>>>> +/**
>>>>>> + * X86CPUAccel:
>>>>>> + * @name: string name of the X86 CPU Accelerator
>>>>>> + *
>>>>>> + * @common_class_init: initializer for the common cpu
>>>>>
>>>>> So this will be called for every single CPU class.
>>>>
>>>> Not really, it's called for every TYPE_X86_CPU cpu class (if an accel 
>>>> interface is registered).
>>>
>>> This means every single non-abstract CPU class in
>>> qemu-system-x86_64, correct?
>>>
>>>>
>>>> This function extends the existing x86_cpu_common_class_init 
>>>> (target/i386/cpu.c),
>>>> where some methods of the base class CPUClass are set.
>>>>
>>>>>
>>>>>> + * @instance_init: cpu instance initialization
>>>>>> + * @realizefn: realize function, called first in x86 cpu realize
>>>>>> + *
>>>>>> + * X86 CPU accelerator-specific CPU initializations
>>>>>> + */
>>>>>> +
>>>>>> +struct X86CPUAccel {
>>>>>> +    const char *name;
>>>>>> +
>>>>>> +    void (*common_class_init)(X86CPUClass *xcc);
>>>>>> +    void (*instance_init)(X86CPU *cpu);
>>>>>> +    void (*realizefn)(X86CPU *cpu, Error **errp);
>>>>>>  };
>>>>>>  
>>>>>> +void x86_cpu_accel_init(const X86CPUAccel *accel);
>>>>> [...]
>>>>>> +static void x86_cpu_accel_init_aux(ObjectClass *klass, void *opaque)
>>>>>> +{
>>>>>> +    X86CPUClass *xcc = X86_CPU_CLASS(klass);
>>>>>> +    const X86CPUAccel **accel = opaque;
>>>>>> +
>>>>>> +    xcc->accel = *accel;
>>>>>> +    xcc->accel->common_class_init(xcc);
>>>>>> +}
>>>>>> +
>>>>>> +void x86_cpu_accel_init(const X86CPUAccel *accel)
>>>>>> +{
>>>>>> +    object_class_foreach(x86_cpu_accel_init_aux, TYPE_X86_CPU, false, 
>>>>>> &accel);
>>>>>> +}
>>>>>
>>>>> This matches the documented behavior.
>>>>>
>>>>> [...]
>>>>>> +void host_cpu_class_init(X86CPUClass *xcc)
>>>>>> +{
>>>>>> +    xcc->host_cpuid_required = true;
>>>>>> +    xcc->ordering = 8;
>>>>>> +    xcc->model_description =
>>>>>> +        g_strdup_printf("%s processor with all supported host features 
>>>>>> ",
>>>>>> +                        xcc->accel->name);
>>>>>> +}
>>>>> [...]
>>>>>> +static void hvf_cpu_common_class_init(X86CPUClass *xcc)
>>>>>> +{
>>>>>> +    host_cpu_class_init(xcc);
>>>>>
>>>>> Why are you calling host_cpu_class_init() for all CPU types?
>>>>
>>>> I am not..
>>>
>>> I don't get it.  You are calling host_cpu_class_init() for every
>>> single non-abstract TYPE_X86_CPU subclass (which includes all CPU
>>> models in qemu-system-x86_64), and I don't understand why, or if
>>> this is really intentional.
>>
>> It is really intentional what is done here,
>>
>> when HVF accelerator is enabled, and only when the HVF accelerator is 
>> enabled,
>>
>> all X86 CPU classes and subclasses (cpu models, which have been
>> implemented as subclasses of TYPE_X86_CPU), are updated with a
>> link to the accelerator-specific HVF interface.
> 
> I'm confused.  That (updating the class with a link) is not what
> host_cpu_class_init() does.
> 
> This is what host_cpu_class_init() does:
> 
> void host_cpu_class_init(X86CPUClass *xcc)
> {
>     xcc->host_cpuid_required = true;
>     xcc->ordering = 8;
>     xcc->model_description =
>         g_strdup_printf("%s processor with all supported host features ",
>                         xcc->accel->name);
> }
> 
> Why do you want to call host_cpu_class_init() for every non-abstract
> TYPE_X86_CPU subclass?
> 

Only now I got your point, sorry for not seeing it before. It is clearly a 
mistake, this should be referenced by the host cpu type!

Thanks a lot!

Claudio