On Fri, Nov 27, 2020 at 07:16:25PM +0100, Claudio Fontana wrote:
> 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.
Thanks! I believe we can clarify in patch 07/11 why some methods
that were not conditional on CONFIG_TCG were moved to
tcg_cpu_common_class_init().
Do you think it would be too complicated to move those methods
from CPUClass to CpusAccel? If they are really accel-specific,
CPUClass looks like the wrong place for them.
> [...]
--
Eduardo
