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

[Eduardo Habkost <ehabkost@redhat.com>]

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

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