[PATCH v2 0/3] Some watchpoint-related patches

[PATCH v2 0/3] Some watchpoint-related patches

[Pavel Dovgalyuk]

The series includes several watchpoint-related patches.

v2 changes:
 - added patch to fix races with interrupts
 - added patch to process watchpoints-on-stack
 - removed upstreamed patches

---

Pavel Dovgalyuk (3):
      icount: preserve cflags when custom tb is about to execute
      softmmu: fix watchpoint-interrupt races
      softmmu: fix watchpoints on memory used by vCPU internals


 accel/tcg/cpu-exec.c |  5 +++++
 softmmu/physmem.c    | 14 ++++++++++++++
 2 files changed, 19 insertions(+)

--
Pavel Dovgalyuk

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

[Pavel Dovgalyuk]

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
+            && (!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;
+        }
         return;
     }
 

[PATCH v2 2/3] softmmu: fix watchpoint-interrupt races

[Pavel Dovgalyuk]

Watchpoint may be processed in two phases. First one is detecting
the instruction with target memory access. And the second one is
executing only one instruction and setting the debug interrupt flag.
Hardware interrupts can break this sequence when they happen after
the first watchpoint phase.
This patch postpones the interrupt request until watchpoint is
processed.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
---
 accel/tcg/cpu-exec.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index df12452b8f..e4526c2f5e 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -742,6 +742,11 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
             qemu_mutex_unlock_iothread();
             return true;
         }
+        /* Process watchpoints first, or interrupts will ruin everything */
+        if (cpu->watchpoint_hit) {
+            qemu_mutex_unlock_iothread();
+            return false;
+        }
 #if !defined(CONFIG_USER_ONLY)
         if (replay_mode == REPLAY_MODE_PLAY && !replay_has_interrupt()) {
             /* Do nothing */

[PATCH v2 3/3] softmmu: fix watchpoints on memory used by vCPU internals

[Pavel Dovgalyuk]

When vCPU processes interrupt request or exception, it can save
register values to the memory. Watchpoints may also be set on
these memory cells. In this case watchpoint processing code should
not retranslate the block which accessed the memory, because there
is no such block at all. "After access" watchpoint also can't be
used in such case.
This patch adds some conditions to prevent failures when watchpoint
is set on memory used for saving the registers on interrupt request.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
---
 softmmu/physmem.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 314f8b439c..53edcf9a51 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -886,6 +886,14 @@ void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
 
     assert(tcg_enabled());
     if (cpu->watchpoint_hit) {
+        if (!ra) {
+            /*
+             * Another memory access after hitting the watchpoint.
+             * There is no translation block and interrupt request
+             * is already set.
+             */
+            return;
+        }
         /*
          * We re-entered the check after replacing the TB.
          * Now raise the debug interrupt so that it will
@@ -936,6 +944,12 @@ void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
                 continue;
             }
             cpu->watchpoint_hit = wp;
+            if (!ra) {
+                /* We're not in the TB, can't stop before the access. */
+                g_assert(!(wp->flags & BP_STOP_BEFORE_ACCESS));
+                cpu_interrupt(cpu, CPU_INTERRUPT_DEBUG);
+                return;
+            }
 
             mmap_lock();
             /* This call also restores vCPU state */

Re: [PATCH v2 0/3] Some watchpoint-related patches

[David Hildenbrand]

On 11.11.21 10:55, Pavel Dovgalyuk wrote:
> The series includes several watchpoint-related patches.
> 
> v2 changes:
>  - added patch to fix races with interrupts
>  - added patch to process watchpoints-on-stack
>  - removed upstreamed patches

Out of interest, do we have any reproducer / tests for this?


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 0/3] Some watchpoint-related patches

[Pavel Dovgalyuk]

On 11.11.2021 13:48, David Hildenbrand wrote:
> On 11.11.21 10:55, Pavel Dovgalyuk wrote:
>> The series includes several watchpoint-related patches.
>>
>> v2 changes:
>>   - added patch to fix races with interrupts
>>   - added patch to process watchpoints-on-stack
>>   - removed upstreamed patches
> 
> Out of interest, do we have any reproducer / tests for this?
> 

I guess no.

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

[Alex Bennée]

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
> +            && (!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.

> +                || cpu_neg(cpu)->icount_decr.u16.low >= tb->icount))
> {

Is u16.low ever set when icount isn't enabled?

> +            /*
> +             * 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;
> +        }
>          return;
>      }
>  


-- 
Alex Bennée

Re: [PATCH v2 2/3] softmmu: fix watchpoint-interrupt races

[Alex Bennée]

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

> Watchpoint may be processed in two phases. First one is detecting
> the instruction with target memory access. And the second one is
> executing only one instruction and setting the debug interrupt flag.
> Hardware interrupts can break this sequence when they happen after
> the first watchpoint phase.
> This patch postpones the interrupt request until watchpoint is
> processed.
>
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
> ---
>  accel/tcg/cpu-exec.c |    5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index df12452b8f..e4526c2f5e 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -742,6 +742,11 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>              qemu_mutex_unlock_iothread();
>              return true;
>          }
> +        /* Process watchpoints first, or interrupts will ruin everything */
> +        if (cpu->watchpoint_hit) {
> +            qemu_mutex_unlock_iothread();
> +            return false;
> +        }

side note: I wonder if it is time to wrap up the locks up with
QEMU_LOCK_GUARD or something similar?

Anyway seems reasonable:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH v2 2/3] softmmu: fix watchpoint-interrupt races

[David Hildenbrand]

On 11.11.21 10:55, Pavel Dovgalyuk wrote:
> Watchpoint may be processed in two phases. First one is detecting
> the instruction with target memory access. And the second one is
> executing only one instruction and setting the debug interrupt flag.
> Hardware interrupts can break this sequence when they happen after
> the first watchpoint phase.
> This patch postpones the interrupt request until watchpoint is
> processed.
> 
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
> ---
>  accel/tcg/cpu-exec.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index df12452b8f..e4526c2f5e 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -742,6 +742,11 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>              qemu_mutex_unlock_iothread();
>              return true;
>          }
> +        /* Process watchpoints first, or interrupts will ruin everything */
> +        if (cpu->watchpoint_hit) {
> +            qemu_mutex_unlock_iothread();
> +            return false;
> +        }
>  #if !defined(CONFIG_USER_ONLY)
>          if (replay_mode == REPLAY_MODE_PLAY && !replay_has_interrupt()) {
>              /* Do nothing */
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

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

[Pavel Dovgalyuk]

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
>> +            && (!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. Sometimes we use non-icount blocks in icount mode.
But AFAIR they are used only for triggering the exeptions, but not for 
real execution.

> 
>> +                || 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;
>> +        }
>>           return;
>>       }
>>   
> 
> 

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

[Alex Bennée]

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

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

[Alex Bennée]

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.

How about this alternative?

--8<---------------cut here---------------start------------->8---
accel/tcg: suppress IRQ check for special TBs

Generally when we set cpu->cflags_next_tb it is because we want to
carefully control the execution of the next TB. Currently there is a
race that causes cflags_next_tb to get ignored if an IRQ is processed
before we execute any actual instructions.

To avoid this we introduce a new compiler flag: CF_NOIRQ to suppress
this check in the generated code so we know we will definitely execute
the next block.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/245

3 files changed, 22 insertions(+), 3 deletions(-)
include/exec/exec-all.h   |  1 +
include/exec/gen-icount.h | 19 ++++++++++++++++---
accel/tcg/cpu-exec.c      |  5 +++++

modified   include/exec/exec-all.h
@@ -503,6 +503,7 @@ struct TranslationBlock {
 #define CF_USE_ICOUNT    0x00020000
 #define CF_INVALID       0x00040000 /* TB is stale. Set with @jmp_lock held */
 #define CF_PARALLEL      0x00080000 /* Generate code for a parallel context */
+#define CF_NOIRQ         0x00100000 /* Generate an uninterruptible TB */
 #define CF_CLUSTER_MASK  0xff000000 /* Top 8 bits are cluster ID */
 #define CF_CLUSTER_SHIFT 24
 
modified   include/exec/gen-icount.h
@@ -21,7 +21,6 @@ static inline void gen_tb_start(const TranslationBlock *tb)
 {
     TCGv_i32 count;
 
-    tcg_ctx->exitreq_label = gen_new_label();
     if (tb_cflags(tb) & CF_USE_ICOUNT) {
         count = tcg_temp_local_new_i32();
     } else {
@@ -42,7 +41,19 @@ static inline void gen_tb_start(const TranslationBlock *tb)
         icount_start_insn = tcg_last_op();
     }
 
-    tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, tcg_ctx->exitreq_label);
+    /*
+     * Emit the check against icount_decr.u32 to see if we should exit
+     * unless we suppress the check with CF_NOIRQ. If we are using
+     * icount and have suppressed interruption the higher level code
+     * should have ensured we don't run more instructions than the
+     * budget.
+     */
+    if (tb_cflags(tb) & CF_NOIRQ) {
+        tcg_ctx->exitreq_label = NULL;
+    } else {
+        tcg_ctx->exitreq_label = gen_new_label();
+        tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, tcg_ctx->exitreq_label);
+    }
 
     if (tb_cflags(tb) & CF_USE_ICOUNT) {
         tcg_gen_st16_i32(count, cpu_env,
@@ -74,7 +85,9 @@ static inline void gen_tb_end(const TranslationBlock *tb, int num_insns)
                            tcgv_i32_arg(tcg_constant_i32(num_insns)));
     }
 
-    gen_set_label(tcg_ctx->exitreq_label);
+    if (tcg_ctx->exitreq_label) {
+        gen_set_label(tcg_ctx->exitreq_label);
+    }
     tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);
 }
 
modified   accel/tcg/cpu-exec.c
@@ -954,11 +954,16 @@ int cpu_exec(CPUState *cpu)
              * after-access watchpoints.  Since this request should never
              * have CF_INVALID set, -1 is a convenient invalid value that
              * does not require tcg headers for cpu_common_reset.
+             *
+             * As we don't want this special TB being interrupted by
+             * some sort of asynchronous event we apply CF_NOIRQ to
+             * disable the usual event checking.
              */
             cflags = cpu->cflags_next_tb;
             if (cflags == -1) {
                 cflags = curr_cflags(cpu);
             } else {
+                cflags |= CF_NOIRQ;
                 cpu->cflags_next_tb = -1;
             }
 
--8<---------------cut here---------------end--------------->8---

>
> 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
> +            && (!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;
> +        }
>          return;
>      }
>  


-- 
Alex Bennée

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

[Richard Henderson]

On 11/17/21 10:47 AM, Alex Bennée wrote:
> -    gen_set_label(tcg_ctx->exitreq_label);
> +    if (tcg_ctx->exitreq_label) {
> +        gen_set_label(tcg_ctx->exitreq_label);
> +    }
>       tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);

The exit_tb is also not reachable, and should go in with the label.

>   }
>   
> modified   accel/tcg/cpu-exec.c
> @@ -954,11 +954,16 @@ int cpu_exec(CPUState *cpu)
>                * after-access watchpoints.  Since this request should never
>                * have CF_INVALID set, -1 is a convenient invalid value that
>                * does not require tcg headers for cpu_common_reset.
> +             *
> +             * As we don't want this special TB being interrupted by
> +             * some sort of asynchronous event we apply CF_NOIRQ to
> +             * disable the usual event checking.
>                */
>               cflags = cpu->cflags_next_tb;
>               if (cflags == -1) {
>                   cflags = curr_cflags(cpu);
>               } else {
> +                cflags |= CF_NOIRQ;
>                   cpu->cflags_next_tb = -1;
>               }

Still missing something to avoid cpu_handle_interrupt firing?


r~

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

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 11/17/21 10:47 AM, Alex Bennée wrote:
>> -    gen_set_label(tcg_ctx->exitreq_label);
>> +    if (tcg_ctx->exitreq_label) {
>> +        gen_set_label(tcg_ctx->exitreq_label);
>> +    }
>>       tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);
>
> The exit_tb is also not reachable, and should go in with the label.

ok

>
>>   }
>>   modified   accel/tcg/cpu-exec.c
>> @@ -954,11 +954,16 @@ int cpu_exec(CPUState *cpu)
>>                * after-access watchpoints.  Since this request should never
>>                * have CF_INVALID set, -1 is a convenient invalid value that
>>                * does not require tcg headers for cpu_common_reset.
>> +             *
>> +             * As we don't want this special TB being interrupted by
>> +             * some sort of asynchronous event we apply CF_NOIRQ to
>> +             * disable the usual event checking.
>>                */
>>               cflags = cpu->cflags_next_tb;
>>               if (cflags == -1) {
>>                   cflags = curr_cflags(cpu);
>>               } else {
>> +                cflags |= CF_NOIRQ;
>>                   cpu->cflags_next_tb = -1;
>>               }
>
> Still missing something to avoid cpu_handle_interrupt firing?

Something as simple as:

--8<---------------cut here---------------start------------->8---
modified   accel/tcg/cpu-exec.c
@@ -721,6 +721,15 @@ static inline bool need_replay_interrupt(int interrupt_request)
 static inline bool cpu_handle_interrupt(CPUState *cpu,
                                         TranslationBlock **last_tb)
 {
+    /*
+     * If we have special cflags lets not get distracted with IRQs. We
+     * shall exit the loop as soon as the next TB completes what it
+     * needs to do.
+     */
+    if (cpu->cflags_next_tb != -1) {
+        return false;
+    }
+
     /* Clear the interrupt flag now since we're processing
      * cpu->interrupt_request and cpu->exit_request.
--8<---------------cut here---------------end--------------->8---

?

>
>
> r~


-- 
Alex Bennée

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

[Richard Henderson]

On 11/17/21 11:29 AM, Alex Bennée wrote:
>> Still missing something to avoid cpu_handle_interrupt firing?
> 
> Something as simple as:
> 
> --8<---------------cut here---------------start------------->8---
> modified   accel/tcg/cpu-exec.c
> @@ -721,6 +721,15 @@ static inline bool need_replay_interrupt(int interrupt_request)
>   static inline bool cpu_handle_interrupt(CPUState *cpu,
>                                           TranslationBlock **last_tb)
>   {
> +    /*
> +     * If we have special cflags lets not get distracted with IRQs. We
> +     * shall exit the loop as soon as the next TB completes what it
> +     * needs to do.
> +     */
> +    if (cpu->cflags_next_tb != -1) {
> +        return false;
> +    }
> +
>       /* Clear the interrupt flag now since we're processing
>        * cpu->interrupt_request and cpu->exit_request.
> --8<---------------cut here---------------end--------------->8---
> 
> ?

Yeah, that should do it.


r~

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

[Pavel Dovgalyuk]

On 17.11.2021 12:47, 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.
> 
> How about this alternative?

I checked all cflags_next_tb assignments.
Looks that this variant should work.

> 
> --8<---------------cut here---------------start------------->8---
> accel/tcg: suppress IRQ check for special TBs
> 
> Generally when we set cpu->cflags_next_tb it is because we want to
> carefully control the execution of the next TB. Currently there is a
> race that causes cflags_next_tb to get ignored if an IRQ is processed
> before we execute any actual instructions.
> 
> To avoid this we introduce a new compiler flag: CF_NOIRQ to suppress
> this check in the generated code so we know we will definitely execute
> the next block.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/245
> 
> 3 files changed, 22 insertions(+), 3 deletions(-)
> include/exec/exec-all.h   |  1 +
> include/exec/gen-icount.h | 19 ++++++++++++++++---
> accel/tcg/cpu-exec.c      |  5 +++++
> 
> modified   include/exec/exec-all.h
> @@ -503,6 +503,7 @@ struct TranslationBlock {
>   #define CF_USE_ICOUNT    0x00020000
>   #define CF_INVALID       0x00040000 /* TB is stale. Set with @jmp_lock held */
>   #define CF_PARALLEL      0x00080000 /* Generate code for a parallel context */
> +#define CF_NOIRQ         0x00100000 /* Generate an uninterruptible TB */
>   #define CF_CLUSTER_MASK  0xff000000 /* Top 8 bits are cluster ID */
>   #define CF_CLUSTER_SHIFT 24
>   
> modified   include/exec/gen-icount.h
> @@ -21,7 +21,6 @@ static inline void gen_tb_start(const TranslationBlock *tb)
>   {
>       TCGv_i32 count;
>   
> -    tcg_ctx->exitreq_label = gen_new_label();
>       if (tb_cflags(tb) & CF_USE_ICOUNT) {
>           count = tcg_temp_local_new_i32();
>       } else {
> @@ -42,7 +41,19 @@ static inline void gen_tb_start(const TranslationBlock *tb)
>           icount_start_insn = tcg_last_op();
>       }
>   
> -    tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, tcg_ctx->exitreq_label);
> +    /*
> +     * Emit the check against icount_decr.u32 to see if we should exit
> +     * unless we suppress the check with CF_NOIRQ. If we are using
> +     * icount and have suppressed interruption the higher level code
> +     * should have ensured we don't run more instructions than the
> +     * budget.
> +     */
> +    if (tb_cflags(tb) & CF_NOIRQ) {
> +        tcg_ctx->exitreq_label = NULL;
> +    } else {
> +        tcg_ctx->exitreq_label = gen_new_label();
> +        tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, tcg_ctx->exitreq_label);
> +    }
>   
>       if (tb_cflags(tb) & CF_USE_ICOUNT) {
>           tcg_gen_st16_i32(count, cpu_env,
> @@ -74,7 +85,9 @@ static inline void gen_tb_end(const TranslationBlock *tb, int num_insns)
>                              tcgv_i32_arg(tcg_constant_i32(num_insns)));
>       }
>   
> -    gen_set_label(tcg_ctx->exitreq_label);
> +    if (tcg_ctx->exitreq_label) {
> +        gen_set_label(tcg_ctx->exitreq_label);
> +    }
>       tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);
>   }
>   
> modified   accel/tcg/cpu-exec.c
> @@ -954,11 +954,16 @@ int cpu_exec(CPUState *cpu)
>                * after-access watchpoints.  Since this request should never
>                * have CF_INVALID set, -1 is a convenient invalid value that
>                * does not require tcg headers for cpu_common_reset.
> +             *
> +             * As we don't want this special TB being interrupted by
> +             * some sort of asynchronous event we apply CF_NOIRQ to
> +             * disable the usual event checking.
>                */
>               cflags = cpu->cflags_next_tb;
>               if (cflags == -1) {
>                   cflags = curr_cflags(cpu);
>               } else {
> +                cflags |= CF_NOIRQ;
>                   cpu->cflags_next_tb = -1;
>               }
>   
> --8<---------------cut here---------------end--------------->8---
> 
>>
>> 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
>> +            && (!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;
>> +        }
>>           return;
>>       }
>>   
> 
>