Re: [RFC v6 07/11] i386: move TCG cpu class initialization out of helper.c

[Eduardo Habkost]

Now that I understand what you are doing here, I have specific
questions about the functions you are moving, below:

On Thu, Nov 26, 2020 at 11:32:14PM +0100, Claudio Fontana wrote:
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
[...]
> @@ -1495,7 +1497,8 @@ static inline uint64_t x86_cpu_xsave_components(X86CPU 
> *cpu)
>             cpu->env.features[FEAT_XSAVE_COMP_LO];
>  }
>  
> -const char *get_register_name_32(unsigned int reg)
> +/* Return name of 32-bit register, from a R_* constant */
> +static const char *get_register_name_32(unsigned int reg)
>  {
>      if (reg >= CPU_NB_REGS32) {
>          return NULL;
> @@ -7012,13 +7015,6 @@ static void x86_cpu_set_pc(CPUState *cs, vaddr value)
>      cpu->env.eip = value;
>  }
>  
> -static void x86_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock *tb)
> -{
> -    X86CPU *cpu = X86_CPU(cs);
> -
> -    cpu->env.eip = tb->pc - tb->cs_base;
> -}

Question to be answered in the commit message: how can somebody
be sure this code is not necessary for any other accelerators?
The TranslationBlock* argument is a hint, but not a guarantee.

Maybe we should rename CPUClass.synchronize_from_tb to
CPUClass.tcg_synchronize_from_tb?  Maybe we should have a
separate TCGCpuOperations struct to carry TCG-specific methods?

(The same questions above apply to the other methods below)


> -
>  int x86_cpu_pending_interrupt(CPUState *cs, int interrupt_request)
>  {
>      X86CPU *cpu = X86_CPU(cs);
> @@ -7252,17 +7248,18 @@ static void x86_cpu_common_class_init(ObjectClass 
> *oc, void *data)
>      cc->class_by_name = x86_cpu_class_by_name;
>      cc->parse_features = x86_cpu_parse_featurestr;
>      cc->has_work = x86_cpu_has_work;
> +
>  #ifdef CONFIG_TCG
> -    cc->do_interrupt = x86_cpu_do_interrupt;
> -    cc->cpu_exec_interrupt = x86_cpu_exec_interrupt;

These two are in seg_helper.c, so I agree it makes sense to keep
it in tcg_cpu_common_class_init().

I'd like to understand why they are TCG-specific, though.  Are
CPUClass.do_interrupt and CPUClass.cpu_exec_enter TCG-specific on
all architectures, or only in x86?

> -#endif
> +    tcg_cpu_common_class_init(cc);
> +#endif /* CONFIG_TCG */
> +
>      cc->dump_state = x86_cpu_dump_state;
>      cc->set_pc = x86_cpu_set_pc;
> -    cc->synchronize_from_tb = x86_cpu_synchronize_from_tb;
>      cc->gdb_read_register = x86_cpu_gdb_read_register;
>      cc->gdb_write_register = x86_cpu_gdb_write_register;
>      cc->get_arch_id = x86_cpu_get_arch_id;
>      cc->get_paging_enabled = x86_cpu_get_paging_enabled;
> +
>  #ifndef CONFIG_USER_ONLY
>      cc->asidx_from_attrs = x86_asidx_from_attrs;
>      cc->get_memory_mapping = x86_cpu_get_memory_mapping;
> @@ -7273,7 +7270,8 @@ static void x86_cpu_common_class_init(ObjectClass *oc, 
> void *data)
>      cc->write_elf32_note = x86_cpu_write_elf32_note;
>      cc->write_elf32_qemunote = x86_cpu_write_elf32_qemunote;
>      cc->vmsd = &vmstate_x86_cpu;
> -#endif
> +#endif /* !CONFIG_USER_ONLY */
> +
>      cc->gdb_arch_name = x86_gdb_arch_name;
>  #ifdef TARGET_X86_64
>      cc->gdb_core_xml_file = "i386-64bit.xml";
> @@ -7281,15 +7279,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc, 
> void *data)
>  #else
>      cc->gdb_core_xml_file = "i386-32bit.xml";
>      cc->gdb_num_core_regs = 50;
> -#endif
> -#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
> -    cc->debug_excp_handler = breakpoint_handler;

That's in bpt_helper.c, also TCG-specific.  Makes sense to move
it to tcg_cpu_common_class_init().

Is CPUClass.debug_excp_handler() TCG-specific in all
architectures, or only in x86?

> -#endif
> -    cc->cpu_exec_enter = x86_cpu_exec_enter;
> -    cc->cpu_exec_exit = x86_cpu_exec_exit;

I have a question about those two functions below[1].

> -#ifdef CONFIG_TCG
> -    cc->tcg_initialize = tcg_x86_init;

The name makes this is obviously TCG-specific, so it makes sense
to move it to tcg_cpu_common_class_init().

> -    cc->tlb_fill = x86_cpu_tlb_fill;

This is in excp_helper.c (TCG-specific), so it makes sense to
move it to tcg_cpu_common_class_init().

Is CPUClass.tlb_fill TCG-specific in all architectures, or only
in x86?

>  #endif
>      cc->disas_set_info = x86_disas_set_info;
>  
[...]
> -/* Frob eflags into and out of the CPU temporary format.  */
> -
> -void x86_cpu_exec_enter(CPUState *cs)
> -{
> -    X86CPU *cpu = X86_CPU(cs);
> -    CPUX86State *env = &cpu->env;
> -
> -    CC_SRC = env->eflags & (CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
> -    env->df = 1 - (2 * ((env->eflags >> 10) & 1));
> -    CC_OP = CC_OP_EFLAGS;
> -    env->eflags &= ~(DF_MASK | CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
> -}
> -
> -void x86_cpu_exec_exit(CPUState *cs)
> -{
> -    X86CPU *cpu = X86_CPU(cs);
> -    CPUX86State *env = &cpu->env;
> -
> -    env->eflags = cpu_compute_eflags(env);
> -}

[1]

How exactly can we be 100% sure this is not used by other
accelerators?

> -
>  #ifndef CONFIG_USER_ONLY
>  uint8_t x86_ldub_phys(CPUState *cs, hwaddr addr)
>  {
[...]

-- 
Eduardo