Re: [PATCH v2 1/3] icount: preserve cflags when custom tb is about to execute

["Alex Bennée" <alex.bennee@linaro.org>]

Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> writes:

> On 11.11.2021 15:20, Alex Bennée wrote:
>> Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> writes:
>> 
>>> When debugging with the watchpoints, qemu may need to create
>>> TB with single instruction. This is achieved by setting cpu->cflags_next_tb.
>>> But when this block is about to execute, it may be interrupted by another
>>> thread. In this case cflags will be lost and next executed TB will not
>>> be the special one.
>>> This patch checks TB exit reason and restores cflags_next_tb to allow
>>> finding the interrupted block.
>>>
>>> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
>>> ---
>>>   accel/tcg/cpu-exec.c |   10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>>> index 2d14d02f6c..df12452b8f 100644
>>> --- a/accel/tcg/cpu-exec.c
>>> +++ b/accel/tcg/cpu-exec.c
>>> @@ -846,6 +846,16 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
>>>            * cpu_handle_interrupt.  cpu_handle_interrupt will also
>>>            * clear cpu->icount_decr.u16.high.
>>>            */
>>> +        if (cpu->cflags_next_tb == -1

Can cpu->cflags_next_tb ever be anything else? It is consumed in
cpu_exec() and it can only be reset if we have executed some
instructions which resulted in some sort of helper call that set it for
the next TB.

>>> +            && (!use_icount || !(tb->cflags & CF_USE_ICOUNT)
>> Why check use_icount here? The cflags should always have
>> CF_USE_ICOUNT
>> set when icount is enabled. Lets not over complicate the inverted ||
>> tests we have here.
>
> Not really.

Right this is were the logic gets complicated to follow. Are we dealing
with icount cases or non-icount cases or some mixture of both?

> Sometimes we use non-icount blocks in icount mode.
> But AFAIR they are used only for triggering the exeptions, but not for
> real execution.

Right so tcg_cpu_init_cflags ensures CF_USE_ICOUNT is set for all blocks
when use_icount() in enabled except the one special case during
exception replay where we suppress it:

  #ifndef CONFIG_USER_ONLY
          if (replay_has_exception()
              && cpu_neg(cpu)->icount_decr.u16.low + cpu->icount_extra == 0) {
              /* Execute just one insn to trigger exception pending in the log */
              cpu->cflags_next_tb = (curr_cflags(cpu) & ~CF_USE_ICOUNT) | 1;
          }
  #endif

which still slightly scrambles my brain because does that affect the
final updating of icount_get_executed() or do we "loose" the instruction
in that case.

>
>> 
>>> +                || cpu_neg(cpu)->icount_decr.u16.low >= tb->icount))
>>> {
>> Is u16.low ever set when icount isn't enabled?
>
> This condition is checked for icount mode only.
> u16.low is not used without icount.
>
>> 
>>> +            /*
>>> +             * icount is disabled or there are enough instructions
>>> +             * in the budget, do not retranslate this block with
>>> +             * different parameters.
>>> +             */
>>> +            cpu->cflags_next_tb = tb->cflags;

Technically this isn't what cpu->cflags_next_tb used to be because the
eventual tb->cflags might get tweaked by various conditions in
tb_gen_code().

It seems to me what we really need is a clear unambiguous indication from
cpu_tb_exec() that the we have executed nothing apart from the initial
preamble generated by gen_tb_start(). If we have advanced beyond that
point it would never be valid to restore the cflag state form the TB.

Richard, what do you think?

-- 
Alex Bennée