[4/4] icount: preserve cflags when custom tb is about to execute

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(+)

On 10/28/21 4:48 AM, Pavel Dovgalyuk wrote:
> +        if (cpu->cflags_next_tb == -1
> +            && (!use_icount || !(tb->cflags & CF_USE_ICOUNT)
> +                || cpu_neg(cpu)->icount_decr.u16.low >= tb->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;
> +        }

I can't see that this will work.

We've been asked to exit to the main loop; probably for an interrupt.  The next thing 
that's likely to happen is that cpu_cc->do_interrupt will adjust cpu state to continue in 
the guest interrupt handler.  The cflags_next_tb flag that you're setting up is not 
relevant to that context.

This seems related to Phil's reported problem

   https://gitlab.com/qemu-project/qemu/-/issues/245

in which an interrupt arrives before we finish processing the watchpoint.

I *think* we need to make cflags_next_tb != -1 be treated like any other interrupt disable 
bit, and delay delivery of the interrupt.  Which also means that we should not check for 
exit_tb at the beginning of any TB we generate for the watchpoint step.

I simply haven't taken the time to investigate this properly.


r~

On 28.10.2021 22:26, Richard Henderson wrote:
> On 10/28/21 4:48 AM, Pavel Dovgalyuk wrote:
>> +        if (cpu->cflags_next_tb == -1
>> +            && (!use_icount || !(tb->cflags & CF_USE_ICOUNT)
>> +                || cpu_neg(cpu)->icount_decr.u16.low >= tb->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;
>> +        }
> 
> I can't see that this will work.

The issue was the following:
- icount is enabled
- watchpoint hit
- next block should contain a single instruction to stop the execution 
after memory access
- tb with one instruction is translated, cflags_next_tb is reset
- main loop breaks the execution for some reason
- after resuming the execution, we are trying to find new tb without any 
filter in cflags
- tb with multiple instructions is executed (instead of previously 
generated)

> 
> We've been asked to exit to the main loop; probably for an interrupt.  
> The next thing that's likely to happen is that cpu_cc->do_interrupt will 
> adjust cpu state to continue in the guest interrupt handler.  The 
> cflags_next_tb flag that you're setting up is not relevant to that context.
> 
> This seems related to Phil's reported problem
> 
>    https://gitlab.com/qemu-project/qemu/-/issues/245
> 
> in which an interrupt arrives before we finish processing the watchpoint.

Right, this is similar.

> 
> I *think* we need to make cflags_next_tb != -1 be treated like any other 
> interrupt disable bit, and delay delivery of the interrupt.  Which also 
> means that we should not check for exit_tb at the beginning of any TB we 
> generate for the watchpoint step.
> 
> I simply haven't taken the time to investigate this properly.
> 
> 
> r~

On 28.10.2021 22:26, Richard Henderson wrote:
> On 10/28/21 4:48 AM, Pavel Dovgalyuk wrote:
>> +        if (cpu->cflags_next_tb == -1
>> +            && (!use_icount || !(tb->cflags & CF_USE_ICOUNT)
>> +                || cpu_neg(cpu)->icount_decr.u16.low >= tb->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;
>> +        }
> 
> I can't see that this will work.
> 
> We've been asked to exit to the main loop; probably for an interrupt.  
> The next thing that's likely to happen is that cpu_cc->do_interrupt will 
> adjust cpu state to continue in the guest interrupt handler.  The 
> cflags_next_tb flag that you're setting up is not relevant to that context.
> 
> This seems related to Phil's reported problem
> 
>    https://gitlab.com/qemu-project/qemu/-/issues/245
> 
> in which an interrupt arrives before we finish processing the watchpoint.
> 
> I *think* we need to make cflags_next_tb != -1 be treated like any other 
> interrupt disable bit, and delay delivery of the interrupt.  Which also 
> means that we should not check for exit_tb at the beginning of any TB we 
> generate for the watchpoint step.
> 
> I simply haven't taken the time to investigate this properly.

Please check the new series, it probably solves the problem described in 
that issue.

Pavel Dovgalyuk