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

[Claudio Fontana]

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

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.


> * 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 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
>>
>>
>