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

[Alex Bennée]

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>

-- 
Alex Bennée