| Message ID | 20201212155530.23098-17-cfontana@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i386 cleanup PART 1 | expand |
On Sat, Dec 12, 2020 at 04:55:23PM +0100, Claudio Fontana wrote: > From: Eduardo Habkost <ehabkost@redhat.com> > > since tcg_cpu_ops.h is only included in cpu.h, > and as a standalone header it is not really useful, > as tcg_cpu_ops.h starts requiring cpu.h defines, > enums, etc, as well as (later on in the series), > additional definitions coming from memattr.h. > > Therefore rename it to tcg_cpu_ops.h.inc, to warn > any potential user that this file is not a standalone > header, but rather a partition of cpu.h that is > included conditionally if CONFIG_TCG is true. What's the benefit of moving definitions to a separate file, if the new file is not a standalone header? If moving the definitions to a separate header is going to require too much work, it's completely OK to keep them in cpu.h by now, and try to move them later. I'm worried that the scope of this series is growing too much, and discussion/review of additional changes in each new version is preventing us from merging the original changes where we already had some consensus. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > [claudio: wrapped in CONFIG_TCG, renamed .h to .inc] > Signed-off-by: Claudio Fontana <cfontana@suse.de> > --- > include/hw/core/cpu.h | 10 +--------- > accel/tcg/cpu-exec.c | 4 ++-- > target/arm/cpu.c | 4 +++- > target/avr/cpu.c | 2 +- > target/hppa/cpu.c | 2 +- > target/i386/tcg/tcg-cpu.c | 2 +- > target/microblaze/cpu.c | 2 +- > target/mips/cpu.c | 4 +++- > target/riscv/cpu.c | 2 +- > target/rx/cpu.c | 2 +- > target/sh4/cpu.c | 2 +- > target/sparc/cpu.c | 2 +- > target/tricore/cpu.c | 2 +- > include/hw/core/{tcg-cpu-ops.h => tcg-cpu-ops.h.inc} | 10 ++++++++++ > 14 files changed, 28 insertions(+), 22 deletions(-) > rename include/hw/core/{tcg-cpu-ops.h => tcg-cpu-ops.h.inc} (55%) [...]
Hi Claudio, Eduardo. On 12/14/20 8:10 PM, Eduardo Habkost wrote: > On Sat, Dec 12, 2020 at 04:55:23PM +0100, Claudio Fontana wrote: >> From: Eduardo Habkost <ehabkost@redhat.com> >> >> since tcg_cpu_ops.h is only included in cpu.h, >> and as a standalone header it is not really useful, >> as tcg_cpu_ops.h starts requiring cpu.h defines, >> enums, etc, as well as (later on in the series), >> additional definitions coming from memattr.h. >> >> Therefore rename it to tcg_cpu_ops.h.inc, to warn >> any potential user that this file is not a standalone >> header, but rather a partition of cpu.h that is >> included conditionally if CONFIG_TCG is true. > > What's the benefit of moving definitions to a separate file, if > the new file is not a standalone header? Claudio, I haven't been following every respin. If you did that change just to please me then the circular dependency remarked by Richard, then if it simplify the series I'm OK if you have to remove the includes. Eduardo, if you are happy with patches 1-8 (x86 specific), maybe you can queue them already. The rest is more TCG generic and will likely go via Richard/Paolo trees IMO. > > If moving the definitions to a separate header is going to > require too much work, it's completely OK to keep them in cpu.h > by now, and try to move them later. > > I'm worried that the scope of this series is growing too much, > and discussion/review of additional changes in each new version > is preventing us from merging the original changes where we > already had some consensus.
On Mon, Dec 14, 2020 at 10:56:13PM +0100, Philippe Mathieu-Daudé wrote: > Hi Claudio, Eduardo. > > On 12/14/20 8:10 PM, Eduardo Habkost wrote: > > On Sat, Dec 12, 2020 at 04:55:23PM +0100, Claudio Fontana wrote: > >> From: Eduardo Habkost <ehabkost@redhat.com> > >> > >> since tcg_cpu_ops.h is only included in cpu.h, > >> and as a standalone header it is not really useful, > >> as tcg_cpu_ops.h starts requiring cpu.h defines, > >> enums, etc, as well as (later on in the series), > >> additional definitions coming from memattr.h. > >> > >> Therefore rename it to tcg_cpu_ops.h.inc, to warn > >> any potential user that this file is not a standalone > >> header, but rather a partition of cpu.h that is > >> included conditionally if CONFIG_TCG is true. > > > > What's the benefit of moving definitions to a separate file, if > > the new file is not a standalone header? > > Claudio, I haven't been following every respin. If you did that > change just to please me then the circular dependency remarked by > Richard, then if it simplify the series I'm OK if you have to > remove the includes. > > Eduardo, if you are happy with patches 1-8 (x86 specific), maybe > you can queue them already. The rest is more TCG generic and > will likely go via Richard/Paolo trees IMO. Patches 01-06 are queued. Patches 07 and 08 need review.
On 12/14/20 10:56 PM, Philippe Mathieu-Daudé wrote: > Hi Claudio, Eduardo. > > On 12/14/20 8:10 PM, Eduardo Habkost wrote: >> On Sat, Dec 12, 2020 at 04:55:23PM +0100, Claudio Fontana wrote: >>> From: Eduardo Habkost <ehabkost@redhat.com> >>> >>> since tcg_cpu_ops.h is only included in cpu.h, >>> and as a standalone header it is not really useful, >>> as tcg_cpu_ops.h starts requiring cpu.h defines, >>> enums, etc, as well as (later on in the series), >>> additional definitions coming from memattr.h. >>> >>> Therefore rename it to tcg_cpu_ops.h.inc, to warn >>> any potential user that this file is not a standalone >>> header, but rather a partition of cpu.h that is >>> included conditionally if CONFIG_TCG is true. >> >> What's the benefit of moving definitions to a separate file, if >> the new file is not a standalone header? > the benefit is avoiding a 100 line ifdef CONFIG_TCG, and already separating out what is tcg-specific and what isn't, but if this is a problem we can avoid that, and revisit later on. > Claudio, I haven't been following every respin. If you did that > change just to please me then the circular dependency remarked by > Richard, then if it simplify the series I'm OK if you have to > remove the includes. Richard, From the answer of Philippe and Eduardo I think they are not ok with .h.inc, I think the option of just putting everything in cpu.h was ok with you, should I go with that? Thanks, Claudio > > Eduardo, if you are happy with patches 1-8 (x86 specific), maybe > you can queue them already. The rest is more TCG generic and > will likely go via Richard/Paolo trees IMO. > >> >> If moving the definitions to a separate header is going to >> require too much work, it's completely OK to keep them in cpu.h >> by now, and try to move them later. >> >> I'm worried that the scope of this series is growing too much, >> and discussion/review of additional changes in each new version >> is preventing us from merging the original changes where we >> already had some consensus. >
On 12/16/20 2:44 AM, Claudio Fontana wrote: > Richard, From the answer of Philippe and Eduardo I think they are not ok with .h.inc, > I think the option of just putting everything in cpu.h was ok with you, > should I go with that? Yes, that would be fine with me. r~
On Mon, Dec 14, 2020 at 05:24:00PM -0500, Eduardo Habkost wrote: > On Mon, Dec 14, 2020 at 10:56:13PM +0100, Philippe Mathieu-Daudé wrote: > > Hi Claudio, Eduardo. > > > > On 12/14/20 8:10 PM, Eduardo Habkost wrote: > > > On Sat, Dec 12, 2020 at 04:55:23PM +0100, Claudio Fontana wrote: > > >> From: Eduardo Habkost <ehabkost@redhat.com> > > >> > > >> since tcg_cpu_ops.h is only included in cpu.h, > > >> and as a standalone header it is not really useful, > > >> as tcg_cpu_ops.h starts requiring cpu.h defines, > > >> enums, etc, as well as (later on in the series), > > >> additional definitions coming from memattr.h. > > >> > > >> Therefore rename it to tcg_cpu_ops.h.inc, to warn > > >> any potential user that this file is not a standalone > > >> header, but rather a partition of cpu.h that is > > >> included conditionally if CONFIG_TCG is true. > > > > > > What's the benefit of moving definitions to a separate file, if > > > the new file is not a standalone header? > > > > Claudio, I haven't been following every respin. If you did that > > change just to please me then the circular dependency remarked by > > Richard, then if it simplify the series I'm OK if you have to > > remove the includes. > > > > Eduardo, if you are happy with patches 1-8 (x86 specific), maybe > > you can queue them already. The rest is more TCG generic and > > will likely go via Richard/Paolo trees IMO. > > Patches 01-06 are queued. Patches 07 and 08 need review. Patches 07-12 are now queued too.
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index ea648d52ad..1c0f523b5b 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -77,7 +77,7 @@ typedef struct CPUWatchpoint CPUWatchpoint; struct TranslationBlock; #ifdef CONFIG_TCG -#include "tcg-cpu-ops.h" +#include "tcg-cpu-ops.h.inc" #endif /* CONFIG_TCG */ /** @@ -110,13 +110,6 @@ struct TranslationBlock; * If the target behaviour here is anything other than "set * the PC register to the value passed in" then the target must * also implement the synchronize_from_tb hook. - * @synchronize_from_tb: Callback for synchronizing state from a TCG - * #TranslationBlock. This is called when we abandon execution - * of a TB before starting it, and must set all parts of the CPU - * state which the previous TB in the chain may not have updated. - * This always includes at least the program counter; some targets - * will need to do more. If this hook is not implemented then the - * default is to call @set_pc(tb->pc). * @tlb_fill: Callback for handling a softmmu tlb miss or user-only * address fault. For system mode, if the access is valid, call * tlb_set_page and return true; if the access is invalid, and @@ -193,7 +186,6 @@ struct CPUClass { void (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list, Error **errp); void (*set_pc)(CPUState *cpu, vaddr value); - void (*synchronize_from_tb)(CPUState *cpu, struct TranslationBlock *tb); bool (*tlb_fill)(CPUState *cpu, vaddr address, int size, MMUAccessType access_type, int mmu_idx, bool probe, uintptr_t retaddr); diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index 50eb92d217..05dba7f2cc 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -192,8 +192,8 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, TranslationBlock *itb) TARGET_FMT_lx "] %s\n", last_tb->tc.ptr, last_tb->pc, lookup_symbol(last_tb->pc)); - if (cc->synchronize_from_tb) { - cc->synchronize_from_tb(cpu, last_tb); + if (cc->tcg_ops.synchronize_from_tb) { + cc->tcg_ops.synchronize_from_tb(cpu, last_tb); } else { assert(cc->set_pc); cc->set_pc(cpu, last_tb->pc); diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 61237d9885..3c1a44a5b3 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -54,6 +54,7 @@ static void arm_cpu_set_pc(CPUState *cs, vaddr value) } } +#ifdef CONFIG_TCG static void arm_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock *tb) { ARMCPU *cpu = ARM_CPU(cs); @@ -69,6 +70,7 @@ static void arm_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock *tb) env->regs[15] = tb->pc; } } +#endif /* CONFIG_TCG */ static bool arm_cpu_has_work(CPUState *cs) { @@ -2245,7 +2247,6 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) cc->cpu_exec_interrupt = arm_cpu_exec_interrupt; cc->dump_state = arm_cpu_dump_state; cc->set_pc = arm_cpu_set_pc; - cc->synchronize_from_tb = arm_cpu_synchronize_from_tb; cc->gdb_read_register = arm_cpu_gdb_read_register; cc->gdb_write_register = arm_cpu_gdb_write_register; #ifndef CONFIG_USER_ONLY @@ -2265,6 +2266,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) cc->disas_set_info = arm_disas_set_info; #ifdef CONFIG_TCG cc->tcg_ops.initialize = arm_translate_init; + cc->tcg_ops.synchronize_from_tb = arm_cpu_synchronize_from_tb; cc->tlb_fill = arm_cpu_tlb_fill; cc->debug_excp_handler = arm_debug_excp_handler; cc->debug_check_watchpoint = arm_debug_check_watchpoint; diff --git a/target/avr/cpu.c b/target/avr/cpu.c index 94306a2aa0..f753c15768 100644 --- a/target/avr/cpu.c +++ b/target/avr/cpu.c @@ -207,7 +207,7 @@ static void avr_cpu_class_init(ObjectClass *oc, void *data) cc->vmsd = &vms_avr_cpu; cc->disas_set_info = avr_cpu_disas_set_info; cc->tcg_ops.initialize = avr_cpu_tcg_init; - cc->synchronize_from_tb = avr_cpu_synchronize_from_tb; + cc->tcg_ops.synchronize_from_tb = avr_cpu_synchronize_from_tb; cc->gdb_read_register = avr_cpu_gdb_read_register; cc->gdb_write_register = avr_cpu_gdb_write_register; cc->gdb_num_core_regs = 35; diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c index 4c778966c2..12a09e93ae 100644 --- a/target/hppa/cpu.c +++ b/target/hppa/cpu.c @@ -143,7 +143,7 @@ static void hppa_cpu_class_init(ObjectClass *oc, void *data) cc->cpu_exec_interrupt = hppa_cpu_exec_interrupt; cc->dump_state = hppa_cpu_dump_state; cc->set_pc = hppa_cpu_set_pc; - cc->synchronize_from_tb = hppa_cpu_synchronize_from_tb; + cc->tcg_ops.synchronize_from_tb = hppa_cpu_synchronize_from_tb; cc->gdb_read_register = hppa_cpu_gdb_read_register; cc->gdb_write_register = hppa_cpu_gdb_write_register; cc->tlb_fill = hppa_cpu_tlb_fill; diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c index 1f2a3e881a..d1414e2970 100644 --- a/target/i386/tcg/tcg-cpu.c +++ b/target/i386/tcg/tcg-cpu.c @@ -60,7 +60,7 @@ void tcg_cpu_common_class_init(CPUClass *cc) { 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->tcg_ops.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_ops.initialize = tcg_x86_init; diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c index bc10518fa3..97d94d9c27 100644 --- a/target/microblaze/cpu.c +++ b/target/microblaze/cpu.c @@ -322,7 +322,7 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data) cc->cpu_exec_interrupt = mb_cpu_exec_interrupt; cc->dump_state = mb_cpu_dump_state; cc->set_pc = mb_cpu_set_pc; - cc->synchronize_from_tb = mb_cpu_synchronize_from_tb; + cc->tcg_ops.synchronize_from_tb = mb_cpu_synchronize_from_tb; cc->gdb_read_register = mb_cpu_gdb_read_register; cc->gdb_write_register = mb_cpu_gdb_write_register; cc->tlb_fill = mb_cpu_tlb_fill; diff --git a/target/mips/cpu.c b/target/mips/cpu.c index bc48573763..4a539349a9 100644 --- a/target/mips/cpu.c +++ b/target/mips/cpu.c @@ -44,6 +44,7 @@ static void mips_cpu_set_pc(CPUState *cs, vaddr value) } } +#ifdef CONFIG_TCG static void mips_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock *tb) { MIPSCPU *cpu = MIPS_CPU(cs); @@ -53,6 +54,7 @@ static void mips_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock *tb) env->hflags &= ~MIPS_HFLAG_BMASK; env->hflags |= tb->flags & MIPS_HFLAG_BMASK; } +#endif /* CONFIG_TCG */ static bool mips_cpu_has_work(CPUState *cs) { @@ -238,7 +240,6 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) cc->cpu_exec_interrupt = mips_cpu_exec_interrupt; cc->dump_state = mips_cpu_dump_state; cc->set_pc = mips_cpu_set_pc; - cc->synchronize_from_tb = mips_cpu_synchronize_from_tb; cc->gdb_read_register = mips_cpu_gdb_read_register; cc->gdb_write_register = mips_cpu_gdb_write_register; #ifndef CONFIG_USER_ONLY @@ -250,6 +251,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) cc->disas_set_info = mips_cpu_disas_set_info; #ifdef CONFIG_TCG cc->tcg_ops.initialize = mips_tcg_init; + cc->tcg_ops.synchronize_from_tb = mips_cpu_synchronize_from_tb; cc->tlb_fill = mips_cpu_tlb_fill; #endif diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 27dd1645c9..a9c30879d3 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -543,7 +543,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data) cc->cpu_exec_interrupt = riscv_cpu_exec_interrupt; cc->dump_state = riscv_cpu_dump_state; cc->set_pc = riscv_cpu_set_pc; - cc->synchronize_from_tb = riscv_cpu_synchronize_from_tb; + cc->tcg_ops.synchronize_from_tb = riscv_cpu_synchronize_from_tb; cc->gdb_read_register = riscv_cpu_gdb_read_register; cc->gdb_write_register = riscv_cpu_gdb_write_register; cc->gdb_num_core_regs = 33; diff --git a/target/rx/cpu.c b/target/rx/cpu.c index a701a09b11..d03c4e0b05 100644 --- a/target/rx/cpu.c +++ b/target/rx/cpu.c @@ -189,7 +189,7 @@ static void rx_cpu_class_init(ObjectClass *klass, void *data) cc->cpu_exec_interrupt = rx_cpu_exec_interrupt; cc->dump_state = rx_cpu_dump_state; cc->set_pc = rx_cpu_set_pc; - cc->synchronize_from_tb = rx_cpu_synchronize_from_tb; + cc->tcg_ops.synchronize_from_tb = rx_cpu_synchronize_from_tb; cc->gdb_read_register = rx_cpu_gdb_read_register; cc->gdb_write_register = rx_cpu_gdb_write_register; cc->get_phys_page_debug = rx_cpu_get_phys_page_debug; diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c index bdc5c9d90b..a33025b5c8 100644 --- a/target/sh4/cpu.c +++ b/target/sh4/cpu.c @@ -222,7 +222,7 @@ static void superh_cpu_class_init(ObjectClass *oc, void *data) cc->cpu_exec_interrupt = superh_cpu_exec_interrupt; cc->dump_state = superh_cpu_dump_state; cc->set_pc = superh_cpu_set_pc; - cc->synchronize_from_tb = superh_cpu_synchronize_from_tb; + cc->tcg_ops.synchronize_from_tb = superh_cpu_synchronize_from_tb; cc->gdb_read_register = superh_cpu_gdb_read_register; cc->gdb_write_register = superh_cpu_gdb_write_register; cc->tlb_fill = superh_cpu_tlb_fill; diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c index 07e48b86d1..baf6c5b587 100644 --- a/target/sparc/cpu.c +++ b/target/sparc/cpu.c @@ -868,7 +868,7 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data) cc->memory_rw_debug = sparc_cpu_memory_rw_debug; #endif cc->set_pc = sparc_cpu_set_pc; - cc->synchronize_from_tb = sparc_cpu_synchronize_from_tb; + cc->tcg_ops.synchronize_from_tb = sparc_cpu_synchronize_from_tb; cc->gdb_read_register = sparc_cpu_gdb_read_register; cc->gdb_write_register = sparc_cpu_gdb_write_register; cc->tlb_fill = sparc_cpu_tlb_fill; diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c index 78b2925955..5edf96c600 100644 --- a/target/tricore/cpu.c +++ b/target/tricore/cpu.c @@ -162,7 +162,7 @@ static void tricore_cpu_class_init(ObjectClass *c, void *data) cc->dump_state = tricore_cpu_dump_state; cc->set_pc = tricore_cpu_set_pc; - cc->synchronize_from_tb = tricore_cpu_synchronize_from_tb; + cc->tcg_ops.synchronize_from_tb = tricore_cpu_synchronize_from_tb; cc->get_phys_page_debug = tricore_cpu_get_phys_page_debug; cc->tcg_ops.initialize = tricore_tcg_init; cc->tlb_fill = tricore_cpu_tlb_fill; diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h.inc similarity index 55% rename from include/hw/core/tcg-cpu-ops.h rename to include/hw/core/tcg-cpu-ops.h.inc index 4475ef0996..6c7cdf7e5e 100644 --- a/include/hw/core/tcg-cpu-ops.h +++ b/include/hw/core/tcg-cpu-ops.h.inc @@ -20,6 +20,16 @@ typedef struct TcgCpuOperations { * Called when the first CPU is realized. */ void (*initialize)(void); + /** + * @synchronize_from_tb: Synchronize state from a TCG #TranslationBlock + * + * This is called when we abandon execution of a TB before + * starting it, and must set all parts of the CPU state which + * the previous TB in the chain may not have updated. This + * will need to do more. If this hook is not implemented then + * the default is to call @set_pc(tb->pc). + */ + void (*synchronize_from_tb)(CPUState *cpu, struct TranslationBlock *tb); } TcgCpuOperations; #endif /* TCG_CPU_OPS_H */