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

[Claudio Fontana]

On 11/27/20 7:09 PM, Eduardo Habkost wrote:
> 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 <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.
> 
> Can you clarify why that was a mistake?  Patch 07/11 makes
> existing functions TCG-specific, but doesn't explain why.



They are functions called only for TCG. You don't need translation blocks and 
cpu_exec() for other accelerators.
I can add this information in the commit message.

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