Re: [RFC v6 07/11] i386: move TCG cpu class initialization out of helper.c
On 11/27/20 9:43 PM, Eduardo Habkost wrote: > On Fri, Nov 27, 2020 at 08:47:00PM +0100, Claudio Fontana wrote: >> On 11/27/20 8:04 PM, Eduardo Habkost wrote: > [...] >>> 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. > > I'm working on it at: > https://gitlab.com/ehabkost/qemu/-/commits/work/tcg-cpu-ops > > Feel free to reuse it, if you want to do it before your series. > Otherwise, I can rebase it after your series is merged. > > I didn't touch do_interrupt(), because of the aarch64 weirdness. > Hi, yes it makes sense to separate more clearly I think what is tcg only among those operations, it is a bit tangent to my series in the sense that those methods need to be set one way or another, either in cc-> or in cc->tcg_ops, but yes, we could put those changes before or after the series, and I think they make sense. Ciao, Claudio
Re: [RFC v6 07/11] i386: move TCG cpu class initialization out of helper.c
On Fri, Nov 27, 2020 at 08:47:00PM +0100, Claudio Fontana wrote: > On 11/27/20 8:04 PM, Eduardo Habkost wrote: [...] > > 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. I'm working on it at: https://gitlab.com/ehabkost/qemu/-/commits/work/tcg-cpu-ops Feel free to reuse it, if you want to do it before your series. Otherwise, I can rebase it after your series is merged. I didn't touch do_interrupt(), because of the aarch64 weirdness. -- Eduardo
Re: [RFC v6 07/11] i386: move TCG cpu class initialization out of helper.c
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
> [...]
>> @@ -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
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
Date: Thu Dec 17 13:37:15 2015 +
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;
>>
Re: [RFC v6 07/11] i386: move TCG cpu class initialization out of helper.c
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
[...]
> @@ -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 = _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 = >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 = >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
[RFC v6 07/11] i386: move TCG cpu class initialization out of helper.c
Signed-off-by: Claudio Fontana
---
target/i386/cpu.c | 33 --
target/i386/cpu.h | 97 ++---
target/i386/helper-tcg.h | 112 ++
target/i386/helper.c | 23 ---
target/i386/meson.build | 1 +
target/i386/tcg-cpu.c | 71 +
target/i386/tcg-cpu.h | 15 +
target/i386/tcg/bpt_helper.c | 1 +
target/i386/tcg/cc_helper.c | 1 +
target/i386/tcg/excp_helper.c | 1 +
target/i386/tcg/fpu_helper.c | 33 +-
target/i386/tcg/int_helper.c | 1 +
target/i386/tcg/mem_helper.c | 1 +
target/i386/tcg/misc_helper.c | 1 +
target/i386/tcg/mpx_helper.c | 1 +
target/i386/tcg/seg_helper.c | 1 +
target/i386/tcg/smm_helper.c | 2 +
target/i386/tcg/svm_helper.c | 1 +
target/i386/tcg/translate.c | 1 +
19 files changed, 244 insertions(+), 153 deletions(-)
create mode 100644 target/i386/helper-tcg.h
create mode 100644 target/i386/tcg-cpu.c
create mode 100644 target/i386/tcg-cpu.h
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b9bd249c8f..3462d0143f 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -24,6 +24,8 @@
#include "qemu/qemu-print.h"
#include "cpu.h"
+#include "tcg-cpu.h"
+#include "helper-tcg.h"
#include "exec/exec-all.h"
#include "sysemu/kvm.h"
#include "sysemu/reset.h"
@@ -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;
-}
-
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;
-#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 = _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;
-#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
cc->disas_set_info = x86_disas_set_info;
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index d6ed45c5d7..a0d64613dc 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -31,9 +31,6 @@
#define KVM_HAVE_MCE_INJECTION 1
-/* Maximum instruction code size */
-#define TARGET_MAX_INSN_SIZE 16
-
/* support for self modifying code even if the modified instruction is
close to the modifying instruction */
#define TARGET_HAS_PRECISE_SMC
@@ -1037,6 +1034,12 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
* using this information. Condition codes are not generated if they
* are only needed for conditional branches.
*/
+
+#define CC_DST (env->cc_dst)
+#define CC_SRC (env->cc_src)
+#define CC_SRC2 (env->cc_src2)
+#define CC_OP (env->cc_op)
+
typedef enum {
CC_OP_DYNAMIC, /* must use dynamic code to get cc_op */
CC_OP_EFLAGS, /* all cc are explicitly computed, CC_SRC = flags */
@@ -1765,12 +1768,6 @@ struct X86CPU {
extern VMStateDescription vmstate_x86_cpu;
#endif
-/**
- * x86_cpu_do_interrupt:
- * @cpu:
