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

[Richard Henderson <richard.henderson@linaro.org>]

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~