On Fri, Nov 27, 2020 at 06:53:23PM +0100, Claudio Fontana wrote: > On 11/27/20 6:41 PM, Eduardo Habkost wrote: > > On Thu, Nov 26, 2020 at 11:32:18PM +0100, Claudio Fontana wrote: > >> i386 is the first user of AccelCPUClass, allowing to 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 AccelCPUClass > >> hvf/cpu.c HVF x86 AccelCPUClass > >> tcg/cpu.c TCG x86 AccelCPUClass > >> > >> Signed-off-by: Claudio Fontana <cfont...@suse.de> > >> --- > > [...] > >> +static void tcg_cpu_class_init(CPUClass *cc) > > > > Is this the only case where we need to provide an > > AccelCPUClass.cpu_class_init method? > > > > > >> +{ > >> + cc->do_interrupt = x86_cpu_do_interrupt; > >> + cc->cpu_exec_interrupt = x86_cpu_exec_interrupt; > >> + cc->synchronize_from_tb = x86_cpu_synchronize_from_tb; > >> + cc->cpu_exec_enter = x86_cpu_exec_enter; > >> + cc->cpu_exec_exit = x86_cpu_exec_exit; > >> + cc->tcg_initialize = tcg_x86_init; > >> + cc->tlb_fill = x86_cpu_tlb_fill; > >> +#ifndef CONFIG_USER_ONLY > >> + cc->debug_excp_handler = breakpoint_handler; > >> +#endif /* !CONFIG_USER_ONLY */ > > > > I find the need for these method overrides suspicious. > > These mechanisms are preexistent. My refactoring only makes them more visible. > > > > > Comparing this with the code on qemu.git master: > > > > static void x86_cpu_common_class_init(ObjectClass *oc, void *data) > > { > > [...] > > #ifdef CONFIG_TCG > > cc->do_interrupt = x86_cpu_do_interrupt; > > cc->cpu_exec_interrupt = x86_cpu_exec_interrupt; > > #endif > > [...] > > cc->synchronize_from_tb = x86_cpu_synchronize_from_tb; > > [...] > > #if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY) > > cc->debug_excp_handler = breakpoint_handler; > > #endif > > cc->cpu_exec_enter = x86_cpu_exec_enter; > > cc->cpu_exec_exit = x86_cpu_exec_exit; > > #ifdef CONFIG_TCG > > cc->tcg_initialize = tcg_x86_init; > > cc->tlb_fill = x86_cpu_tlb_fill; > > #endif > > [...] > > } > > > > So, we have two kinds of CPUClass fields above: > > * Code that was never conditional on CONFIG_TCG, and now is > > conditional (synchronize_from_tb, cpu_exec_enter, > > cpu_exec_exit). > > > > synchronize_from_tb, cpu_exec_enter and cpu_exec_exit only makes sense for > TCG, > and their code should not be compiled in for non-TCG. > > This is part of the effort to separate away non-pertinent code into > accelerator-specific builds (and in the future modules). > > The fact that they were unconditionally compiled in before was a mistake, or > at least this is the assumption I am making when changing this.
Can you clarify why that was a mistake? Patch 07/11 makes existing functions TCG-specific, but doesn't explain why. > > > > * Code that was conditional on CONFIG_TCG at compile time, and is > > now conditional on TCG being enabled at runtime. > > > > On both cases, we are replacing static initialization of CPUClass > > fields with dynamically initialization of CPUClass depending on > > the chosen accelerator. What makes this necessary? > > > I am not sure about the definitions of "static" and "dynamic" > you are using here, as all the above looks "dynamic" to me, > before and after. The existing code initializes all CPUClass fields as soon as possible (at class_init), and we can be 100% sure the CPUClass methods above are always pointing to the right function and will never change. This patch makes those CPUClass fields change at runtime, after accelerator initialization. This might be a reasonable approach if we have a good reason for it, but what can we do to simplify this? If those methods really depend on the accelerator (and not the CPU class), why aren't they part of the CpusAccel struct instead of CPUClass? > > The overrides and additional fields in the CPUClass are only > necessary for TCG, and all that code should not be compiled in > if TCG is not built-in. > > Ciao, > > Claudio > > > > > > >> +} > >> + > >> +/* > >> + * TCG-specific defaults that override all CPU models when using TCG > >> + */ > >> +static PropValue tcg_default_props[] = { > >> + { "vme", "off" }, > >> + { NULL, NULL }, > >> +}; > >> + > >> +static void tcg_cpu_instance_init(CPUState *cs) > >> +{ > >> + X86CPU *cpu = X86_CPU(cs); > >> + /* Special cases not set in the X86CPUDefinition structs: */ > >> + x86_cpu_apply_props(cpu, tcg_default_props); > >> +} > >> + > >> +static void tcg_cpu_accel_class_init(ObjectClass *oc, void *data) > >> +{ > >> + AccelCPUClass *acc = ACCEL_CPU_CLASS(oc); > >> + > >> + acc->cpu_realizefn = tcg_cpu_realizefn; > >> + acc->cpu_class_init = tcg_cpu_class_init; > >> + acc->cpu_instance_init = tcg_cpu_instance_init; > >> +} > >> +static const TypeInfo tcg_cpu_accel_type_info = { > >> + .name = ACCEL_CPU_NAME("tcg"), > >> + > >> + .parent = TYPE_ACCEL_CPU, > >> + .class_init = tcg_cpu_accel_class_init, > >> + .abstract = true, > >> +}; > >> +static void tcg_cpu_accel_register_types(void) > >> +{ > >> + type_register_static(&tcg_cpu_accel_type_info); > >> +} > >> +type_init(tcg_cpu_accel_register_types); > >> diff --git a/target/i386/tcg/meson.build b/target/i386/tcg/meson.build > >> index 02794226c2..9e439df9c7 100644 > >> --- a/target/i386/tcg/meson.build > >> +++ b/target/i386/tcg/meson.build > >> @@ -10,4 +10,5 @@ i386_ss.add(when: 'CONFIG_TCG', if_true: files( > >> 'seg_helper.c', > >> 'smm_helper.c', > >> 'svm_helper.c', > >> - 'translate.c'), if_false: files('tcg-stub.c')) > >> + 'translate.c', > >> + 'cpu.c'), if_false: files('tcg-stub.c')) > >> -- > >> 2.26.2 > >> > >> > > > -- Eduardo