| Message ID | 20230417164041.684562-4-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Deprecate/rename singlestep command line option, monitor interfaces | expand |
On 4/17/23 18:40, Peter Maydell wrote: > @@ -219,8 +221,8 @@ static void tcg_set_one_insn_per_tb(Object *obj, bool value, Error **errp) > { > TCGState *s = TCG_STATE(obj); > s->one_insn_per_tb = value; > - /* For the moment, set the global also: this changes the behaviour */ > - singlestep = value; > + /* Set the global also: this changes the behaviour */ > + qatomic_set(&one_insn_per_tb, value); > } Oh, one question: is it worth having the TCGState member at all? Seems like these accessors could work just fine with only the global. r~
On Tue, 18 Apr 2023 at 09:05, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 4/17/23 18:40, Peter Maydell wrote: > > @@ -219,8 +221,8 @@ static void tcg_set_one_insn_per_tb(Object *obj, bool value, Error **errp) > > { > > TCGState *s = TCG_STATE(obj); > > s->one_insn_per_tb = value; > > - /* For the moment, set the global also: this changes the behaviour */ > > - singlestep = value; > > + /* Set the global also: this changes the behaviour */ > > + qatomic_set(&one_insn_per_tb, value); > > } > > Oh, one question: is it worth having the TCGState member at all? > Seems like these accessors could work just fine with only the global. True at the moment, but if we do ever want to do that refactoring to use a tcg_global_cflags, then we will need the TCGState field, because you can't go from a tcg_global_cflags value back to "what are the one_insn_per_tb and nochain settings currently?". thanks -- PMM
On 17/4/23 18:40, Peter Maydell wrote: > The only place left that looks at the old 'singlestep' global > variable is the TCG curr_cflags() function. Replace the old global > with a new 'one_insn_per_tb' which is defined in tcg-all.c and > declared in accel/tcg/internal.h. This keeps it restricted to the > TCG code, unlike 'singlestep' which was available to every file in > the system and defined in multiple different places for softmmu vs > linux-user vs bsd-user. > > While we're making this change, use qatomic_read() and qatomic_set() > on the accesses to the new global, because TCG will read it without > holding a lock. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > In discussion on v2, we talked about combining this with the > 'nochain' flag so as to have a single 'tcg_cflags_global' that > held the flags for the current (one_insn_per_tb, nochain) state. > I have not attempted that here, because it's a little tricky: > * util/log.c is built into some binaries that don't have an > accelerator at all (the tools), so it can't simply call > current_accel() to get the TCG accelerator > * the initial value of the logging flags is set before the > TCG accelerator is even created > So I leave that to somebody else to have a go at if they like. > --- > accel/tcg/internal.h | 2 ++ > include/exec/cpu-common.h | 2 -- > accel/tcg/cpu-exec.c | 2 +- > accel/tcg/tcg-all.c | 6 ++++-- > bsd-user/main.c | 1 - > linux-user/main.c | 1 - > softmmu/globals.c | 1 - > 7 files changed, 7 insertions(+), 8 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h index 96f198b28b4..7bb0fdbe149 100644 --- a/accel/tcg/internal.h +++ b/accel/tcg/internal.h @@ -67,4 +67,6 @@ static inline target_ulong log_pc(CPUState *cpu, const TranslationBlock *tb) extern int64_t max_delay; extern int64_t max_advance; +extern bool one_insn_per_tb; + #endif /* ACCEL_TCG_INTERNAL_H */ diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h index 6feaa40ca7b..0be74f1e706 100644 --- a/include/exec/cpu-common.h +++ b/include/exec/cpu-common.h @@ -163,8 +163,6 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr, void *ptr, size_t len, bool is_write); /* vl.c */ -extern int singlestep; - void list_cpus(const char *optarg); #endif /* CPU_COMMON_H */ diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index 8370c92c05e..bc0e1c3299a 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -159,7 +159,7 @@ uint32_t curr_cflags(CPUState *cpu) */ if (unlikely(cpu->singlestep_enabled)) { cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | CF_SINGLE_STEP | 1; - } else if (singlestep) { + } else if (qatomic_read(&one_insn_per_tb)) { cflags |= CF_NO_GOTO_TB | 1; } else if (qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) { cflags |= CF_NO_GOTO_TB; diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c index fcf361c8db6..a831f8d7c37 100644 --- a/accel/tcg/tcg-all.c +++ b/accel/tcg/tcg-all.c @@ -31,6 +31,7 @@ #include "qapi/error.h" #include "qemu/error-report.h" #include "qemu/accel.h" +#include "qemu/atomic.h" #include "qapi/qapi-builtin-visit.h" #include "qemu/units.h" #if !defined(CONFIG_USER_ONLY) @@ -110,6 +111,7 @@ static void tcg_accel_instance_init(Object *obj) } bool mttcg_enabled; +bool one_insn_per_tb; static int tcg_init_machine(MachineState *ms) { @@ -219,8 +221,8 @@ static void tcg_set_one_insn_per_tb(Object *obj, bool value, Error **errp) { TCGState *s = TCG_STATE(obj); s->one_insn_per_tb = value; - /* For the moment, set the global also: this changes the behaviour */ - singlestep = value; + /* Set the global also: this changes the behaviour */ + qatomic_set(&one_insn_per_tb, value); } static int tcg_gdbstub_supported_sstep_flags(void) diff --git a/bsd-user/main.c b/bsd-user/main.c index 09b84da190c..a9e5a127d38 100644 --- a/bsd-user/main.c +++ b/bsd-user/main.c @@ -49,7 +49,6 @@ #include "host-os.h" #include "target_arch_cpu.h" -int singlestep; static bool opt_one_insn_per_tb; uintptr_t guest_base; bool have_guest_base; diff --git a/linux-user/main.c b/linux-user/main.c index 489694ad654..c7020b413bc 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -68,7 +68,6 @@ char *exec_path; char real_exec_path[PATH_MAX]; -int singlestep; static bool opt_one_insn_per_tb; static const char *argv0; static const char *gdbstub; diff --git a/softmmu/globals.c b/softmmu/globals.c index 39678aa8c58..e83b5428d12 100644 --- a/softmmu/globals.c +++ b/softmmu/globals.c @@ -43,7 +43,6 @@ int vga_interface_type = VGA_NONE; bool vga_interface_created; Chardev *parallel_hds[MAX_PARALLEL_PORTS]; int win2k_install_hack; -int singlestep; int fd_bootchk = 1; int graphic_rotate; QEMUOptionRom option_rom[MAX_OPTION_ROMS];
The only place left that looks at the old 'singlestep' global variable is the TCG curr_cflags() function. Replace the old global with a new 'one_insn_per_tb' which is defined in tcg-all.c and declared in accel/tcg/internal.h. This keeps it restricted to the TCG code, unlike 'singlestep' which was available to every file in the system and defined in multiple different places for softmmu vs linux-user vs bsd-user. While we're making this change, use qatomic_read() and qatomic_set() on the accesses to the new global, because TCG will read it without holding a lock. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- In discussion on v2, we talked about combining this with the 'nochain' flag so as to have a single 'tcg_cflags_global' that held the flags for the current (one_insn_per_tb, nochain) state. I have not attempted that here, because it's a little tricky: * util/log.c is built into some binaries that don't have an accelerator at all (the tools), so it can't simply call current_accel() to get the TCG accelerator * the initial value of the logging flags is set before the TCG accelerator is even created So I leave that to somebody else to have a go at if they like. --- accel/tcg/internal.h | 2 ++ include/exec/cpu-common.h | 2 -- accel/tcg/cpu-exec.c | 2 +- accel/tcg/tcg-all.c | 6 ++++-- bsd-user/main.c | 1 - linux-user/main.c | 1 - softmmu/globals.c | 1 - 7 files changed, 7 insertions(+), 8 deletions(-)