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

[Claudio Fontana]

Hi Eduardo,

On 11/20/20 8:00 PM, Eduardo Habkost wrote:
> On Fri, Nov 20, 2020 at 07:47:11PM +0100, Claudio Fontana wrote:
>> On 11/20/20 6:44 PM, Eduardo Habkost wrote:
>>> On Fri, Nov 20, 2020 at 03:49:09PM +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 link to the accel class is set in the X86CPUClass classes
>>>> at MODULE_INIT_ACCEL_CPU time, when the accelerator is known.
>>>>
>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>> [...]
>>>> +static void hvf_cpu_accel_class_init(ObjectClass *oc, void *data)
>>>> +{
>>>> +    X86CPUAccelClass *acc = X86_CPU_ACCEL_CLASS(oc);
>>>> +
>>>> +    acc->cpu_realizefn = host_cpu_realizefn;
>>>> +    acc->cpu_common_class_init = hvf_cpu_common_class_init;
>>>> +    acc->cpu_instance_init = hvf_cpu_instance_init;
>>>> +};
>>>> +static const TypeInfo hvf_cpu_accel_type_info = {
>>>> +    .name = X86_CPU_ACCEL_TYPE_NAME("hvf"),
>>>> +
>>>> +    .parent = TYPE_X86_CPU_ACCEL,
>>>> +    .class_init = hvf_cpu_accel_class_init,
>>>> +};
>>>> +static void hvf_cpu_accel_register_types(void)
>>>> +{
>>>> +    type_register_static(&hvf_cpu_accel_type_info);
>>>> +}
>>>> +type_init(hvf_cpu_accel_register_types);
>>>> +
>>>> +static void hvf_cpu_accel_init(void)
>>>> +{
>>>> +    if (hvf_enabled()) {
>>>> +        x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("hvf"));
>>>> +    }
>>>> +}
>>>> +
>>>> +accel_cpu_init(hvf_cpu_accel_init);
>>>
>>> The point of my suggestion of using QOM is to not require
>>> separate accel_cpu_init() functions and (hvf|tcg|kvm)_enabled()
>>> checks.
>>>
>>> If we still have separate accel_cpu_init() functions for calling
>>> x86_cpu_accel_init() with the right argument, using a pointer to
>>> static variables like &hvf_cpu_accel (like you did before) was
>>> simpler and required less boilerplate code.
>>
>>
>>
>> Yes I agree.
>>
>>
>>
>>
>>>
>>> However, the difference is that with the X86_CPU_ACCEL_TYPE_NAME
>>> macro + object_class_by_name(), you don't need the separate
>>> accel_cpu_init() functions for each accelerator.
>>>
>>> All you need is a single:
>>>
>>>   x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME(chosen_accel_name));
>>>
>>> call somewhere in the initialization path.
>>
>>
>> Makes sense. The problem is just determining chosen_accel_name.
> 
> Yeah, that was a challenge.  do_configure_accelerator() knows
> what's the chosen accel name, though.
> 
> We can also do it inside accel_init_machine(), if we can
> determine the correct accel name from the AccelState object.



I think that the fact that we cannot answer really simple questions like

"what is the selected cpu model? what is the selected accelerator?"

in a QEMU mode-independent way, easily, with no ifs and buts,
with our current codebase should be giving us a bit of pause.

It is my hope that in the future we will try to draw some synthesis from
all the different frameworks and systems we have in QEMU.

> 
>>
>>
>>>
>>> A good place for the x86_cpu_accel_init() call would be
>>> do_configure_accelerator(), but the function is arch-specific.
>>> That's why I suggested a cpu_accel_arch_init() function at
>>> 20201118220750.GP1509407@habkost.net">https://lore.kernel.org/qemu-devel/20201118220750.GP1509407@habkost.net
>>>
>>
>>
>> Fine by me. I'd use a specific init step for this, but that also works.
> 
> A separate module init function has no easy access to the accel
> name, but in this case I'd say it's on purpose: the intended use
> case for module init functions is to unconditionally register
> features provided by a code module.  They shouldn't look at any
> runtime configuration or runtime state.
> 

Ok, I'll take this up as a requirement for the next attempt.

Thank you!

CLaudio