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. > > > > > > 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. -- Eduardo