Pavel Dovgalyuk <pavel.dovgal...@ispras.ru> writes:
> On 11.11.2021 15:20, Alex Bennée wrote:
>> Pavel Dovgalyuk <pavel.dovgal...@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.dovgal...@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
