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 <cfont...@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 >>> https://lore.kernel.org/qemu-devel/20201118220750.gp1509...@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