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