Re: [RFC v6 11/11] i386: split cpu accelerators from cpu.c, using AccelCPUClass

[Eduardo Habkost <ehabkost@redhat.com>]

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 <cfontana@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.

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).
* 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?


> +}
> +
> +/*
> + * 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