Re: [PATCH for-6.1 v6 17/17] accel/tcg: Record singlestep_enabled in tb->cflags

[Richard Henderson]

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~