| Message ID | 20210720195439.626594-18-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [for-6.1,v6,01/17] accel/tcg: Reduce CF_COUNT_MASK to match TCG_MAX_INSNS | expand |
On Tue, 20 Jul 2021 at 20:55, Richard Henderson <richard.henderson@linaro.org> wrote: > > Set CF_SINGLE_STEP when single-stepping is enabled. > This avoids the need to flush all tb's when turning > single-stepping on or off. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
Richard Henderson <richard.henderson@linaro.org> writes: > Set CF_SINGLE_STEP when single-stepping is enabled. > This avoids the need to flush all tb's when turning > single-stepping on or off. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/exec/exec-all.h | 1 + > accel/tcg/cpu-exec.c | 7 ++++++- > accel/tcg/translate-all.c | 4 ---- > accel/tcg/translator.c | 7 +------ > cpu.c | 4 ---- > 5 files changed, 8 insertions(+), 15 deletions(-) > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index 6873cce8df..5d1b6d80fb 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -497,6 +497,7 @@ struct TranslationBlock { > #define CF_COUNT_MASK 0x000001ff > #define CF_NO_GOTO_TB 0x00000200 /* Do not chain with goto_tb */ > #define CF_NO_GOTO_PTR 0x00000400 /* Do not chain with goto_ptr */ > +#define CF_SINGLE_STEP 0x00000800 /* gdbstub single-step in effect */ > #define CF_LAST_IO 0x00008000 /* Last insn may be an IO access. */ > #define CF_MEMI_ONLY 0x00010000 /* Only instrument memory ops */ > #define CF_USE_ICOUNT 0x00020000 > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c > index 5cc6363f4c..fc895cf51e 100644 > --- a/accel/tcg/cpu-exec.c > +++ b/accel/tcg/cpu-exec.c > @@ -150,10 +150,15 @@ uint32_t curr_cflags(CPUState *cpu) > uint32_t cflags = cpu->tcg_cflags; > > /* > + * Record gdb single-step. We should be exiting the TB by raising > + * EXCP_DEBUG, but to simplify other tests, disable chaining too. > + * > * For singlestep and -d nochain, suppress goto_tb so that > * we can log -d cpu,exec after every TB. > */ > - if (singlestep) { > + if (unlikely(cpu->singlestep_enabled)) { > + cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | CF_SINGLE_STEP | > 1; What does CF_SINGLE_STEP achieve that isn't already handled by having: cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | 1; (btw did we mask CF_COUNT_MASK somewhere else?). Because surely the CF_COUNT is part of cflags so limits the TB's that could be returned anyway? > + } else if (singlestep) { > 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/translate-all.c b/accel/tcg/translate-all.c > index bf82c15aab..bbfcfb698c 100644 > --- a/accel/tcg/translate-all.c > +++ b/accel/tcg/translate-all.c > @@ -1432,10 +1432,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu, > } > QEMU_BUILD_BUG_ON(CF_COUNT_MASK + 1 != TCG_MAX_INSNS); > > - if (cpu->singlestep_enabled) { > - max_insns = 1; > - } > - > buffer_overflow: > tb = tcg_tb_alloc(tcg_ctx); > if (unlikely(!tb)) { > diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c > index b45337f3ba..c53a7f8e44 100644 > --- a/accel/tcg/translator.c > +++ b/accel/tcg/translator.c > @@ -38,11 +38,6 @@ bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest) > return false; > } > > - /* Suppress goto_tb in the case of single-steping. */ > - if (db->singlestep_enabled) { > - return false; > - } > - > /* Check for the dest on the same page as the start of the TB. */ > return ((db->pc_first ^ dest) & TARGET_PAGE_MASK) == 0; > } > @@ -60,7 +55,7 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, > db->is_jmp = DISAS_NEXT; > db->num_insns = 0; > db->max_insns = max_insns; > - db->singlestep_enabled = cpu->singlestep_enabled; > + db->singlestep_enabled = cflags & CF_SINGLE_STEP; > > ops->init_disas_context(db, cpu); > tcg_debug_assert(db->is_jmp == DISAS_NEXT); /* no early exit */ > diff --git a/cpu.c b/cpu.c > index d6ae5ae581..e1799a15bc 100644 > --- a/cpu.c > +++ b/cpu.c > @@ -326,10 +326,6 @@ void cpu_single_step(CPUState *cpu, int enabled) > cpu->singlestep_enabled = enabled; > if (kvm_enabled()) { > kvm_update_guest_debug(cpu, 0); > - } else { > - /* must flush all the translated code to avoid inconsistencies */ > - /* XXX: only flush what is necessary */ > - tb_flush(cpu); > } > trace_breakpoint_singlestep(cpu->cpu_index, enabled); > }
On 7/21/21 12:38 AM, Alex Bennée wrote: > > Richard Henderson <richard.henderson@linaro.org> writes: > >> Set CF_SINGLE_STEP when single-stepping is enabled. >> This avoids the need to flush all tb's when turning >> single-stepping on or off. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> include/exec/exec-all.h | 1 + >> accel/tcg/cpu-exec.c | 7 ++++++- >> accel/tcg/translate-all.c | 4 ---- >> accel/tcg/translator.c | 7 +------ >> cpu.c | 4 ---- >> 5 files changed, 8 insertions(+), 15 deletions(-) >> >> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h >> index 6873cce8df..5d1b6d80fb 100644 >> --- a/include/exec/exec-all.h >> +++ b/include/exec/exec-all.h >> @@ -497,6 +497,7 @@ struct TranslationBlock { >> #define CF_COUNT_MASK 0x000001ff >> #define CF_NO_GOTO_TB 0x00000200 /* Do not chain with goto_tb */ >> #define CF_NO_GOTO_PTR 0x00000400 /* Do not chain with goto_ptr */ >> +#define CF_SINGLE_STEP 0x00000800 /* gdbstub single-step in effect */ >> #define CF_LAST_IO 0x00008000 /* Last insn may be an IO access. */ >> #define CF_MEMI_ONLY 0x00010000 /* Only instrument memory ops */ >> #define CF_USE_ICOUNT 0x00020000 >> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c >> index 5cc6363f4c..fc895cf51e 100644 >> --- a/accel/tcg/cpu-exec.c >> +++ b/accel/tcg/cpu-exec.c >> @@ -150,10 +150,15 @@ uint32_t curr_cflags(CPUState *cpu) >> uint32_t cflags = cpu->tcg_cflags; >> >> /* >> + * Record gdb single-step. We should be exiting the TB by raising >> + * EXCP_DEBUG, but to simplify other tests, disable chaining too. >> + * >> * For singlestep and -d nochain, suppress goto_tb so that >> * we can log -d cpu,exec after every TB. >> */ >> - if (singlestep) { >> + if (unlikely(cpu->singlestep_enabled)) { >> + cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | CF_SINGLE_STEP | >> 1; > > What does CF_SINGLE_STEP achieve that isn't already handled by having: > > cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | 1; It sets DisasContextBase.singlestep_enabled. With only this patch set, we still check that and emit EXCP_DEBUG at the end of every TB. After the 6.2 singlestep cleanup, we still have one reference to DisasContextBase.singlestep_enabled in target/mips for the branch delay slot thing that we discussed on IRC yesterday. > > (btw did we mask CF_COUNT_MASK somewhere else?). Because surely the > CF_COUNT is part of cflags so limits the TB's that could be returned > anyway? Here in curr_cflags(), CF_COUNT_MASK begins at zero. r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 7/21/21 12:38 AM, Alex Bennée wrote: >> Richard Henderson <richard.henderson@linaro.org> writes: >> >>> Set CF_SINGLE_STEP when single-stepping is enabled. >>> This avoids the need to flush all tb's when turning >>> single-stepping on or off. >>> >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>> --- >>> include/exec/exec-all.h | 1 + >>> accel/tcg/cpu-exec.c | 7 ++++++- >>> accel/tcg/translate-all.c | 4 ---- >>> accel/tcg/translator.c | 7 +------ >>> cpu.c | 4 ---- >>> 5 files changed, 8 insertions(+), 15 deletions(-) >>> >>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h >>> index 6873cce8df..5d1b6d80fb 100644 >>> --- a/include/exec/exec-all.h >>> +++ b/include/exec/exec-all.h >>> @@ -497,6 +497,7 @@ struct TranslationBlock { >>> #define CF_COUNT_MASK 0x000001ff >>> #define CF_NO_GOTO_TB 0x00000200 /* Do not chain with goto_tb */ >>> #define CF_NO_GOTO_PTR 0x00000400 /* Do not chain with goto_ptr */ >>> +#define CF_SINGLE_STEP 0x00000800 /* gdbstub single-step in effect */ >>> #define CF_LAST_IO 0x00008000 /* Last insn may be an IO access. */ >>> #define CF_MEMI_ONLY 0x00010000 /* Only instrument memory ops */ >>> #define CF_USE_ICOUNT 0x00020000 >>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c >>> index 5cc6363f4c..fc895cf51e 100644 >>> --- a/accel/tcg/cpu-exec.c >>> +++ b/accel/tcg/cpu-exec.c >>> @@ -150,10 +150,15 @@ uint32_t curr_cflags(CPUState *cpu) >>> uint32_t cflags = cpu->tcg_cflags; >>> /* >>> + * Record gdb single-step. We should be exiting the TB by raising >>> + * EXCP_DEBUG, but to simplify other tests, disable chaining too. >>> + * >>> * For singlestep and -d nochain, suppress goto_tb so that >>> * we can log -d cpu,exec after every TB. >>> */ >>> - if (singlestep) { >>> + if (unlikely(cpu->singlestep_enabled)) { >>> + cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | CF_SINGLE_STEP | >>> 1; >> What does CF_SINGLE_STEP achieve that isn't already handled by >> having: >> cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | 1; > > It sets DisasContextBase.singlestep_enabled. Ahh fair enough... I was only thinking of the effect on stored and looked up translations. I guess we still have bits we can rob if we need to until the day we expand cflags and flags to full 64 bit values. > With only this patch set, we still check that and emit EXCP_DEBUG at > the end of every TB. After the 6.2 singlestep cleanup, we still have > one reference to DisasContextBase.singlestep_enabled in target/mips > for the branch delay slot thing that we discussed on IRC yesterday. > >> (btw did we mask CF_COUNT_MASK somewhere else?). Because surely the >> CF_COUNT is part of cflags so limits the TB's that could be returned >> anyway? > > Here in curr_cflags(), CF_COUNT_MASK begins at zero. OK: Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index 6873cce8df..5d1b6d80fb 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -497,6 +497,7 @@ struct TranslationBlock { #define CF_COUNT_MASK 0x000001ff #define CF_NO_GOTO_TB 0x00000200 /* Do not chain with goto_tb */ #define CF_NO_GOTO_PTR 0x00000400 /* Do not chain with goto_ptr */ +#define CF_SINGLE_STEP 0x00000800 /* gdbstub single-step in effect */ #define CF_LAST_IO 0x00008000 /* Last insn may be an IO access. */ #define CF_MEMI_ONLY 0x00010000 /* Only instrument memory ops */ #define CF_USE_ICOUNT 0x00020000 diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index 5cc6363f4c..fc895cf51e 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -150,10 +150,15 @@ uint32_t curr_cflags(CPUState *cpu) uint32_t cflags = cpu->tcg_cflags; /* + * Record gdb single-step. We should be exiting the TB by raising + * EXCP_DEBUG, but to simplify other tests, disable chaining too. + * * For singlestep and -d nochain, suppress goto_tb so that * we can log -d cpu,exec after every TB. */ - if (singlestep) { + if (unlikely(cpu->singlestep_enabled)) { + cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | CF_SINGLE_STEP | 1; + } else if (singlestep) { 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/translate-all.c b/accel/tcg/translate-all.c index bf82c15aab..bbfcfb698c 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -1432,10 +1432,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu, } QEMU_BUILD_BUG_ON(CF_COUNT_MASK + 1 != TCG_MAX_INSNS); - if (cpu->singlestep_enabled) { - max_insns = 1; - } - buffer_overflow: tb = tcg_tb_alloc(tcg_ctx); if (unlikely(!tb)) { diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c index b45337f3ba..c53a7f8e44 100644 --- a/accel/tcg/translator.c +++ b/accel/tcg/translator.c @@ -38,11 +38,6 @@ bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest) return false; } - /* Suppress goto_tb in the case of single-steping. */ - if (db->singlestep_enabled) { - return false; - } - /* Check for the dest on the same page as the start of the TB. */ return ((db->pc_first ^ dest) & TARGET_PAGE_MASK) == 0; } @@ -60,7 +55,7 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, db->is_jmp = DISAS_NEXT; db->num_insns = 0; db->max_insns = max_insns; - db->singlestep_enabled = cpu->singlestep_enabled; + db->singlestep_enabled = cflags & CF_SINGLE_STEP; ops->init_disas_context(db, cpu); tcg_debug_assert(db->is_jmp == DISAS_NEXT); /* no early exit */ diff --git a/cpu.c b/cpu.c index d6ae5ae581..e1799a15bc 100644 --- a/cpu.c +++ b/cpu.c @@ -326,10 +326,6 @@ void cpu_single_step(CPUState *cpu, int enabled) cpu->singlestep_enabled = enabled; if (kvm_enabled()) { kvm_update_guest_debug(cpu, 0); - } else { - /* must flush all the translated code to avoid inconsistencies */ - /* XXX: only flush what is necessary */ - tb_flush(cpu); } trace_breakpoint_singlestep(cpu->cpu_index, enabled); }
Set CF_SINGLE_STEP when single-stepping is enabled. This avoids the need to flush all tb's when turning single-stepping on or off. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- include/exec/exec-all.h | 1 + accel/tcg/cpu-exec.c | 7 ++++++- accel/tcg/translate-all.c | 4 ---- accel/tcg/translator.c | 7 +------ cpu.c | 4 ---- 5 files changed, 8 insertions(+), 15 deletions(-)