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

[Claudio Fontana <cfontana@suse.de>]

Hi Eduardo,

On 11/27/20 8:04 PM, Eduardo Habkost wrote:
> 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.


easiest is to just trace the calls, but there is a also helpful description of all these methods in hw/core/cpu.h.

$ mkid --include="C"
$ gid synchronize_from_tb

include/hw/core/cpu.h:110: *       also implement the synchronize_from_tb hook.
include/hw/core/cpu.h:111: * @synchronize_from_tb: Callback for synchronizing state from a TCG
include/hw/core/cpu.h:194:    void (*synchronize_from_tb)(CPUState *cpu, struct TranslationBlock *tb);
accel/tcg/cpu-exec.c:195:        if (cc->synchronize_from_tb) {
accel/tcg/cpu-exec.c:196:            cc->synchronize_from_tb(cpu, last_tb);
target/arm/cpu.c:2245:    cc->synchronize_from_tb = arm_cpu_synchronize_from_tb;
target/avr/cpu.c:210:    cc->synchronize_from_tb = avr_cpu_synchronize_from_tb;
target/hppa/cpu.c:146:    cc->synchronize_from_tb = hppa_cpu_synchronize_from_tb;
target/microblaze/cpu.c:325:    cc->synchronize_from_tb = mb_cpu_synchronize_from_tb;
target/mips/cpu.c:241:    cc->synchronize_from_tb = mips_cpu_synchronize_from_tb;
target/riscv/cpu.c:546:    cc->synchronize_from_tb = riscv_cpu_synchronize_from_tb;
target/rx/cpu.c:192:    cc->synchronize_from_tb = rx_cpu_synchronize_from_tb;
target/sh4/cpu.c:225:    cc->synchronize_from_tb = superh_cpu_synchronize_from_tb;
target/sparc/cpu.c:871:    cc->synchronize_from_tb = sparc_cpu_synchronize_from_tb;
target/tricore/cpu.c:165:    cc->synchronize_from_tb = tricore_cpu_synchronize_from_tb;
target/i386/tcg/cpu.c:129:    cc->synchronize_from_tb = x86_cpu_synchronize_from_tb;

> Maybe we should rename CPUClass.synchronize_from_tb to
> CPUClass.tcg_synchronize_from_tb?  Maybe we should have a

possibly, yes.

> separate TCGCpuOperations struct to carry TCG-specific methods?


interesting, will think about it.

> 
> (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?

cpu_exec_enter: yes, tcg only on all archs.

Traces as above, it is only called by

accel/tcg/cpu-exec.c

cc->do_interrupt() is very interesting, it _should_ be the same story,
the use of cc->do_interrupt() is normally relegated to accel/tcg/cpu-exec.c,
or to the various implementations of cpu_exec_interrupt (also tcg-specific).

_BUT_ there is an exception in arm64 where it seems to be (strangely) used for kvm64:

So in this case the arm kvm64 code is the outlier. There are two calls, one introduced in 2015 and one in 2020:

commit e24fd076a59604c4ba3c05fe9d19ea6fc5320a12
Author: Dongjiu Geng <gengdongjiu@huawei.com>
Date:   Tue May 12 11:06:08 2020 +0800

    target-arm: kvm64: handle SIGBUS signal from kernel or KVM


commit 34c45d53026d9c135415d034a8bc30c1a30acf1c
Author: Alex Bennée <alex.bennee@linaro.org>
Date:   Thu Dec 17 13:37:15 2015 +0000

    target-arm: kvm - re-inject guest debug exceptions


Maybe Alex and Dongjiu Geng (or maybe Peter?) can shed some light on whether this is intentional, or an oversight?

It is the one and only use of cc->do_interrupt outside of TCG. So strange.


> 
>> -#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?


TCG-specific on all, real CPU has its own real TLB.

Only accel/tcg/ uses the code.

> 
>>  #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?


The only calls to these methods are from accel/tcg/cpu-exec.c .



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

I think that this exercise has been useful, and btw I am really interested about the possible comments from Alex or Geng DongJiu
on the extraordinary use of cc->do_interrupt for kvm64 on AArch64,

Thanks,

Ciao,

Claudio