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

[Claudio Fontana]

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.


> 
>>
>>>
>>>> +}
>>> [...]
>>>> +static void kvm_cpu_common_class_init(X86CPUClass *xcc)
>>>> +{
>>>> +    host_cpu_class_init(xcc);
>>>> +}
>>>
>>> Same question as above.
>>>
>>
>> Ciao,
>>
>> Claudio
>>
>